Phase 1 (5/5): ManimView SwiftUI preview, demo scene, and docs - #5
Conversation
|
Deferred-work issues for the stack, as promised in the description:
Generated by Claude Code |
There was a problem hiding this comment.
Code Review
This pull request introduces the SwiftUI-based ManimView player and a comprehensive demo scene to showcase Phase 1 features, alongside updated documentation and tests. Feedback on the implementation highlights several key areas for improvement: resetting anchorDate on view appearance to prevent premature timeline progression, removing redundant background fills in the canvas to reduce overdraw, guarding against zero or negative dimensions in frameSize to avoid division-by-zero errors, and clamping stroke path bounds to prevent rendering artifacts during overshoot animations.
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.
| .background(uiColor(background)) | ||
| } |
There was a problem hiding this comment.
Initializing @State private var anchorDate = Date() at the property declaration level means the date is captured when the ManimView struct is first instantiated. In SwiftUI, view structs can be created well before they are actually displayed on screen (e.g., when pre-rendering in a navigation stack, tab view, or parent view update).
When the view finally appears and starts playing, anchorDate will be in the past, causing the animation to instantly skip forward by that time difference (or skip the entire animation if the delay was longer than the duration).
To fix this, we should reset anchorDate to the current time when the view actually appears on screen.
.background(uiColor(background))
.onAppear {
if isPlaying {
anchorDate = Date()
}
}
}There was a problem hiding this comment.
Fixed in caf2a65 ("Address review: re-anchor clock on appear, canvas guards, trim clamps"): added the suggested .onAppear that re-anchors anchorDate when autoplay is on, so a view instantiated before display doesn't start partway through (or past) the scene. Not unit-testable (needs a real SwiftUI lifecycle); noted in the PR description's test-strategy caveat and verified via the preview.
Generated by Claude Code
| Canvas { context, size in | ||
| context.fill( | ||
| Path(CGRect(origin: .zero, size: size)), | ||
| with: .color(uiColor(background)) | ||
| ) | ||
| guard size.width > 0, size.height > 0 else { return } |
There was a problem hiding this comment.
This consolidated change addresses two opportunities for improvement:
- Redundant Background Fill (Overdraw): The
Canvasis filling its entire bounds with the background color on every frame, which is redundant because the parent view already has the background color set via.background(uiColor(background))on line 68. Removing this fill reduces overdraw and improves rendering performance. - Defensive Guard: If a user passes a custom
frameSizewith a width (x) or height (y) of zero or less, thescalecalculation on line 110 will result inInfinityorNaN. Adding a guard ensures we handle this safely.
Canvas { context, size in
guard size.width > 0, size.height > 0, frameSize.x > 0, frameSize.y > 0 else { return }There was a problem hiding this comment.
Fixed in caf2a65, both parts: the per-frame background fill is gone (the view's .background already covers the canvas), and the guard now also rejects non-positive frameSize components so the scale can't become infinite or NaN.
Generated by Claude Code
| if mobject.strokeStart > 0 || mobject.strokeEnd < 1 { | ||
| strokePath = path.trimmedPath( | ||
| from: CGFloat(mobject.strokeStart), | ||
| to: CGFloat(mobject.strokeEnd) | ||
| ) | ||
| } |
There was a problem hiding this comment.
SwiftUI's Path.trimmedPath(from:to:) expects values strictly within the range 0...1. If an easing function with overshoot (such as backIn or custom spring-like rate functions) drives strokeStart or strokeEnd slightly below 0 or above 1, trimmedPath can produce rendering artifacts or unexpected behavior.
We should clamp these values to 0...1 before passing them to trimmedPath.
| if mobject.strokeStart > 0 || mobject.strokeEnd < 1 { | |
| strokePath = path.trimmedPath( | |
| from: CGFloat(mobject.strokeStart), | |
| to: CGFloat(mobject.strokeEnd) | |
| ) | |
| } | |
| if mobject.strokeStart > 0 || mobject.strokeEnd < 1 { | |
| strokePath = path.trimmedPath( | |
| from: CGFloat(clamp(mobject.strokeStart, 0...1)), | |
| to: CGFloat(clamp(mobject.strokeEnd, 0...1)) | |
| ) | |
| } |
There was a problem hiding this comment.
Fixed in caf2a65: trimmedPath bounds are clamped to 0...1 exactly as suggested. Belt-and-suspenders with PR #3's clamped effective alphas — strokeStart/strokeEnd themselves can still exceed the range under overshooting custom rates, so the renderer clamps at the boundary it owns.
Generated by Claude Code
a651331 to
f7e98fd
Compare
caf2a65 to
6c1ae2e
Compare
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. You're on a roll. 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/merge. Checklist:
Remaining notes:
Generated by Claude Code |
055b590 to
6ce95d0
Compare
6c1ae2e to
04dffab
Compare
6ce95d0 to
62b1b1b
Compare
04dffab to
be52abe
Compare
62b1b1b to
572b8e4
Compare
be52abe to
9e701ee
Compare
c4267c6 to
9cb42b9
Compare
|
Head refresh note: this branch was rebased several times as #2–#4 absorbed review fixes; its own diff is unchanged since Codex's clean review (only the README shape-table row and the demo scene inherit upstream behavior fixes). Current head Generated by Claude Code |
- ManimView: a SwiftUI player for ManimScene. Renders snapshots into a Canvas in Manim coordinates (origin center, +y up, 8-unit-tall frame, uniform fit), strokes partial outlines via trimmedPath for draw-in animations, and fades fills through the mobject's opacity factors. Playback state is just an anchor time plus the TimelineView clock, so play/pause, restart, scrubbing, and looping are pure arithmetic; a paused schedule stops the render loop entirely. - ManimScene.demo: a short tour of Phase 1 (create, shift, parallel rotate+scale, two morphing transforms, fades) wired into #Preview and exercised by timeline tests that run on all platforms. - README: full Phase 1 documentation - scene building, semantics, ManimView usage, shape reference, roadmap. SwiftUI-only code is fenced behind #if canImport(SwiftUI); the Linux CI job builds and tests everything else. Stacked on the animation timeline PR; part 5/5 of the Phase 1 stack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
- onAppear re-anchors the playback clock: the view struct (and its @State defaults) can be created well before display, which would otherwise start autoplay partway through the scene. - Drop the redundant per-frame background fill (the view's .background already covers the canvas) and guard non-positive frameSize so the scale can't become infinite or NaN. - Clamp trimmedPath bounds to 0...1 in case a custom rate function overshoots. - README: rectangle/square default is white (matches the PR 3 fix). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
8d26054 to
6dd092d
Compare
9cb42b9 to
5b7cbd9
Compare
0db8d66
into
claude/picomanim-phase1-02-bezier
Summary
The presentation layer that completes Phase 1:
ManimView— a SwiftUI player forManimScene. Renderssnapshot(at:)into aCanvasin Manim coordinates (origin center, +y up, 8-unit-tall frame, uniform fit), strokes partial outlines viatrimmedPathfor draw-in animations, fills through the mobject's opacity factors. Playback state is just an anchor time + theTimelineViewclock, so play/pause, restart, scrubbing, and looping are pure arithmetic; a paused schedule stops the render loop entirely. Controls (play/pause, restart, scrubber, time label) carry accessibility labels.ManimScene.demo— a short tour (create → shift → morph to square → parallel rotate/scale → second morph → fades), wired into#Preview.The whole file is fenced behind
#if canImport(SwiftUI); the Linux CI job proves the rest of the package never depends on it.Stack
PR 5/5 — stacked on #4. Closes out the Phase 1 stack (#1 → #5).
Intentionally not included (Phase 2+, tracked in issues)
Text/LaTeX mobjects, groups, axes, updaters, video export, camera moves — plus the deferred technical improvements filed as issues and linked from this PR after it opens.
Test strategy
The demo scene is pure timeline logic, so CI exercises it headlessly: evaluates non-empty at 21 sample times and ends fully faded out.
Canvasrendering itself is not unit-testable; verification is#Previewon a Mac (blue circle draws in, morphs to red square, rotates/scales, morphs to hexagon, fades out). Called out here explicitly rather than pretending pixel coverage exists.Concurrency
ManimViewis a plainViewstruct; the only mutable state in the entire package is its SwiftUI@State(main-actor by construction). No@unchecked Sendableanywhere in Phase 1.🤖 Generated with Claude Code
https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
Generated by Claude Code