Rework projection code #3040
Rework projection code #3040
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 to_ndim(trg::VT, src::VecTypes{N2}) where {N, N2, VT <: VecTypes{N}} | ||
| VT(ntuple(Val(N)) do i | ||
| @inbounds i > N2 ? trg[i] : src[i] | ||
| end) | ||
| end |
There was a problem hiding this comment.
I did some benchmarking on to_ndim while working on this pr. It's relatively slow if the type isn't known at compile time. You can see this by benchmarking the function directly:
T = Point4f
p2 = Point2f(1, 2)
fillval = 1f0
@benchmark Makie.to_ndim($T, $p2, $fillval) # ~74nsAnd comparing with a wrapped version where the type is hardcoded:
f2(p, v) = Makie.to_ndim(Point4f, p, v)
@benchmark f2($p2, $fillval) # 1.4nsUsing a function barrier does not improve performance:
f1(T, p, v) = Makie.to_ndim(T, p, v)
@benchmark f1($T, $p2, $fillval) # ~74nsHowever if you use a instantiated object rather than passing a type things become fast with a "dynamic" type:
p4 = Point4f(1)
@benchmark Makie.to_ndim($p4, $p2) # 1.4nsThis also has a real effect on Makie.project. With 1000 Point2f's it cuts down 150µs to about 9.6µs for me.
CairoMakie/src/primitives.jl
Outdated
| # For correct z-ordering we need to be in eye (camera), clip, pixel or | ||
| # relative space | ||
| if get_value(primitive, :space, :data) in (:data, :transformed, :world) | ||
| zs = last.(Makie.project(primitive, pos, output_space = :eye)) | ||
| zorder = sortperm(zs, rev = false) | ||
| elseif length(pos[1]) > 2 | ||
| zs = getindex.(zs, 3) | ||
| zorder = sortperm(zs, rev = false) | ||
| else | ||
| zorder = eachindex(pos) | ||
| end |
There was a problem hiding this comment.
I benchmarked this with some simplifications (no matrix-matrix product, just one random matrix). The old version takes about 90µs for me, the new about 17µs with 1000 Point3f's. Using project with a transform_func drops it to about 22µs.
| lock = Observables.listeners(camera.projectionview)[1][2]::ObservableLocker | ||
| lock!(lock) | ||
| camera.view[] = view | ||
| unlock!(lock) | ||
| camera.projection[] = projection |
There was a problem hiding this comment.
Alternatively pull observables or JuliaGizmos/Observables.jl#109
|
I've been thinking about whether Conceptually I see In practice it seems reasonable to me that any plot can be scaled, translated, rotated or even have a nonllinear transform_func like a polar transform before being placed into a world, even if that world is in pixel space or clip space. (This is also the way it works if we use a different camera rather than adjusting space.) Detaching Changes I'm thinking about making:
|
|
With higher test accuracy this fails Makie.jl/ReferenceTests/src/tests/examples3d.jl Lines 461 to 472 in 83fc003 because the axis limits have become a bit larger, changing the generated grid. I assume this happens because the data_limits of the contour plot changed to include the z-translation it has. Not sure what caused this specifically, but I think this is something we probably want anyway. |
|
I think this pr was trying to do too much at once. I'll close this and hopefully work on it piece by piece |
Since this pr is shaping out to be breaking I'm planning on handling #3013, #2881 and #1730 here.
Description
This pr has two main goals. The first is to reduce the amount of (more or less) redundant code dealing with projecting positions. The second is to add missing
spaceoptions. The pipeline of coordinate spaces/transformations we have looks like thiswhere transformed, world and eye space are not accessible through
space. (See also #2766) These are added here.Something I'm not sure about yet is whether pixel and relative space should ignore
Transformations. It might be a bit weird that things likescale!()or polar transforms don't work with them. If we do allow them to affect pixel and relative space it might make sense to separate them completely fromspace. Otherwise naming will get confusing I think. We should also disallow transform propagation if spaces don't match.I'm also considering restarting #1730 here, as it may change how some things need to work.
Closes #3013
Closes #2766 since this also implements transformed space.
TODO
projectto be more useful. It now includesmodelandtransform_funcdepending on input space and can deal with vector inputs.projection_obsto simplify projecting data in observablesboundingboxcleanup #2881 hereOther changes
transform_functo be passed directly as a plot kwargtransformationandtransform_funcfrom plot attributes on constructionType of change
While
projectmay or may not be internal, I think it is fairly likely to be used outside in similar situations aserrorbars,h/vlines,h/vspan,bracket, etc. (I.e. whether part of or the full plot needs to be in pixel space.)Checklist