Skip to content

Ensure stateless layer instances #4448

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

Merged
merged 6 commits into from
May 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
# ggplot2 (development version)

* Fix a bug in the layer implementation that introduced a new state after the
first render which could lead to a different look when rendered the second
time (@thomasp85, #4204)

* Make sure `label_bquote()` has access to the calling environment when
evaluating the labels (@thomasp85, #4141)

* Fix bug in `annotate_logticks()` that would cause an error when used together
with `coord_flip()` (@thomasp85, #3954)

* Fix a bug in `guide_bins()` where keys would disappear if the guide was
reversed (@thomasp85, #4210)

* Fix a bug in legend justification where justification was lost of the legend
dimensions exceeded the available size (@thomasp85, #3635)

Expand Down
22 changes: 14 additions & 8 deletions R/layer.r
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,12 @@ Layer <- ggproto("Layer", NULL,
geom_params = NULL,
stat = NULL,
stat_params = NULL,

# These two fields carry state throughout rendering but will always be
# calculated before use
computed_geom_params = NULL,
computed_stat_params = NULL,

data = NULL,
aes_params = NULL,
mapping = NULL,
Expand Down Expand Up @@ -276,9 +282,9 @@ Layer <- ggproto("Layer", NULL,
if (empty(data))
return(new_data_frame())

params <- self$stat$setup_params(data, self$stat_params)
data <- self$stat$setup_data(data, params)
self$stat$compute_layer(data, params, layout)
self$computed_stat_params <- self$stat$setup_params(data, self$stat_params)
data <- self$stat$setup_data(data, self$computed_stat_params)
self$stat$compute_layer(data, self$computed_stat_params, layout)
},

map_statistic = function(self, data, plot) {
Expand Down Expand Up @@ -339,8 +345,8 @@ Layer <- ggproto("Layer", NULL,
c(names(data), names(self$aes_params)),
snake_class(self$geom)
)
self$geom_params <- self$geom$setup_params(data, c(self$geom_params, self$aes_params))
self$geom$setup_data(data, self$geom_params)
self$computed_geom_params <- self$geom$setup_params(data, c(self$geom_params, self$aes_params))
self$geom$setup_data(data, self$computed_geom_params)
},

compute_position = function(self, data, layout) {
Expand All @@ -363,7 +369,7 @@ Layer <- ggproto("Layer", NULL,
},

finish_statistics = function(self, data) {
self$stat$finish_layer(data, self$stat_params)
self$stat$finish_layer(data, self$computed_stat_params)
},

draw_geom = function(self, data, layout) {
Expand All @@ -372,8 +378,8 @@ Layer <- ggproto("Layer", NULL,
return(rep(list(zeroGrob()), n))
}

data <- self$geom$handle_na(data, self$geom_params)
self$geom$draw_layer(data, self$geom_params, layout, layout$coord)
data <- self$geom$handle_na(data, self$computed_geom_params)
self$geom$draw_layer(data, self$computed_geom_params, layout, layout$coord)
}
)

Expand Down
10 changes: 10 additions & 0 deletions tests/testthat/test-layer.r
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ test_that("if an aes is mapped to a function that returns NULL, it is removed",
expect_identical(names(p[[1]]), c("x", "PANEL", "group"))
})

test_that("layers are stateless except for the computed params", {
df <- data.frame(x = 1:10, y = 1:10)
p <- ggplot(df) +
geom_col(aes(x = x, y = y), width = 0.8, fill = "red")
col_layer <- as.list(p$layers[[1]])
stateless_names <- setdiff(names(col_layer), c("computed_geom_params", "computed_stat_params"))
invisible(ggplotGrob(p))
expect_identical(as.list(p$layers[[1]])[stateless_names], col_layer[stateless_names])
})

# Data extraction ---------------------------------------------------------

test_that("layer_data returns a data.frame", {
Expand Down