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.

Sign up for GitHub

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

Jump to bottom

Continuation of polar axis #2990

Merged
merged 72 commits into from Jul 18, 2023
Merged

Continuation of polar axis #2990

merged 72 commits into from Jul 18, 2023

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented May 30, 2023 •
edited

Description

Continuation of #2014

I rewrote the axis decorations to use a polar transformedscene to avoid most of the explicit polar space -> pixel space transformation. I'm not sure if this is something we want in the end so I'm using my own branch for this.

TODO:

  • add polar space overlay scene for axis
  • convert axis decorations to polar space
  • camera/interactivity:
    • add axis preserving zoom
    • add reset key
    • maybe add rotation of axis
    • maybe add translation of everything (with modifier key)
    • maybe add zoom of everything (with modifier key, restricted zoom out)
    • maybe move code to a camera controller
  • allow non-square scenes
  • figure out label padding
  • reserve space for labels
  • get clip working again
  • replace $\theta$ (currently theta instead)
  • maybe add interaction interface
  • consistently track of observables/observerfunctions (i.e. on(f, scene, obs...))
  • rename PolarAxisTransform to Polar and move it to transformation.jl

Reminders on cleanup/visual improvements:

  • cleanup debug colors, comments, dead code
  • add stroke to r-ticks for visibility
  • hide minor grid by default
  • reduce frequency of theta ticks
  • switch radians to degrees

Type of change

  • 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.

jkrumbiegel and others added 30 commits June 28, 2022 18:26
@jkrumbiegel @asinghvi17
start implementation
ba2318a
@asinghvi17
First implementation of polar axis
ad8900c 
The theme should be mostly set, grids exist but ticks need to be plotted.
@asinghvi17
Try to make the polar axis "plottable-to"
9f662fb
@asinghvi17
Revert MakieLayout.jl to master state
828c9a4
@asinghvi17
Correct documentation of adjustcam!
be9302b
@asinghvi17
Implement angular ticks
8a1d0a6
@asinghvi17
Inherit a bit more
9f2c0a3
@asinghvi17
Implement radial ticks
8c2dc4b
@asinghvi17
Fix translation for GLMakie
3d0f20c
@asinghvi17
In Camera2D's ratio check, use only Float32 to compare
eff6180
@asinghvi17
Fix bad bbox method, add comments
603f706
@asinghvi17
Remove debug statements
25dca9a
@asinghvi17
Update for MakieLayout module removal
4fd1d5d
@asinghvi17
Update the lines when the camera changes.
5539a5a
@asinghvi17
Lift on camera also
0e4292d
@asinghvi17
Add protrusion calculation for theta ticks
1d133a1
@asinghvi17
Allow plot!(po::PolarAxis, ...)
0ea9644
@asinghvi17
Manual protrusion calc by checking text bboxes individually.
e9ba2e4
@asinghvi17
Implement a foundation for hacked theta-lims
227b0b5
@asinghvi17
Fix update_cam!(::Scene)
e662ee6
@asinghvi17
Quick fix for overlooked item
aa0bb82
@asinghvi17
Implement text_bbox(plot::Text)
08d7a91 
This reduces the amount of attribute expansions in the main code.
@asinghvi17
Implement inherit for nested theme attributes
8fb816f 
This allows plots to inherit from nested attrs in the scene's theme, 
i.e., a plot can inherit `scene.Axis.yticklabelsvisible` if it exists.

This is arbitrary and robust, following the same logic (check if exists 
in current scene, if not go to parent, if not return default value) as 
the implementation for a single Symbol did.
@asinghvi17
Implement title for PolarAxis
44eb5ba
@asinghvi17
Switch default theme to inheriting from Axis
e2a3582
@asinghvi17
Separate r and theta minor grids in theme
3dba936
@asinghvi17
Update NEWS
bfc2c1d
@asinghvi17
General cleanup
f2c90c8
@asinghvi17
Adjust ticks, grid and camera on scene area / limit change
60ee94a
@asinghvi17
Change r-tick angle to avoid overlap
9432e1f
@ffreyer
Copy link
Collaborator Author

ffreyer commented Jul 15, 2023

I added an blocks example page for PolarAxis (do I need to add it in some list to include it?), some refimg tests, some tests for the polar transform and fixed a bunch of smaller issues I found. The ref images I added are also in the documentation:

Screenshot from 2023-07-15 17-16-49

Screenshot from 2023-07-15 17-16-57
^ This may fail in WGLMakie due to dashed lines

Screenshot from 2023-07-15 17-17-35
^ This may fail in CairoMakie due to different text stroke handling (GLMakie expands, CairoMakie doesn't) and in WGLMakie due to missing stroke.

@github-actions
Copy link
Contributor

Missing reference images

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

@ffreyer ffreyer marked this pull request as ready for review July 15, 2023 16:56
@ffreyer
Copy link
Collaborator Author

ffreyer commented Jul 17, 2023

I added outline options for theta ticks since they are already there for r ticks. Adjusted refimg:
Screenshot from 2023-07-17 16-29-11

@github-actions
Copy link
Contributor

Missing reference images

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

SimonDanisch
SimonDanisch reviewed Jul 17, 2023
View reviewed changes
src/makielayout/blocks/polaraxis.jl Outdated

allattrs = merge(attributes, Attributes(kw_attributes))

# cycle = get_cycle_for_plottype(allattrs, P)
Copy link
Member

Choose a reason for hiding this comment

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

Ah guess we should try to add those back?

SimonDanisch
SimonDanisch approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@SimonDanisch SimonDanisch left a comment •
edited

Choose a reason for hiding this comment

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

Alright, I added cycler support, so I think this is good to go!

@github-actions
Copy link
Contributor

Missing reference images

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

@jkrumbiegel
Copy link
Collaborator

This doesn't yet play well with themes:

grafik

@jkrumbiegel
Copy link
Collaborator

Other than that, so far I think it looks pretty great!

@SimonDanisch
Copy link
Member

I think we can leave that for later ...

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jul 18, 2023

Fixed the background color, now it's just the spine that's off. But it's kind of weird to inherit that from e.g. just the top spine of an Axis... We should probably just have a PolarAxis section in the theme instead?
Screenshot 2023-07-18 122932

@jkrumbiegel
Copy link
Collaborator

We should probably just have a PolarAxis section in the theme instead

Yes, that sounds good, a polar axis could then be added to the example plot https://docs.makie.org/stable/documentation/theming/predefined_themes/

@jkrumbiegel
Copy link
Collaborator

Ah actually I think the spine color of Axis already inherits linecolor, so that could be done for PolarAxis as well

@SimonDanisch SimonDanisch mentioned this pull request Jul 18, 2023
Merged
@SimonDanisch SimonDanisch merged commit a55197c into master Jul 18, 2023
14 checks passed
@SimonDanisch SimonDanisch deleted the ff/jk-as-polar-axis branch July 18, 2023 14:31
@SimonDanisch SimonDanisch mentioned this pull request Jul 18, 2023
Closed
12 tasks
@juliohm juliohm mentioned this pull request Jul 24, 2023
Closed
8 tasks
@ffreyer ffreyer mentioned this pull request Jul 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonDanisch SimonDanisch SimonDanisch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
PR review
Status: Ready, but unreviewed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ffreyer @MakieBot @lazarusA @jkrumbiegel @SimonDanisch @asinghvi17 @Moelf