Skip to content

Feature request: make the input panels of Facet$draw_panels() predictable #6406

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

Open
Yunuuuu opened this issue Apr 9, 2025 · 11 comments · May be fixed by #6407 or #6421
Open

Feature request: make the input panels of Facet$draw_panels() predictable #6406

Yunuuuu opened this issue Apr 9, 2025 · 11 comments · May be fixed by #6407 or #6421

Comments

@Yunuuuu
Copy link
Contributor

Yunuuuu commented Apr 9, 2025

Related to #6398 and #6396.

After testing, I found it difficult to handle the original feature request using a single Facet object alone, since the panels input to Facet$draw_panels() is unpredictable. It’s challenging to break down panels into individual layer grobs.

Several issues contribute to this unpredictability:

  • The Layer$draw_geom method doesn't always return a grob if the developer doesn’t follow the expected conventions. No error or warning is issued in such cases, and gList(NULL) or gList(gList()) simply works silently.
  • Coord$draw_panel will sometimes wrap all layer grobs into single gTree in CoordRadial, sometimes it won't

Solutions:

  • Ensure that Layer$draw_geom() always returns a single grob or a gTree.
  • always wrap the all layer grobs into a single gTree before passing to Coord$draw_panel

In this way, the input panels will always be a list of gTree object for each panel.
each gTree object contains 3 children for Coord$render_fg, Coord$render_bg and another gTree of Facet$draw_back, all layer grobs, and Facet$draw_front

@teunbrand
Copy link
Collaborator

On what part do you get stuck if you try to implement panel spanning dendrograms? I'm unsure the premise of #6396 (using communicating grobs) is the right way to go in this case. If we don't need communicating grobs, then the current situation with how Facet$draw_panels() handles empty/collapsed grobs doesn't matter.

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Apr 9, 2025

Thanks, and apologies for the frequent PRs recently. I'm a big fan of circle plots and currently working on the circular design.

The main issue I'm encountering relates to drawing order: ggplot2 renders each facet panel sequentially, meaning the entire drawing process for one panel is completed before the next begins.

a segment drawn in one panel can be overridden by the background of the panel to its right:

library(ggplot2)
ggplot(data.frame(x = c(1, NA), facet = c("a", "b"))) +
  annotation_custom(
    grob = grid::roundrectGrob(
      gp = grid::gpar(fill = "red", col = "red"),
      width = 0.5, height =   0.5
    ),
    xmin = -Inf, xmax = Inf, ymin = -Inf, ymax = Inf
  ) +
  geom_segment(aes(x, y = 1L), yend = 1, xend = 2L) +
  facet_wrap(vars(facet), nrow = 1L) +
  coord_cartesian(clip = FALSE)

Image

if the layer order is reversed, the segment overrides the annotation—even though the annotation was added after:

ggplot(data.frame(x = c(NA, 1), facet = c("a", "b"))) +
  geom_segment(aes(x, y = 1L), yend = 1, xend = 0) +
  annotation_custom(
    grob = grid::roundrectGrob(
      gp = grid::gpar(fill = "red", col = "red"),
      width = 0.5, height =   0.5
    ),
    xmin = -Inf, xmax = Inf, ymin = -Inf, ymax = Inf
  ) +
  facet_wrap(vars(facet), nrow = 1L) +
  coord_cartesian(clip = FALSE)

Image

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Apr 9, 2025

In the Facet I'm implementing, it's necessary to decompose each panel and manage a global drawing order across panels to ensure correct rendering and layering.

@teunbrand
Copy link
Collaborator

Thanks for the explanation. Isn't it possible to implement #6407 in the Facet class you're implementing to a degree that you don't get these overlap assymmetries? I also don't understand the desired behaviour here: do you want the annotation to be occluded by the neighbouring panel or not?

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Apr 9, 2025

I want the overall drawing order to follow the sequence in which layers are added. Specifically, this means starting with Facet$draw_back across all panels, followed by each plot layer in sequence across all panels, and finally rendering Coord$render_bg and Coord$render_fg (assuming theme(panel.ontop = TRUE)).

This approach ensures that what we input is exactly what we see.

If the input panels for Facet is predictable, I think I can manage all things well.

@teunbrand
Copy link
Collaborator

If we were to put the following part under control of Facet instead of Layout, would that give you enough freedom to implement the thing you want?

ggplot2/R/layout.R

Lines 63 to 85 in 8b2a764

facet_bg <- self$facet$draw_back(data,
self$layout,
self$panel_scales_x,
self$panel_scales_y,
theme,
self$facet_params
)
facet_fg <- self$facet$draw_front(
data,
self$layout,
self$panel_scales_x,
self$panel_scales_y,
theme,
self$facet_params
)
# Draw individual panels, then assemble into gtable
panels <- lapply(seq_along(panels[[1]]), function(i) {
panel <- lapply(panels, `[[`, i)
panel <- c(facet_bg[i], panel, facet_fg[i])
panel <- self$coord$draw_panel(panel, self$panel_params[[i]], theme)
ggname(paste("panel", i, sep = "-"), panel)
})

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Apr 9, 2025

Yes, if that part were moved under the control of Facet, I believe it would give me enough flexibility to implement the drawing order as intended.

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Apr 13, 2025

Hi, I'm planning to implement the feature of the ggalign package — the chord diagram — which will depend on this functionality. Would it be okay for me to open another PR to modify the relevant part of the code and add them to the Facet's draw_panel method?

@teunbrand
Copy link
Collaborator

I think it is better to create a new method for the Facet class than to cram it into the draw_panels method. Some extension override the draw_panel method, and we advertise its eligibility for extensions (here), so we cannot simply slap on the responsibility of arranging the panel sandwich to that particular method.

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Apr 13, 2025

Thanks! Could you suggest a name for the method? Do you think draw_panel_grobs/build_panels would be appropriate?

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Apr 13, 2025

I've named the method draw_facet_panels. Do you think this is an appropriate name, or would you suggest something else?

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