Skip to content
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

Add space=:transformed #2766

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add space=:transformed #2766

wants to merge 10 commits into from

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Mar 14, 2023

Description

Basically data space, without applying apply_transform. Works pretty well.

MWE:

fig, ax, plt = lines(1:10, 1:10; axis = (; xscale = log10))
lines!(ax, LinRange(0, 10, 10), 1:10; space = :transformed)

download-2

This is useful if you want to control when the plot's transform_func is applied, especially in a recipe!

I'm working on a potential solution to plot Tyler tiles in geographic projections by plotting a pre-transformed mesh, and texturing it by the image. The WIP code is in this gist, and the relevant issue is MakieOrg/Tyler.jl#25.

More generally, we could also use this transformed space in poly, and provide the mesh triangulation points in transformed space!

Needs tests and docs.

Type of change

Delete options that do not apply:

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

Basically data space, without applying `apply_transform`.  Works pretty well.

MWE:
```julia
fig, ax, plt = lines(1:10, 1:10; axis = (; xscale = log10))
lines!(ax, LinRange(0, 10, 10), 1:10; space = :transformed)
```

This is useful if you want to control when the plot's transform_func is applied, especially in a recipe!
@MakieBot
Copy link
Collaborator

MakieBot commented Mar 14, 2023

Compile Times benchmark

Note, 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))
using create display create display
GLMakie 4.66s (4.59, 4.70) 0.04+- 133.90ms (117.16, 139.07) 7.71+- 538.32ms (513.00, 558.76) 14.07+- 8.92ms (8.71, 9.20) 0.19+- 26.83ms (26.67, 27.10) 0.16+-
master 4.69s (4.59, 4.79) 0.07+- 134.15ms (130.07, 139.05) 3.56+- 547.26ms (538.04, 552.89) 5.16+- 8.89ms (8.49, 9.14) 0.26+- 26.77ms (26.64, 26.96) 0.11+-
evaluation 1.01x invariant, -0.03s (-0.58d, 0.31p, 0.05std) 1.00x invariant, -0.25ms (-0.04d, 0.94p, 5.64std) 1.02x invariant, -8.93ms (-0.84d, 0.16p, 9.61std) 1.00x invariant, 0.03ms (0.15d, 0.78p, 0.22std) 1.00x invariant, 0.06ms (0.46d, 0.41p, 0.14std)
CairoMakie 3.64s (3.63, 3.65) 0.01+- 106.02ms (104.52, 107.33) 1.04+- 130.74ms (127.87, 132.08) 1.37+- 7.78ms (7.75, 7.84) 0.03+- 977.06μs (970.82, 988.36) 6.45+-
master 3.74s (3.64, 3.86) 0.10+- 106.40ms (104.66, 107.60) 1.01+- 129.81ms (129.01, 130.24) 0.41+- 7.62ms (7.53, 7.70) 0.07+- 971.88μs (966.30, 978.67) 4.68+-
evaluation 1.03x faster ✓, -0.1s (-1.49d, 0.03p, 0.05std) 1.00x invariant, -0.38ms (-0.37d, 0.51p, 1.03std) 0.99x invariant, 0.93ms (0.92d, 0.13p, 0.89std) 0.98x slower X, 0.16ms (3.15d, 0.00p, 0.05std) 0.99x invariant, 5.18μs (0.92d, 0.11p, 5.56std)
WGLMakie 4.43s (4.34, 4.51) 0.06+- 106.00ms (103.56, 109.54) 1.94+- 8.83s (8.50, 9.06) 0.19+- 9.82ms (9.61, 9.96) 0.11+- 70.97ms (69.84, 73.70) 1.32+-
master 4.44s (4.37, 4.54) 0.06+- 107.15ms (104.12, 113.21) 3.06+- 9.06s (9.00, 9.19) 0.07+- 10.18ms (9.69, 10.89) 0.36+- 70.14ms (69.51, 70.81) 0.41+-
evaluation 1.00x invariant, -0.01s (-0.15d, 0.78p, 0.06std) 1.01x invariant, -1.15ms (-0.45d, 0.42p, 2.50std) 1.03x faster ✓, -0.24s (-1.64d, 0.02p, 0.13std) 1.04x faster ✓, -0.36ms (-1.35d, 0.04p, 0.23std) 0.99x invariant, 0.83ms (0.85d, 0.15p, 0.87std)

@asinghvi17 asinghvi17 marked this pull request as ready for review March 15, 2023 10:17
@ffreyer
Copy link
Collaborator

ffreyer commented Mar 15, 2023

I got a bit confused that this doesn't touch the transformation code, but there we are already only allowing space = :data

apply_transform(f, data, space) = space === :data ? apply_transform(f, data) : data

I don't like the name that much but I can see the point of this.

Mixing in the opengl terminology from leanropengl we'd have

  • :data -> (this) -> world -> view (camera) -> :clip -> (opengl internal screen)
  • :relative -> :clip -> (opengl internal screen)
  • :pixel -> :clip -> (opengl internal screen)

where data -> (this) applies transform_func and (this) -> world applies the model matrix (i.e. transformation.scale, etc). Since everything beyond transform_func is a linear transformation, maybe :linear_data would be a more precise name for this?

@asinghvi17
Copy link
Member Author

asinghvi17 commented Mar 15, 2023

Hmm, I see what you're saying but I'm not sure :linear_data is the most obvious name. Although :transformed isn't good either, since it could be seen as related to e.g. plot.transformation.

Another possible name might be :transformed_data, I wanted it to evoke apply_transform and transform_func in some way. If it doesn't make sense, I'm fine going with :linear_data as well.

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 15, 2023

Maybe it's better to leave it as is now and adjust things later if we find a better name.

This stuff may need some adjustments as well:

# project between different coordinate systems/spaces
function space_to_clip(cam::Camera, space::Symbol, projectionview::Bool=true)
if is_data_space(space)
return projectionview ? cam.projectionview[] : cam.projection[]
elseif is_pixel_space(space)
return cam.pixel_space[]
elseif is_relative_space(space)
return Mat4f(2, 0, 0, 0, 0, 2, 0, 0, 0, 0, 1, 0, -1, -1, 0, 1)
elseif is_clip_space(space)
return Mat4f(I)
else
error("Space $space not recognized. Must be one of $(spaces())")
end
end
function clip_to_space(cam::Camera, space::Symbol)
if is_data_space(space)
return inv(cam.projectionview[])
elseif is_pixel_space(space)
w, h = cam.resolution[]
return Mat4f(0.5w, 0, 0, 0, 0, 0.5h, 0, 0, 0, 0, -10_000, 0, 0.5w, 0.5h, 0, 1) # -10_000
elseif is_relative_space(space)
return Mat4f(0.5, 0, 0, 0, 0, 0.5, 0, 0, 0, 0, 1, 0, 0.5, 0.5, 0, 1)
elseif is_clip_space(space)
return Mat4f(I)
else
error("Space $space not recognized. Must be one of $(spaces())")
end
end

It's used for scatter and text. I think it applies after transformations, so it's probably fine to consider :transformed as :data here?

@asinghvi17
Copy link
Member Author

already done :)

yep, the idea was that it's the same projection matrix as :data, but apply_transform is not applied.

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 15, 2023

Oh yea.

Another question is whether this should affect limits. Since it applies before "world" space I'd say it probably should?

@asinghvi17
Copy link
Member Author

asinghvi17 commented Mar 15, 2023

What would happen if the transformation doesn't have an inverse defined?

We have a nice method to deal with bounding box transformations for those transformations which are not linearly separable, but have an inverse defined. So that's there, at least.

I guess we could check applicable(Makie.inverse_transform(trans))? And then just ignore the plot if the inverse transformation is not defined?

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 23, 2023

Is that not a mandatory part of the interface? If not then we have to check like this and potentially ignore.

@asinghvi17
Copy link
Member Author

Unfortunately, it's not :( - at least, not when a plot is nested within a recipe. If you try putting a bracket inside a recipe, the bounding box is computed from the raw points (which are in pixel space)...

We should really overhaul this entire pipeline.
@asinghvi17
Copy link
Member Author

asinghvi17 commented Mar 25, 2023

I tried to patch data_limits to skip all plots whose space is not in data space.
It doesn't seem to work, though.

@recipe(Test1) do scene
       Attributes()
end

# brackets were having bbox issues in recipes earlier, this should ideally look like the docs example
function Makie.plot!(p::Test1)
       bracket!(p, pi/2, 1, 5pi/2, 1, offset = 5, text = "Period length", style = :square)

       bracket!(p, pi/2, 1, pi/2, -1, text = "Amplitude", orientation = :down,
           linestyle = :dash, rotation = 0, align = (:right, :center), textoffset = 4, linewidth = 2, color = :red, textcolor = :red)

       bracket!(p, 2.3, sin(2.3), 4.0, sin(4.0),
           text = "Falling", offset = 10, orientation = :up, color = :purple, textcolor = :purple)

       bracket!(p, Point(5.5, sin(5.5)), Point(7.0, sin(7.0)),
           text = "Rising", offset = 10, orientation = :down, color = :orange, textcolor = :orange,
           fontsize = 30, textoffset = 30, width = 50)
end

fig = Figure()
test1(fig[1. 1])
fig

iTerm2 WBdMrA

Comment on lines 147 to 149
if space != :data
tuple()
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should apply from the outside via exclude

function data_limits(scenelike, exclude=(p)-> false)
bb_ref = Base.RefValue(Rect3f())
foreach_plot(scenelike) do plot
if !exclude(plot)
update_boundingbox!(bb_ref, data_limits(plot))
end
end
return bb_ref[]
end

starting from

function getlimits(la::Axis, dim)
# find all plots that don't have exclusion attributes set
# for this dimension
if !(dim in (1, 2))
error("Dimension $dim not allowed. Only 1 or 2.")
end
function exclude(plot)
# only use plots with autolimits = true
to_value(get(plot, dim == 1 ? :xautolimits : :yautolimits, true)) || return true
# only if they use data coordinates
is_data_space(to_value(get(plot, :space, :data))) || return true
# only use visible plots for limits
return !to_value(get(plot, :visible, true))
end
# get all data limits, minus the excluded plots
boundingbox = Makie.data_limits(la.scene, exclude)
# if there are no bboxes remaining, `nothing` signals that no limits could be determined
Makie.isfinite_rect(boundingbox) || return nothing
# otherwise start with the first box
mini, maxi = minimum(boundingbox), maximum(boundingbox)
return (mini[dim], maxi[dim])
end

Scene should already be fine

Makie.jl/src/scenes.jl

Lines 571 to 583 in ffc31a0

function not_in_data_space(p)
!is_data_space(to_value(get(p, :space, :data)))
end
function center!(scene::Scene, padding=0.01, exclude = not_in_data_space)
bb = boundingbox(scene, exclude)
bb = transformationmatrix(scene)[] * bb
w = widths(bb)
padd = w .* padding
bb = Rect3f(minimum(bb) .- padd, w .+ 2padd)
update_cam!(scene, bb)
scene
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually axis should be fine as is too, since it has is_data_space(to_value(get(plot, :space, :data))) || return true .

@ffreyer ffreyer mentioned this pull request Jun 14, 2023
@ffreyer
Copy link
Collaborator

ffreyer commented Jun 14, 2023

fig, ax, plt = lines(1:10, 1:10; axis = (; xscale = log10))
lines!(ax, LinRange(0, 10, 10), 1:10; space = :transformed)

This produces
Screenshot from 2023-06-14 21-22-07
because the 10^1.0 is 1.0 in world space, not 10. To get the result from the initial post you need LinRange(0, 1, 10).

@ffreyer
Copy link
Collaborator

ffreyer commented Jun 14, 2023

I tried to patch data_limits to skip all plots whose space is not in data space. It doesn't seem to work, though.

@recipe(Test1) do scene
       Attributes()
end

# brackets were having bbox issues in recipes earlier, this should ideally look like the docs example
function Makie.plot!(p::Test1)
       bracket!(p, pi/2, 1, 5pi/2, 1, offset = 5, text = "Period length", style = :square)

       bracket!(p, pi/2, 1, pi/2, -1, text = "Amplitude", orientation = :down,
           linestyle = :dash, rotation = 0, align = (:right, :center), textoffset = 4, linewidth = 2, color = :red, textcolor = :red)

       bracket!(p, 2.3, sin(2.3), 4.0, sin(4.0),
           text = "Falling", offset = 10, orientation = :up, color = :purple, textcolor = :purple)

       bracket!(p, Point(5.5, sin(5.5)), Point(7.0, sin(7.0)),
           text = "Rising", offset = 10, orientation = :down, color = :orange, textcolor = :orange,
           fontsize = 30, textoffset = 30, width = 50)
end

fig = Figure()
test1(fig[1. 1])
fig

iTerm2 WBdMrA

I would consider this a bounding box/limits problem. test! generates no space attribute and most places that need it call get(plot, :space, :data). Axis limits do that too, so it assumes everything here to be in data space

@ffreyer ffreyer mentioned this pull request Jul 2, 2023
20 tasks
@asinghvi17
Copy link
Member Author

Hey @SimonDanisch, could we add this to v0.21 as well potentially?

@asinghvi17 asinghvi17 closed this May 30, 2024
@asinghvi17 asinghvi17 reopened this May 30, 2024
@asinghvi17
Copy link
Member Author

This PR should still work if merged, it's pretty light on changes :D. Could we get this in soon?

@asinghvi17 asinghvi17 requested a review from jkrumbiegel May 30, 2024 13:35
@ffreyer
Copy link
Collaborator

ffreyer commented May 30, 2024

Are Axis limits and Float64 conversion working fine with this after 0.21?

@asinghvi17
Copy link
Member Author

Will have to check that - might just open a new PR then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants