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 kwarg to rotate Toggle #4471

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

Conversation

bjarthur
Copy link
Contributor

@bjarthur bjarthur commented Oct 10, 2024

Description

fixes #4378; supersedes #4445.

adds orientation kwarg to Toggle that specifies the angle in radians to rotate the toggle. default of 0 is horizontal with active being to the right.

Type of change

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

Checklist

  • Added an entry in CHANGELOG.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.

@bjarthur
Copy link
Contributor Author

Screenshot 2024-10-10 at 5 55 32 PM

@bjarthur bjarthur marked this pull request as ready for review October 11, 2024 16:32
Comment on lines 50 to 53
R = Makie.rotationmatrix_z(angle_rad)
T = Makie.translationmatrix(-center)
Tinv = Makie.translationmatrix(center)
topscene.transformation.model[] = Tinv * R * T
Copy link
Collaborator

Choose a reason for hiding this comment

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

It probably makes sense to wait for #4472 and replace this with

Suggested change
R = Makie.rotationmatrix_z(angle_rad)
T = Makie.translationmatrix(-center)
Tinv = Makie.translationmatrix(center)
topscene.transformation.model[] = Tinv * R * T
translate_origin!(topscene, center)
rotate!(topscene, angle_rad)

once its merged.

@bjarthur
Copy link
Contributor Author

bjarthur commented Oct 31, 2024

i just noticed that the vertical alignment is off when rotated:

Screenshot 2024-10-31 at 4 09 19 PM

i would've thought the top edges of both toggles should be at the same height. can someone please suggest how i can fix this?

julia> using GLMakie

julia> fig = Figure()

julia> scatter(fig[1,1], [1,2,3], axis = (; height=200))
Makie.AxisPlot(Axis (1 plots), Scatter{Tuple{Vector{Point{2, Float64}}}})

julia> Toggle(fig[1,2], orientation=:vertical, valign=:top)
Toggle()

julia> Toggle(fig[1,3], orientation=:horizontal, valign=:top)
Toggle()

@bjarthur
Copy link
Contributor Author

bjarthur commented Nov 4, 2024

i've fixed the alignment issue in the previous comment by refactoring to do my own rotation instead of using @ffreyer 's method. i couldn't figure out another way to do it.

everything seems to work, and i suspect tests will pass, but only because none of the reference images uses the width or height kwargs to Toggle.

there is a slight backwards incompatibility that might warrant a bump in Makie's minor version with this PR as the size of the toggle is now controlled by length and markersize instead of width and height. with the new orientation kwarg i feel this is a much more natural parameterization. let me know if you disagree.

@bjarthur
Copy link
Contributor Author

bjarthur commented Nov 5, 2024

i'm confused about the CI failure for ref img "Button - Slider - Toggle - Textbox":

running 241: Button - Slider - Toggle - Textbox
Button - Slider - Toggle - Textbox: Error During Test at /home/runner/work/Makie.jl/Makie.jl/ReferenceTests/src/database.jl:51
  Test threw exception
  Expression: save_result(joinpath(RECORDING_DIR[], "Button - Slider - Toggle - Textbox"), result)
  No applicable_savers found for UNKNOWN

it works fine locally and nothing in this PR should effect that test.

@bjarthur
Copy link
Contributor Author

could you please review again?

@ffreyer
Copy link
Collaborator

ffreyer commented Nov 22, 2024

Tests fail because I merged the new refimg with an existing one. So no problem there.

I'm not sure about the (width, height) -> (length, markersize) change. I see why it's there but I wonder if width, height is supposed to be something users can always set. Right now it gets overwritten if I'm not mistaken.

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

Successfully merging this pull request may close these issues.

vertical toggle
3 participants