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

Generalize GR tick label rotations #3782

Merged
merged 29 commits into from
Oct 5, 2021

Conversation

BioTurboNick
Copy link
Member

@BioTurboNick BioTurboNick commented Aug 29, 2021

Resolves #3776

PR for reference images: JuliaPlots/PlotReferenceImages.jl#109

@BioTurboNick
Copy link
Member Author

I think I have the offsets working well, let me know what you think.

@t-bltg
Copy link
Member

t-bltg commented Sep 18, 2021

I think I have the offsets working well, let me know what you think.

I have a few simplifications to propose, do you mind if I push the changes here after your commits ?
I don't want to mess things up 😉.

@BioTurboNick
Copy link
Member Author

Sure!

@t-bltg
Copy link
Member

t-bltg commented Sep 18, 2021

Sure!

So I've simplified based on the assumption that the expected for y axis is what we do for x axis, rotated by 90°.

Also, I've removed the offsets (sorry about the time you spent on this), based on the look of ref29 which now looks like:

To me the 100 label is still to much pushed to the left:
bug-rot

, compared to the previous img http://docs.juliaplots.org/latest/generated/gr/#gr-ref29.

I you disagree on anything, I'd be happy to revert 😃.

@BioTurboNick
Copy link
Member Author

Yeah, longer labels aren't adjusted the same, I'm seeing. Might be worth adding a graph with a long tick label to the test suite, like a DateTime, which would pick up on that.

Looking into the issue.

@t-bltg
Copy link
Member

t-bltg commented Sep 18, 2021

Yeah, longer labels aren't adjusted the same, I'm seeing. Might be worth adding a graph with a long tick label to the test suite, like a DateTime, which would pick up on that.

Looking into the issue.

To me, the rotated mwe and ref29 look OK without the offsets.

@BioTurboNick
Copy link
Member Author

You convinced me it looks better with an offset haha. I think I have it working, and this generic positioning code allows the angle-based alignment code to be removed.

Only issue is the 3d plots' ticks aren't adjusted correctly anymore.

Copy link
Member

@isentropic isentropic left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I'm not that proficient in GR

@t-bltg
Copy link
Member

t-bltg commented Sep 24, 2021

Pros:

Cons:

  • Pardon me for saying that, but the new code is a mess 😆 .

@BioTurboNick
Copy link
Member Author

Oh it's definitely a mess.

I can try to simplify it, since the output is good.

@BioTurboNick
Copy link
Member Author

I'm within inches of a much better positioning logic for especially 3d plots, but I keep tripping up on all the angles involved. I'll come back to this.

@BioTurboNick
Copy link
Member Author

Well, I don't know if it is that much simpler to look at..., but the 3d plots positioning is now perfect IMO.

@BeastyBlacksmith
Copy link
Member

@BioTurboNick can you make a PR with the most recent images?

@BioTurboNick
Copy link
Member Author

It doesn't seem like there are any changes vs. the last one.

@BioTurboNick
Copy link
Member Author

Oh I see, very tiny differences...

@BioTurboNick
Copy link
Member Author

Okay I'm not sure what's going on. I keep seeing no difference on my end.

@BeastyBlacksmith
Copy link
Member

@daschw, @t-bltg can you have a look if you can reproduce on your machine?

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #3782 (ed9ed26) into master (a65cda8) will decrease coverage by 0.36%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3782      +/-   ##
==========================================
- Coverage   63.20%   62.84%   -0.37%     
==========================================
  Files          28       28              
  Lines        7491     7200     -291     
==========================================
- Hits         4735     4525     -210     
+ Misses       2756     2675      -81     
Impacted Files Coverage Δ
src/recipes.jl 58.61% <60.00%> (-0.89%) ⬇️
src/colorbars.jl 90.19% <83.33%> (-0.55%) ⬇️
src/backends/gr.jl 88.85% <97.95%> (-0.41%) ⬇️
src/backends/unicodeplots.jl 74.13% <0.00%> (-1.26%) ⬇️
src/backends/pgfplotsx.jl 60.25% <0.00%> (-0.92%) ⬇️
src/args.jl 72.61% <0.00%> (-0.86%) ⬇️
src/output.jl 28.09% <0.00%> (-0.59%) ⬇️
src/pipeline.jl 90.09% <0.00%> (-0.55%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d56e72...ed9ed26. Read the comment docs.

@t-bltg
Copy link
Member

t-bltg commented Oct 5, 2021

1 passes, but I still don't know why ref40 does not pass on nightly. Oddly seems related to #3860.

@BeastyBlacksmith
Copy link
Member

Thanks. Then we take it as is for now and need to look into 40 on nightly in another PR

@BeastyBlacksmith BeastyBlacksmith merged commit aca2aa4 into JuliaPlots:master Oct 5, 2021
@BioTurboNick
Copy link
Member Author

Thanks all!

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

Successfully merging this pull request may close these issues.

[BUG] Recent update causes angled axis labels to be mis-positioned
4 participants