Phase 2-A: Arc-length morph resampling + total interpolate by ronaldmannak · Pull Request #11 · PicoMLX/PicoManim

ronaldmannak · 2026-07-07T02:06:54Z

Phase 2-A: Arc-length morph resampling + total `interpolate`

Closes #7. Closes #9.

Scope

Resolves the two issues deferred from Phase 1 review:

Arc-length-weighted morph alignment (Morph quality: arc-length-based path resampling for transform alignment #7). BezierPath.aligned(with:) previously subdivided curves evenly by count, so a subpath made of one long edge and several short ones morphed with visibly uneven point density. Curves are now subdivided proportionally to their approximate arc length (every curve keeps at least one piece), and closed subpaths with equal curve counts are additionally rotated to the cyclic offset that minimizes total squared start-point travel — the same trick Manim uses to stop closed shapes from "twisting" during a morph. Rotation only applies to chains that close explicitly (last.p1 ≈ first.p0); implicit closures are left alone so the closing gap can't end up mid-outline.
Total interpolate contract (BezierPath.interpolate: define behavior for structurally mismatched inputs #9). BezierPath.interpolate(_:_:_:) previously assumed pre-aligned inputs and produced garbage on mismatched structures. It now detects structural mismatch (structurallyMatches(_:)) and aligns on the fly (non-recursively), and closed same-count pairs get the same rotation alignment as the transform pipeline — so the function is total: any two paths interpolate sensibly.

Public API

extension CubicCurve {
    /// 8-sample polyline approximation of the curve's length.
    public var approximateLength: Double
}

extension BezierPath {
    /// True when both paths have identical subpath and per-subpath curve
    /// counts. Deliberately ignores isClosed: aligned(with:) equalizes
    /// counts but not closedness, so a closedness check could never be
    /// satisfied by alignment (interpolate treats a mixed pair as open).
    public func structurallyMatches(_ other: BezierPath) -> Bool
    // aligned(with:) and interpolate(_:_:_:) keep their signatures;
    // behavior is upgraded as described above.
}

No source-breaking changes; existing callers get better morphs for free.

Design notes

Weighting uses approximateLength (8-sample polyline), which is plenty for distribution decisions and keeps the hot path allocation-free.
Rotation alignment is O(n²) in curve count per closed subpath, using squared distances (no hypot) with an early-exit — fine at mobject scale.
interpolate is deliberately non-recursive: aligned output matches structurally by construction, and inlining the aligned interpolation means no future semantic drift can stack-overflow.
Empty subpaths fall back to the Phase 1 even subdivision so degenerate inputs keep their previous behavior.

Test strategy

weightedSubdivisionFollowsArcLength — a subpath with one long and one short edge distributes pieces proportionally.
closedSubpathRotationMinimizesTravel — a rotated copy of a square aligns back with near-zero start-point travel.
interpolateRotationAlignsSameCountClosedPaths — direct interpolation of two same-count authorings of a square doesn't twist/collapse.
implicitlyClosedSubpathsDoNotRotate — implicit closures are never rotated.
interpolateAlignsMismatchedInputsOnTheFly — mismatched paths interpolate sensibly with exact endpoints.
All Phase 1 morph tests still pin the exact-path-at-poles and anchor-origin behaviors.

Follow-ups

Morph rotation alignment can jump trimmed strokes at the final frame #16 — skip rotation alignment when a morph pole carries a trimmed stroke (needs ManimScene plumbing that would conflict with Phase 2-B: MobjectGroup, layout helpers, and group animations #12; full analysis in the issue).

Concurrency

Pure value types only; no @unchecked Sendable.

Stack

Independent — based on main. PRs 2-B/2-C are also independent; 2-D and 2-E stack on 2-B.

Closes the two deferred morph-quality issues: - aligned(with:) distributes extra subdivision splits in proportion to each curve's arc length (new CubicCurve.approximateLength and Subpath.subdividedWeighted), so matched pieces cover similar fractions of the outline and morphs progress evenly instead of bunching on short segments (#7). - Closed subpath pairs are rotation-aligned: the counterpart's curve list is cyclically rotated to minimize total start-point travel, so morphing between the same shape authored from different corners is a no-op rather than a spin (#7). - BezierPath.interpolate is now total: structurally mismatched inputs are aligned on the fly instead of silently truncating to the shorter structure, killing the phantom-geometry foot-gun for unaligned callers; pre-aligned callers (the timeline) are unaffected (#9). All pinned by new tests; existing alignment and timeline tests are unchanged and still pass by construction (equal-length curves divide exactly as before). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags

gemini-code-assist

Code Review

This pull request improves path morphing and interpolation by introducing weighted subdivision based on arc length, rotation alignment to minimize travel for closed subpaths, and on-the-fly alignment for mismatched inputs during interpolation. The review feedback highlights three key areas for improvement: a warning regarding the implementation of structurallyMatches to prevent potential infinite recursion, an optimization for the greedy subdivision algorithm in subdividedWeighted to reduce its complexity from $O((target - count) \times count)$ to $O(N \log N)$, and a performance optimization in rotatedToMinimizeTravel to use squared distance instead of Euclidean distance to avoid costly square root calculations inside the nested loop.

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.

gemini-code-assist · 2026-07-07T02:09:08Z

+    public func structurallyMatches(_ other: BezierPath) -> Bool {
+        guard subpaths.count == other.subpaths.count else { return false }
+        for i in subpaths.indices where subpaths[i].curves.count != other.subpaths[i].curves.count {
+            return false
+        }
+        return true
+    }


There is a mismatch between the PR description and the implementation of structurallyMatches(_:). The PR description states that it returns true when both paths have identical subpath/curve counts and closedness. However, the implementation does not check isClosed.

Note that if we did check isClosed in structurallyMatches, it would cause infinite recursion in interpolate because aligned(with:) does not align closedness. Therefore, the current implementation is correct to omit the closedness check, but the PR description and any future maintainers should be warned about this to prevent stack overflows.

Fixed in 38d44bf, plus a hardening step beyond the doc change: interpolate no longer recurses at all — it aligns mismatched inputs inline and interpolates directly, so no future drift between structurallyMatches and aligned(with:) semantics can produce infinite recursion. The structurallyMatches doc comment now states explicitly that isClosed is ignored on purpose and why, and the PR description has been corrected to drop the "and closedness" claim.

Generated by Claude Code

gemini-code-assist · 2026-07-07T02:09:08Z

+        // Every curve keeps at least one piece; hand out the extras one at a
+        // time to the curve furthest below its length-proportional share.
+        var pieces = Array(repeating: 1, count: count)
+        let shares = lengths.map { $0 / total * Double(target) }
+        for _ in 0..<(target - count) {
+            var best = 0
+            var bestDeficit = -Double.infinity
+            for i in 0..<count {
+                let deficit = shares[i] - Double(pieces[i])
+                if deficit > bestDeficit {
+                    bestDeficit = deficit
+                    best = i
+                }
+            }
+            pieces[best] += 1
+        }


The current greedy algorithm distributes extra pieces one by one in an $O((target - count) \times count)$ loop. If target is large (e.g., when morphing highly detailed paths or text glyphs), this nested loop can become a performance bottleneck.

We can optimize this to $O(N \log N)$ by first allocating the base integer shares and then distributing the remaining pieces to the curves with the largest fractional remainders. This completely avoids the nested loop.

var pieces = Array(repeating: 1, count: count) let shares = lengths.map { $0 / total * Double(target) } var remaining = target - count // Allocate the base integer shares above 1 let extraPieces = shares.map { Swift.max(0, Int($0) - 1) } let allocated = extraPieces.reduce(0, +) if allocated <= remaining { for i in 0..<count { pieces[i] += extraPieces[i] } remaining -= allocated } // Distribute any remaining pieces to the curves with the largest deficits if remaining > 0 { let deficits = (0..<count).map { i in (index: i, deficit: shares[i] - Double(pieces[i])) } let sortedIndices = deficits.sorted { $0.deficit > $1.deficit }.map { $0.index } for i in 0..<remaining { pieces[sortedIndices[i]] += 1 } }

Declined. Alignment operates per subpath pair, and a subpath is a single glyph contour or shape outline — target is the larger of the two curve counts, in practice tens of curves (a text glyph tops out around ~60), so the greedy loop is a few hundred operations executed once per morph, not per frame. At that size the simple loop is easier to verify than the sort-based allocation.

The suggested snippet also has a correctness gap: when allocated > remaining the base-share branch is skipped but remaining stays target - count, and the final loop indexes sortedIndices[i] for i in 0..<remaining, which can exceed count and crash. Happy to revisit with a (correct) largest-remainder version if profiling ever shows this loop hot.

(Source: comment r3533121709 on PR #11 — greedy piece distribution in subdividedWeighted.)

Generated by Claude Code

gemini-code-assist · 2026-07-07T02:09:08Z

+            var cost = 0.0
+            for i in 0..<count {
+                cost += (reference.curves[i].p0 - curves[(i + offset) % count].p0).length
+                if cost >= bestCost { break }
+            }


Using .length (which calls hypot / square root) inside the nested $O(N^2)$ loop is a performance bottleneck for paths with many curves.

We can optimize this by using the squared distance instead. Minimizing the sum of squared distances (least squares) is mathematically equivalent or even superior for morphing (as it penalizes larger individual point travels more heavily to prevent twisting), while being significantly faster.

var cost = 0.0 for i in 0..<count { let diff = reference.curves[i].p0 - curves[(i + offset) % count].p0 cost += diff.x * diff.x + diff.y * diff.y if cost >= bestCost { break } }

Fixed in 38d44bf — the rotation cost is now the summed squared start-point distance (no hypot in the O(n²) loop, keeps the existing early-exit), and the doc comment says so. The existing closedSubpathRotationMinimizesTravel test still pins the zero-travel optimum, which is identical under both metrics.

Generated by Claude Code

…cost - interpolate now aligns mismatched inputs inline instead of recursing, so no combination of structurallyMatches and aligned(with:) semantics can ever stack-overflow; documented that structurallyMatches ignores isClosed on purpose (Gemini). - rotatedToMinimizeTravel minimizes summed squared start-point distance: no hypot in the O(n^2) loop and least-squares favors twist-free matchings (Gemini).

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38d44bf4d1

ℹ️ 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".

chatgpt-codex-connector · 2026-07-07T02:30:22Z

+        // through the guard again would turn any future mismatch between
+        // `aligned(with:)` and `structurallyMatches` into infinite
+        // recursion instead of a slightly padded interpolation.
+        let (pa, pb) = a.structurallyMatches(b) ? (a, b) : a.aligned(with: b)


Rotate same-count closed paths before interpolating

When two closed paths already have the same subpath/curve counts but were authored from different starting vertices, this fast path skips aligned(with:), so the new rotatedToMinimizeTravel logic never runs. For example, BezierPath.interpolate(square, shiftedSquare, 0.5) still collapses/twists the square even though the transform pipeline pre-aligns the same pair correctly; this leaves the new “total” interpolate contract inconsistent for same-count closed shapes unless callers remember to pre-align manually.

Useful? React with 👍 / 👎.

Fixed in 1a4c470 — the structurally-matched fast path now rotates closed same-count subpath pairs with rotatedToMinimizeTravel before lerping, so direct interpolate(square, shiftedSquare, t) behaves like the transform pipeline. Pre-aligned inputs resolve to offset zero and pass through unchanged, so the per-frame cost for the transform hot path is one early-exiting scan. Test added: interpolateRotationAlignsSameCountClosedPaths (midpoint of the two same-square authorings must still span the full box instead of collapsing through the center).

Generated by Claude Code

chatgpt-codex-connector · 2026-07-07T02:30:22Z

+            // For closed pairs, rotate the counterpart's start point so
+            // matched control points travel the least during the morph.
+            if a[i].isClosed && b[i].isClosed && a[i].curves.count == b[i].curves.count {
+                b[i] = b[i].rotatedToMinimizeTravel(against: a[i])


Preserve trimmed-stroke start when rotating targets

When a transform target is a closed path with non-default strokeStart/strokeEnd, this rotation changes only the target path’s start point/parameterization. ManimScene.apply uses the aligned path for 0 < p < 1 but switches to the exact unrotated target at p >= 1, while ManimView trims strokes from the path start, so the visible segment can jump on the final frame whenever rotation chooses a nonzero offset.

Useful? React with 👍 / 👎.

Deferred to #16. Real, but only reachable when a morph pole carries a non-default strokeStart/strokeEnd (a partially drawn outline) — full strokes render identically under any cyclic offset. The clean fix is to skip rotation when either pole has a trimmed stroke, and that decision needs Mobject-level context in ManimScene.play (e.g. aligned(with:minimizeTravel:)), a file this PR doesn't touch and which #12 is concurrently restructuring — bolting it on here would manufacture a cross-PR conflict. Tracked with the full analysis and proposed plumbing in #16.

Generated by Claude Code

chatgpt-codex-connector · 2026-07-07T02:30:22Z

+        let rotated = Array(curves[bestOffset...] + curves[..<bestOffset])
+        return BezierPath.Subpath(curves: rotated, isClosed: isClosed)


Avoid rotating paths that close implicitly

For closed subpaths where isClosed supplies the final edge (last.p1 != first.p0), rotating the curve array can change the rendered geometry instead of just its start point. After a nonzero offset, the former implicit closing edge is no longer at the array boundary, and ManimView starts from the new first p0 while subsequent curves are appended from the current point, so a valid closed chain can become discontinuous during the morph.

Useful? React with 👍 / 👎.

Fixed in 1a4c470 — rotatedToMinimizeTravel now also requires the chain to close explicitly (last.p1 ≈ first.p0) and returns the subpath unchanged otherwise, so an implicit closing edge can never end up mid-array. All shape factories in the package close explicitly, so they keep the rotation benefit. Test added: implicitlyClosedSubpathsDoNotRotate.

Generated by Claude Code

- rotatedToMinimizeTravel now refuses chains whose closing edge is implicit (isClosed bridging last.p1 back to first.p0): rotating the array would move that gap into the middle of the outline (Codex). Pinned by implicitlyClosedSubpathsDoNotRotate. - interpolate's structurally-matched fast path rotates closed pairs to the least-travel matching, so directly interpolating two same-count shapes behaves like the aligned(with:) pipeline; pre-aligned inputs resolve to offset zero and pass through unchanged (Codex). Pinned by interpolateRotationAlignsSameCountClosedPaths.

ronaldmannak · 2026-07-07T02:42:22Z

@codex review

Generated by Claude Code

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a4c470745

ℹ️ 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".

chatgpt-codex-connector · 2026-07-07T02:49:38Z

+            // For closed pairs, rotate the counterpart's start point so
+            // matched control points travel the least during the morph.
+            if a[i].isClosed && b[i].isClosed && a[i].curves.count == b[i].curves.count {
+                b[i] = b[i].rotatedToMinimizeTravel(against: a[i])


Skip rotation for trimmed transform paths

When a transform target carries a partial stroke (strokeStart > 0 or strokeEnd < 1), rotating b here changes the path start used for every intermediate frame, while ManimScene.apply still interpolates the trim fractions independently and switches back to the exact unrotated target at p >= 1; ManimView then trims from that current path start. Transforming into a partially drawn closed shape authored from another corner will show the wrong edge during the morph and jump to a different edge at completion, so rotation needs to be disabled or coordinated with trim state for those poles.

Useful? React with 👍 / 👎.

Deferred to #16 (same root cause as the earlier trimmed-stroke thread on this line). This is the transform-pipeline manifestation: aligned(with:) rotating b while apply switches to the exact unrotated target at p >= 1 and ManimView trims from the path start. Only reachable when a morph pole carries a non-default strokeStart/strokeEnd; full strokes are offset-invariant. The fix — skip rotation alignment when either pole has a trimmed stroke — needs Mobject-level trim context threaded into aligned(with:minimizeTravel:) from ManimScene.play, which #12 is concurrently restructuring, so doing it here would manufacture a cross-PR conflict. Tracked with the full analysis and proposed plumbing in #16 (scope updated to cover both the direct-interpolate and transform-pipeline paths).

(Source: comment r3533251848 on PR #11 — please route further trimmed-stroke rotation findings to #16 rather than re-flagging here.)

Generated by Claude Code

ronaldmannak · 2026-07-09T04:00:33Z

Ready for human review/merge

Phase 2-A (arc-length morph resampling + total interpolate, closes #7 and #9) is merge-ready.

Focused diff — morph-quality only: CubicCurve.approximateLength, arc-length-weighted subdivision, squared-distance rotation alignment (explicit closures only), and a total non-recursive interpolate.
CI green — test-macos and test-linux both pass on the head commit (1a4c470).
Tests — weightedSubdivisionFollowsArcLength, closedSubpathRotationMinimizesTravel, interpolateRotationAlignsSameCountClosedPaths, implicitlyClosedSubpathsDoNotRotate, interpolateAlignsMismatchedInputsOnTheFly, plus the retained Phase 1 morph tests.
Automated review resolved — Codex reviewed the head commit; Gemini + Codex comments each answered individually (2 fixed, 1 declined with reasoning, 2 fixed on the follow-up round).
Deferred work tracked — the trimmed-stroke rotation edge case (only reachable with a partially drawn closed morph pole) is tracked in Morph rotation alignment can jump trimmed strokes at the final frame #16 with full analysis and proposed plumbing; it needs ManimScene context that Phase 2-B: MobjectGroup, layout helpers, and group animations #12 is concurrently restructuring.
PR description updated to match the implementation.

Independent — based on main. Not merging; awaiting your go-ahead.

Generated by Claude Code

gemini-code-assist Bot reviewed Jul 7, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

ronaldmannak mentioned this pull request Jul 7, 2026

Morph rotation alignment can jump trimmed strokes at the final frame #16

Open

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

ronaldmannak merged commit 947b9f8 into main Jul 9, 2026
2 checks passed

ronaldmannak deleted the claude/picomanim-phase2-01-morph branch July 9, 2026 04:02

		let rotated = Array(curves[bestOffset...] + curves[..<bestOffset])
		return BezierPath.Subpath(curves: rotated, isClosed: isClosed)

Uh oh!

Conversation

ronaldmannak commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Phase 2-A: Arc-length morph resampling + total interpolate

Scope

Public API

Design notes

Test strategy

Follow-ups

Concurrency

Stack

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

ronaldmannak commented Jul 9, 2026

Ready for human review/merge

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ronaldmannak commented Jul 7, 2026 •

edited

Loading

Phase 2-A: Arc-length morph resampling + total `interpolate`