-
-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SpecApi #3281
SpecApi #3281
Conversation
Compile Times benchmarkNote, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running: using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
|
function lift_convert(key, value, plot, screen) | ||
return lift_convert_inner(value, Key{key}(), Key{Makie.plotkey(plot)}(), plot, screen) | ||
end | ||
|
||
function lift_convert_inner(value, key, plot_key, plot) | ||
function lift_convert_inner(value, key, plot_key, plot, screen) | ||
return lift(plot, value) do value | ||
screen.requires_update = true | ||
return convert_attribute(value, key, plot_key) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell all plot update tracking comes from this now? In that case I think it misses updates to camera matrices and lighting, because those get added to a render object from scene data. Though those should probably also be handled in screen.jl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, didn't notice that, since enough is usually going on that it seems to be enough for most plots to rely on those updates...
Yes, I think we should add that here:
https://github.com/MakieOrg/Makie.jl/blob/master/GLMakie/src/screen.jl#L438
A few Benchmarks: using GLMakie, WGLMakie
function test(ax, last_pl, n=500)
for i in 1:n
delete!(ax, last_pl)
last_pl = scatter!(ax, rand(1000); color=rand(1000), markersize=10, colormap=:viridis)
end
end
f, ax, pl = scatter(rand(1000); color=rand(1000), markersize=10, colormap=:viridis)
fsize = Base.summarysize(f) / 10^6
@time test(ax, pl, 500)
function test2(f, ax, n=100)
for i in 1:n
delete!(ax)
ax, pl = scatter(f[1, 1], rand(1000); color=rand(1000), markersize=10, colormap=:viridis)
yield()
end
end
f, ax, pl = scatter(rand(1000); color=rand(1000), markersize=10, colormap=:viridis)
fsize = Base.summarysize(f) / 10^6
@time test2(f, ax)
fsize2 = Base.summarysize(f) / 10^6 PRtest1
test2
Taggedtest1
test2
|
Continues #2831 ! Still needs to check, if I rebased correctly and didn't incorrectly apply some of the changes! ## Merged PRs - #2598 - #2746 - #2346 - #2544 - #3082 - #2868 - #3062 - #3106 - #3281 - #3246 ## TODOS - [x] fix flaky test `@test GLMakie.window_size(screen.glscreen) == scaled(screen, (W, H))` - [x] Merge axis type inferences from #2220 - [x] Test on different resolution screens, IJulia, Pluto, VSCode, Windowed - [x] rebase to only have merge commits from the PRs - [x] investigate unexpected speed ups - [x] reset camera settings from tests - [ ] check doc image generation - [x] rethink default near/far in Camera3D (compatability with OIT) - [x] merge #3246 - [x] fix WGLMakie issues/tests: - [x] fix line depth issues (see tests: ~~hexbin colorrange~~ (not new), LaTeXStrings in Axis3, Axis3 axis reversal) - [x] fix lighting of surface with nan points (fixed in #3246) - ~~volume/3D contour artifacts (see 3D Contour with 2D contour slices)~~ not new - ~~artifacting in "colorscale (lines)"~~ not new - [x] GLMakie: - [x] slight outline in "scatter image markers" test - ~~clipping/z-fighting in "volume translated"~~ not new - [x] CairoMakie: - ~~Artfiacting in `colorscale (lines)"~~ not new - ~~markersize in "scatter rotations" changed?~~ not new - ~~color change in "colorscale (poly)"~~ not new - ~~transparency/render order of "OldAxis + Surface"~~ not new - ~~render order in "Merged color mesh"~~ not new - ~~render order of "Surface + wireframe + contour"~~ not new - [x] Check "SpecApi in convert_arguments" (colors swapped?) ## Fixes the following errors - fixes #2721 via #2746 - fixes #1600 via #2746 - fixes #1236 via #2746 - fixes MakieOrg/GeoMakie.jl#133 via #2598 - closes #2522 - closes #3239 via #3246 - fixes #3238 via #3246 - fixes #2985 via #3246 - fixes #3307 via #3281
Similar to PlotSpec, just for having
Observable{Figure}
delete!(block)
anddelete!(ax/scene, plot)
, especially for WGLMakiebbox(::BezierPath)
is really expensive and was run many times formarkersize=large_array
. See Poor performance of scatter with non-scalar markersize #3307convert_arguments(...)::FigureSpec
easier to integrate and removes doubleconvert_arguments
from previous implementation. This should also make Unit support for Axes & Recipes, a.k.a axis converts #3226 easier!Examples from the docs: