fix(particle): make size-over-lifetime TwoCurves random mode work#3060
fix(particle): make size-over-lifetime TwoCurves random mode work#3060cptbtptpbcptdtptp wants to merge 1 commit into
Conversation
Two stacked bugs since #1682 left SOL's random-between-two-curves mode entirely inert: an operator-precedence bug in _updateShaderData kept RENDERER_SOL_CURVE_MODE / RENDERER_SOL_IS_RANDOM_TWO from ever being enabled, and the per-particle random factor in instance slot 21 (a_Random0.z) was never written unless the noise module happened to be enabled (#2953 gated the shared slot's write on noise.enabled only). Fix the parenthesization, give SizeOverLifetimeModule its own Rand (using the already-reserved SizeOverLifetime sub-seed), and write the shared slot when SOL needs it; noise keeps precedence when both are on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThis PR extends ChangesSizeOverLifetime random seed integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ParticleGenerator
participant NoiseModule
participant SizeOverLifetimeModule
ParticleGenerator->>ParticleGenerator: _resetGlobalRandSeed(seed)
ParticleGenerator->>SizeOverLifetimeModule: _resetRandomSeed(seed)
ParticleGenerator->>ParticleGenerator: _addNewParticle()
alt noise.enabled
ParticleGenerator->>NoiseModule: get random strength value
NoiseModule-->>ParticleGenerator: random value
else SizeOverLifetimeModule._isRandomMode()
ParticleGenerator->>SizeOverLifetimeModule: _sizeRand.random()
SizeOverLifetimeModule-->>ParticleGenerator: random value
end
ParticleGenerator->>ParticleGenerator: write value into slot 21 (a_Random0.z)
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3060 +/- ##
===========================================
+ Coverage 79.37% 79.44% +0.06%
===========================================
Files 903 903
Lines 100632 100660 +28
Branches 11260 11274 +14
===========================================
+ Hits 79879 79971 +92
+ Misses 20569 20505 -64
Partials 184 184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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.
🧹 Nitpick comments (2)
tests/src/core/particle/SizeOverLifetime.test.ts (2)
77-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
separateAxes+TwoCurvescombination.
_isRandomMode()has a distinct branch requiring all three axes to beTwoCurveswhenseparateAxesis true, but no test exercisesseparateAxes = truewith random curves on x/y/z. This branch is part of the fixed precedence logic and would benefit from direct coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/particle/SizeOverLifetime.test.ts` around lines 77 - 189, Add a test in SizeOverLifetime.test.ts that covers the _isRandomMode branch for separateAxes=true with all three axes set to TwoCurves. Reuse createParticleRenderer, ParticleCompositeCurve, and the sizeOverLifetime setup, then enable separateAxes on the SOL module and assign random TwoCurves to x/y/z so the renderer exercises the distinct precedence path. Verify the expected SOL_CURVE_MODE_MACRO and SOL_RANDOM_TWO_MACRO behavior and that the random slot upload occurs as intended, similar to the existing TwoCurves and noise precedence tests.
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoding the particle instance stride here.
ParticleBufferUtilsisn’t part of the public API, so the test shouldn’t mirror42by hand; use a shared test helper or another source of the instance layout instead.
- Add a
separateAxes = truecase so_isRandomMode()covers the all-axes branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/particle/SizeOverLifetime.test.ts` around lines 20 - 21, The SizeOverLifetime test is hardcoding the particle instance stride and should instead derive it from a shared test helper or the instance layout used by particle buffers, so update the setup around FLOAT_STRIDE to avoid mirroring ParticleBufferUtils internals. Also extend the _isRandomMode() coverage in SizeOverLifetime by adding a separateAxes = true case so the all-axes branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/src/core/particle/SizeOverLifetime.test.ts`:
- Around line 77-189: Add a test in SizeOverLifetime.test.ts that covers the
_isRandomMode branch for separateAxes=true with all three axes set to TwoCurves.
Reuse createParticleRenderer, ParticleCompositeCurve, and the sizeOverLifetime
setup, then enable separateAxes on the SOL module and assign random TwoCurves to
x/y/z so the renderer exercises the distinct precedence path. Verify the
expected SOL_CURVE_MODE_MACRO and SOL_RANDOM_TWO_MACRO behavior and that the
random slot upload occurs as intended, similar to the existing TwoCurves and
noise precedence tests.
- Around line 20-21: The SizeOverLifetime test is hardcoding the particle
instance stride and should instead derive it from a shared test helper or the
instance layout used by particle buffers, so update the setup around
FLOAT_STRIDE to avoid mirroring ParticleBufferUtils internals. Also extend the
_isRandomMode() coverage in SizeOverLifetime by adding a separateAxes = true
case so the all-axes branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f245d376-8f8f-41a4-bdca-ab213fa6d856
📒 Files selected for processing (3)
packages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/modules/SizeOverLifetimeModule.tstests/src/core/particle/SizeOverLifetime.test.ts
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
修复 size-over-lifetime TwoCurves（random between two curves）模式两个叠加的 latent bug，方向正确、落点在根因层、测试是链路级带反向证伪。逐条核对代码后无 P0/P1/P2，可合。
两个 bug 都真实存在且已验证：
-
运算符优先级 bug（
SizeOverLifetimeModule._updateShaderData）—— 基线代码isRandomCurveMode || separateAxes ? A : B因优先级解析成(isRandomCurveMode || separateAxes) ? A : B。手工枚举四种情况确认：非 separate + TwoCurves →isCurveMode = (sizeX===Curve)= false（sizeX 是 TwoCurves）；separate + all-TwoCurves → 取 then 分支all===Curve= false。两种 random 路径下isCurveMode恒 false，RENDERER_SOL_CURVE_MODE/RENDERER_SOL_IS_RANDOM_TWO都不启用，模块自 #1682 起在 TwoCurves 模式完全 inert（连 max 曲线都被忽略），shader 的mix(min, max, a_Random0.z)分支是不可达死码。修复isRandomCurveMode || (separateAxes ? ... : ...)后四种情况全部正确，等价性已逐一验证。 -
缺 per-particle random 写入 —— slot 21（
a_Random0.z）自 #2953 起只在noise.enabled时写入，noise 关闭时恒为 0，导致 SOL random-two 的mix因子恒取 min 曲线。同一 slot 被_getParticleColorAndSize（sub-emitter inherit-size 的 CPU 路径，ParticleGenerator.ts:1372）读取，同样受影响。
已核实的正确性要点：
- slot 映射一致：
Random0是 byte offset 76 的 Vector4 → float offset 19，.x=19/.y=20/.z=21/.w=22。逐一对照 shader：ColorOverLifetime 用.y（slot 20）、Noise/SOL 用.z（slot 21）、RotationOverLifetime 用.w（slot 22），全部对齐。 - 共享 slot 优先级是既有 Vector4 布局约束（stride 168B = 42 floats 已满），非本 PR 引入；noise 优先保留既有 noise 随机序列，代码注释与 PR 描述都显式说明了这个取舍——同意这是合理设计，不是 bug。
_sizeRand/_resetRandomSeed/_isRandomMode()模式与 NoiseModule 逐字一致（_noiseRand = new Rand(0, ParticleRandomSubSeeds.Noise)+ 同形_resetRandomSeed），ParticleRandomSubSeeds.SizeOverLifetime = 0x591bc05c原本预留未用，此处正好接上。@ignoreClone与其它 rand 字段惯例一致。- 无 e2e baseline 影响：
git grep所有启用 SOL 的 e2e case（force / burst-cycles / rateOverDistance / fire / dream / sub-emitter…）全部用ParticleCurveMode.Curve单曲线，_isRandomMode()返回 false，新else if分支不触发、slot 21 保持不变，宏/上传行为零改动。sub-emitter case 走 CPU inherit-size 路径同样单曲线（忽略随机因子），无漂移。 - 反向证伪成立：revert 修复后，Test 1（
isEnable(SOL_CURVE_MODE_MACRO)断言 true）与 Test 3（verts[21] > 0断言）恰好各转红，其余 3 个 guard 测试两态皆过——与 PR 描述"exactly the two targeted cases fail"一致。测试从公开 API 输入、断言 macro/buffer 输出，是链路级测试。
问题
- [P3] tests/src/core/particle/SizeOverLifetime.test.ts — 缺
separateAxes = true+ 三轴全TwoCurves的用例。_isRandomMode()的 separate 分支（要求三轴都是 TwoCurves）目前无直接覆盖，与非 separate 分支对称、逻辑简单，补一条更完整（CodeRabbit 已提同一点）。不阻塞。
简化建议
无。fix 精准、无多余抽象，_isRandomMode() 抽取消除了 _updateShaderData 与 _addNewParticle 两处对"是否 random 模式"判断的潜在双源，反而更干净。
|
建议这里不要让 SizeOverLifetime 的 random 和 Noise 的 random 共用同一个 现在的实现里，Noise 和 SOL 从 Unity / Unreal 的语义看，更常见的是“同一个粒子有可复现 seed”，但不同属性/模块从 seed 派生各自的随机值；只有用户显式创建共享 random attribute 时，多个属性才共用同一个随机因子。这里的共用更像是 instance buffer slot 不够时的实现取舍，不太适合作为默认行为。 建议优先考虑：
这个点不影响本 PR 修复 “noise 关闭时 SOL random 恒为 0” 的方向，但我建议不要把共用 random 固化成最终设计。 |
3 participants
Problem
Size-over-lifetime's
TwoCurves(random between two curves) mode has never worked since it was introduced in #1682, due to two stacked bugs:Operator precedence bug in
SizeOverLifetimeModule._updateShaderData:isRandomCurveMode || separateAxes ? A : Bparses as(isRandomCurveMode || separateAxes) ? A : B, soisCurveModeis alwaysfalsewhen the mode isTwoCurves. NeitherRENDERER_SOL_CURVE_MODEnorRENDERER_SOL_IS_RANDOM_TWOever gets enabled — the module is entirely inert in TwoCurves mode (even the max curve is ignored), and the shader's random-two branch (mix(min, max, a_Random0.z)inSizeOverLifetime.glsl) was unreachable dead code.Missing per-particle random: instance-buffer slot 21 (
a_Random0.z) was reserved for the SOL random factor in Add new particle renderer #1682 (left as a commented-out placeholder in_addNewParticle), and bothSizeOverLifetime.glslandNoiseModule.glslsample it. Since feat(particle): add NoiseModule for simplex noise turbulence #2953 the CPU write is gated onnoise.enabledonly, so with noise disabled the factor is always 0 and size would stick to the min curve. The sub-emitter inherit-size CPU path (ParticleGenerator._getParticleColorAndSize) reads the same slot and was equally affected.Fix
_sizeRandtoSizeOverLifetimeModule— theParticleRandomSubSeeds.SizeOverLifetimesub-seed already existed unused — plus_isRandomMode()/_resetRandomSeed(), registered in_resetGlobalRandSeed.Tests
New
tests/src/core/particle/SizeOverLifetime.test.tswith 5 cases: macro enabling + min/max curve upload for TwoCurves, single-Curve regression (curve macro without random-two), per-particle random written toa_Random0.z, slot untouched in non-random modes, and noise precedence on the shared slot (same seed with/without SOL random yields the identical noise value).Red/green verified: with the source fix reverted, exactly the two targeted cases fail. Full particle suite (155 tests) passes with the fix. All existing e2e cases use single-Curve SOL only, whose macro/upload behavior is unchanged by this patch, so no visual baselines are affected.
🤖 Generated with Claude Code
Summary by CodeRabbit