Preserve motion binding order when removing motions

[eugene-doobu]

Summary

Replace swap-back removal in MotionStorage.RemoveAll() with stable batch compaction so unrelated removals cannot reverse the binding order of surviving motions.

The diff is limited to one runtime file, MotionStorage.cs (23 additions / 34 deletions). Preserve(), completed-motion binding, and handle lifetime are left unchanged. No new API, setting, dependency, per-component pool, or package version change is introduced.

Tradeoff: stable removal has a wider runtime scope than the Sequential symptom and increases the cost of sparse early deletions in large storages. The measurements and limitations below are part of this proposal, not a claim of a free performance improvement.

Problem and triggering conditions

A Sequential LitMotionAnimation can play consecutive motions that write to the same property, such as a linear float FadeIn (0 -> 1) followed by FadeOut (1 -> 0).

Each step's handle is preserved. A completed preserved step remains in storage, and UpdateRunner continues invoking its binding on subsequent updates. This lets the later step win while it is processed after the earlier step.

However, the original RemoveAt() fills a removed dense slot with the last entry. Removing an unrelated motion can therefore change survivor order:

Before removal:       [X, completed FadeIn, playing FadeOut]
Original swap-back:   [playing FadeOut, completed FadeIn]
Stable compaction:    [completed FadeIn, playing FadeOut]

With swap-back, FadeOut writes its interpolated value and the completed FadeIn immediately overwrites it with 1. The fade can appear absent even though its motion advances and completes.

The unrelated motion may target another object. Handles and targets are not mixed up: the error is the changed order of bindings that already target the same property. Sequential step-start order is distinct from the dense storage's binding order.

Deterministic reproduction

Using one ManualMotionDispatcher and the same float options, adapter, and scheduler:

1. Schedule a long-lived unrelated float motion X first.
2. Play a Sequential animation with two components binding the same float: FadeIn 0 -> 1 over exactly 1s, then FadeOut 1 -> 0 over 1s.
3. Call dispatcher.Update(1.0): FadeIn completes and FadeOut is registered.
4. Cancel X, then call dispatcher.Update(0.0) to remove it.
5. Call dispatcher.Update(0.25).

Expected: 0.75. Original upstream: 1. With this patch: 0.75.

The completed FadeIn binding still executes, its original handle remains active, and it is not canceled. It simply remains before FadeOut. Advancing the remaining 0.75s reaches 0, with one completion callback for each step in this reproduction.

Implementation

RemoveAll() now:

1. Returns immediately for an empty removal list.
2. Frees the removed sparse slots in the original list order, marks their lookup entries with the existing default/Version == 0 sentinel, and finds the first removed dense index.
3. Scans the affected suffix once, copying surviving unmanaged data, managed data, and sparse indices forward in their existing order.
4. Updates each moved survivor's sparse-slot DenseIndex.
5. Clears all three vacated array tails and updates the count.

The job-produced removal list contains unique dense indices but can be unordered because it is populated by parallel jobs. Marking all removals before moving data handles that ordering without sorting or another scratch buffer.

Compaction uses the storage's current tail, including motions appended by completion callbacks after job completion and before removal. The original free-list reuse order is retained.

The old RemoveAt() helper and temporary NativeArray<SparseIndex> copy are deleted. No additional allocation is introduced in the removal implementation.

Compatibility and scope

• Completed preserved motions continue binding; no cancellation or resource release is added on completion.
• Animation's playback-control and UpdateRunner binding/callback conditions are unchanged from upstream.
• Survivor handle identities are retained; moved dense indices are reflected in the sparse map, and freed handles are invalidated through the existing version mechanism.
• This changes survivor binding/delegate order after removal throughout each typed storage, not only for Sequential animations. Code that happened to depend on swap-back reordering will observe different ordering.
• It preserves relative order within a storage; it does not establish an ordering guarantee across different types or schedulers, or resolve arbitrary competing writers across those boundaries.
• The existing internal valid/unique removal-list precondition remains. Reentrant dispatcher reset during callback execution is not addressed.
• Only Core changes; Animation remains unchanged from upstream. No package versions are bumped.

Validation

Baseline: upstream ab6e92bfe78ff911def2fd3c4e9c79bb5a186946.

Environment: Windows, Unity Editor 6000.3.17f1 PlayMode, Test Framework 1.6.0, Collections 2.6.7, Burst 1.8.29, Mathematics 1.3.3; isolated local UPM harness.

• 6/6 focused local tests pass: Sequential order after unrelated removal; repeated completed binding/no automatic restart; explicit seek; Parallel binding behavior; first/middle/multiple removal; callback-created motion appended in the same update that collects removals.
• 48 deterministic invariant scenarios pass across 0/1/31/32/33/127 entries with empty/all/random unordered deletions: survivor order, handle-to-target mapping through Time, stale-handle rejection after slot reuse, original free-list reuse order, unchanged input list, and cleared tails.
• The three focused ordering cases and the invariant ordering check fail on unmodified upstream. The preservation-focused cases already pass there.
• 73 selected existing Core tests: 70 pass, 3 fail. The same three names and failure messages were confirmed on unmodified upstream:
  • CallbackTest.Test_WithCancelOnError: System.Exception : Test.
  • DelayTest.Test_Delay_SkipValuesDuringDelay: expected less than 0.899999976, actual 1.
  • SchedulerTest.Test_Scheduler_FixedUpdate: expected True, actual False.
• No C# compilation errors; git diff --check passes.

The focused fixtures, invariant fixture, and benchmark are local validation only and are not included in this runtime-only PR. The full upstream suite, Player/IL2CPP builds, and minimum supported dependency versions have not been validated. Both original and patched full harness runs also reported 55 persistent individual allocations on Editor shutdown; their underlying allocator-lifecycle cause is outside this change.

Performance tradeoff

Original removal is O(k) swap-back work for k removals. Stable removal is O(k + affected suffix), up to O(n) for a single early removal. This can be a meaningful regression when a large shared storage frequently loses entries near its front.

The following are isolated RemoveAll median times in microseconds, not whole-frame times or device results. The actual method was bound to a delegate once; reflection, motion creation/reset, and assertions were outside the timed region. Each case used 5 warmups and 31 samples, float motions with a shared no-op binding, and a frame boundary between samples to release the original temporary scratch allocation.

Motion count	Removed entries	Original median (us)	Patched median (us)
128	First 1	3.9	13.3
1,024	First 1	4.6	83.0
16,384	First 1	12.8	1,245.4
16,384	None	8.4	1.9
16,384	Last 1	12.5	8.9
16,384	Alternating half	3,008.0	2,291.5
16,384	All	5,834.6	3,280.4

For 16,384 motions / first-entry removal, isolated p95 increased from 15.0 to 1,751.0 us. A separate whole-ManualMotionDispatcher.Update measurement for that case increased from a median 1,040.3 to 1,868.9 us; job scheduling and host noise make those whole-update measurements less isolated.

Timed sections reported 0 current-thread managed allocated bytes in both implementations; this does not measure native or worker-thread allocations. Deletion-free and dense-removal cases can improve, but they do not negate the sparse-removal cost. These Editor microbenchmarks should be evaluated against representative Player workloads before adopting the global ordering change.