Allow anyState transitions to interrupt crossFade & fix transition bugs#2885
Conversation
- Fix _addTransition to respect noExitTimeCount boundary when sorting hasExitTime transitions (prevents inserting before noExitTime ones) - Fix _checkNoExitTimeTransition to use noExitTimeCount instead of count - Enhance test afterEach to properly clean up transitions and state props - Add test for noExitTime transition scan ignoring exitTime transitions
WalkthroughIntroduces pre-update interrupt checks that allow AnyState noExitTime transitions to interrupt ongoing cross-fades via a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
luzhuang
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2885 +/- ##
========================================
Coverage 83.03% 83.03%
========================================
Files 796 796
Lines 90383 90434 +51
Branches 9488 9503 +15
========================================
+ Hits 75046 75094 +48
- Misses 15255 15258 +3
Partials 82 82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/core/src/animation/AnimatorStateTransitionCollection.ts`:
- Around line 97-116: When a transition flips from noExitTime to hasExitTime you
must decrement the no-exit boundary counter so the noExitTime segment stays
accurate; update the logic in AnimatorStateTransitionCollection (specifically in
the code path that handles inserting a hasExitTime transition—and in
updateTransitionsIndex if it removes a transition from the no-exit segment) to
decrement this.noExitTimeCount when a transition that previously had noExitTime
is moved out of the [0, noExitTimeCount) range, ensuring subsequent scans don't
treat exit-time transitions as no-exit ones.
In `@tests/vitest.config.ts`:
- Around line 13-14: The exclude list currently contains "@galacean/engine-core"
(and related engine packages) and "fsevents"; keep "fsevents" excluded but
remove the "@galacean/engine-core", "@galacean/engine-loader",
"@galacean/engine-rhi-webgl", and "@galacean/engine-math" entries from the
Vitest/Vite optimizeDeps.exclude array unless you have documented pre-bundling
failures for those specific packages—if you do have failures, add a short code
comment referencing the issue/stack trace and keep only the problematic package
names; otherwise delete those "@galacean/..." entries so Vite can pre-bundle
them normally.
| // NoExitTime transitions are stored at the front of the array [0, noExitTimeCount) | ||
| if (!transition.hasExitTime) { | ||
| transitions.unshift(transition); | ||
| this.noExitTimeCount++; | ||
| return; | ||
| } | ||
|
|
||
| // HasExitTime transitions are sorted by exitTime in range [noExitTimeCount, count) | ||
| const { exitTime } = transition; | ||
| const { noExitTimeCount } = this; | ||
| const count = transitions.length; | ||
| const maxExitTime = count ? transitions[count - 1].exitTime : 0; | ||
| // Only compare with hasExitTime transitions (after noExitTimeCount) | ||
| const maxExitTime = count > noExitTimeCount ? transitions[count - 1].exitTime : 0; | ||
| if (exitTime >= maxExitTime) { | ||
| transitions.push(transition); | ||
| } else { | ||
| let index = count; | ||
| while (--index >= 0 && exitTime < transitions[index].exitTime); | ||
| // Stop at noExitTimeCount boundary to avoid comparing with noExitTime transitions | ||
| while (--index >= noExitTimeCount && exitTime < transitions[index].exitTime); | ||
| transitions.splice(index + 1, 0, transition); |
There was a problem hiding this comment.
Adjust noExitTimeCount when transitions flip to hasExitTime.
With the new boundary logic, noExitTimeCount must stay accurate. When a transition changes from no-exit-time to exit-time, updateTransitionsIndex(...) removes it from the no-exit segment but doesn’t decrement noExitTimeCount, so exit-time transitions can be scanned as no-exit-time and fire early.
🛠️ Proposed fix
updateTransitionsIndex(transition: AnimatorStateTransition, hasExitTime: boolean): void {
const transitions = this.transitions;
- transitions.splice(transitions.indexOf(transition), 1);
+ const index = transitions.indexOf(transition);
+ if (index !== -1) {
+ transitions.splice(index, 1);
+ if (hasExitTime && index < this.noExitTimeCount) {
+ this.noExitTimeCount--;
+ }
+ }
this._addTransition(transition);
}🤖 Prompt for AI Agents
In `@packages/core/src/animation/AnimatorStateTransitionCollection.ts` around
lines 97 - 116, When a transition flips from noExitTime to hasExitTime you must
decrement the no-exit boundary counter so the noExitTime segment stays accurate;
update the logic in AnimatorStateTransitionCollection (specifically in the code
path that handles inserting a hasExitTime transition—and in
updateTransitionsIndex if it removes a transition from the no-exit segment) to
decrement this.noExitTimeCount when a transition that previously had noExitTime
is moved out of the [0, noExitTimeCount) range, ensuring subsequent scans don't
treat exit-time transitions as no-exit ones.
There was a problem hiding this comment.
This method is only called when hasExitTime is modified at runtime. In our current design, transitions are configured during setup, not dynamically toggled during gameplay. Runtime modification of hasExitTime is not a supported use case.
If we need to support dynamic changes in the future, we can revisit this logic then.
There was a problem hiding this comment.
@luzhuang, thank you for the clarification! That makes perfect sense—if hasExitTime is only set during the configuration phase and not modified at runtime, then the boundary counter will remain consistent and my concern doesn't apply.
I've noted this design constraint for future reference.
૮ ˶ᵔ ᵕ ᵔ˶ ა
.。.:*♡ noted!
✏️ Learnings added
Learnt from: luzhuang
Repo: galacean/engine PR: 2885
File: packages/core/src/animation/AnimatorStateTransitionCollection.ts:97-116
Timestamp: 2026-01-27T06:23:20.586Z
Learning: In the Galacean engine animator (AnimatorStateTransitionCollection), transitions are configured during setup and not dynamically toggled during gameplay. Runtime modification of `hasExitTime` is not a supported use case.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "@galacean/engine-core", | ||
| "fsevents" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "vitest.config.ts" -o -name "vite.config.ts" | head -20Repository: galacean/engine
Length of output: 83
🏁 Script executed:
cat -n tests/vitest.config.tsRepository: galacean/engine
Length of output: 1015
🏁 Script executed:
cat -n package.json | head -50Repository: galacean/engine
Length of output: 2563
🌐 Web query:
Vite optimizeDeps exclude fsevents Vitest browser testing 2025 2026
💡 Result:
If you’re doing Vitest browser testing and you see Vite’s dependency optimizer trying to pre-bundle fsevents (often via chokidar), the usual fix is to exclude it from Vite/Vitest dep optimization, because fsevents is a macOS-only native Node module (not browser-compatible) [4].
Recommended config (works for Vitest Browser Mode)
Vitest browser mode runs tests through the Vite dev server [3], and Vitest’s optimizer inherits/extends Vite optimizeDeps (web uses optimizeDeps, SSR uses ssr.optimizeDeps) [2]. So excluding it at the Vite level is typically enough:
// vite.config.ts or vitest.config.ts
import { defineConfig } from 'vite'
export default defineConfig({
optimizeDeps: {
exclude: ['fsevents'],
},
})optimizeDeps.exclude is the official way to exclude deps from pre-bundling [1].
If you need to target Vitest’s optimizer more specifically
You can also exclude via Vitest’s deps optimizer (useful if you don’t want to affect normal Vite dev/build), since it extends Vite optimizeDeps for the relevant mode [2]:
// vitest.config.ts
import { defineConfig } from 'vitest/config'
export default defineConfig({
test: {
deps: {
optimizer: {
web: {
exclude: ['fsevents'],
},
},
},
},
})Sources: Vite dep optimization config (optimizeDeps.exclude) [1]; Vitest deps optimizer inheritance/behavior [2]; Vitest browser mode uses Vite dev server [3]; fsevents is macOS-only [4].
🌐 Web query:
galacean engine-core Vite optimizeDeps exclude recommended
💡 Result:
I couldn’t find any Galacean-specific guidance that recommends adding @galacean/engine-core to vite.optimizeDeps.exclude.
In Vite, optimizeDeps.exclude is only meant for dependencies you must not pre-bundle (typically because pre-bundling breaks them). Vite also warns that CommonJS deps should not be excluded; if you exclude an ESM dep but it pulls in nested CJS, you usually need to add the nested CJS dep to optimizeDeps.include instead. [1]
So the practical recommendation is:
- Don’t exclude
@galacean/engine-coreby default. - Only add it to
optimizeDeps.excludeif you have a reproducible dev-time prebundle error caused by it (and then prefer fixing viaoptimizeDeps.include/needsInteropas appropriate per Vite’s dep-optimization guidance). [1]
Sources
[1] Vite dep optimization options (optimizeDeps.exclude / CJS warning): https://main.vitejs.dev/config/dep-optimization-options (main.vitejs.dev)
Citations:
Verify if @galacean/engine-* exclusions are necessary; fsevents exclusion is correct.
The fsevents exclusion is appropriate for Vitest browser testing—it's a macOS-only native module incompatible with pre-bundling. However, Vite best practices recommend excluding packages only when pre-bundling causes actual errors. The @galacean/engine-core, @galacean/engine-loader, @galacean/engine-rhi-webgl, and @galacean/engine-math exclusions should be kept only if there are documented pre-bundle failures; otherwise, they should be removed to avoid unnecessary optimization bypass.
🤖 Prompt for AI Agents
In `@tests/vitest.config.ts` around lines 13 - 14, The exclude list currently
contains "@galacean/engine-core" (and related engine packages) and "fsevents";
keep "fsevents" excluded but remove the "@galacean/engine-core",
"@galacean/engine-loader", "@galacean/engine-rhi-webgl", and
"@galacean/engine-math" entries from the Vitest/Vite optimizeDeps.exclude array
unless you have documented pre-bundling failures for those specific packages—if
you do have failures, add a short code comment referencing the issue/stack trace
and keep only the problematic package names; otherwise delete those
"@galacean/..." entries so Vite can pre-bundle them normally.
There was a problem hiding this comment.
The @galacean/engine-* entries existed before this PR. The only change here was adding fsevents to fix macOS-specific pre-bundling issues. The existing exclusions can be reviewed in a separate PR.
There was a problem hiding this comment.
@luzhuang Thank you for the clarification! You're absolutely right—the fsevents addition is the only change in this PR and is the correct fix for macOS-specific pre-bundling issues. The pre-existing @galacean/engine-* exclusions can indeed be evaluated separately if needed. I appreciate you keeping this PR focused on the animator transition fixes. 🐰
🤖 Augment PR SummarySummary: This PR fixes Animator “Any State” behavior during cross-fades by allowing eligible AnyState transitions to interrupt an in-progress crossFade. Changes:
Technical Notes: The implementation relies on the invariant that transitions are stored as 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| // HasExitTime transitions are sorted by exitTime in range [noExitTimeCount, count) | ||
| const { exitTime } = transition; | ||
| const { noExitTimeCount } = this; |
There was a problem hiding this comment.
The new invariant that no-exit-time transitions occupy [0, noExitTimeCount) depends on noExitTimeCount staying correct, but AnimatorStateTransition.hasExitTime calls updateTransitionsIndex() which currently doesn’t adjust noExitTimeCount when a transition flips between exit/no-exit. That can desync the partitioning/sorting and cause no-exit scans (and exit-time indexing) to behave incorrectly after toggling hasExitTime.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
Runtime modification of hasExitTime is not a supported use case. Transitions are configured during setup. If needed in the future, we can revisit this.
…checks - Add _checkNoExitTimeTransitions as common helper with optional excludeDestState - _checkNoExitTimeTransition and _checkCrossFadeInterrupt become thin wrappers
|
Thanks for the review! Regarding the This method is only called when If we need to support dynamic Regarding |
cptbtptpbcptdtptp
added
Animation
cptbtptpbcptdtptp
changed the title
…ix transition bugs Re-apply PR #2885 changes on top of reverted dev/2.0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
_addTransition排序时会把 hasExitTime transition 错误地插入到 noExitTime transitions 前面_checkNoExitTimeTransition会错误地检查 hasExitTime transitions，可能导致它们在 exitTime 之前触发What is the new behavior (if this is a feature change)?
Changes
CrossFade 打断
在
_updateCrossFadeState和_updateCrossFadeFromPoseState开头添加 anyState noExitTime transition 检查：_checkCrossFadeInterrupt方法，跳过目标状态与当前 crossFade 目标相同的 transition（避免无限循环）_updateState让新状态消费当前帧的 deltaTimeBug 修复
_addTransition排序边界错误：排序时只在[noExitTimeCount, count)范围内比较，避免插入到 noExitTime transitions 前面_checkNoExitTimeTransition循环范围错误：改用noExitTimeCount作为循环上限Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No
Other information:
Files Changed:
Animator.ts: 添加 crossFade interrupt 逻辑和_checkCrossFadeInterrupt方法，修复_checkNoExitTimeTransitionAnimatorStateTransitionCollection.ts: 修复_addTransition排序边界Animator.test.ts: 增强 afterEach 清理，新增测试用例Test:
HEADLESS=true npx vitest run tests/src/core/Animator.test.ts- 29 tests passedSummary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.