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

LineAxis: add arbitrary {x,y}labelrotation #2478

Merged
merged 20 commits into from Dec 21, 2022
Merged

Conversation

t-bltg
Copy link
Collaborator

@t-bltg t-bltg commented Dec 8, 2022

Fix #1187.
Fix #2222.

Description

As discussed with @jkrumbiegel on discord, add the ability to rotate axis labels:

julia> using CairoMakie
julia> fap = lines(0..2π, sin; axis = (; xlabel = "x", ylabel = "y", ylabelrotation = 0))
julia> save("sin.png", fap)

sin

Any comments on adding docs about this (where should this go ?) or tests (do you want static tests or new ref images maybe ?) would be appreciated.

TODO:

  • handle alignements

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.

@t-bltg t-bltg mentioned this pull request Dec 8, 2022
@jkrumbiegel
Copy link
Collaborator

A good reference test would have two steps

@fatteneder
Copy link
Contributor

Closes #2222

@jkrumbiegel
Copy link
Collaborator

And it seems the LineAxis attributes also need a labelrotation default if I look at the failing Makie CI. Maybe you were talking about that on discord and I misunderstood what you meant.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

Yeah, I think I need to add a labelrotation to the @Block Colorbar.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

For the alignements, I've reused (and renamed) the calculate_real_ticklabel_align routine, but it was implemented for discrete rotation values, so it's not very suited to arbitrary rotation values.

Would it be acceptable to use sin(rot) and cos(rot) instead ?

@jkrumbiegel
Copy link
Collaborator

Those functions are just meant for ticklabels which have to be aligned sensibly relative to the tick marks, and this behavior should be reasonably automatic given the rotation angle.

For the axis labels, an alignment of (:center, :center) should always work, only the distance from the axis must be computed accordingly (using the text's boundingbox).

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

So I've got this simple example.

julia> N = 18  # number of rotations
julia> fig, ax, _ = scatter(0:1; axis = (; xlabel = "a nice and long x label for this axis", ylabel = "a nice and long y label for this axis"))
julia> record(fig, "rot.gif") do io
        for rot in LinRange(0, 2π, N)
            ax.xlabelrotation[] = rot
            ax.ylabelrotation[] = rot
            recordframe!(io)
        end
    end

rot

Regarding the test I have a few questions:

  • what is a Stepper and how is it used in the tests ? why a single Makie.step!(st) ?
  • what do we really want in terms of alignment ?

@t-bltg t-bltg changed the title LineAxis: add arbitrary {x,y} rotation LineAxis: add arbitrary {x,y} label rotation Dec 8, 2022
@t-bltg t-bltg changed the title LineAxis: add arbitrary {x,y} label rotation LineAxis: add arbitrary {x,y}labelrotation Dec 8, 2022
@jkrumbiegel
Copy link
Collaborator

jkrumbiegel commented Dec 8, 2022

Ok you can see in your example how the layout does not react to the different space taken up by the rotating labels. I would make the alignment behave like Label does when you set rotation on it, treat the whole thing like it takes up a rectangular box around the center of which it rotates. That's why I said align = (:center, :center) and that the anchor position has to be adjusted given the boundingbox width (for y label) or height (for x label).

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

How do I draw text bounding boxes again ?
I have an earlier snippet here but it does not work in this case:

foreach(Any, fig.layout) do l
  wireframe!(fig.scene, l.layoutobservables.suggestedbbox[])
end

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

If the alignment is set to (:center, :center), where is the code that deals with anchoring ?

@jkrumbiegel
Copy link
Collaborator

jkrumbiegel commented Dec 8, 2022

By anchor I just mean the point at which the text is plotted. This is not the same anymore as it was before your PR, it must be shifted outwards by half the boundingbox.

And I think it's something like wireframe!(scene, boundingbox(textobject))

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

How can I access the axis decorations (for accessing labeltext) ? I mean the decorations dict is filled, but doesn't seem stored somewhere.

@jkrumbiegel
Copy link
Collaborator

Could be that this dict is just a remnant at this point, do you need it? The LineAxis just needs to communicate its protrusion value outwards, so at least the Axis shouldn't need access. Or is this just for debugging?

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

I though so:

decorations = Dict{Symbol, Any}()
, just for debugging bounding boxes. I will move forward without it then.

@jkrumbiegel
Copy link
Collaborator

I was mostly using it for debugging as well at the time, must have dropped out of use without me noticing.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

I doesn't seem possible to know the textlabel boundingbox beforehand right ?
I think I have to first create the textlabel, and then shift it according to the given rotation ?

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 8, 2022

Probably more effort needed (how can we trigger viewport update ? notify(...what ?...), but it starts to work:

rot

EDIT: how can I lower the gif fps using record ? ==> more frames for now.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 11, 2022

I've minimized the changes and removed unrelated modifications, but the WGLMakie ci error still persists.
The error is related to the Colorbar block, but I fail to see the link with the camera.

I am a bit clueless here ...

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 11, 2022

I tried to reproduce locally using WGLMakie, and got a slightly different error, but still occurring when setting the Colorbar :

using JSServe, Markdown, WGLMakie
Page(exportable=true, offline=true)
WGLMakie.activate!()
fig, ax, plot = scatter(0:1)
Colorbar(fig[1, 2]; label = "vertical cbar")
display(fig)
ERROR: TypeError: scene is undefined
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] evaljs_value(session::Session, js::JSServe.JSCode; error_on_closed::Bool, time_out::Float64)
    @ JSServe ~/.julia/packages/JSServe/kIK9q/src/session.jl:238
  [3] evaljs_value
    @ ~/.julia/packages/JSServe/kIK9q/src/session.jl:206 [inlined]
  [4] insert!(td::WGLMakie.ThreeDisplay, scene::Scene, plot::Poly{Tuple{Vector{GeometryBasics.HyperRectangle{2, Float32}}}})
    @ WGLMakie ~/.julia/packages/WGLMakie/L5URO/src/three_plot.jl:24
  [5] insert!(td::WGLMakie.Screen, scene::Scene, plot::Poly{Tuple{Vector{GeometryBasics.HyperRectangle{2, Float32}}}})
    @ WGLMakie ~/.julia/packages/WGLMakie/L5URO/src/display.jl:173
  [6] push!(scene::Scene, plot::Poly{Tuple{Vector{GeometryBasics.HyperRectangle{2, Float32}}}})
    @ Makie [...]/Makie.jl/src/scenes.jl:409
  [7] plot!(scene::Scene, P::Type{Poly{Tuple{Vector{GeometryBasics.HyperRectangle{2, Float32}}}}}, attributes::Attributes, input::Tuple{Observable{Vector{GeometryBasics.HyperRectangle{2, Float32}}}}, args::Observable{Tuple{Vector{GeometryBasics.HyperRectangle{2, Float32}}}})
    @ Makie [...]/Makie.jl/src/interfaces.jl:422
  [8] plot!(scene::Scene, P::Type{Poly}, attributes::Attributes, args::Observable{Vector{GeometryBasics.HyperRectangle{2, Float32}}}; kw_attributes::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Makie [...]/Makie.jl/src/interfaces.jl:336
  [9] plot!
    @ [...]/Makie.jl/src/interfaces.jl:303 [inlined]
 [10] #plot!#165
    @ [...]/Makie.jl/src/interfaces.jl:287 [inlined]
 [11] poly!(::Scene, ::Vararg{Any}; attributes::Base.Pairs{Symbol, Any, Tuple{Symbol, Symbol, Symbol}, NamedTuple{(:color, :visible, :inspectable), Tuple{Observable{Vector{ColorTypes.RGBA{Float64}}}, Observable{Bool}, Bool}}})
    @ MakieCore [...]/Makie.jl/MakieCore/src/recipes.jl:38
 [12] initialize_block!(cb::Colorbar)
    @ Makie [...]/Makie.jl/src/makielayout/blocks/colorbar.jl:197
 [13] _block(::Type{Colorbar}, ::Figure; bbox::Nothing, kwargs::Base.Pairs{Symbol, Any, Tuple{Symbol, Symbol}, NamedTuple{(:label, :labelrotation), Tuple{String, Int64}}})
    @ Makie [...]/Makie.jl/src/makielayout/blocks.jl:408
 [14] _block(::Type{Colorbar}, ::GridPosition; kwargs::Base.Pairs{Symbol, Any, Tuple{Symbol, Symbol}, NamedTuple{(:label, :labelrotation), Tuple{String, Int64}}})
    @ Makie [...]/Makie.jl/src/makielayout/blocks.jl:294
 [15] #_#1077
    @ [...]/Makie.jl/src/makielayout/blocks.jl:279 [inlined]
 [16] top-level scope
    @ REPL[21]:1
 [17] eval
    @ ./boot.jl:368 [inlined]
 [18] eval_user_input(ast::Any, backend::REPL.REPLBackend)
    @ REPL ~/share/julia/stdlib/v1.8/REPL/src/REPL.jl:151
 [19] repl_backend_loop(backend::REPL.REPLBackend)
    @ REPL ~/share/julia/stdlib/v1.8/REPL/src/REPL.jl:247
 [20] start_repl_backend(backend::REPL.REPLBackend, consumer::Any)
    @ REPL ~/share/julia/stdlib/v1.8/REPL/src/REPL.jl:232
 [21] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool)
    @ REPL ~/share/julia/stdlib/v1.8/REPL/src/REPL.jl:369
 [22] run_repl(repl::REPL.AbstractREPL, consumer::Any)
    @ REPL ~/share/julia/stdlib/v1.8/REPL/src/REPL.jl:355
 [23] (::Base.var"#967#969"{Bool, Bool, Bool})(REPL::Module)
    @ Base ./client.jl:419
 [24] #invokelatest#2
    @ ./essentials.jl:729 [inlined]
 [25] invokelatest
    @ ./essentials.jl:726 [inlined]
 [26] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool)
    @ Base ./client.jl:404
 [27] top-level scope
    @ none:40

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 12, 2022

I've tried this PR against #2428, same error.
The previous example with the colorbar also fails in WGLMakie on the latest Makie release.
I'm going to open a new issue.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 15, 2022

Dropping a note to say that this PR is ready, and only needs new reference images merged.

@jkrumbiegel jkrumbiegel reopened this Dec 21, 2022
@jkrumbiegel jkrumbiegel merged commit f124a01 into MakieOrg:master Dec 21, 2022
@jkrumbiegel
Copy link
Collaborator

Thank you, @t-bltg!

@t-bltg t-bltg deleted the rot branch December 21, 2022 10:00
@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 21, 2022

Thanks for the help and the constructive comments made on this PR, I couldn't have done without (or would have written sloppy code for sure :)) !

@github-actions
Copy link
Contributor

Missing reference images

Found 1 new images without existing references.
Upload new reference images before merging this PR.

@jkrumbiegel jkrumbiegel mentioned this pull request Dec 27, 2022
t-bltg added a commit to t-bltg/Makie.jl that referenced this pull request Dec 31, 2022
* `LineAxis`: add arbitrary `{x,y}` rotation

* add news entry

* handle alignements

* start adding tests

* fix attribute

* update position

* revert un-necessary rename

* trigger `calculate_protrusion` on `labelrotation`

* add comments

* fix flip

* add tests

* restore `labelalign`

* enhance tests using colorbars

* cleanup temporary file

* temporarily revert stylistic changes and type asserts

* do not use  `@lift` for `ignore_equal_values` at the expense of readability

* split test as `WGLMakie` bug workaround

* update changelog

* minor simplifications - readability

Co-authored-by: Julius Krumbiegel <22495855+jkrumbiegel@users.noreply.github.com>
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.

Rotating the title of the y-axis? ylabelrotation/rotate_ylabel for Axis
3 participants