fix(particle): make size-over-lifetime TwoCurves random mode work#3060
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds SizeOverLifetime random-seed handling, updates shader curve-mode selection, writes SizeOverLifetime random values to shared particle slot 21 when noise is disabled, and adds deterministic unit and end-to-end coverage. ChangesSizeOverLifetime random seed integration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ParticleGenerator
participant SizeOverLifetimeModule
participant ShaderData
participant E2ERunner
ParticleGenerator->>SizeOverLifetimeModule: reset random seed
ParticleGenerator->>SizeOverLifetimeModule: request random size value
SizeOverLifetimeModule-->>ParticleGenerator: random value
ParticleGenerator->>ParticleGenerator: write shared slot 21
SizeOverLifetimeModule->>ShaderData: upload curve data and macros
E2ERunner->>ParticleGenerator: create mesh particle scenario
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3060 +/- ##
===========================================
+ Coverage 79.82% 85.41% +5.59%
===========================================
Files 904 810 -94
Lines 101297 94753 -6544
Branches 11423 11344 -79
===========================================
+ Hits 80865 80938 +73
+ Misses 20250 13727 -6523
+ Partials 182 88 -94
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 收益低于成本。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The mesh-path SOL shader rewrite (3447163) had no automated guard: unit tests only assert TS-side macro/upload/slot state, and no e2e case combined mesh render mode with an enabled sizeOverLifetime module. This case pins computeParticleSizeMesh end-to-end: separateAxes TwoCurves on all three axes with a fixed seed, so per-particle random size variation is baked into the baseline — before the fix every cuboid rendered at constant startSize. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@hhhhkrx 感谢指出,这个耦合确实存在,你的三条建议逐一回应: 现状与约束: 方案 2(shader 内从 seed 派生) 我们评估过:GPU 端需要浮点 hash,跨设备精度差异会导致分布不一致,且 sub-emitter inherit-size 的 CPU 路径( 方案 3(文档化 + 测试覆盖) 首个提交已包含: 同意不把共用固化为最终设计:这是槽位布局约束下的显式取舍,不是语义终态。独立 random slot(方案 1)留作后续扩展 instance 布局时的独立 PR 处理。 |
补充:mesh + SOL 的 e2e baseline(8e9560c → 88bc982)回应 @GuoLei1990 的 P2(mesh 路径 shader 修复无自动化守护、e2e 无 mesh+SOL 组合): 新增 排障记录(88bc982,供后人参考):该用例首版在 CI 曾 3×180s 静默超时(黑屏、无 console 报错),一度误归因为 SwiftShader 对特定宏组合的编译问题;经带打点的 probe 定位(#3072,已关闭),真正原因是:PR CI 构建的是与 dev/2.0 的 merge 产物,base 上 #3037 已移除 本地验证:用例连续两次运行像素级 match,Particle 分类 26/26 通过(新用例阈值 0.005 取既有 mesh 用例区间中位)。 另:codecov/patch 在纯 e2e 提交上假红(vitest 覆盖收集把 PR 描述中的测试措辞已按建议校准:单测覆盖 TS 侧宏/上传/slot 写入,shader 执行由本 e2e baseline 守护。 |
Vitest's coverage collection sweeps e2e/**/*.ts into the report at 0% (they only run under Playwright, never under the unit-test runner), so any e2e-only commit craters codecov's patch metric — this PR's baseline commit dropped it from 97% to 35% with zero source changes. Ignore the directory at the codecov layer; unit coverage semantics are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The RENDERER_SOL_IS_SEPARATE + RENDERER_SOL_IS_RANDOM_TWO macro combo hangs SwiftShader's shader compiler (evaluateParticleCurve inlined at 10 call sites in one program), so the case never reached initScreenshot on GPU-less CI runners while passing on real GPUs. Non-separate TwoCurves keeps the mesh path and the per-particle random mix under pixel guard; the separate combo stays covered by unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…call The earlier CI hangs had nothing to do with shader compilation: PR CI builds the merge with dev/2.0, where #3037 removed engine.canvas.resizeByClientSize (updating all existing e2e cases), so any newly added case still calling it died in an unhandled rejection before initScreenshot — silently, since the rejection never hits the console. Locally the branch's own build still had the API, which is why every variant passed here. With the call dropped, the full separateAxes three-axis TwoCurves case runs fine on CI, so restore it (macro coverage: CURVE_MODE + IS_RANDOM_TWO + IS_SEPARATE) along with its original baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
自上轮 review（tip 3447163a7）以来的增量是纯测试 / CI 基础设施，packages/ 源码（.ts / .glsl）零改动——compare 3447163a7...88bc9825d 只有 4 个文件：新增 e2e/case/particleRenderer-emit-mesh-size-over-lifetime.ts + e2e/config.ts 注册 + baseline 图 + codecov.yml。这个 commit 直接闭环我上轮唯一的 [P2]（mesh shader 修复无自动化守护、无 mesh+SOL 组合 e2e）。逐条核对后无 P0/P1/P2/P3 新问题，可合。
已关闭问题清单（本轮全部闭环）
- [P2 我上轮] mesh shader 修复无回归守护 → ✅ 已修：新 e2e 用例
mesh 渲染 + separateAxes 三轴 TwoCurves + 固定种子。逐链核实这是真反向证伪守护，非空壳：sizeX/Y/Z各用new ParticleCompositeCurve(curveMin, curveMax)（两ParticleCurve参）→ 构造器落mode = TwoCurves（ParticleCompositeCurve.ts:154-157）；separateAxes = true+ 三轴全 TwoCurves →_isRandomMode()返回 true（SizeOverLifetimeModule.ts:181-185）→isCurveModetrue → 三宏RENDERER_SOL_CURVE_MODE/IS_SEPARATE/IS_RANDOM_TWO全启用，正是重写后computeParticleSizeMesh依赖的宏组合。- noise 未启用 +
_isRandomMode()true → slot 21（a_Random0.z）写入_sizeRand.random()→ mesh shadermix(min,max,a_Random0.z)产出逐粒子尺寸差异。修复前 mesh 分支挂死宏 → 16 个 cuboid 全渲染成等大startSize(0.7)，与 baseline 像素差巨大。revert 任一 commit 的 fix（TS 侧或.glslmesh 重写）都会改变基线图 → 真守护成立。
- [P2 我上轮] PR 描述"覆盖 mesh separateAxes"措辞 → ✅ 已校准：现为"单测覆盖 TS 侧状态，mesh shader 路径由 e2e baseline 端到端守护"。
- [P3 我首轮] 缺 separateAxes 三轴 TwoCurves 用例 → ✅ 已覆盖（单测 + 本 e2e）。
- [hhhhkrx] SOL/Noise 共用
a_Random0.z耦合 → ✅ 作者解释成立（已闭环）：instance 布局已满（168B=42 floats，四分量全有主），独立 slot 需动整个布局超出本 bugfix scope；noise 优先已文档化 + 测试钉住；明示为布局约束下的显式取舍非语义终态。按已合理解释处理，不再重提。
核对要点
- e2e 确定性：
useAutoRandomSeed = false固定种子，SphereShape发射随之确定；作者报连续 2× 像素级 match，CI e2e 4/4 全绿。threshold:0+diffPercentage:0.005取既有 mesh 用例区间，零色差容忍 + 薄 buffer，与仓库惯例一致。 - CI-hang 根因已消除：final 用例无
resizeByClientSize调用（#3037已在 dev/2.0 移除该 API 而 PR CI 构建 merge 产物，旧版调用致 unhandled rejection 静默超时）。churn 两 commit（12c2cfca8drop separateAxes →88bc9825drestore + drop canvas API）净成预期三轴版，无残留。 codecov.ymlignoree2e/**是正确根修，非掩盖覆盖率：e2e 是截图场景用例，从不被单测覆盖，把e2e/**/*.ts以 0% 计入 report 会稀释 patch coverage（无源码行时 97%→35%）；只排除 e2e、不碰任何packages/源码，codecov 现全绿。- CI 全绿（build 3/3 + e2e 4/4 + codecov 全项 + lint）。
简化建议
无。本轮是纯回归网补齐，是净收敛。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
总结
自上轮 review（tip 88bc9825d）以来，1e5480c73 只是把最新 dev/2.0 合回分支；PR 自有净增量是 b1000bb3b 的谓词复用重构。逐链核对后，SOL 的 mode 判定、slot 21 写入、shader 宏/曲线上传、sub-emitter CPU 求值与 bounds 均保持原语义，先前三个 bugfix 和 mesh e2e 守护没有回归；CI 13/13 全绿。无 P0/P1，存在一项可在本 PR 内直接收敛的 P2 冗余。
已关闭问题清单
- mesh shader 无端到端守护：
88bc9825d的 mesh + separateAxes 三轴 TwoCurves e2e 已闭环，本轮仍有效。 - separateAxes 三轴 TwoCurves 单测缺口与 PR 描述措辞：均已闭环，本轮不重提。
- Noise/SOL 共用
a_Random0.z：instance 布局约束、noise 优先和测试契约均已解释并验证成立，本轮不重提。
问题
- [P2]
packages/core/src/particle/modules/ParticleCompositeCurve.ts:287— 新增的_isRandomCurveMode()只是mode === ParticleCurveMode.TwoCurves的单消费者包装；而同一个类型已经以mode持有权威事实，并有_isCurveMode()/_isRandomMode()两个通用分类。现在又新增第三个可机械推导的分类，并在ParticleCurve.test.ts复制四格真值表，增加了一个需要随 enum 演进同步的 owner，却没有减少 SizeOverLifetime 的模块级组合逻辑。建议保留ParticleCompositeCurve.mode为权威 owner，删除本次新增 helper 及四条对应断言，让SizeOverLifetimeModule._isRandomCurveMode()在唯一消费点直接判断各轴mode === TwoCurves（或由已有两个通用谓词机械组合）。删除后数据流仍是 curve mode → SOL 跨轴聚合 → slot/macro/shader，行为与现有 6 条 SOL 单测及 e2e 完全等价。
简化建议
除上述删除外无新增建议。SOL 模块级 helper 仍有两个真实消费者（shader 状态与新粒子 slot 写入），应保留；应删除的是 ParticleCompositeCurve 上这层仅服务它的额外包装。
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.Mesh render mode: SOL entirely dead (found in review, fixed in the second commit):
computeParticleSizeMeshwas gated on four macros (RENDERER_SOL_CURVE/RENDERER_SOL_RANDOM_CURVES/RENDERER_SOL_CURVE_SEPARATE/RENDERER_SOL_RANDOM_CURVES_SEPARATE) that no TS code has ever defined, and its random branch referenced a non-existent uniformu_SOLSizeGradientMax. Mesh particles ignored size-over-lifetime completely — plainCurvemode included — since Add new particle renderer #1682; the other half of the same port leftover.Fix
_sizeRandtoSizeOverLifetimeModule— theParticleRandomSubSeeds.SizeOverLifetimesub-seed already existed unused — plus_isRandomMode()/_resetRandomSeed(), registered in_resetGlobalRandSeed.a_Random0components taken), so the two modules share the slot per the existing shader layout; keeping noise first preserves the existing noise random sequence. This sharing is an explicit layout-constrained trade-off, not final semantics — decoupling (a dedicated random slot) is left to a follow-up layout extension (see discussion below).SizeOverLifetime.glslonto the same macro set as the billboard path (RENDERER_SOL_CURVE_MODE/IS_RANDOM_TWO/IS_SEPARATE), extended to three axes. Pure shader-side fix — the TS upload side already provided all six curve uniforms.Tests
tests/src/core/particle/SizeOverLifetime.test.tswith 6 cases covering the TS side (macro enabling, min/max curve upload, slot-21 writes): TwoCurves macros + upload, single-Curve regression, per-particle random written toa_Random0.z, slot untouched in non-random modes, noise precedence on the shared slot (same seed with/without SOL random yields the identical noise value), and mesh +separateAxesthree-axis TwoCurves.particleRenderer-emit-mesh-size-over-lifetime: mesh render mode +separateAxesthree-axis TwoCurves with a fixed seed — per-particle random size variation is baked into the image, so the mesh shader path (computeParticleSizeMesh) is guarded end-to-end across all three macros (RENDERER_SOL_CURVE_MODE/IS_RANDOM_TWO/IS_SEPARATE); unit tests assert TS-side state only, and before the fix this scene rendered uniformstartSizecuboids. No remote assets, deterministic across runs. (An earlier revision hung on CI — not a shader issue: the case calledengine.canvas.resizeByClientSize, which refactor(rhi-webgl): rework canvas resolution API, follow display size by default #3037 removed on dev/2.0 while PR CI builds the merge; dropped in 88bc982.) All pre-existing e2e cases use single-Curve SOL on billboard/stretched modes only, so no existing baselines are affected.🤖 Generated with Claude Code
Summary by CodeRabbit