Skip to content

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Dec 3, 2022

This PR aims to fix #3271.

In brief:

  • It repeats the first row in a group to after the last row in a group.
  • Due to this, the rect_to_poly() function in geom_rect() made a redundant edge. While amending this, I found a more efficient way to polygonise rectangles, which is 2x faster for n = 2 rectangles, but 200x faster for n = 1000 rectangles.
  • A few visual tests changed due to closing the shape, but I manually checked them and they're invisible changes.

A visual comparison:
Before:

library(ggplot2)

offset <- function(value, offset) as.vector(outer(value, offset, `+`))

df <- data.frame(
  x = offset(c(-0.5, 0, 1, 1.5, 1, 0, 0, 0, 1, 1), c(0, 2)),
  y = offset(c(0.5, 0, 0, 0.5, 1, 1, 0.25, 0.75, 0.75, 0.25), c(0, 2)),
  group = rep(1:2, c(10, 10)),
  subgroup = rep(1:4, c(6, 4, 6, 4))
)

ggplot(df, aes(x, y, group = group, fill = factor(group))) +
  geom_polygon(aes(subgroup = subgroup)) +
  coord_polar()

After:

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

ggplot(df, aes(x, y, group = group, fill = factor(group))) +
  geom_polygon(aes(subgroup = subgroup)) +
  coord_polar()

Created on 2022-12-03 by the reprex package (v2.0.1)

@teunbrand teunbrand marked this pull request as draft December 3, 2022 20:33
@teunbrand teunbrand marked this pull request as ready for review December 3, 2022 21:19
@teunbrand

This comment was marked as outdated.

@teunbrand
Copy link
Collaborator Author

The snapshots of maps have revert to their old state after removing the closing point.
However, rectangles in non-linear coordinates lose a closing point. Linejoins on rectangles look like they should.

@thomasp85
Copy link
Member

Yes - rectangles always had one too many so this change is considered a fix

Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

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

LGTM, but if you could provide a test that shows how this fixes rendering of closed paths it would be great

@teunbrand teunbrand merged commit aa2ddbe into tidyverse:main May 8, 2023
@teunbrand teunbrand deleted the munch_closed_poly branch May 26, 2023 09:43
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.

coord_munch should convert closed shapes to closed form
2 participants