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 projection space option as a generic attribute #1596

Merged
merged 44 commits into from Mar 4, 2022
Merged

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jan 15, 2022

Description

This pr adds space as a general attribute to switch between camera space, pixel space, unit space (0..1) and maybe clip space (-1..1). That way you can adjust the units of the plot without creating another scene or messing with camera matrices.

Currently works in GLMakie:

using GLMakie
fig = Figure()
# ax = LScene(fig[1, 1])
# ax = Axis(fig[1, 1])
ax = Axis3(fig[1, 1])
meshscatter!(ax, Point3f.(1:10, 1:10, 1:10), space = :data, color = :blue)
# scatter!(ax, Point2f.(1:10, 1:10), space = :data, color = :blue)
scatter!(ax, Point2f.(200, 100:100:500), space = :pixel, color = :red)
scatter!(ax, Point2f.(0.6, 0.1:0.1:0.9), space = :relative, color = :green)
scatter!(ax, Point2f.(0.9, -0.9:0.2:0.9), space = :clip, color = :orange)
display(fig)

Screenshot from 2022-01-15 14-46-11

Type of change

Breaking change because the space attribute in text has been renamed to markerspace and space now means something else.

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.
  • Works in GLMakie
  • Works in CairoMakie
  • Works in WGLMakie
  • Works in RPRMakie I don't know how to make it work
  • Works with nested plots
  • Works with mixed markerspace in all combinations (scatter, text)
  • pass space through everything poly - I think for most recipes there isn't really a point in adding space as an attribute

Related issues

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 16, 2022

I think I got all combinations of space and markerspace to work with text and scatter now

fig = Figure(resolution = (700, 700))
ax = Axis(fig[1, 1], width = 600, height = 600)
spaces = (:data, :pixel, :relative, :clip)
xs = [
    [0.1, 0.35, 0.6, 0.85], 
    [0.1, 0.35, 0.6, 0.85] * 600,
    [0.1, 0.35, 0.6, 0.85],
    2 .* [0.1, 0.35, 0.6, 0.85] .- 1
]
for (i, space) in enumerate(spaces)
    for (mspace, j, scale) in zip(spaces, 1:4, (0.02, 12, 0.02, 0.04))
        scatter!(
            ax, Point2f(xs[i][i], xs[i][j]), 
            markersize = 5scale, space = space, markerspace = mspace
        )
        text!(
            ax, "$space\n$mspace", position = Point2f(xs[i][i], xs[i][j]), 
            textsize = scale, space = space, markerspace = mspace,
            align = (:center, :center)
        )
    end
end
xlims!(ax, 0, 1)
ylims!(ax, 0, 1)
fig

Screenshot from 2022-01-17 02-01-31

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 20, 2022

The depth shift test should fail here because I changed some things back to how they were when the test was added:

  1. I removed the axes. It seems like the scenekw=(show_axis = false,) didn't get adjusted with the scene rework.
  2. I added an explicit center!. It seems like that used to run after translate! and scale! before, but not anymore.

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 20, 2022

I checked a bunch of the lower scoring tests. In GLMakie:

  • "Depth Shift" is fine, see above
  • "Streamplot animation" seems fine
  • "image scatter" is a 3D scatter plot with screenspace markers so it's scaling got changed
  • "Text rotation" looks exactly the same as the refimage to me?
  • "3D Contour with 2d countour slices" looks different because it's from before fix fuzzy contours #1559
  • "3D screenspace annotations" hides part of the text inside the scattered spheres which is technically correct but differs from before

I also checked a few of the worst cases in CairoMakie. Most of them are just 3D plots

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 1, 2022

#1632 moves the the preprojection math into shaders in GLMakie and WGLMakie. I kept it separate to make it a bit easier to review (though it ended up being very little work in shaders)

@MakieBot
Copy link
Collaborator

MakieBot commented Mar 1, 2022

Compile Times benchmark

@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 1, 2022

Tests are still fine

* remove overly eager optimizations (#1713)

* small clean ups

* clean up implementation a bit, use only one name for space

* address review
@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 3, 2022

Another TODO:

  • scatter with FastPixel currently doesn't have camera matrices attached in GLMakie

@SimonDanisch
Copy link
Member

SimonDanisch commented Mar 3, 2022

and:

  • fix text offset
    text-off2

@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 3, 2022

I checked a couple of rectangles in the example above. Seems like it's generally a bit better centered after the pr. Before the offsets of the centers of text bounding boxes to centers of heatmap rectangles at the corners are

(1, 0.5) ... (0.5, 0.5)
   :              :
(1, 1.5) ... (0.5, 1.5)

pixel and after it's (-0.5, +0.5) with some rectangle inside being (-0.5, +0.5). I'd say it's better now.

@SimonDanisch
Copy link
Member

Ok, I fixed the FastPixel marker. I tried uploading the new reference images, but it currently doesnt work :(
I'll merge this nevertheless, to finally be able to merge all these other PRs that have merge conflict with this one. We can update the reference images later.

@SimonDanisch SimonDanisch merged commit 432b8ab into master Mar 4, 2022
@SimonDanisch SimonDanisch deleted the ff/transform branch March 4, 2022 09:22
@ffreyer ffreyer mentioned this pull request Mar 6, 2022
8 tasks
@simonbyrne
Copy link
Contributor

This appears to have broken GeoMakie:

julia> fig = Figure()

julia> ax = GeoAxis(fig[1,1]; dest = "+proj=wintri")
ERROR: Space screen not recognized. Must be one of (:data, :pixel, :relative, :clip)
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:33
  [2] space_to_clip
    @ ~/.julia/dev/Makie/src/camera/projection_math.jl:324 [inlined]
  [3] space_to_clip
    @ ~/.julia/dev/Makie/src/camera/projection_math.jl:315 [inlined]
  [4] (::GLMakie.var"#143#154"{Camera})(s::Symbol, ms::Symbol, pv::StaticArrays.SMatrix{4, 4, Float32, 16}, res::Vec{2, Float32})
    @ GLMakie ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:384
  [5] map(::GLMakie.var"#143#154"{Camera}, ::Observable{Symbol}, ::Observable{Symbol}, ::Vararg{Any}; kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Observables ~/.julia/packages/Observables/OFj0u/src/Observables.jl:477
  [6] map(::GLMakie.var"#143#154"{Camera}, ::Observable{Symbol}, ::Observable{Symbol}, ::Observable{StaticArrays.SMatrix{4, 4, Float32, 16}}, ::Vararg{Any})
    @ Observables ~/.julia/packages/Observables/OFj0u/src/Observables.jl:466
  [7] (::GLMakie.var"#133#144"{Scene, MakieCore.Text{Tuple{Makie.GlyphCollection}}})(gl_attributes::Dict{Symbol, Any})
    @ GLMakie ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:383
  [8] (::GLMakie.var"#103#106"{GLMakie.var"#133#144"{Scene, MakieCore.Text{Tuple{Makie.GlyphCollection}}}, GLMakie.Screen, Scene, MakieCore.Text{Tuple{Makie.GlyphCollection}}})()
    @ GLMakie ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:107
  [9] get!(default::GLMakie.var"#103#106"{GLMakie.var"#133#144"{Scene, MakieCore.Text{Tuple{Makie.GlyphCollection}}}, GLMakie.Screen, Scene, MakieCore.Text{Tuple{Makie.GlyphCollection}}}, h::Dict{UInt64, GLMakie.GLAbstraction.RenderObject}, key::UInt64)
    @ Base ./dict.jl:464
 [10] cached_robj!(robj_func::GLMakie.var"#133#144"{Scene, MakieCore.Text{Tuple{Makie.GlyphCollection}}}, screen::GLMakie.Screen, scene::Scene, x::MakieCore.Text{Tuple{Makie.GlyphCollection}})
    @ GLMakie ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:85
 [11] draw_atomic
    @ ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:304 [inlined]
 [12] insert!
    @ ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:121 [inlined]
 [13] (::GLMakie.var"#109#110"{GLMakie.Screen, Scene})(x::MakieCore.Text{Tuple{Makie.GlyphCollection}})
    @ GLMakie ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:126
 [14] foreach(f::GLMakie.var"#109#110"{GLMakie.Screen, Scene}, itr::Vector{AbstractPlot})
    @ Base ./abstractarray.jl:2694
 [15] insert!(screen::GLMakie.Screen, scene::Scene, x::Combined)
    @ GLMakie ~/.julia/dev/Makie/GLMakie/src/drawing_primitives.jl:123
 [16] push!(scene::Scene, plot::MakieCore.Text{Tuple{String}})
    @ Makie ~/.julia/dev/Makie/src/scenes.jl:368
 [17] plot!(scene::Scene, P::Type{MakieCore.Text{Tuple{String}}}, attributes::Attributes, input::Tuple{Observable{Any}}, args::Observable{Tuple{String}})
    @ Makie ~/.julia/dev/Makie/src/interfaces.jl:403
 [18] plot!(scene::Scene, P::Type{MakieCore.Text}, attributes::Attributes, args::Observable{Any}; kw_attributes::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Makie ~/.julia/dev/Makie/src/interfaces.jl:320
 [19] plot!
    @ ~/.julia/dev/Makie/src/interfaces.jl:288 [inlined]
 [20] #plot!#145
    @ ~/.julia/dev/Makie/src/interfaces.jl:271 [inlined]
 [21] text!(::Scene, ::Vararg{Any}; attributes::Base.Pairs{Symbol, Any, NTuple{9, Symbol}, NamedTuple{(:textsize, :color, :position, :visible, :align, :rotation, :font, :markerspace, :inspectable), Tuple{Observable{Any}, Observable{Any}, Observable{Point{2, Float32}}, Observable{Any}, Observable{Tuple{Symbol, Symbol}}, Observable{Float32}, Observable{Any}, Symbol, Bool}}})
    @ MakieCore ~/.julia/packages/MakieCore/A0hGm/src/recipes.jl:37
 [22] Makie.MakieLayout.LineAxis(parent::Scene; kwargs::Base.Pairs{Symbol, Observable, NTuple{36, Symbol}, NamedTuple{(:endpoints, :limits, :flipped, :ticklabelrotation, :ticklabelalign, :labelsize, :labelpadding, :ticklabelpad, :labelvisible, :label, :labelfont, :ticklabelfont, :ticklabelcolor, :labelcolor, :tickalign, :ticklabelspace, :ticks, :tickformat, :ticklabelsvisible, :ticksvisible, :spinevisible, :spinecolor, :spinewidth, :ticklabelsize, :trimspine, :ticksize, :reversed, :tickwidth, :tickcolor, :minorticksvisible, :minortickalign, :minorticksize, :minortickwidth, :minortickcolor, :minorticks, :scale), Tuple{Observable{Tuple{Point{2, Float32}, Point{2, Float32}}}, Observable{Tuple{Float32, Float32}}, Observable{Bool}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Bool}, Observable{Symbol}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}, Observable{Any}}}})
    @ Makie.MakieLayout ~/.julia/dev/Makie/src/makielayout/lineaxis.jl:174
 [23] layoutable(::Type{Axis}, fig_or_scene::Figure; bbox::Nothing, kwargs::Base.Pairs{Symbol, Any, NTuple{4, Symbol}, NamedTuple{(:aspect, :xticks, :yticks, :limits), Tuple{DataAspect, Tuple{Vector{Float32}, Vector{String}}, Tuple{Vector{Float32}, Vector{String}}, Tuple{Tuple{Int64, Int64}, Tuple{Int64, Int64}}}}})
    @ Makie.MakieLayout ~/.julia/dev/Makie/src/makielayout/layoutables/axis.jl:208
 [24] #_layoutable#11
    @ ~/.julia/dev/Makie/src/makielayout/layoutables.jl:69 [inlined]
 [25] _layoutable(::Type{Axis}, ::GridPosition; kwargs::Base.Pairs{Symbol, Any, NTuple{4, Symbol}, NamedTuple{(:aspect, :xticks, :yticks, :limits), Tuple{DataAspect, Tuple{Vector{Float32}, Vector{String}}, Tuple{Vector{Float32}, Vector{String}}, Tuple{Tuple{Int64, Int64}, Tuple{Int64, Int64}}}}})
    @ Makie.MakieLayout ~/.julia/dev/Makie/src/makielayout/layoutables.jl:64
 [26] #_#9
    @ ~/.julia/dev/Makie/src/makielayout/layoutables.jl:49 [inlined]
 [27] GeoAxis(args::GridPosition; source::String, dest::String, transformation::Proj4.Transformation, lonticks::StepRange{Int64, Int64}, latticks::StepRange{Int64, Int64}, hidespines::Bool, coastlines::Bool, coastline_attributes::NamedTuple{(), Tuple{}}, aspect::DataAspect, kw::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ GeoMakie ~/.julia/dev/GeoMakie/src/geoaxis.jl:74
 [28] top-level scope
    @ REPL[7]:1

@simonbyrne
Copy link
Contributor

Ah, nevermind, I hadn't dev-ed MakieCore.

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