Phase 2-E: Updaters (scene.always) - #15
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces per-frame modifiers (updaters) to ManimScene, allowing pure functions of time to be layered on top of animated states. It also adds a comprehensive suite of unit tests to verify updater behaviors. Feedback on the changes suggests improving encapsulation by marking the internal Updater struct as private, and simplifying the optional mapping logic for endTime to make it more readable and idiomatic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| /// A registered per-frame modifier: a pure function of time applied to | ||
| /// the target's animated state during evaluation. | ||
| struct Updater: Sendable { |
There was a problem hiding this comment.
Fixed in 56fcafb — Updater is now private struct Updater, matching the visibility of the updaters array that holds it.
Generated by Claude Code
| } | ||
| // Updaters layer on top of the animated state, in registration order. | ||
| for updater in updaters { | ||
| guard t >= updater.startTime, updater.endTime.map({ t <= $0 }) ?? true else { continue } |
There was a problem hiding this comment.
Using .map on an optional Double to perform a comparison can be hard to read and introduces minor overhead in the hot path of snapshot(at:).
We can simplify this by splitting the conditions into a straightforward guard and an optional binding check, which is more idiomatic in Swift.
guard t >= updater.startTime else { continue }
if let endTime = updater.endTime, t > endTime { continue }There was a problem hiding this comment.
Fixed in 56fcafb — the window check is now a plain guard t >= updater.startTime else { continue } followed by if let endTime = updater.endTime, t > endTime { continue }, as suggested.
Generated by Claude Code
56fcafb to
e31549b
Compare
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
scene.always(_:during:_:) registers a @sendable (time, state) -> state closure that layers on top of the animated state during evaluation. Because updaters are pure functions of time, snapshot(at:) stays pure: scrubbing, looping, and offline rendering keep working unchanged. Semantics, each pinned by a test: - Active from the registration point in the timeline onward, or exactly over an explicit `during` window (outside it the underlying animated state shows again). - Layers over animations (an updater sees the animated state at t) and multiple updaters compose in registration order. - Evaluation-only: the build cursor and later animations' start poles see the un-updated state. - Registering for a never-seen mobject shows it at the current point, like add. Stacked on the MobjectGroup PR (shares the ManimScene surface). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
- Updater is only ever used by ManimScene's private updaters array, so mark the struct itself private (Gemini). - Replace the Optional.map window check in snapshot with a guard plus optional binding (Gemini).
e31549b to
d12aca8
Compare
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Ready for human review/mergePhase 2-E (updaters —
Stacked on #12 (Phase 2-B) — base is Generated by Claude Code |
Phase 2-E: Updaters (
scene.always)Stacked on #12 (Phase 2-B) — base branch is
claude/picomanim-phase2-02-groups; will be retargeted tomainonce #12 merges. Only the final commit is new here.Scope
Manim's
add_updaterequivalent, redesigned to fit PicoManim's pure-snapshot model: a per-mobject function of absolute time that post-processes the animated state.Public API
Design notes
dtupdaters mutate state each frame, which breaks scrubbing and pure evaluation. Here an updater receives the absolute scene time and the fully-animated state, and returns a new state —snapshot(at:)stays deterministic and side-effect-free, so scrubbing/looping/offline render keep working. This is the one intentional semantic departure from Manim, called out in the docs.during.lowerBound) until scene end (orduring.upperBound).playtargeting an updated mobject starts from the animated cursor state, not the updated one — updaters are a render-time overlay, so removing one never changes animation targets. Pinned by a test.@Sendableand stored in aSendablestruct — real sendability, no@unchecked Sendable.Test strategy
UpdaterTests.swift: purity (same t twice → same state), layering over a concurrent animation, registration-time start,duringwindowing, auto-add of unseen mobjects, registration-order composition, and cursor independence.Generated by Claude Code