Particle system add ForceOverLifetime module
#2584
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.5 #2584 +/- ##
===========================================
- Coverage 68.94% 68.49% -0.46%
===========================================
Files 960 962 +2
Lines 100199 100654 +455
Branches 8646 8654 +8
===========================================
- Hits 69087 68940 -147
- Misses 30852 31451 +599
- Partials 260 263 +3
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:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (21)
packages/core/src/particle/enums/ParticleRandomSubSeeds.ts (1)
20-20: Consider removing or reintroducing commented enumeration.
This comment hints at a future extension. IfForceOverLifetimeis indeed planned, reintroduce it as a valid enum field and ensure it’s used. Otherwise, removing the commented line helps keep the enum tidy.packages/core/src/particle/modules/ForceOverLifetimeModule.ts (3)
27-29: Remove or implement commented-out_forceRand.
Leaving_forceRanddeactivated can create confusion or false expectations. If randomness is needed for force calculations, re-enable it with test coverage; otherwise, consider removing it.
103-156: Add unit tests for_updateShaderDatain all curve modes.
Coverage tools show that these lines lack test coverage. Verify correct behavior of each branch (random curves, single curve, and constants) to ensure shader macros and properties are set correctly.Would you like help drafting test scaffolding to validate the different modes?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-154: packages/core/src/particle/modules/ForceOverLifetimeModule.ts#L109-L154
Added lines #L109 - L154 were not covered by tests
165-170: Verify proper event deregistration to avoid memory leaks.
This method swaps event handlers when curve references change. Repeated property changes must not cause orphaned references.e2e/case/particleRenderer-dream.ts (10)
93-94: Add tests for newly introduced ForceOverLifetime.
No functional concerns, but CodeCov warns these lines are uncovered. Consider adding e2e or unit tests to ensure coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-94: e2e/case/particleRenderer-dream.ts#L93-L94
Added lines #L93 - L94 were not covered by tests
148-153: Consider factoring out repeated code for ForceOverLifetime setup.
This pattern of enabling and initializing force is repeated for each particle effect. A shared helper method might simplify maintenance. Also, coverage for these lines is recommended.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 148-152: e2e/case/particleRenderer-dream.ts#L148-L152
Added lines #L148 - L152 were not covered by tests
154-157: Check negative vs. positive force ranges.
Ensure the Y-axis forces are intended and that test coverage includes verifying the final effect on particles.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 154-156: e2e/case/particleRenderer-dream.ts#L154-L156
Added lines #L154 - L156 were not covered by tests
158-160: Ensure zero force on Z-axis is the desired behavior.
Consider clarifying code comments or tests explaining why Z-axis force is zero.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 158-160: e2e/case/particleRenderer-dream.ts#L158-L160
Added lines #L158 - L160 were not covered by tests
239-239: Destructuring ForceOverLifetime.
This is consistent with other modules. Add tests for coverage if feasible.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 239-239: e2e/case/particleRenderer-dream.ts#L239
Added line #L239 was not covered by tests
293-295: Setting Y-axis force from 0 to 8.
This partial vertical push is plausible. Please consider verifying final effect in e2e tests.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 293-295: e2e/case/particleRenderer-dream.ts#L293-L295
Added lines #L293 - L295 were not covered by tests
297-299: Zero Z-axis force.
Same reasoning as with the Debris particle. Confirm this setting is intended, especially if the effect is purely 2D.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 297-299: e2e/case/particleRenderer-dream.ts#L297-L299
Added lines #L297 - L299 were not covered by tests
315-315: Destructure modules for highlights particle.
No immediate concerns, but coverage is recommended for new usage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 315-315: e2e/case/particleRenderer-dream.ts#L315
Added line #L315 was not covered by tests
372-375: Double-check negative X force.
As with Debris, negative X might shift highlights to the left. Ensure it's visually desirable.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 371-375: e2e/case/particleRenderer-dream.ts#L371-L375
Added lines #L371 - L375 were not covered by tests
381-383: Zero Z-axis force for highlights.
Same note as other modules; ensure Z-axis is intentionally disabled, or clarify in docs.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 381-383: e2e/case/particleRenderer-dream.ts#L381-L383
Added lines #L381 - L383 were not covered by testspackages/core/src/particle/ParticleGenerator.ts (7)
42-43: Static vectors for force offset calculations.
Initialization looks correct. You may want to add docstrings to clarify usage.
63-65: Add ForceOverLifetime module to ParticleGenerator.
This mirrors existing modules like velocityOverLifetime. Ensure test coverage for the new property.
505-505: Updating shader data to include ForceOverLifetime.
Looks good. To maintain coverage, add a test or e2e scenario that checks the updated shader behavior.
1102-1106: Initialize local/world offset vectors to zero.
This is a sensible approach before accumulative calculations. Consider adding test coverage for boundary cases.
1112-1113: Copy bounding box min/max from origin.
Helps base the final transform calculations on the original bounds. Coverage is recommended for transformations.
1130-1137: Apply velocity offset in world space.
Similar approach to local space. Ensure coverage includes world-space bounding expansions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1130-1132: packages/core/src/particle/ParticleGenerator.ts#L1130-L1132
Added lines #L1130 - L1132 were not covered by tests
[warning] 1134-1137: packages/core/src/particle/ParticleGenerator.ts#L1134-L1137
Added lines #L1134 - L1137 were not covered by tests
1175-1180: Add local offsets, transform, then apply world offsets.
This chaining approach is correct, but might be complex. Consider verifying it with unit tests for each scenario (local vs. world).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
e2e/fixtures/originImage/Particle_particleRenderer-dream.jpgis excluded by!**/*.jpgpackages/core/src/shaderlib/extra/particle.vs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/force_over_lifetime_module.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/particle_common.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/velocity_over_lifetime_module.glslis excluded by!**/*.glsl
📒 Files selected for processing (5)
e2e/case/particleRenderer-dream.ts(6 hunks)packages/core/src/particle/ParticleGenerator.ts(8 hunks)packages/core/src/particle/enums/ParticleRandomSubSeeds.ts(1 hunks)packages/core/src/particle/modules/ForceOverLifetimeModule.ts(1 hunks)packages/core/src/shaderlib/particle/index.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/particle/modules/ForceOverLifetimeModule.ts
[warning] 89-93: packages/core/src/particle/modules/ForceOverLifetimeModule.ts#L89-L93
Added lines #L89 - L93 were not covered by tests
[warning] 109-154: packages/core/src/particle/modules/ForceOverLifetimeModule.ts#L109-L154
Added lines #L109 - L154 were not covered by tests
e2e/case/particleRenderer-dream.ts
[warning] 93-94: e2e/case/particleRenderer-dream.ts#L93-L94
Added lines #L93 - L94 were not covered by tests
[warning] 148-152: e2e/case/particleRenderer-dream.ts#L148-L152
Added lines #L148 - L152 were not covered by tests
[warning] 154-156: e2e/case/particleRenderer-dream.ts#L154-L156
Added lines #L154 - L156 were not covered by tests
[warning] 158-160: e2e/case/particleRenderer-dream.ts#L158-L160
Added lines #L158 - L160 were not covered by tests
[warning] 239-239: e2e/case/particleRenderer-dream.ts#L239
Added line #L239 was not covered by tests
[warning] 287-291: e2e/case/particleRenderer-dream.ts#L287-L291
Added lines #L287 - L291 were not covered by tests
[warning] 293-295: e2e/case/particleRenderer-dream.ts#L293-L295
Added lines #L293 - L295 were not covered by tests
[warning] 297-299: e2e/case/particleRenderer-dream.ts#L297-L299
Added lines #L297 - L299 were not covered by tests
[warning] 315-315: e2e/case/particleRenderer-dream.ts#L315
Added line #L315 was not covered by tests
[warning] 371-375: e2e/case/particleRenderer-dream.ts#L371-L375
Added lines #L371 - L375 were not covered by tests
[warning] 377-379: e2e/case/particleRenderer-dream.ts#L377-L379
Added lines #L377 - L379 were not covered by tests
[warning] 381-383: e2e/case/particleRenderer-dream.ts#L381-L383
Added lines #L381 - L383 were not covered by tests
packages/core/src/particle/ParticleGenerator.ts
[warning] 1130-1132: packages/core/src/particle/ParticleGenerator.ts#L1130-L1132
Added lines #L1130 - L1132 were not covered by tests
[warning] 1134-1137: packages/core/src/particle/ParticleGenerator.ts#L1134-L1137
Added lines #L1134 - L1137 were not covered by tests
[warning] 1145-1171: packages/core/src/particle/ParticleGenerator.ts#L1145-L1171
Added lines #L1145 - L1171 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (10)
packages/core/src/shaderlib/particle/index.ts (2)
7-7: Good import for the new force module.
Import statement aligns well with the existing pattern.
22-22: Properly exported 'force_over_lifetime_module'.
Ensuring this new module is part of the exports is crucial for integrating with the shader pipeline.packages/core/src/particle/modules/ForceOverLifetimeModule.ts (1)
81-93: Increase test coverage for thespaceproperty.
Static analysis indicates these lines are not covered by tests. Confirm that settingspacetriggers the appropriate parameter updates.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-93: packages/core/src/particle/modules/ForceOverLifetimeModule.ts#L89-L93
Added lines #L89 - L93 were not covered by testse2e/case/particleRenderer-dream.ts (4)
287-287: Enable ForceOverLifetime for sparks.
Looks correct and consistent. Remember to update tests for coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 287-291: e2e/case/particleRenderer-dream.ts#L287-L291
Added lines #L287 - L291 were not covered by tests
288-291: Use of positive X force range might push sparks outward.
Double-check if the effect is visually correct, and add relevant coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 287-291: e2e/case/particleRenderer-dream.ts#L287-L291
Added lines #L287 - L291 were not covered by tests
371-371: Enable ForceOverLifetime for highlights.
Implementation is consistent. Consider adding tests to confirm correct highlight physics.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 371-375: e2e/case/particleRenderer-dream.ts#L371-L375
Added lines #L371 - L375 were not covered by tests
377-379: Y-axis force range for highlights.
Consider verifying that the partial upward push is visually aligned with design expectations.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 377-379: e2e/case/particleRenderer-dream.ts#L377-L379
Added lines #L377 - L379 were not covered by testspackages/core/src/particle/ParticleGenerator.ts (3)
173-173: Instantiate ForceOverLifetimeModule.
No issues found; consistent approach with other modules.
1122-1129: Apply velocity offset in local space.
Implementation is correct. Validate with a test that checks final bounding box expansions.
1140-1173: Incorporate ForceOverLifetime offset.
The logic parallels velocityOverLifetime. Coverage tests should ensure correct bounding expansions for local vs. world space forces.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1145-1171: packages/core/src/particle/ParticleGenerator.ts#L1145-L1171
Added lines #L1145 - L1171 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
e2e/case/particleRenderer-force.ts (5)
1-24: Clarify documentation title and category.
The doc comment references “Particle Dream,” but the file name is “particleRenderer-force.” It might improve clarity to ensure the doc comment aligns with the force-focused context of this module.
48-75: Avoid external dependencies in tests if possible.
Loading textures from external URLs can slow down or break tests if the URLs become inaccessible. Consider embedding small test assets or mocking them locally.
165-228: Repeated logic across different particle types.
The glow particle configuration is structurally similar tocreateDebrisParticle, just with different parameter values. Extracting shared configurations into helper functions could improve maintainability.
229-300: Repeated logic identified again increateSparksParticle.
Several modules are configured similarly to the debris or glow particles. A shared utility function might reduce duplicated code across these setup methods.
302-385: Highlights particle configuration repeats patterns seen earlier.
This function again mirrors the same structural approach (main, emission, velocity, color, size, force). Refactoring or modularizing the code would further ease maintenance and facilitate updates if additional parameters need to be tuned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-force.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
e2e/case/particleRenderer-force.ts(1 hunks)e2e/config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (3)
e2e/config.ts (1)
208-212: Configuration entry forforceOverLifetimelooks good!
This new entry neatly adds a test case for the new particle feature.e2e/case/particleRenderer-force.ts (2)
26-47: Confirm the correctness of the canvas identifier.
Make sure the"canvas"identifier used here matches the actual DOM element ID or query selector.
78-163: Debris particle configuration appears consistent.
Module usage for main, emission, size/color/velocity over lifetime, and force over lifetime is correct. The negative X and positive Y forces will produce the intended drifting effect.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts (3)
1-9: Unused imports detected.The
RandandParticleRandomSubSeedsimports are currently not referenced anywhere in the file, as the relevant code is commented out. Consider removing these imports or uncommenting the code if randomization logic is intended to be reinstated soon.
30-31: Confirm the necessity of commented-out_forceRand.These lines appear to be placeholder code for future or experimental randomization. If the feature is no longer needed, removing the commented code and unused annotations can keep the module lean; otherwise, consider adding a TODO or a feature flag referencing the intended usage.
14-14: Add or update documentation references.Although the docstring is provided for the
ForceOverLifetimeModuleclass, the overall PR summary notes that project documentation updates are pending. Consider expanding or linking to official docs to ensure new users understand how to configure and utilize this force-over-lifetime feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts (1)
177-183: Good event registration strategy.Unregistering the old curve’s change listener and registering the new one prevents stale subscriptions, ensuring shader data updates remain consistent. This is a clear approach for dynamically swapping out force curves.
packages/core/src/shaderlib/particle/velocity_over_lifetime_module.glsl
Outdated
Show resolved
Hide resolved
packages/core/src/shaderlib/particle/velocity_over_lifetime_module.glsl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/particle/ParticleGenerator.ts (1)
1103-1108: Simplified initialization of world offset vectors.The code now uses
set(0, 0, 0)for initializing vectors, which is more readable than individual assignments.Consider applying the same pattern to other vector initializations in the file for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHIL
6D40
L
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/core/src/shaderlib/extra/particle.vs.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/force_over_lifetime_module.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/particle_common.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/velocity_over_lifetime_module.glslis excluded by!**/*.glsl
📒 Files selected for processing (5)
packages/core/src/particle/ParticleGenerator.ts(9 hunks)packages/core/src/particle/modules/ForceOverLifetimeModule.ts(1 hunks)packages/core/src/particle/modules/ParticleGeneratorModule.ts(2 hunks)packages/core/src/particle/modules/SizeOverLifetimeModule.ts(0 hunks)packages/core/src/particle/modules/VelocityOverLifetimeModule.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/core/src/particle/modules/VelocityOverLifetimeModule.ts
- packages/core/src/particle/modules/SizeOverLifetimeModule.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/particle/modules/ForceOverLifetimeModule.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (15)
packages/core/src/particle/modules/ParticleGeneratorModule.ts (2)
4-4: New import for ParticleCompositeCurve.The added import enables the new
_onCompositeCurveChangemethod to properly handle composite curve type parameters.
41-46: Well-implemented event management for composite curves.This method properly manages event handlers when composite curves change. It follows best practices by:
- Unregistering the callback from the previous value
- Registering it for the new value
- Triggering an immediate update
The use of optional chaining operators prevents errors when either value is null or undefined.
packages/core/src/particle/ParticleGenerator.ts (13)
31-31: New ForceOverLifetimeModule import.This import supports the new force over lifetime functionality being introduced to the particle system.
61-63: ForceOverLifetimeModule property declaration.The property is correctly marked with
@deepCloneto ensure proper cloning behavior, and is declared as readonly to maintain encapsulation. The documentation comment clearly indicates its purpose.
171-171: ForceOverLifetimeModule initialization.The module is properly initialized in the constructor, consistent with how other modules are initialized.
503-503: Updated shader data to include force over lifetime.The shader data is now correctly updated to include the force over lifetime module, allowing the rendering system to account for these forces.
519-519: Reset random seed for force over lifetime.The random seed management now includes the force over lifetime module, ensuring consistent randomization across all modules.
856-862: New force randomization for particles.This code properly initializes force-related random values when creating new particles, following the established pattern for other randomized properties.
1115-1116: Improved variable bounds initialization.Using
copyFromto initialize bounds from the origin bounds is more concise and less error-prone than setting individual components.
1124-1127: Optimized velocity scaling.Scaling the velocity vectors once before using them improves performance and readability compared to scaling each component individually.
1129-1142: Refactored velocity bounds calculations.The code now separates local and world space calculations more clearly, making the transformation flow easier to understand and maintain.
1145-1179: Implementation of force over lifetime bounds calculations.The force calculations correctly follow the same pattern as velocity calculations but account for the quadratic nature of acceleration (0.5 * t²). The code:
- Gets extreme values for each force component
- Applies the appropriate scaling coefficient for acceleration
- Updates bounds based on the simulation space
- Handles both local and world space calculations
This implementation ensures that particles affected by forces will have accurate bounding boxes.
1181-1184: Improved bounds transformation sequence.The code now properly:
- Transforms the local bounds using the rotation matrix
- Adds world space offsets that don't need transformation
- Adds the world position
This sequence results in more accurate bounds calculations when both local and world forces/velocities are present.
1192-1192: Fixed variable name consistency.The method now uses
outparameter consistently to store the result, making the code easier to understand.
1212-1226: Refactored gravity bounds calculation.The gravity effect calculation now:
- Sets min/max vectors directly instead of modifying in place
- Adds the original bounds at the end
This approach is cleaner and less error-prone than the previous implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/particle/ParticleGenerator.ts (2)
1145-1179: Bounding volume expansion with constant-acceleration assumption
Using a half-squared-time formula (0.5 * maxLifetime² * force) to estimate force displacement can be an over-approximation for dynamic or curve-based forces, resulting in significantly inflated bounding boxes. If accuracy is paramount, consider partial integration or sampling the curve.
1181-1184: Oriented bounding box transformation
ApplyingrotateMatto min and max vectors is a straightforward approach, though it can overestimate bounds after rotation. For tighter bounding volumes, consider transforming all corner points or sampling the bounding box under rotations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
e2e/fixtures/originImage/Particle_particleRenderer-force.jpgis excluded by!**/*.jpgpackages/core/src/shaderlib/particle/particle_common.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/velocity_over_lifetime_module.glslis excluded by!**/*.glsl
📒 Files selected for processing (2)
packages/core/src/particle/ParticleGenerator.ts(9 hunks)packages/core/src/particle/modules/ForceOverLifetimeModule.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/core/src/particle/ParticleGenerator.ts (2)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts (9) (9)
ForceOverLifetimeModule(14:195)forceX(53:55)forceX(57:63)forceY(68:70)forceY(72:78)forceZ(83:85)forceZ(87:93)space(98:100)space(102:107)packages/core/src/particle/modules/VelocityOverLifetimeModule.ts (2) (2)
space(99:101)space(103:108)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (6)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts (3)
129-139: Unaddressed partial mismatch of force curve modes
If only one or two axes are configured as TwoCurves (or Curve) while another differs (e.g. is set to a single Constant), the logic in_updateShaderDataand_isRandomMode()silently drops into a fallback path for constants or random mode, potentially causing unexpected force behavior. Consider logging a warning or throwing an error when there's a partial mismatch to avoid silent misconfiguration.Would you like me to generate a script to scan for any partial mismatch usage (e.g., X is
Curvewhile Y isTwoCurves) in the codebase or test files?Also applies to: 150-165, 185-193
174-179: Consistent sub-seed handling
The_resetRandomSeedmethod cleanly aligns with similar approaches in other modules, ensuring each sub-seed is reset for predictable behavior.
1-195: Overall structural clarity
The newly introducedForceOverLifetimeModuleis logically organized, using distinct curves for each axis and clear code paths for single vs. random modes. Beyond the partial mismatch handling, the implementation demonstrates good maintainability.packages/core/src/particle/ParticleGenerator.ts (3)
31-31: New ForceOverLifetimeModule property & instantiation
IntegratingforceOverLifetimemirrors the existing module pattern. The property declaration and constructor call align well with the particle system’s design.Also applies to: 61-63, 171-171
500-500: Shader data integration for force
InvokingforceOverLifetime._updateShaderData(shaderData)alongside other modules ensures force calculations are properly reflected in the shader.Also applies to: 503-503
519-520: Global random seed synchronization
Resetting the force module's seed within_resetGlobalRandSeedmaintains consistency with other modules, preventing inconsistencies after replay or scene reload.
packages/core/src/shaderlib/particle/velocity_over_lifetime_module.glsl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/case/particleRenderer-force.ts (1)
1-398: Overall excellent implementation with one suggestion.The code effectively demonstrates all available modes of the new ForceOverLifetime module, but has significant code duplication. Consider extracting common particle setup logic into helper functions.
A possible refactoring approach could be to create helper functions:
function setupCommonParticleProperties(particleRenderer: ParticleRenderer, blendMode: BlendMode, texture: Texture2D, priority: number): void { const material = new ParticleMaterial(particleRenderer.entity.engine); material.blendMode = blendMode; material.baseTexture = texture; particleRenderer.setMaterial(material); particleRenderer.priority = priority; particleRenderer.generator.useAutoRandomSeed = false; } function setupCommonEmission(emission: ParticleEmissionModule, boxSize: Vector3): void { const boxShape = new BoxShape(); boxShape.size.copy(boxSize); emission.shape = boxShape; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-26: e2e/case/particleRenderer-force.ts#L25-L26
Added lines #L25 - L26 were not covered by tests
[warning] 29-34: e2e/case/particleRenderer-force.ts#L29-L34
Added lines #L29 - L34 were not covered by tests
[warning] 36-38: e2e/case/particleRenderer-force.ts#L36-L38
Added lines #L36 - L38 were not covered by tests
[warning] 41-44: e2e/case/particleRenderer-force.ts#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 46-46: e2e/case/particleRenderer-force.ts#L46
Added line #L46 was not covered by tests
[warning] 48-71: e2e/case/particleRenderer-force.ts#L48-L71
Added lines #L48 - L71 were not covered by tests
[warning] 73-73: e2e/case/particleRenderer-force.ts#L73
Added line #L73 was not covered by tests
[warning] 75-78: e2e/case/particleRenderer-force.ts#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-82: e2e/case/particleRenderer-force.ts#L80-L82
Added lines #L80 - L82 were not covered by tests
[warning] 84-84: e2e/case/particleRenderer-force.ts#L84
Added line #L84 was not covered by tests
[warning] 86-91: e2e/case/particleRenderer-force.ts#L86-L91
Added lines #L86 - L91 were not covered by tests
[warning] 93-93: e2e/case/particleRenderer-force.ts#L93
Added line #L93 was not covered by tests
[warning] 95-96: e2e/case/particleRenderer-force.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-99: e2e/case/particleRenderer-force.ts#L99
Added line #L99 was not covered by tests
[warning] 101-103: e2e/case/particleRenderer-force.ts#L101-L103
Added lines #L101 - L103 were not covered by tests
[warning] 105-107: e2e/case/particleRenderer-force.ts#L105-L107
Added lines #L105 - L107 were not covered by tests
[warning] 109-111: e2e/case/particleRenderer-force.ts#L109-L111
Added lines #L109 - L111 were not covered by tests
[warning] 114-114: e2e/case/particleRenderer-force.ts#L114
Added line #L114 was not covered by tests
[warning] 116-118: e2e/case/particleRenderer-force.ts#L116-L118
Added lines #L116 - L118 were not covered by tests
[warning] 121-122: e2e/case/particleRenderer-force.ts#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-128: e2e/case/particleRenderer-force.ts#L124-L128
Added lines #L124 - L128 were not covered by tests
[warning] 131-134: e2e/case/particleRenderer-force.ts#L131-L134
Added lines #L131 - L134 were not covered by tests
[warning] 137-140: e2e/case/particleRenderer-force.ts#L137-L140
Added lines #L137 - L140 were not covered by tests
[warning] 142-144: e2e/case/particleRenderer-force.ts#L142-L144
Added lines #L142 - L144 were not covered by tests
[warning] 146-148: e2e/case/particleRenderer-force.ts#L146-L148
Added lines #L146 - L148 were not covered by tests
[warning] 150-154: e2e/case/particleRenderer-force.ts#L150-L154
Added lines #L150 - L154 were not covered by tests
[warning] 156-158: e2e/case/particleRenderer-force.ts#L156-L158
Added lines #L156 - L158 were not covered by tests
[warning] 160-162: e2e/case/particleRenderer-force.ts#L160-L162
Added lines #L160 - L162 were not covered by tests
[warning] 164-165: e2e/case/particleRenderer-force.ts#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 167-169: e2e/case/particleRenderer-force.ts#L167-L169
Added lines #L167 - L169 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
e2e/fixtures/originImage/Particle_particleRenderer-force.jpgis excluded by!**/*.jpgpackages/core/src/shaderlib/particle/force_over_lifetime_module.glslis excluded by!**/*.glslpackages/core/src/shaderlib/particle/velocity_over_lifetime_module.glslis excluded by!**/*.glsl
📒 Files selected for processing (1)
e2e/case/particleRenderer-force.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
e2e/case/particleRenderer-force.ts
[warning] 25-26: e2e/case/particleRenderer-force.ts#L25-L26
Added lines #L25 - L26 were not covered by tests
[warning] 29-34: e2e/case/particleRenderer-force.ts#L29-L34
Added lines #L29 - L34 were not covered by tests
[warning] 36-38: e2e/case/particleRenderer-force.ts#L36-L38
Added lines #L36 - L38 were not covered by tests
[warning] 41-44: e2e/case/particleRenderer-force.ts#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 46-46: e2e/case/particleRenderer-force.ts#L46
Added line #L46 was not covered by tests
[warning] 48-71: e2e/case/particleRenderer-force.ts#L48-L71
Added lines #L48 - L71 were not covered by tests
[warning] 73-73: e2e/case/particleRenderer-force.ts#L73
Added line #L73 was not covered by tests
[warning] 75-78: e2e/case/particleRenderer-force.ts#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-82: e2e/case/particleRenderer-force.ts#L80-L82
Added lines #L80 - L82 were not covered by tests
[warning] 84-84: e2e/case/particleRenderer-force.ts#L84
Added line #L84 was not covered by tests
[warning] 86-91: e2e/case/particleRenderer-force.ts#L86-L91
Added lines #L86 - L91 were not covered by tests
[warning] 93-93: e2e/case/particleRenderer-force.ts#L93
Added line #L93 was not covered by tests
[warning] 95-96: e2e/case/particleRenderer-force.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-99: e2e/case/particleRenderer-force.ts#L99
Added line #L99 was not covered by tests
[warning] 101-103: e2e/case/particleRenderer-force.ts#L101-L103
Added lines #L101 - L103 were not covered by tests
[warning] 105-107: e2e/case/particleRenderer-force.ts#L105-L107
Added lines #L105 - L107 were not covered by tests
[warning] 109-111: e2e/case/particleRenderer-force.ts#L109-L111
Added lines #L109 - L111 were not covered by tests
[warning] 114-114: e2e/case/particleRenderer-force.ts#L114
Added line #L114 was not covered by tests
[warning] 116-118: e2e/case/particleRenderer-force.ts#L116-L118
Added lines #L116 - L118 were not covered by tests
[warning] 121-122: e2e/case/particleRenderer-force.ts#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-128: e2e/case/particleRenderer-force.ts#L124-L128
Added lines #L124 - L128 were not covered by tests
[warning] 131-134: e2e/case/particleRenderer-force.ts#L131-L134
Added lines #L131 - L134 were not covered by tests
[warning] 137-140: e2e/case/particleRenderer-force.ts#L137-L140
Added lines #L137 - L140 were not covered by tests
[warning] 142-144: e2e/case/particleRenderer-force.ts#L142-L144
Added lines #L142 - L144 were not covered by tests
[warning] 146-148: e2e/case/particleRenderer-force.ts#L146-L148
Added lines #L146 - L148 were not covered by tests
[warning] 150-154: e2e/case/particleRenderer-force.ts#L150-L154
Added lines #L150 - L154 were not covered by tests
[warning] 156-158: e2e/case/particleRenderer-force.ts#L156-L158
Added lines #L156 - L158 were not covered by tests
[warning] 160-162: e2e/case/particleRenderer-force.ts#L160-L162
Added lines #L160 - L162 were not covered by tests
[warning] 164-165: e2e/case/particleRenderer-force.ts#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 167-169: e2e/case/particleRenderer-force.ts#L167-L169
Added lines #L167 - L169 were not covered by tests
🔇 Additional comments (8)
e2e/case/particleRenderer-force.ts (8)
1-4: Good file header with proper categorization.The file is correctly categorized under "Particle" which aligns with the PR objective of adding the
ForceOverLifetimemodule to the particle system.
95-96: Good implementation showing the new ForceOverLifetime module.The code properly destructures the
forceOverLifetimemodule from the particle generator, demonstrating that it's now available as part of the particle system API.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 95-96: e2e/case/particleRenderer-force.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
138-140: Velocity configuration has min greater than max.The velocityX has a constantMin (2) greater than constantMax (1). This is counterintuitive and might lead to unexpected behavior.
- velocityOverLifetime.velocityX.constantMin = 2; - velocityOverLifetime.velocityX.constantMax = 1; + velocityOverLifetime.velocityX.constantMin = 1; + velocityOverLifetime.velocityX.constantMax = 2;🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 137-140: e2e/case/particleRenderer-force.ts#L137-L140
Added lines #L137 - L140 were not covered by tests
150-162: Good demonstration of ForceOverLifetime with Constant mode.The implementation correctly shows how to use ForceOverLifetime with the Constant mode. This provides a clear example for users on how to implement constant forces in their particle systems.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 150-154: e2e/case/particleRenderer-force.ts#L150-L154
Added lines #L150 - L154 were not covered by tests
[warning] 156-158: e2e/case/particleRenderer-force.ts#L156-L158
Added lines #L156 - L158 were not covered by tests
[warning] 160-162: e2e/case/particleRenderer-force.ts#L160-L162
Added lines #L160 - L162 were not covered by tests
230-242: Good demonstration of ForceOverLifetime with TwoConstants mode.The implementation correctly shows how to use ForceOverLifetime with the TwoConstants mode, which allows for randomly selecting a force value between min and max for each particle.
303-312: Great example of ForceOverLifetime with Curve mode.The implementation correctly demonstrates how to use ForceOverLifetime with the Curve mode, allowing for force values to change over the particle's lifetime according to a specified curve.
384-396: Excellent demonstration of ForceOverLifetime with TwoCurves mode.The implementation showcases the most complex mode (TwoCurves) which allows for both min and max curves to be defined, providing the most flexible control over particle forces.
1-398: Missing documentation for the ForceOverLifetime module.As noted in the PR description, documentation hasn't been added yet. Consider adding detailed comments explaining how the ForceOverLifetime module works, its parameters, and use cases.
Would you like me to generate documentation for the ForceOverLifetime module that could be added to this file and used as a basis for official documentation?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-26: e2e/case/particleRenderer-force.ts#L25-L26
Added lines #L25 - L26 were not covered by tests
[warning] 29-34: e2e/case/particleRenderer-force.ts#L29-L34
Added lines #L29 - L34 were not covered by tests
[warning] 36-38: e2e/case/particleRenderer-force.ts#L36-L38
Added lines #L36 - L38 were not covered by tests
[warning] 41-44: e2e/case/particleRenderer-force.ts#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 46-46: e2e/case/particleRenderer-force.ts#L46
Added line #L46 was not covered by tests
[warning] 48-71: e2e/case/particleRenderer-force.ts#L48-L71
Added lines #L48 - L71 were not covered by tests
[warning] 73-73: e2e/case/particleRenderer-force.ts#L73
Added line #L73 was not covered by tests
[warning] 75-78: e2e/case/particleRenderer-force.ts#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-82: e2e/case/particleRenderer-force.ts#L80-L82
Added lines #L80 - L82 were not covered by tests
[warning] 84-84: e2e/case/particleRenderer-force.ts#L84
Added line #L84 was not covered by tests
[warning] 86-91: e2e/case/particleRenderer-force.ts#L86-L91
Added lines #L86 - L91 were not covered by tests
[warning] 93-93: e2e/case/particleRenderer-force.ts#L93
Added line #L93 was not covered by tests
[warning] 95-96: e2e/case/particleRenderer-force.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-99: e2e/case/particleRenderer-force.ts#L99
Added line #L99 was not covered by tests
[warning] 101-103: e2e/case/particleRenderer-force.ts#L101-L103
Added lines #L101 - L103 were not covered by tests
[warning] 105-107: e2e/case/particleRenderer-force.ts#L105-L107
Added lines #L105 - L107 were not covered by tests
[warning] 109-111: e2e/case/particleRenderer-force.ts#L109-L111
Added lines #L109 - L111 were not covered by tests
[warning] 114-114: e2e/case/particleRenderer-force.ts#L114
Added line #L114 was not covered by tests
[warning] 116-118: e2e/case/particleRenderer-force.ts#L116-L118
Added lines #L116 - L118 were not covered by tests
[warning] 121-122: e2e/case/particleRenderer-force.ts#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-128: e2e/case/particleRenderer-force.ts#L124-L128
Added lines #L124 - L128 were not covered by tests
[warning] 131-134: e2e/case/particleRenderer-force.ts#L131-L134
Added lines #L131 - L134 were not covered by tests
[warning] 137-140: e2e/case/particleRenderer-force.ts#L137-L140
Added lines #L137 - L140 were not covered by tests
[warning] 142-144: e2e/case/particleRenderer-force.ts#L142-L144
Added lines #L142 - L144 were not covered by tests
[warning] 146-148: e2e/case/particleRenderer-force.ts#L146-L148
Added lines #L146 - L148 were not covered by tests
[warning] 150-154: e2e/case/particleRenderer-force.ts#L150-L154
Added lines #L150 - L154 were not covered by tests
[warning] 156-158: e2e/case/particleRenderer-force.ts#L156-L158
Added lines #L156 - L158 were not covered by tests
[warning] 160-162: e2e/case/particleRenderer-force.ts#L160-L162
Added lines #L160 - L162 were not covered by tests
[warning] 164-165: e2e/case/particleRenderer-force.ts#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 167-169: e2e/case/particleRenderer-force.ts#L167-L169
Added lines #L167 - L169 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts (1)
14-18: Consider enhancing class-level documentation.
Providing additional details on how force curves and random modes interact helps developers configure this module without guesswork.packages/core/src/particle/ParticleGenerator.ts (2)
61-63: Add explanatory JSDoc forforceOverLifetime.
This property is a key piece for controlling particle physics over time. Documenting the typical usage and parameter expectations can help future contributors.
1145-1176: Force bounding logic is consistent.
Similar to velocity, adding local or world offsets upon force application keeps the calculations coherent. However, if partial mismatch of force modes occurs (e.g., X=TwoCurves, Y=Curve), it may silently blend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/particle/ParticleGenerator.ts(9 hunks)packages/core/src/particle/modules/ForceOverLifetimeModule.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts (1)
packages/core/src/particle/ParticleGenerator.ts (1) (1)
ParticleGenerator(36-1230)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e (22.x)
🔇 Additional comments (9)
packages/core/src/particle/modules/ForceOverLifetimeModule.ts (2)
129-165: No explicit handling of partial mismatch across force modes.
As in previous reviews, ifforceXis set to a different curve mode thanforceYandforceZ, the code will silently use either random or constant mode. Consider logging a warning or enforcing uniform modes to avoid unexpected force behavior.
184-196: _isRandomMode() method looks clean.
The logic for determining random mode based on curve modes is straightforward and consistent.packages/core/src/particle/ParticleGenerator.ts (7)
171-171: Constructor instantiation looks correct.
InitializingForceOverLifetimeModulemirrors the pattern used for other modules.
503-503: Seamless shader data integration.
Including_updateShaderData()for the force module ensures parity with other lifetime modules.
519-519: Properly resetting the random seed.
This maintains reproducibility for the force module whenever a new seed is set.
856-862: Good use of random X/Y/Z offsets.
Conditionally adding random values to the instance buffer aligns well with existing per-axis random behavior in the system.
1178-1181: Transforming bounding box with offsets.
Applying rotation and adding local/world offsets to min/max is a sound approach for updating bounding volumes.
1188-1223: Gravity bounding additions.
Including upward or downward gravity extremes in the bounding box is correct, ensuring the final volume accounts for gravitational displacement.
1103-1143:✅ Verification successful
Velocity-based world offset handling.
CheckingvelocityOverLifetime.spaceand adding world-local offsets selectively is logical. If more particle spaces are introduced, ensure they are integrated here too.Could you confirm that no third simulation space exists in the codebase by searching the enum definition? Feel free to run:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'enum ParticleSimulationSpace { $$$ }'Length of output: 576
Verified Simulation Space Enum Handling
I confirmed that the enum inpackages/core/src/particle/enums/ParticleSimulationSpace.tsonly includesLocalandWorld. The velocity-based world offset code inParticleGenerator.tscorrectly handles these cases. If additional simulation spaces are introduced later, the logic here will need to be updated accordingly.
Please check if the PR fulfills these requirements
Formula to compute displacement caused by
curvemode force