[transition] retarget opacity transitions and make revival residue ownership explicit - #4
Merged
Merged
Conversation
When a removing renderable is revived, its in-flight remove animations are kept so an insert transition can compose additively and reverse from the current visual state. When no insert transition runs, nothing composes with the leftovers, and the additive remainder glides the renderable around its restored resting state (a visible overshoot for unclamped properties like position). Remove the root-layer animations on such revivals so the renderable snaps to its resting state, and document the insert/remove pairing contract on RenderableTransition.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4 +/- ##
==========================================
+ Coverage 93.61% 93.84% +0.23%
==========================================
Files 95 96 +1
Lines 5386 5526 +140
==========================================
+ Hits 5042 5186 +144
+ Misses 344 340 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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: 6990b9c55d
ℹ️ 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".
Stacked additive opacity animations do not compose on screen: the render server clamps opacity per animation, so an interrupt that stacks an opposing animation makes the rendered value diverge from the additive sum and jump. Rewrite the .opacity transition to retarget instead: evaluate the in-flight animations' current value and velocity, remove them, and continue with a single additive animation towards the new target, injecting the velocity into a spring's initialVelocity. The evaluation is backed by CABasicAnimation.scalarValue(at:), built on solveForInput(_:) on CAMediaTimingFunction (unit bezier solve) and CASpringAnimation (damped spring solve). Both mirror Core Animation's private _solveForInput: and are parity-tested against it, which surfaced that Core Animation clamps a spring's damping at critical and solves springs by fraction of duration. Hand a cancelled removal's residue to whoever takes over the renderable: with an insert transition, the insert observes the live in-flight state and establishes its own animation and model value (previously the cancel block reset the model first, corrupting what a retargeting insert sampled, which snapped revived renderables to full opacity); without one, the remove transition's resetForReuse undoes the model residue and its own animations at the revival site. The framework no longer strips all root-layer animations on such revivals, so unrelated content animations survive, and the slide transition gains a resetForReuse for its position animations. Add playground pages for interactive verification: a transition revival page (iOS) and a pure Core Animation additive-opacity demo (macOS).
A delayed opacity retarget used to stay scheduled with no way to cancel it. Retargeting is destructive when it fires: it removes the in-flight opacity animations (completing their transitions) and writes its own model value. A stale delayed retarget could therefore tear down a newer transition's animation, complete a removal with stale values (detaching and pooling the renderable), and then write an animation and model value onto the recycled layer. Store the pending retarget's timer on the layer, and cancel it whenever a new retarget starts or the transition's resetForReuse runs, so at most one scheduled retarget exists per layer. delay() now returns its timer for cancellation. Cancelling aborts the task even when the deadline has already passed but the task hasn't executed: libdispatch drops a pending unstarted event handler invocation on same-thread cancellation, which the new DelayTests pin.
When a removing renderable was revived, the revival skipped the remove transition's resetForReuse whenever any insert transition ran, assuming the insert would take over the removal's residue. The runtime pairing at a revival is the old item's remove and the new item's insert, which are not required to animate the same properties: an opacity removal revived under an insert that doesn't animate opacity left the model opacity at 0 with nothing ever restoring it, a permanently invisible renderable. Make the takeover an explicit InsertTransition capability instead: takesOverInFlightRemoval, defaulting to false. The revival calls the remove transition's resetForReuse unless the reviving insert declares the takeover, so custom and mismatched pairings snap to the resting state and stay correct by default. The opacity transition opts in: its insert retargets from the live in-flight state. The slide transition does not: its revival now resets its position leftovers and slides in cleanly instead of stacking on them. The resetForReuse contract is stated in terms of property ownership: a transition owns the root-layer animations of the key paths it animates, and leaves other properties' animations alone.
The retarget injected the interrupted velocity into the spring animation's initialVelocity after makeAnimation had already derived the animation's duration from the zero-velocity descriptor. A spring with a carried velocity settles later than the still spring, so the animation could be removed before the spring settled and snap to the model value. The injected magnitude was also unbounded (velocity over an arbitrarily small delta) and ignored the timing's speed. Carry the velocity through the SpringDescriptor instead, before makeAnimation runs, so the derived duration accounts for the spring that actually plays. The conversion divides by the timing's speed (a scaled time space needs a scaled local-time velocity for the same wall-clock rate) and drops the carry for a from-to distance below 1% of opacity, where the normalized velocity diverges and momentum is imperceptible. This also removes the CASpringAnimation cast and the spring special case from the animation update closure.
An in-flight removal was tracked across three parallel dictionaries keyed by the same node id (the renderable, its remove transition, and its completion), inserted and cleared in lockstep at four sites, and the revival site read the transition through an optional that could never be nil. Track one RemovingRenderable record instead, carrying the renderable, the remove transition, and the completion. The lockstep invariant is enforced by construction: one insertion at removal, one removal in each completion path, and a non-optional transition at the revival site. Cancelling the record's completion both cancels the removal and clears it from the map. The test hook shrinks from three accessors to one.
Core Animation resolves an animation's begin time when the transaction commits: an animation added in the current run loop turn still has a zero begin time. scalarValue(at:) treated that zero as an epoch, so the elapsed time computed against a real layer-time query was huge, the animation evaluated as finished, and a retarget interrupting a transition started in the same run loop turn sampled the model value instead of the animation's start value, reproducing the snap the retargeting exists to remove. Evaluate a zero begin time at zero elapsed time, matching Core Animation's "begin as soon as committed" convention. Existing tests move their epochs off zero accordingly. Also harden the private-API parity tests: the trampoline validates _solveForInput:'s type encoding before the call so an ABI change reads as unavailable instead of a mismatched-convention call, the parity loops skip when the private implementation is unavailable, and a canary test fails loudly so the parity cannot silently stop being verified.
…ites Add CALayer.basicAnimations(forKeyPath:) and removeBasicAnimations(forKeyPath:) as the canonical key-path animation queries, replacing four copies of the same filter loop across the opacity transition, the slide transition, and tests. Route the opacity transition's model writes (the resetForReuse restore and the delayed-insert hide) through setKeyPathValue, which keeps a view-backed renderable's alpha in sync with the layer opacity. interruptedOpacityState() is now a pure query, with the animation removal an explicit call at its single call site. Add the remaining behavior tests: the opacity resetForReuse contract (own residue undone, other properties' animations survive) and the production-path immediate interrupt, which evaluates an animation whose begin time Core Animation hasn't resolved yet. Playground: invalidate the revival page's sampling timer when the view leaves the window, log with layer-converted times, and move the macOS additive-opacity demo behind a Demos menu instead of opening at launch.
honghaoz
changed the title
…sition A revival reset whenever the reviving insert didn't declare the boolean takeover, which broke the slide transition's remove-then-insert: the revival stripped the in-flight slide-out and the insert re-entered from offscreen, teleporting the renderable instead of continuing its motion. For an unclamped additive property like position, keeping the leftover is the correct takeover: the insert's starting delta exactly compensates the model change, so the rendered position is continuous at the revival instant and the two animations compose into the new motion. A boolean takeover cannot express this safely, because slide opting in would also claim removals it cannot take over (an opacity removal revived by a slide insert would strand invisible). Scope the takeover by key paths instead: a remove transition declares the key paths it animates, an insert transition declares the key paths it takes over, and the revival resets unless every animated key path is taken over. An unknown (empty) animated set always resets, keeping custom transitions safe by default. The opacity transition declares "opacity" on both halves and keeps retargeting. The slide transition declares "position" on both halves and composes additively on revival.
resetForReuse required every remove transition to hand-remove the animations of the key paths it had already declared in animatedKeyPaths: the slide transition's entire reset closure was a transcription of its own declaration, a transition declaring key paths without a closure silently left animations in flight on reset, and the public contract instructed third-party authors to perform a removal whose canonical helper is internal to the package. Make the declaration load-bearing: resetForReuse(renderable:) removes the root-layer animations of animatedKeyPaths itself, then runs the closure, whose job shrinks to restoring the model values the transition wrote. The slide transition's closure disappears, and the opacity transition's shrinks to cancelling its pending retarget and restoring the model opacity. The removal helper widens from CABasicAnimation to CAPropertyAnimation, so a keyframe animation on an owned key path no longer survives a reset.
…veTransition An unknown animated footprint was encoded as an empty set, which fights set algebra (an empty set is a subset of everything), forcing the revival site to invert the coverage check inline, and permanently stealing the meaning of a genuinely empty footprint. Encode unknown as nil instead, and move the takeover decision onto RemoveTransition as isTakenOver(by:), next to the data it compares. The predicate requires a known, non-empty footprint fully covered by the insert transition's takesOverKeyPaths: an unknown footprint can't be verified, and an empty footprint has nothing to continue (resetting it is harmless, and protects a removal that writes model values without animations). The revival site shrinks to a single predicate call.
…ion does The interrupted-state sampler summed only additive animations and treated everything else as contributing nothing, while the retarget removed all of them. A foreign non-additive opacity animation was therefore destroyed and its value ignored, so the retarget started from the model value instead of the opacity on screen: a snap produced by the code that exists to prevent snaps. Compose the animations in key order the way Core Animation applies them: an additive animation contributes on top of the composed value, a non-additive one replaces it. An animation the evaluator can't read is flagged with an assertion instead of silently contributing zero. Velocity is now dropped when it pushes past a saturated bound, where the motion isn't visible and carries no momentum, so it can't be injected into a spring as overshoot outside [0, 1]. A pending delayed retarget also counts as in-flight state: reviving during a delayed removal's wait window used to find no animations and restart from the transition's fresh start value, snapping a fully visible renderable invisible before fading it in. The retarget now continues from the current opacity, which also lets the delayed "hold at the start value" behavior live in one place instead of being decided twice at different instants. The velocity-carry threshold and the finite-difference sampling interval become documented constants.
A slide revival is continuous because the insertion's entry offset cancels the model position change, leaving the leftover exit offset to decay: the rendered position doesn't move at the revival instant. The two offsets only cancel when they are the same distance in the same direction, which holds when the renderable enters from the side it exits to. A cross-side slide (e.g. from .left, to .right) has opposing offsets, so the unconditional takeover jumped the renderable by nearly two content widths and then drifted as the leftover decayed. Declare the takeover only for a slide that enters from the side it exits to. A cross-side revival resets the leftover instead and slides in from its entry side, which is what the configuration asks for. The playground's transition button now cycles through the three revival behaviors (fade retarget, same-side slide continuation, cross-side slide reset) so each can be verified by hand, and skips its sampling timer where the DEBUG-only debug events that track the layer are unavailable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
.opacitytransition keeps a single opacity animation per renderable: starting a transition while another is in flight evaluates the in-flight animations' current value and velocity, removes them, and continues with one additive animation towards the new target. Spring timings carry the interrupted velocity through theSpringDescriptorbefore the animation is made, so the derived duration accounts for it. In-flight animations compose the way Core Animation applies them (additive contributes, non-additive replaces), and a pending delayed retarget counts as in-flight state.CABasicAnimation.scalarValue(at:)evaluates an animation's value at a given time, backed bysolveForInput(_:)onCAMediaTimingFunction(unit-bezier solve) andCASpringAnimation(closed-form damped spring). Both mirror Core Animation's private_solveForInput:and are parity-tested against the real private implementation in the test target (trampoline with type-encoding validation, skip-when-unavailable, and an availability canary). A zerobeginTimeis treated as unscheduled, matching Core Animation's "begin as soon as committed" convention.RemoveTransitiondeclaresanimatedKeyPaths(its root-layer footprint,nilwhen unknown); anInsertTransitiondeclarestakesOverKeyPaths(the in-flight removal state it can continue from).RemoveTransition.isTakenOver(by:)decides a revival: the residue is left for the insert transition only when it covers every animated key path, otherwiseresetForReusesnaps the renderable to its resting state.resetForReuseremoves the animations ofanimatedKeyPathsitself, so a transition only restores its own model values, and animations of other properties (e.g. a renderable's own content animation) always survive..opacitytakes overopacityby retargeting..slidetakes overpositionby additive composition, where the entry offset cancels the model change, and only for a slide that enters from the side it exits to.delay()returns its timer; each retarget cancels a pending one, andresetForReusecancels too, so at most one scheduled retarget exists per layer.RemovingRenderablerecord.Why
Stacked additive opacity animations do not compose on screen: the render server clamps opacity per animation while compositing, so opposing stacked animations make the rendered value diverge from the unclamped sum that
presentation()reports. Interrupting a fade (remove during insert, or re-insert during remove) produced visible opacity jumps. Retargeting preserves value continuity, and for springs momentum continuity, at every interrupt.The ownership work fixes a family of revival bugs: resetting the model opacity before a retargeting insert sampled the in-flight state snapped revived renderables to full opacity; inferring the takeover from "an insert transition exists" stranded mismatched pairings (an opacity removal revived under an insert that doesn't animate opacity stayed invisible); an unconditional reset teleported a revived slide instead of continuing its motion; and an unconditional slide takeover jumped a cross-side slide by nearly two content widths. Scoping the takeover by key paths, and deciding it per pairing, handles all of them with a safe default for custom transitions.
Findings from the
_solveForInput:parity tests, mirrored by the implementation: the spring solve takes a fraction ofduration(a spring animation is its own timing curve over its duration), Core Animation clamps a spring's damping at critical (an overdamped configuration behaves as critically damped), and the private spring solver initializes whendurationis set.Known limitations
animate()to Core Animation's nativebeginTimescheduling removes the timer entirely and is deferred to a follow-up.How to test
cd ComposeUI && swift test— 767 tests, 0 failures. Each commit builds and passes the full suite standalone.