From c4c77c373b01a42cc29b0d4f3eefe2dd4159130b Mon Sep 17 00:00:00 2001 From: bbcuffer Date: Mon, 22 Jun 2026 10:42:15 +0100 Subject: [PATCH 1/3] Fix frame distribution in view_step_manual (issue #499) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues caused frames to be distributed unequally across pauses and transitions, making wrap=FALSE animations zoom back and wrap=TRUE animations spend more time pausing than transitioning. Issue 1 — setup_params: an unnecessary phase for pause_first=FALSE The phase vectors were built by prepending a zero-pause slot and appending a zero-step slot, then conditionally zeroing elements: pause_length <- c(0, pause_length) # e.g. c(0, 1, 1) step_length <- c(step_length, 0) # e.g. c(1, 1, 0) if (!wrap) pause_length[last] <- 0 # e.g. c(0, 1, 0) for wrap=FALSE For wrap=FALSE this made no difference. It left a dead (0, 0) trailing phase; for wrap=TRUE it left a live (1, 0) trailing phase that allocated frames to a w1 pause that should not exist (the gif loop provides that continuity). Fix: drop the append entirely and trim the last pause in one step: pause_length <- c(0, pause_length[-length(pause_length)]) The behaviour becomes: wrap=FALSE: pause=c(0,1) step=c(1,0) — transition then freeze wrap=TRUE: pause=c(0,1) step=c(1,1) — transition, pause, return Bug 2 — train: nframes inflated by 1 for wrap=TRUE if (params$wrap) nframes <- nframes + 1 This made distribute_frames divide one extra frame across all phases, which in practice biased allocation toward pauses. Wrap is already handled by padding params$windows with the first window row; the inflated nframes was not needed and is removed. Adds tests for all four wrap/pause_first combinations covering both the phase structure produced by setup_params and the resulting frame allocation from distribute_frames. Co-Authored-By: Claude Sonnet 4.6 --- R/view-step-manual.R | 11 ++-- tests/testthat/test-view-step-manual.R | 71 ++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 6 deletions(-) create mode 100644 tests/testthat/test-view-step-manual.R diff --git a/R/view-step-manual.R b/R/view-step-manual.R index 0391b72..4d93ab1 100644 --- a/R/view-step-manual.R +++ b/R/view-step-manual.R @@ -44,9 +44,10 @@ ViewStepManual <- ggproto('ViewStepManual', ViewStep, step_length <- rep(params$step_length, length.out = nsteps) pause_length <- rep(params$pause_length, length.out = nsteps) if (!params$pause_first) { - pause_length <- c(0, pause_length) - step_length <- c(step_length, 0) - if (!params$wrap) pause_length[length(pause_length)] <- 0 + pause_length <- c(0, pause_length[-length(pause_length)]) + if (!params$wrap) { + step_length[length(step_length)] <- 0 + } } else if (!params$wrap) { step_length[length(step_length)] <- 0 } @@ -55,9 +56,7 @@ ViewStepManual <- ggproto('ViewStepManual', ViewStep, params }, train = function(self, data, params) { - nframes <- params$nframes - if (params$wrap) nframes <- nframes + 1 - frames <- distribute_frames(params$pause_length, params$step_length, nframes) + frames <- distribute_frames(params$pause_length, params$step_length, params$nframes) params$windows <- vec_rbind0( params$windows, params$windows[rep(1, length(frames$static_length) - nrow(params$windows) + 1), , drop = FALSE] diff --git a/tests/testthat/test-view-step-manual.R b/tests/testthat/test-view-step-manual.R new file mode 100644 index 0000000..e712a31 --- /dev/null +++ b/tests/testthat/test-view-step-manual.R @@ -0,0 +1,71 @@ +# Helper: create a 2-window ViewStepManual object and run setup_params +step_manual_params <- function(wrap, pause_first) { + vsm <- view_step_manual( + wrap = wrap, pause_first = pause_first, + xmin = c(1, 4), xmax = c(7, 7), ymin = 0, ymax = 2.5 + ) + vsm$setup_params(NULL, vsm$params) +} + +# --- Phase structure (setup_params) --- +# Each phase is a (pause, step) pair. pause_length and step_length should +# reflect the minimum number of phases needed for the given wrap/pause_first. + +test_that("wrap=FALSE pause_first=FALSE produces 2 phases: transition then pause", { + p <- step_manual_params(wrap = FALSE, pause_first = FALSE) + expect_equal(p$pause_length, c(0, 1)) + expect_equal(p$step_length, c(1, 0)) +}) + +test_that("wrap=FALSE pause_first=TRUE produces 2 phases: pause-transition then pause", { + p <- step_manual_params(wrap = FALSE, pause_first = TRUE) + expect_equal(p$pause_length, c(1, 1)) + expect_equal(p$step_length, c(1, 0)) +}) + +test_that("wrap=TRUE pause_first=FALSE produces 2 phases: transition then pause+transition", { + p <- step_manual_params(wrap = TRUE, pause_first = FALSE) + expect_equal(p$pause_length, c(0, 1)) + expect_equal(p$step_length, c(1, 1)) +}) + +test_that("wrap=TRUE pause_first=TRUE produces 2 phases: both with transitions for wrap", { + p <- step_manual_params(wrap = TRUE, pause_first = TRUE) + expect_equal(p$pause_length, c(1, 1)) + expect_equal(p$step_length, c(1, 1)) +}) + +# --- Frame allocation (distribute_frames) --- +# nframes chosen to divide cleanly across phases (no rounding artefacts). + +test_that("wrap=FALSE pause_first=FALSE: frames split equally between transition and pause", { + # total weight = 2, nframes = 100 -> 50 frames each + p <- step_manual_params(wrap = FALSE, pause_first = FALSE) + frames <- gganimate:::distribute_frames(p$pause_length, p$step_length, 100) + expect_equal(frames$static_length, c(0, 50)) + expect_equal(frames$transition_length, c(50, 0)) +}) + +test_that("wrap=FALSE pause_first=TRUE: frames split equally across pause, transition, pause", { + # total weight = 3, nframes = 99 -> 33 frames each + p <- step_manual_params(wrap = FALSE, pause_first = TRUE) + frames <- gganimate:::distribute_frames(p$pause_length, p$step_length, 99) + expect_equal(frames$static_length, c(33, 33)) + expect_equal(frames$transition_length, c(33, 0)) +}) + +test_that("wrap=TRUE pause_first=FALSE: frames split equally across transition, pause, transition", { + # total weight = 3, nframes = 99 -> 33 frames each; no trailing w1 pause + p <- step_manual_params(wrap = TRUE, pause_first = FALSE) + frames <- gganimate:::distribute_frames(p$pause_length, p$step_length, 99) + expect_equal(frames$static_length, c( 0, 33)) + expect_equal(frames$transition_length, c(33, 33)) +}) + +test_that("wrap=TRUE pause_first=TRUE: frames split equally across pause, transition, pause, transition", { + # total weight = 4, nframes = 100 -> 25 frames each + p <- step_manual_params(wrap = TRUE, pause_first = TRUE) + frames <- gganimate:::distribute_frames(p$pause_length, p$step_length, 100) + expect_equal(frames$static_length, c(25, 25)) + expect_equal(frames$transition_length, c(25, 25)) +}) From 443a26d37b704a8852ad3dff0aa6502f23f98878 Mon Sep 17 00:00:00 2001 From: bbcuffer Date: Mon, 22 Jun 2026 14:16:54 +0100 Subject: [PATCH 2/3] Apply view_step_manual fixes to view_step Co-Authored-By: Claude Sonnet 4.6 --- R/view-step.R | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/R/view-step.R b/R/view-step.R index 6551198..9bce405 100644 --- a/R/view-step.R +++ b/R/view-step.R @@ -94,10 +94,10 @@ ViewStep <- ggproto('ViewStep', View, pause_length <- rep(params$pause_length, length.out = nsteps) look_ahead <- rep(params$look_ahead, length.out = nsteps) if (!params$pause_first) { - pause_length <- c(0, pause_length) - step_length <- c(step_length, 0) - look_ahead <- c(look_ahead, look_ahead[1]) - if (!params$wrap) pause_length[length(pause_length)] <- 0 + pause_length <- c(0, pause_length[-length(pause_length)]) + if (!params$wrap) { + step_length[length(step_length)] <- 0 + } } else if (!params$wrap) { step_length[length(step_length)] <- 0 } @@ -167,7 +167,7 @@ ViewStep <- ggproto('ViewStep', View, } } frame_ranges <- frame_ranges[frame_ranges$.frame <= params$nframes, ] - frame_ranges$.frame <- (frame_ranges$.frame + round(params$delay * frames$mod)) %% params$nframes + frame_ranges$.frame <- ((frame_ranges$.frame - 1L) + round(params$delay * frames$mod)) %% params$nframes frame_ranges <- frame_ranges[order(frame_ranges$.frame), ] params$frame_ranges <- frame_ranges params From 756e4985e9b6407ca4c9722a7628b71f3c349542 Mon Sep 17 00:00:00 2001 From: bbcuffer Date: Thu, 25 Jun 2026 09:11:29 +0100 Subject: [PATCH 3/3] simplify logic Removes redundant nesting of if statements --- R/view-step-manual.R | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/R/view-step-manual.R b/R/view-step-manual.R index 4d93ab1..2ba25e0 100644 --- a/R/view-step-manual.R +++ b/R/view-step-manual.R @@ -44,11 +44,9 @@ ViewStepManual <- ggproto('ViewStepManual', ViewStep, step_length <- rep(params$step_length, length.out = nsteps) pause_length <- rep(params$pause_length, length.out = nsteps) if (!params$pause_first) { - pause_length <- c(0, pause_length[-length(pause_length)]) - if (!params$wrap) { - step_length[length(step_length)] <- 0 - } - } else if (!params$wrap) { + pause_length <- c(0, head(pause_length, -1)) + } + if (!params$wrap) { step_length[length(step_length)] <- 0 } params$step_length <- step_length