feat(animation): add sound playback mode selector#3210
Conversation
![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 updates the animation export logic to use onPlay instead of the legacy onStart for sound bindings, now including the loop property. Correspondingly, several tests in animation.test.ts that were previously skipped have been re-enabled, and a new test case was added to ensure non-loop sound bindings are correctly preserved during project export and load. I have no feedback to provide.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, focused revert of the temporary onStart workaround back to the intended onPlay format. The logic change is correct, the skipped tests are properly re-enabled, and the new round-trip test adds good coverage.
Note (out-of-diff): The ActionConfig.loop JSDoc at animation.ts:14 says "for onPlay defaults to true", but the load path uses onPlay?.loop ?? false — defaulting to false. Since exports now always write loop explicitly, this comment is inaccurate and should be updated to avoid misleading future readers (e.g. simply "Whether to loop the sound; defaults to false"). This line isn't in the diff but is worth a quick follow-up fix.
nighca
force-pushed
the
codex/issue-3208-onplay
branch
from
f4a7faf to
3815c91
Compare
nighca
force-pushed
the
codex/issue-3208-onplay
branch
from
3815c91 to
67e8bb4
Compare
This reverts commit 6fdacde.
nighca
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
nighca
force-pushed
the
codex/issue-3208-onplay
branch
from
e54ee9a to
6235f3e
Compare
nighca
force-pushed
the
codex/issue-3208-onplay
branch
from
6235f3e to
feea632
Compare
| for (const audio of audios) { | ||
| audio.muted = muted | ||
| } | ||
| if (props.sound != null) { |
There was a problem hiding this comment.
这里是特意判断 props.sound 是否为空而不是判断 audios 是否为空；因为 audios 为空有可能是在两个 audio 播放的间隙（playback: Once 的情况下），并不代表没有声音
| * Play the audio with `playback: AnimationSoundPlayback.Once`. | ||
| * The sound is triggered once per animation cycle (duration). | ||
| * If the previous sound is still playing when the next cycle starts, it will keep playing. | ||
| * In that case, multiple sound instances can overlap. |
There was a problem hiding this comment.
NIP: 意思是如果 animation duration 比 sound 时长短，就会有同一个 sound 的不同时段叠加播放，极限情况可能叠加 10 遍？这样设计是合理的吗
There was a problem hiding this comment.
如果 animation duration 比 sound 时长短，就会有同一个 sound 的不同时段叠加播放
是的
极限情况可能叠加 10 遍
在编辑器这边限制为 10，在 spx 运行的时候我不确定，可能没有限制
这样设计是合理的吗
它针对的是某个动作，其音效比视觉上的时间略久的情况；比如人物做一次攻击，1s 就做完动作了，但是对应的声音有可能是 1.2s，这后边的 0.2s 有可能是动作的尾音，这从效果上是合理的。如果人物很快又做一次攻击动作，那么两个声音发生重叠也是预期的。
因此一般来说选择 playback: once 的应该是
- 像攻击、跳跃这种（区别于待机、行走等）不会默认循环播放的动画
- 比较短的、跟动画本身时长在一个数量级的音效
所以实际上不会有问题
如果这里真的达到了到 10 的限制，那大概率是配错了，我没想到有什么合理的场景，是需要为一个 animation 绑一个时长是它 10 倍的 playback: once 的 sound 的
2 participants
Summary
soundLoopmodel flag withAnimationSoundPlayback.onStart.onPlay.loopoption from animation sound bindings.This PR covers the Builder-side UI/model work for #3208. The issue still also depends on a future SPX version with the updated
onStart/onPlayengine semantics, so this PR references the issue instead of closing it.Do not merge this PR yet. Keep it open until Builder upgrades to an SPX version that implements the behavior concluded in #3208 (comment); after that SPX upgrade lands, this PR can be merged.
Refs #3208
Tests