Phase 1 (2/5): Cubic Bézier engine — curves, paths, partials, morph alignment - #2
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the core vector graphics foundation for PicoManim, adding BezierPath and CubicCurve structs along with comprehensive unit tests. These types enable path construction, bounding box calculation, path splitting, and alignment/interpolation for morphing animations. The review feedback highlights critical improvements for robustness and animation quality: clamping samplesPerCurve to prevent crashes or division-by-zero errors in boundingBox, introducing a fallbackAnchor during path subdivision to avoid visual morphing artifacts from the origin, and robustly resolving anchor points when degenerate subpaths are created near empty subpaths.
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.
| public func boundingBox(samplesPerCurve: Int = 8) -> (min: Vec2, max: Vec2)? { | ||
| var minPoint = Vec2(Double.infinity, Double.infinity) | ||
| var maxPoint = Vec2(-Double.infinity, -Double.infinity) | ||
| var found = false | ||
| for subpath in subpaths { | ||
| for curve in subpath.curves { | ||
| for i in 0...samplesPerCurve { | ||
| let p = curve.point(at: Double(i) / Double(samplesPerCurve)) | ||
| minPoint = Vec2(Swift.min(minPoint.x, p.x), Swift.min(minPoint.y, p.y)) | ||
| maxPoint = Vec2(Swift.max(maxPoint.x, p.x), Swift.max(maxPoint.y, p.y)) | ||
| found = true | ||
| } | ||
| } | ||
| } | ||
| return found ? (minPoint, maxPoint) : nil | ||
| } |
There was a problem hiding this comment.
If samplesPerCurve is negative, creating the closed range 0...samplesPerCurve will cause a runtime crash (precondition failure) in Swift. If it is 0, it will result in a division by zero, producing NaN coordinates. Clamping samplesPerCurve to at least 1 avoids these issues safely.
| public func boundingBox(samplesPerCurve: Int = 8) -> (min: Vec2, max: Vec2)? { | |
| var minPoint = Vec2(Double.infinity, Double.infinity) | |
| var maxPoint = Vec2(-Double.infinity, -Double.infinity) | |
| var found = false | |
| for subpath in subpaths { | |
| for curve in subpath.curves { | |
| for i in 0...samplesPerCurve { | |
| let p = curve.point(at: Double(i) / Double(samplesPerCurve)) | |
| minPoint = Vec2(Swift.min(minPoint.x, p.x), Swift.min(minPoint.y, p.y)) | |
| maxPoint = Vec2(Swift.max(maxPoint.x, p.x), Swift.max(maxPoint.y, p.y)) | |
| found = true | |
| } | |
| } | |
| } | |
| return found ? (minPoint, maxPoint) : nil | |
| } | |
| public func boundingBox(samplesPerCurve: Int = 8) -> (min: Vec2, max: Vec2)? { | |
| let samples = Swift.max(1, samplesPerCurve) | |
| var minPoint = Vec2(Double.infinity, Double.infinity) | |
| var maxPoint = Vec2(-Double.infinity, -Double.infinity) | |
| var found = false | |
| for subpath in subpaths { | |
| for curve in subpath.curves { | |
| for i in 0...samples { | |
| let p = curve.point(at: Double(i) / Double(samples)) | |
| minPoint = Vec2(Swift.min(minPoint.x, p.x), Swift.min(minPoint.y, p.y)) | |
| maxPoint = Vec2(Swift.max(maxPoint.x, p.x), Swift.max(maxPoint.y, p.y)) | |
| found = true | |
| } | |
| } | |
| } | |
| return found ? (minPoint, maxPoint) : nil | |
| } |
There was a problem hiding this comment.
Fixed in 56d1d3b ("Address review: clamp bbox samples, anchor degenerate morph curves"). boundingBox(samplesPerCurve:) clamps to at least 1 sample, so 0 can't divide by zero and negatives can't trap on range creation. Added boundingBoxToleratesNonPositiveSampleCounts covering both cases. Codex flagged the same issue (same fix). No impact on later PRs (they call it with the default).
Generated by Claude Code
| for i in a.indices { | ||
| let target = Swift.max(a[i].curves.count, b[i].curves.count) | ||
| a[i] = a[i].subdividedEvenly(to: target) | ||
| b[i] = b[i].subdividedEvenly(to: target) | ||
| } |
There was a problem hiding this comment.
Pass the start point of the corresponding subpath as the fallbackAnchor when subdividing. This ensures that if one of the subpaths is empty, its degenerate curves are anchored at the other shape's subpath start point, preventing the 'fly-in from origin' visual artifact during morphing.
for i in a.indices {
let target = Swift.max(a[i].curves.count, b[i].curves.count)
let anchorA = a[i].curves.first?.p0 ?? b[i].curves.first?.p0 ?? .zero
let anchorB = b[i].curves.first?.p0 ?? a[i].curves.first?.p0 ?? .zero
a[i] = a[i].subdividedEvenly(to: target, fallbackAnchor: anchorA)
b[i] = b[i].subdividedEvenly(to: target, fallbackAnchor: anchorB)
}There was a problem hiding this comment.
Fixed in 56d1d3b, exactly as suggested: aligned(with:) computes per-pair anchors (own start, else counterpart's start, else origin) and passes them to subdividedEvenly(to:fallbackAnchor:). Added alignmentAnchorsEmptySubpathAtCounterpart, which pins that the degenerate side sits at the counterpart's start point rather than the origin. This improves PR #4's transform morphs for any future multi-subpath shapes; no call-site changes needed there.
Generated by Claude Code
| public func subdividedEvenly(to target: Int) -> BezierPath.Subpath { | ||
| let count = curves.count | ||
| guard target > count else { return self } | ||
| guard count > 0 else { | ||
| // An empty subpath still has to match the other path's structure: | ||
| // fill it with degenerate point-curves. | ||
| let point = Vec2.zero | ||
| let degenerate = CubicCurve(p0: point, c1: point, c2: point, p1: point) | ||
| return BezierPath.Subpath( | ||
| curves: Array(repeating: degenerate, count: target), | ||
| isClosed: isClosed | ||
| ) | ||
| } |
There was a problem hiding this comment.
When subdividing an empty subpath, hardcoding the degenerate curves to Vec2.zero causes them to fly in from the origin during morphing animations. Adding a fallbackAnchor parameter allows the alignment logic to position these degenerate curves at the start of the corresponding subpath on the other shape, resulting in a much smoother morph.
public func subdividedEvenly(to target: Int, fallbackAnchor: Vec2 = .zero) -> BezierPath.Subpath {
let count = curves.count
guard target > count else { return self }
guard count > 0 else {
// An empty subpath still has to match the other path's structure:
// fill it with degenerate point-curves.
let degenerate = CubicCurve(p0: fallbackAnchor, c1: fallbackAnchor, c2: fallbackAnchor, p1: fallbackAnchor)
return BezierPath.Subpath(
curves: Array(repeating: degenerate, count: target),
isClosed: isClosed
)
}There was a problem hiding this comment.
Fixed in 56d1d3b together with the alignment-side change: subdividedEvenly now takes fallbackAnchor: Vec2 = .zero and fills an empty subpath with degenerate point-curves at that anchor. Covered by alignmentAnchorsEmptySubpathAtCounterpart.
Generated by Claude Code
| func degenerateSubpath(near subpaths: [Subpath]) -> Subpath { | ||
| let anchor = subpaths.last?.curves.last?.p1 ?? .zero | ||
| return Subpath( | ||
| curves: [CubicCurve(p0: anchor, c1: anchor, c2: anchor, p1: anchor)], | ||
| isClosed: false | ||
| ) | ||
| } |
There was a problem hiding this comment.
If the last subpath in subpaths is empty (has no curves), subpaths.last?.curves.last will be nil, causing the anchor to fall back to .zero. Iterating backwards to find the last non-empty subpath provides a much more robust anchor point.
| func degenerateSubpath(near subpaths: [Subpath]) -> Subpath { | |
| let anchor = subpaths.last?.curves.last?.p1 ?? .zero | |
| return Subpath( | |
| curves: [CubicCurve(p0: anchor, c1: anchor, c2: anchor, p1: anchor)], | |
| isClosed: false | |
| ) | |
| } | |
| func degenerateSubpath(near subpaths: [Subpath]) -> Subpath { | |
| let anchor = subpaths.reversed().first(where: { !$0.curves.isEmpty })?.curves.last?.p1 ?? .zero | |
| return Subpath( | |
| curves: [CubicCurve(p0: anchor, c1: anchor, c2: anchor, p1: anchor)], | |
| isClosed: false | |
| ) | |
| } |
There was a problem hiding this comment.
Fixed in 56d1d3b: degenerateSubpath(near:) walks backwards to the last non-empty subpath for its anchor (subpaths.reversed().first { !$0.curves.isEmpty }), falling back to the origin only when every subpath is empty.
Generated by Claude Code
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 119a685d64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| for subpath in subpaths { | ||
| for curve in subpath.curves { | ||
| for i in 0...samplesPerCurve { | ||
| let p = curve.point(at: Double(i) / Double(samplesPerCurve)) |
There was a problem hiding this comment.
When a caller passes samplesPerCurve as 0, this divides by zero and feeds NaN into the min/max updates, so a non-empty path returns an invalid (inf, -inf) bounding box; negative values also trap when constructing 0...samplesPerCurve. Since this is a public parameter, clamp it to at least 1 or explicitly reject invalid counts before iterating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 56d1d3b: samplesPerCurve is clamped to at least 1 (Swift.max(1, samplesPerCurve)), which handles both the zero (NaN bbox) and negative (range trap) cases. Regression test boundingBoxToleratesNonPositiveSampleCounts covers both. Gemini raised the same point; one fix resolves both threads.
Generated by Claude Code
3e7994e to
56d1d3b
Compare
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56d1d3b360
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| ) | ||
| } | ||
|
|
||
| while a.count < b.count { a.append(degenerateSubpath(near: a)) } |
There was a problem hiding this comment.
When aligning a completely empty path with a real path, this append creates a one-curve degenerate subpath anchored at .zero because a has no non-empty subpaths yet. Since the placeholder is no longer empty, the later fallbackAnchor logic does not move it to the counterpart's start, so BezierPath().aligned(with: BezierPath.line(from: Vec2(2, 3), to: Vec2(4, 3))) still morphs in from the origin despite the surrounding code trying to avoid that behavior for empty subpaths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in add883e. Nice catch — the padding loop ran before subdividedEvenly's fallback could apply (the placeholder already has one curve, so the empty-subpath path never fires). degenerateSubpath now takes the counterpart and the pair index, anchoring at the counterpart subpath's start when its own path has no non-empty subpath. Your exact repro (BezierPath().aligned(with: line)) is pinned by the new aligningWhollyEmptyPathAnchorsAtCounterpart test. Stack (#3–#5) rebased on top.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: add883eadf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| let anchor = own.reversed().first { !$0.curves.isEmpty }?.curves.last?.p1 | ||
| ?? (pairIndex < counterpart.count ? counterpart[pairIndex].curves.first?.p0 : nil) |
There was a problem hiding this comment.
When one side starts as BezierPath() and the other side has multiple subpaths, the first while append makes own non-empty, so the next padding subpath takes own's previous degenerate endpoint here instead of counterpart[pairIndex]. For example, aligning an empty path with two separated line subpaths anchors both generated subpaths at the first line's start, so the second line morphs in from the wrong place rather than its own start; keep track of the originally empty side or prefer the counterpart while filling that side.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63ff304. Right again — the own-side scan saw the previous pad's degenerate subpath as "non-empty" and chained off it. The own-side anchor is now resolved once against the pre-padding subpaths (lastPointA/lastPointB captured before the while loops), so an originally empty side anchors every pad at its own counterpart subpath's start, while a non-empty side keeps the grow-from-own-end behavior. Your multi-subpath repro is pinned by paddingEmptyPathAnchorsEachSubpathAtItsOwnCounterpart (two separated lines → pads at (1,1) and (5,5) respectively). Stack (#3–#5) rebased on top.
Generated by Claude Code
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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 |
Phase 1 (1/5): Package scaffold, CI, and core math layer
Everything a shape or morph needs, mirroring Manim's VMobject representation where all geometry - including straight edges - is stored as cubic Beziers: - CubicCurve: evaluation, de Casteljau split/clip, even subdivision. - BezierPath: multi-subpath container with constructors (polygon, polyline, line, arc, circle, ellipse, rectangle, regular polygon), sampled bounding boxes, point mapping/transforms. - partial(upTo:): leading portion of a path for draw-in animations, splitting the boundary curve at fractional progress. - aligned(with:)/interpolate: restructure two paths to matching subpath/curve counts so any shape can interpolate into any other. Stacked on #<PR1>; part 2/5 of the Phase 1 stack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
- boundingBox(samplesPerCurve:) clamps to at least 1 sample so a zero or negative count can't trap on range creation or divide by zero. - aligned(with:) anchors an empty subpath's degenerate point-curves at the counterpart subpath's start (and pads missing subpaths at the last non-empty subpath's end), so morphs never fly in from the origin. subdividedEvenly gains a fallbackAnchor parameter for this. Both behaviors are pinned by new tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
Aligning a path with zero subpaths against a real path padded a placeholder subpath anchored at the origin (the fallbackAnchor logic in subdividedEvenly only fires for empty subpaths, and the placeholder already has one curve). The padding helper now falls back to the start of the counterpart subpath it will pair with, so such morphs no longer fly in from the origin. Pinned by a new test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
Padding an originally empty path against a multi-subpath counterpart anchored the second and later pads at the first pad's degenerate point (the own-side scan saw the earlier pad as a non-empty subpath). The own-side anchor is now resolved once against the pre-padding subpaths, so an originally empty side anchors every pad at its own counterpart subpath's start. Pinned by a new test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
63ff304 to
593df5e
Compare
d8cd893
into
claude/picomanim-phase1-01-scaffold
2 participants
Summary
The geometry engine, mirroring Manim's
VMobjectrepresentation: all geometry — including straight edges — is stored as cubic Béziers, so any shape can morph into any other.CubicCurve— evaluation, de Casteljausplit(at:)/clipped(from:to:), even subdivision.BezierPath— multi-subpath container: constructors (polygon, polyline, line, arc, circle, ellipse, rectangle, regular polygon), sampled bounding boxes, point mapping /Transform2Dapplication.partial(upTo:)— the leading portion of a path (splits the boundary curve at fractional progress); this is what draw-in animations consume in PR 4.aligned(with:)/interpolate(_:_:_:)— restructure two paths to matching subpath/curve counts, then lerp control points; this is the morph (transform) machinery.Arcs use the standard
k = 4/3·tan(Δ/4)cubic approximation, one segment per ≤45° of sweep (negative sweeps covered by a test); circles are 8 segments so they morph smoothly.Stack
PR 2/5 — stacked on #1 (base branch
claude/picomanim-phase1-01-scaffold; will be retargeted tomainwhen #1 merges). PRs 3–5 build on this.Intentionally not included
Styling, identity, or mobjects (PR 3); anything time-based (PR 4). This PR is pure geometry with no rendering.
Test strategy
Algebraic identities recomputed by hand: split/clip continuity against de Casteljau (endpoints + parameter remapping),
partialcurve counting incl. fractional boundary splits, alignment count equalization (8-curve circle vs 4-curve square → 8/8; subpath padding), interpolation endpoints reproducing inputs, sampled circle bounds within 1e-2 of the radius.Concurrency
Pure
Sendable/Hashablevalue types; no shared state.🤖 Generated with Claude Code
https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
Generated by Claude Code