Phase 2-B: MobjectGroup, layout helpers, and group animations - #12
Phase 2-B: MobjectGroup, layout helpers, and group animations#12ronaldmannak wants to merge 5 commits into
Conversation
The VGroup counterpart. Groups are a build-time convenience with no runtime identity: group transformations return repositioned copies of the children (identity preserved) and group animations expand into per-child animations, so the timeline and renderer are untouched. - Mobject layout helpers: boundingBox, width/height, visual center, edge(_:), and nextTo(_:direction:gap:) placement. - MobjectGroup: union bounding box, shifted/moved/rotated/scaled about the group center, and arranged(direction:spacing:) row layout. - Group animation factories (create/fadeIn/fadeOut/shift/move/rotate/ scale/transform) returning [ManimAnimation]; a lag parameter staggers children via the new ManimAnimation.delay, and group transform pairs children by index, fading unmatched extras in or out. - New animation kinds rotateAbout/scaleAbout (also exposed on the single-mobject rotate/scale factories via an optional `about` pivot): positions orbit the pivot along a circular arc / move radially, rather than cutting across the chord. - ManimScene honors per-animation delay (entries start at group start plus delay; the group advances by the longest delay + duration) and gains a play overload accepting several animation lists at once. Covered by a new GroupTests suite (layout, arrangement, union bounds, staggered create, arc orbits, radial scaling, group morphs with unmatched extras, mixed group/single play). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
Leading-dot lookup resolves against the contextual type, and a play call expecting [ManimAnimation] looks members up on Array - static factories declared on ManimAnimation are invisible there, so scene.play(.create(group)) would not compile. The group factories now live in `extension Array where Element == ManimAnimation`, with inner per-child calls fully qualified (leading-dot inside map closures has no contextual base to infer from). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGb3ukB5dQy4vgz7Z1hags
![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: 4eff597f06
ℹ️ 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".
| duration: Double = 1, | ||
| rate: RateFunction = .smooth | ||
| ) -> [ManimAnimation] { | ||
| let delta = point - group.center |
There was a problem hiding this comment.
After any earlier animation has moved the children, this uses the original MobjectGroup value rather than the scene's current state to compute the delta. In a normal sequence such as scene.play(.shift(group, by: Vec2(10, 0))) followed by scene.play(.move(group, to: .zero)), ManimScene animates the live children from their shifted positions but this delta is still based on the old center, so the group ends offset instead of at the requested point. The same stale-center calculation is used for the rotate/scale pivots below.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30c1fb7. The move/rotate/scale factories no longer bake in a delta or pivot from the captured group value. They now emit group-relative kinds (groupMove/groupRotate/groupScale) that carry their sibling mobjects, and ManimScene.play resolves the group's bounding-box center from the live scene state (falling back to the carried snapshots for mobjects the scene hasn't met), rewriting them into concrete shift/rotateAbout/scaleAbout kinds before the entry is built — snapshot(at:) stays untouched. Tests added: groupMoveResolvesFromTheLiveCenter (shift then move-to-origin lands exactly at the origin) and groupRotateOrbitsTheLivePivot (half-turn after a shift swaps the dots about the live center instead of flinging them around the stale one).
Generated by Claude Code
| @@ -110,6 +116,7 @@ public struct ManimScene: Sendable { | |||
|
|
|||
There was a problem hiding this comment.
Once animations can start after groupStart, entries are no longer guaranteed to be chronological, but snapshot(at:) still folds them in append order and completed entries keep applying their final value. If a delayed animation on a mobject is listed before an immediate sibling that drives the same property, the immediate sibling is applied after the delayed one even after it has completed, so the delayed motion is overwritten instead of taking effect when its delay elapses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30c1fb7 — play now buffers the group's entries and appends them sorted by start time (stable on ties, preserving the documented "later listed wins" rule for simultaneous same-property animations). Since a play group's entries all start at or before the next group's start, sorting within the group keeps the whole timeline chronological, so the snapshot fold applies entries in the order they actually begin. Test added: delayedSiblingDrivesThePropertyAfterItsDelay (a delayed move listed before an immediate sibling on the same property takes over when its delay elapses).
Generated by Claude Code
| public func edge(_ direction: Vec2) -> Vec2 { | ||
| guard let box = boundingBox else { return position } | ||
| let half = (box.max - box.min) / 2 | ||
| return center + Vec2(direction.x * half.x, direction.y * half.y) |
There was a problem hiding this comment.
When callers pass a non-cardinal unit direction, such as Vec2.direction(.pi / 4), this multiplies the half-extents by about 0.707 and returns a point inside the box instead of on its edge. The same component-wise scaling is used by nextTo/arranged, so diagonal layouts with unit direction vectors place boxes too close and can overlap rather than sitting outside the previous mobject.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30c1fb7 — edge(_:) is now the point where the ray from the center exits the bounding box, so a unit diagonal and Manim-style Vec2(1, 1) both name the corner (direction magnitude no longer matters), and nextTo places this mobject's facing edge point gap away from the other's along the direction — cardinal placements are numerically unchanged, diagonals now meet corner-to-corner outside the box. Test added: diagonalEdgeAndNextToStayOnTheBox.
Generated by Claude Code
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
- Group move/rotate/scale factories now emit group-relative kinds (groupMove/groupRotate/groupScale) that carry their siblings; play() resolves the delta or pivot from the live scene state, so sequential group animations act on where the group actually is instead of the stale value the factory captured (Codex P1). Pinned by groupMoveResolvesFromTheLiveCenter and groupRotateOrbitsTheLivePivot. - play() buffers a group's entries and appends them sorted by start time (stable on ties), so a delayed animation listed before an immediate sibling on the same property takes over when its delay elapses instead of being overwritten by the finished sibling (Codex). Pinned by delayedSiblingDrivesThePropertyAfterItsDelay. - edge(_:) is now the ray-box exit point, so diagonal directions (unit or Manim-style (1,1)) name corners instead of interior points, and nextTo places facing edge points gap apart along the direction; cardinal behavior is numerically unchanged (Codex). Pinned by diagonalEdgeAndNextToStayOnTheBox.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c1fb7098
ℹ️ 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 indexed = newEntries.enumerated().sorted { | ||
| ($0.element.startTime, $0.offset) < ($1.element.startTime, $1.offset) | ||
| } | ||
| entries.append(contentsOf: indexed.map { $0.element }) |
There was a problem hiding this comment.
This sort only fixes the order used by snapshot(at:); currentStates has already been advanced in the original caller order in the loop above. When a delayed animation targeting the same property is listed before an immediate sibling, the rendered timeline can end at the delayed value, but state(of:) and any later play still start from the immediate sibling's value, causing follow-up animations to jump or use the wrong live state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4853453 — the build cursor is now advanced after the group's entries are sorted, folding them in the same chronological order snapshot(at:) uses, so state(of:) and follow-up plays agree with the rendered timeline. The delayed-sibling test now also asserts scene.state(of: dot) ends at the delayed animation's value.
Generated by Claude Code
| let ratioY = half.y > 0 ? abs(direction.y) / half.y : (direction.y == 0 ? 0 : .infinity) | ||
| let maxRatio = Swift.max(ratioX, ratioY) | ||
| guard maxRatio > 0, maxRatio.isFinite else { return center } | ||
| return center + direction / maxRatio |
There was a problem hiding this comment.
The current diff still computes a ray/box intersection, which does not return the documented Manim-style diagonal corner for non-square boxes; for example a 10×2 rectangle's edge(Vec2(1, 1)) returns the top edge near (1, 1) instead of the upper-right corner (5, 1). nextTo can then overlap differently shaped rectangles in diagonal layouts because the two objects may anchor to different edge axes rather than opposite corners.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4853453 — edge(_:) now uses Manim's critical-point semantics: each axis independently picks the box's min/center/max by the sign of that direction component, so Vec2(1, 1) (or a unit diagonal) on a 10×2 rectangle returns the true corner (5, 1). diagonalEdgeAndNextToStayOnTheBox now pins exactly that wide-rectangle case.
Generated by Claude Code
|
|
||
| /// Plays several animation lists (for example group animations, which | ||
| /// expand to one animation per child) together in one parallel group. | ||
| public mutating func play(_ animationGroups: [ManimAnimation]...) { |
There was a problem hiding this comment.
This overload only accepts variadic animation arrays, while the existing overload only accepts variadic single animations, so the intended mixed call shape scene.play(.create(group), .shift(solo, by: ...)) has no matching overload: the first argument is [ManimAnimation] and the second is ManimAnimation. Users have to manually wrap every solo animation in an array, which makes the advertised group+solo mixing API fail to type-check in normal use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4853453 — added play(_ animationGroup: [ManimAnimation], _ animations: ManimAnimation...), so scene.play(.create(group), .shift(solo, by: ...)) type-checks as advertised (group array first, any number of solo animations after). Arbitrary interleavings concatenate arrays (play(.create(a) + [b, c])), noted in the doc comment. The mixed test now uses the natural un-wrapped shape.
Generated by Claude Code
- edge(_:) uses Manim's critical-point semantics: each axis picks the box min/center/max by the sign of that direction component, so diagonals name true corners even for non-square boxes (Codex). Pinned by the wide-rectangle assertions in diagonalEdgeAndNextToStayOnTheBox. - The build cursor now advances over the group's entries in the same chronological order the snapshot fold uses, so state(of:) and follow-up plays agree with the rendered timeline when delayed siblings drive the same property (Codex). Pinned in delayedSiblingDrivesThePropertyAfterItsDelay. - New play overload ([ManimAnimation], ManimAnimation...) so the advertised mixed shape scene.play(.create(group), .shift(solo, ...)) type-checks without wrapping solos in arrays (Codex). Pinned by mixedGroupAndSingleAnimationsPlayTogether.
|
@codex review /gemini review Generated by Claude Code |
![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 MobjectGroup (the counterpart to Manim's VGroup) along with layout helpers, group-relative animations, and staggered animation support. It also updates ManimScene to resolve group animations against live scene state and handle delayed animations chronologically. The review feedback highlights a performance bottleneck in group center resolution where the group center is redundantly recalculated for each member, resulting in
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.
| var newEntries: [Entry] = [] | ||
|
|
||
| for animation in animations { | ||
| let id = animation.mobject.id | ||
| let entryStart = groupStart + max(0, animation.delay) | ||
| // Group-relative kinds carry their sibling mobjects; resolve the | ||
| // pivot or delta against the live scene state now, so a group | ||
| // animation acts on wherever the group actually is, not on the | ||
| // (possibly stale) group value the factory captured. | ||
| let kind = Self.resolvingGroupKind(animation.kind, states: groupStartStates) |
There was a problem hiding this comment.
Performance Bottleneck: $O(N^2)$ Complexity in Group Center Resolution
When playing a group animation with resolvingGroupKind is called for each member. Inside resolvingGroupKind, liveCenter is invoked, which maps all play call.
For larger groups (such as text with many characters or complex grids), this can cause a severe rendering bottleneck. We can optimize this to
| var newEntries: [Entry] = [] | |
| for animation in animations { | |
| let id = animation.mobject.id | |
| let entryStart = groupStart + max(0, animation.delay) | |
| // Group-relative kinds carry their sibling mobjects; resolve the | |
| // pivot or delta against the live scene state now, so a group | |
| // animation acts on wherever the group actually is, not on the | |
| // (possibly stale) group value the factory captured. | |
| let kind = Self.resolvingGroupKind(animation.kind, states: groupStartStates) | |
| var newEntries: [Entry] = [] | |
| var centerCache: [[Mobject.ID]: Vec2] = [:] | |
| for animation in animations { | |
| let id = animation.mobject.id | |
| let entryStart = groupStart + max(0, animation.delay) | |
| // Group-relative kinds carry their sibling mobjects; resolve the | |
| // pivot or delta against the live scene state now, so a group | |
| // animation acts on wherever the group actually is, not on the | |
| // (possibly stale) group value the factory captured. | |
| let kind = Self.resolvingGroupKind(animation.kind, states: groupStartStates, centerCache: ¢erCache) |
There was a problem hiding this comment.
Fixed in 632fba3 — play now carries a groupCenterCache keyed by the members' id list and threads it through resolvingGroupKind, so a group's bounding-box center is computed once and reused for all N members instead of recomputed per member. The dominant cost (transforming every child's curves + sampling its bounding box) drops from O(N²) to O(N) per play call. Pinned by twoGroupsInOnePlayResolveIndependentCenters, which also guards the cache keys against collisions when two distinct groups animate in the same play.
Generated by Claude Code
| private static func resolvingGroupKind( | ||
| _ kind: ManimAnimation.Kind, | ||
| states: [Mobject.ID: Mobject] | ||
| ) -> ManimAnimation.Kind { | ||
| func liveCenter(_ members: [Mobject]) -> Vec2 { | ||
| MobjectGroup(members.map { states[$0.id] ?? $0 }).center | ||
| } | ||
| switch kind { | ||
| case .groupMove(let point, let members): | ||
| return .shift(by: point - liveCenter(members)) | ||
| case .groupRotate(let angle, let members): | ||
| return .rotateAbout(pivot: liveCenter(members), by: angle) | ||
| case .groupScale(let factor, let members): | ||
| return .scaleAbout(pivot: liveCenter(members), by: factor) | ||
| default: | ||
| return kind | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance Bottleneck: $O(N^2)$ Complexity in Group Center Resolution
Update resolvingGroupKind to accept and utilize the centerCache to avoid redundant bounding box calculations and path transformations for members of the same group.
private static func resolvingGroupKind(
_ kind: ManimAnimation.Kind,
states: [Mobject.ID: Mobject],
centerCache: inout [[Mobject.ID]: Vec2]
) -> ManimAnimation.Kind {
func liveCenter(_ members: [Mobject]) -> Vec2 {
let ids = members.map { $0.id }
if let cached = centerCache[ids] {
return cached
}
let center = MobjectGroup(members.map { states[$0.id] ?? $0 }).center
centerCache[ids] = center
return center
}
switch kind {
case .groupMove(let point, let members):
return .shift(by: point - liveCenter(members))
case .groupRotate(let angle, let members):
return .rotateAbout(pivot: liveCenter(members), by: angle)
case .groupScale(let factor, let members):
return .scaleAbout(pivot: liveCenter(members), by: factor)
default:
return kind
}
}There was a problem hiding this comment.
Fixed in 632fba3 — resolvingGroupKind now takes centerCache: inout [[Mobject.ID]: Vec2] and its liveCenter memoizes the computed center per members-id list, exactly as suggested, so members of the same group reuse one bounding-box computation. (Companion to the reply on the caller-side thread above.)
Generated by Claude Code
|
Codex Review: Didn't find any major issues. 🎉 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". |
resolvingGroupKind computed the group's bounding-box center once per member, so a group animation with N members did O(N) work N times (O(N^2) per play call — costly for large groups like multi-character text). Cache the resolved center per members-id list within a play call, dropping the dominant bounding-box cost to O(N) (Gemini). Pinned by twoGroupsInOnePlayResolveIndependentCenters, which also guards the cache keys against collisions between distinct groups in one play.
|
@codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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/mergePhase 2-B (MobjectGroup, layout helpers, group animations) is merge-ready.
Based on Generated by Claude Code |
2 participants
Phase 2-B: MobjectGroup, layout helpers, and group animations
Scope
Manim's VGroup workflow: position mobjects relative to each other, treat a set of them as one unit, and animate them together (optionally staggered).
Mobject—boundingBox,width,height,center(visual bbox center),edge(_:)(Manim's critical point: each axis picks min/center/max by the direction component's sign, so diagonals name true corners), andnextTo(_:direction:gap:)(facing edge points placedgapapart along the direction).MobjectGroup— a value type wrapping[Mobject]with a unionboundingBox, groupcenter, and whole-group modifiers:shifted(by:),moved(to:),rotated(by:)/scaled(by:)about the group center, andarranged(direction:spacing:)(chainednextTo).[ManimAnimation]—.create(group, lag:),.fadeIn/.fadeOut(group, lag:),.shift,.move,.rotate,.scale, and.transform(group, into:)(pairs by index; extra sources fade out, extra targets fade in). Move/rotate/scale resolve the group's center from the live scene state when played (not the captured group value), so sequential group animations act on wherever the group actually is.playshapes —play([ManimAnimation]...)for several groups, andplay([ManimAnimation], ManimAnimation...)so the mixed shapescene.play(.create(group), .shift(solo, by: ...))type-checks; arbitrary mixes concatenate arrays.ManimAnimation.delay(per-animation start offset inside a play group; whatlagcompiles to),rotateAbout/scaleAboutkinds (.rotate(m, by:, about:)orbits the pivot along a circular arc;.scale(m, by:, about:)moves radially), and group-relative kindsgroupMove/groupRotate/groupScalethat carry their siblings and are rewritten into concrete kinds byplayafter resolving the live group center.Public API sketch
Design notes
[ManimAnimation]:scene.play(.create(group))resolves the leading dot against the parameter type[ManimAnimation], so the group factories must be declared in an extension of that array type — statics onManimAnimationreturning arrays would not be found by contextual lookup.playresolves the union-bbox center from the current build cursor (falling back to the carried snapshots for unseen mobjects) and rewrites them into concreteshift/rotateAbout/scaleAboutbefore the entry is built.snapshot(at:)never sees group kinds.delayvs nested timelines: a per-animation delay keepssnapshot(at:)a pure fold over flat entries.playappends a group's entries sorted by start time (stable on ties) and advances the build cursor in that same chronological order, so delayed siblings compose correctly with the documented "later wins" rule andstate(of:)agrees with the rendered timeline.rotateAboutorbits,scaleAboutmoves radially — matching Manim'sRotate(about_point:).Test strategy
GroupTests.swift: layout math (nextTo/edge/arranged, including diagonal corners on non-square boxes), union bbox, group modifiers about the center, staggered lag timing, arc-orbit and radial-scale midpoints, pivot rotation, group transform with mismatched counts, mixed group + solo play (natural un-wrapped shape), live-center move/rotate after earlier animations, and delayed-sibling chronology incl. the build cursor.Concurrency
Pure value types; no
@unchecked Sendable.Stack
Based on
main. PRs 2-D (Axes, #14) and 2-E (Updaters, #15) stack on this branch — please review/merge this one first among the three.