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 lineend and linejoin params to geom_rect() and geom_tile() #3050

Merged
merged 18 commits into from May 1, 2019

Conversation

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Dec 27, 2018

Fix #3037

Just as #2132 for geom_segment(), this PR changes

  1. geom_rect()'s default of linejoin from "butt" to "square", and
  2. makes it configurable via linejoin parameter by users.
  3. draw_key_polygon() is changed as well.

In addition, as a workaround for Windows, this PR

  1. specifies the lineend parameter.

Note that I expect this breaks many visual tests as geom_rect() and draw_key_polygon() are used in many places.

library(ggplot2)


l <- function(linejoin) {
  geom_tile(aes(x = !!linejoin, y = 0), fill = NA, size = 5, width = 0.7, height = 0.7, linejoin = linejoin, colour = "black")
}

ggplot() +
  l("mitre") +
  l("round") +
  l("bevel") +
  ylim(-1, 1)

Created on 2019-05-01 by the reprex package (v0.2.1)

@ptoche
Copy link

@ptoche ptoche commented Jan 19, 2019

I'm new to this, so forgive me if I'm talking nonsense, but do you need to update the legend key as well?

draw_key_rect <- function(data, params, size) {

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Jan 19, 2019

Thanks, you are right!

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 11, 2019

@yutannihilation can you have a look at this again and see if it is ready for review? I think exposing a lineend argument is weird in a graphic element that is always closed. Can we handle it under the hood based on the linejoin argument?

@thomasp85 thomasp85 added this to the ggplot2 3.2.0 milestone Apr 11, 2019
@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Apr 13, 2019

I think exposing a lineend argument is weird in a graphic element that is always closed.

Yeah, I fixed the implementation to use linejoin only (sorry for not updating the description of this PR). But, I'm stuck in confusion about these two points:

  1. Why do some visual tests fail with the open form, which polygonGrob() expects? (c.f. #3037 (comment))
  2. Should we specify linend = "square" fixedly as a workaround for Windows' bug (c.f. #3037 (comment))? As geom_rect() is one of the fundamental geoms, I expect this will break many existing visual tests, but I'm not sure if this is worth.

Let me think a bit if I can finish this...

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 23, 2019

  1. is probably as you mention that closed form is needed for correct munching - I didn't anticipate this when making the initial suggestion

  2. I think lineend should simply follow the linejoin parameter and not be user-accessible... linejoin = 'round' -> lineend = 'round', everything else lineend = 'square'... Further, this is a graphic device problem, and I think there a limits to the lengths we need to go to to fix it in ggplot2

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Apr 23, 2019

is probably as you mention that closed form is needed for correct munching - I didn't anticipate this when making the initial suggestion

Thanks, then I'll revert it to closed form.

For the second one, I meant, changing the default lineend (butt) will break visual tests, which means we might have many broken revdeps. As you say, this is a graphic device problem. Do you think it's worth? (I agree it's only linejoin that we should expose to the users)

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 24, 2019

Are the visual tests not only run on linux systems, where the lineend parameter is rightfully ignored?

In any way I think it is worth breaking the tests... they are mainly there to avoid unforeseen regressions

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Apr 25, 2019

where the lineend parameter is rightfully ignored?

Oh, I didn't know this. Thanks.

Then, I'll try to finish this PR!

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 29, 2019

hmm... can I get you to look into the test failures? Apparently the tests are run on AppVeyor after all. It seems you missed updating the one in test-scales-breaks-labels.R

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Apr 29, 2019

Yes, I will. Sorry that I can't make much time for this...

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 29, 2019

No worries - didn't know you were hung up... Is there anything I can help with?

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 1, 2019

It seems you missed updating the one in test-scales-breaks-labels.R

Thanks, this comment really helped me! I didn't merge the current master so the new visual tests added in #2334 were not included here.

Currently I'm trying to figure out what the draw_legend() should be. I didn't notice geom_rect() doesn't use draw_key_rect()...

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 1, 2019

@thomasp85
I'm really sorry that I couldn't make this in due. Should this be still included in v3.2.0? I think this is ready for review now.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented May 1, 2019

This is fine - well add it🙂. Reviewing later today

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 1, 2019

Sure. I'll do my best to be responsive this week to let this be merged early.

Copy link
Member

@thomasp85 thomasp85 left a comment

LGTM

@thomasp85 thomasp85 merged commit 3b69172 into tidyverse:master May 1, 2019
4 checks passed
@yutannihilation yutannihilation deleted the feature-geom-tile-lineend branch May 1, 2019
@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 1, 2019

Thanks!

@lock
Copy link

@lock lock bot commented Oct 28, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants