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 2 commits 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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds dedicated SizeOverLifetime random-seed handling, updates shader curve-mode selection, writes SizeOverLifetime random values to the shared particle slot when noise is disabled, and adds deterministic coverage for shader and particle-buffer behavior. 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.75% +0.37%
===========================================
Files 903 904 +1
Lines 100632 101275 +643
Branches 11260 11385 +125
===========================================
+ Hits 79879 80770 +891
+ Misses 20569 20323 -246
+ Partials 184 182 -2
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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
建议这里不要让 SizeOverLifetime 的 random 和 Noise 的 random 共用同一个 现在的实现里，Noise 和 SOL 从 Unity / Unreal 的语义看，更常见的是“同一个粒子有可复现 seed”，但不同属性/模块从 seed 派生各自的随机值；只有用户显式创建共享 random attribute 时，多个属性才共用同一个随机因子。这里的共用更像是 instance buffer slot 不够时的实现取舍，不太适合作为默认行为。 建议优先考虑：
这个点不影响本 PR 修复 “noise 关闭时 SOL random 恒为 0” 的方向，但我建议不要把共用 random 固化成最终设计。 |
computeParticleSizeMesh was gated on four macros (RENDERER_SOL_CURVE, RENDERER_SOL_RANDOM_CURVES, RENDERER_SOL_CURVE_SEPARATE, RENDERER_SOL_RANDOM_CURVES_SEPARATE) that no TS module ever enables — the random branch even referenced a nonexistent uniform (u_SOLSizeGradientMax) — so the module was entirely inert for mesh-mode particles. Rewrite it against the macro set the billboard path and SizeOverLifetimeModule already use, extended to all three axes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cptbtptpbcptdtptp
left a comment
cptbtptpbcptdtptp
left a comment
There was a problem hiding this comment.
CR 结论
逐条核对了 diff 与既有评审意见。billboard 路径的修复本身验证无误:宏四态枚举正确、slot 21 写入条件与宏启用条件一致(enabled && _isRandomMode())、curveMin 裸调与 Force/Noise/Rotation/Velocity 等模块的既有 GIGO 约定一致、与 #3049 无 instance-slot 交叠、_resetGlobalRandSeed 各子种子独立顺序无关。
新发现:P2 — mesh 渲染模式下 SOL 整体失效(已修,3447163a7)
computeParticleSizeMesh(SizeOverLifetime.glsl)gate 在 RENDERER_SOL_CURVE / RENDERER_SOL_RANDOM_CURVES / RENDERER_SOL_CURVE_SEPARATE / RENDERER_SOL_RANDOM_CURVES_SEPARATE 四个宏上——全仓 TS 侧从未定义过任何一个(SizeOverLifetimeModule 只启用 RENDERER_SOL_CURVE_MODE / IS_SEPARATE / IS_RANDOM_TWO),random 分支还引用了不存在的 uniform u_SOLSizeGradientMax。即 mesh 粒子的 SOL 自 #1682 起完全 inert(不止 TwoCurves,单 Curve 也一样)——与本 PR 修的 billboard 优先级 bug 是同一次移植的另一半遗留。base 里 _evaluateOverLifetime 的 CPU inherit-size 路径按 "shader gates on RENDERER_SOL_CURVE_MODE" 假设工作,mesh 死宏还造成 CPU/GPU 不一致。
修复:按 billboard 路径的宏体系重写 mesh 分支(扩展到三轴)。uniform 声明块与 TS 上传侧本来就齐备(X/Y/Z min/max),纯 shader 侧修复。
Baseline 影响:9 个 mesh e2e case 虽解构了 sizeOverLifetime 变量但没有一个启用它,修复不影响任何现有 e2e baseline。
验证:
- 新增用例 mesh + separateAxes + 三轴 TwoCurves:宏(CURVE_MODE/IS_RANDOM_TWO/IS_SEPARATE)、三轴 min/max 曲线上传、slot 21 写入全断言通过;粒子套件 157/157 过。
- 真机像素级验证(examples + WebGL readPixels,双系统对比):SOL 关的对照组粒子横截面恒定 33px;SOL TwoCurves 组尺寸随生命周期 7px→77px 增长且个体间随机抖动,shader 编译零错误。修复前该组与对照组行为相同(恒定)。
对既有意见的处理
- GuoLei1990 / CodeRabbit 的 P3(缺 separateAxes+TwoCurves 覆盖):新增的 mesh 用例即 separateAxes=true + 三轴 TwoCurves,
_isRandomMode()的 separate 分支已获直接覆盖。 - CodeRabbit 的 stride 硬编码(Trivial):维持现状——
ParticleBufferUtils.instanceVertexFloatStride非公开 API,42 与 stride 168B 的注释已互为印证,引入访问私有工具类的 helper 收益低于成本。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
新增 commit 3447163a7（"make size-over-lifetime work in mesh render mode"）把 computeParticleSizeMesh 从一套没人启用的旧宏（RENDERER_SOL_CURVE / RENDERER_SOL_RANDOM_CURVES / RENDERER_SOL_CURVE_SEPARATE / RENDERER_SOL_RANDOM_CURVES_SEPARATE，且随机分支引用了不存在的 uniform u_SOLSizeGradientMax）重写到 billboard 路径与 SizeOverLifetimeModule 已在用的新宏集（RENDERER_SOL_CURVE_MODE / RENDERER_SOL_IS_RANDOM_TWO / RENDERER_SOL_IS_SEPARATE），并扩到三个轴。方向正确、在本 PR scope 内（首个 commit 修好了 billboard 却漏了 mesh，这次补齐），逐行核对后 shader 逻辑无 P0/P1，可合。
核对要点：
- 旧 mesh 分支确是死码：全仓（TS/core）无任何模块设置那四个旧宏——只有 base 树上残留的
.glsl文本引用它们；u_SOLSizeGradientMax在别处零声明，若那条RENDERER_SOL_RANDOM_CURVES分支曾被启用会是编译错误。所以 mesh 模式在 TwoCurves / 甚至普通 Curve 下此前完全 inert，重写是修真 bug 不是重构。 - 与 billboard 路径逐字对齐：新 mesh 函数结构与
computeParticleSizeBillboard完全一致，差异仅在 mesh 处理 Z 轴（vec3）而 billboard 只处理 X/Y（vec2）——这是 3D vs 2D 的正确区分。非 separate 分支size *= lifeSizeX对 vec3 三分量等比缩放，与旧RENDERER_SOL_CURVE的标量广播语义等价。 - CPU 路径与 bounds 一致：
_getParticleColorAndSize（sub-emitter inherit-size，ParticleGenerator.ts:1375）separate 模式已按sizeX/Y/Z.evaluate逐轴、非 separate 单因子，与新 shader 语义一致；_updateBounds（:1595）separate 模式已Math.max(sizeX/Y/Z._getMax())取每轴最大做保守包围盒，覆盖新的逐轴 mesh 行为。两处均为 base commit 既有代码，本 commit 未动，语义吻合。 - 宏集与
SizeOverLifetimeModule._updateShaderData声明的三个宏一一对应；renderer_SOLMaxCurveY/Z、renderer_SOLMinCurveY/Z仅在RENDERER_SOL_IS_SEPARATEguard 内声明/使用，无越界。 - CI 全绿（build 三平台 + e2e 4/4 + lint + codecov）。
问题
- [P2] tests/src/core/particle/SizeOverLifetime.test.ts:193 — 新测试无法反向证伪本次 shader 修复。它断言的三项——macros（
SOL_CURVE_MODE/SOL_IS_RANDOM_TWO/SOL_IS_SEPARATE）、六条 curve 数组、slot 21——全部由 TS 侧 (_updateShaderData/_addNewParticle) 产出，且在 separate 模式下本就正确；把本 commit 的.glsl改动整体 revert，这个测试仍全绿。也就是说真正被修的computeParticleSizeMesh死宏问题没有任何自动化守护：.glsl在 jsdom 里跑不了、CodeRabbit 也按!**/*.glsl排除，而 e2e 里无一 case 组合 mesh 模式 + SOL（10 个 mesh e2e 全部没sizeOverLifetime.enabled = true，仅从 generator 解构未启用；6 个启用 SOL 的 e2e 全是 billboard/stretched 单 Curve）。建议补一个 mesh + SOL 的 e2e baseline（哪怕单 Curve），让这条 shader 路径真正进回归网——否则下次误改 mesh 宏又会静默 inert。这条不阻塞合并，但 PR 描述里"新增 test 覆盖 mesh separateAxes"的措辞应校准为"覆盖 TS 宏/上传/slot 写入"，shader 执行本身仍靠 code review 而非测试保障。
简化建议
无。mesh 与 billboard 现在共享同一套宏和同构写法，双源真相被消除（旧 mesh 那套 4 宏 + 幽灵 uniform 彻底删除），是净收敛。若想更进一步，billboard 与 mesh 两个函数体除 vec2/vec3 与 Z 轴外几乎逐行相同，未来可考虑抽公共 helper，但当前重复度低、GLSL 无泛型，不值得为此引入宏拼接，保持现状即可。
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