Fix particleRenderer enable startFrame #2333
Conversation
WalkthroughThe recent updates significantly enhance the particle simulation and rendering pipeline by refining frame management and UV coordinate handling. Key improvements include clearer property access, enhanced shader capabilities for randomization, and a more dynamic approach to frame evaluations. Collectively, these changes boost the flexibility and visual fidelity of particle animations, allowing for richer and more varied effects. Changes
Sequence Diagram(s)sequenceDiagram
participant ParticleGenerator
participant TextureSheetAnimationModule
participant Shader
ParticleGenerator->>TextureSheetAnimationModule: Access properties for UV calculation
TextureSheetAnimationModule->>Shader: Update shader data with new frame logic
Shader->>Shader: Evaluate UV coordinates
Shader-->>ParticleGenerator: Render particle with updated UVs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
- packages/core/src/particle/enums/ParticleRandomSubSeeds.ts (1 hunks)
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts (4 hunks)
- packages/core/src/shaderlib/extra/particle.vs.glsl (2 hunks)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1 hunks)
Additional comments not posted (12)
packages/core/src/particle/enums/ParticleRandomSubSeeds.ts (1)
18-18: Addition ofStartFrameEnum Value.The addition of
StartFrame = 0xabcdef01to theParticleRandomSubSeedsenum is consistent with the PR's goal of enhancing particle animation capabilities by enabling the specification of the starting frame.packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (3)
1-11: Introduction of New Uniform Variables.The addition of
renderer_TSAFrameMaxConstantand conditionally declaredrenderer_TSAFrameMinConstantsupports the new functionality for handling constant frame values. This aligns with the goal of enhancing control over particle animations.
21-32: Expanded Conditional Logic for Frame Calculation.The restructuring of conditional checks to handle both constant and curve-based frame calculations is well-implemented. This allows for more flexible rendering scenarios, which is consistent with the PR objectives.
35-35: Adjustment in Frame Calculation Logic.The inclusion of
a_SimulationUV.zin the frame calculation potentially refines UV coordinate computation. Ensure this change is tested for performance impacts.packages/core/src/shaderlib/extra/particle.vs.glsl (2)
23-23: Expanded Conditional Compilation fora_Random1.The inclusion of
RENDERER_TSA_FRAME_RANDOM_CONSTANTin the conditional compilation fora_Random1supports enhanced randomization capabilities in the shader.
102-102: Refinement in UV Computation Logic.The change in how
simulateUVis computed simplifies the texture mapping process. Verify the visual outcomes to ensure the desired rendering effects.packages/core/src/particle/modules/TextureSheetAnimationModule.ts (5)
16-17: Initialization of shader macros is appropriate.The use of
ShaderMacro.getByNamefor initializing_frameConstantMacroand_frameRandomConstantMacrois consistent with standard practices for shader management.
21-22: Initialization of shader properties is appropriate.The use of
ShaderProperty.getByNamefor initializing_frameMinConstantPropertyand_frameMaxConstantPropertyis consistent with standard practices for shader property management.
46-48: Initialization of randomization property is appropriate.The introduction of
_startFrameRandusingRandenhances the randomization capabilities for the start frame, aligning with the PR objectives.
74-94: Enhanced control flow for frame data handling is appropriate.The updates to
_updateShaderDatato accommodateParticleCurveMode.ConstantandParticleCurveMode.TwoConstantsprovide more nuanced control over frame data handling, aligning with the PR objectives.
105-105: Resetting randomization property is appropriate.The addition of
_startFrameRand.resetin_resetRandomSeedensures consistent randomization behavior for the start frame.packages/core/src/particle/ParticleGenerator.ts (1)
810-813: Destructuring and dynamic frame evaluation is appropriate.The destructuring of properties from
textureSheetAnimationand the use ofstartFrame.evaluatewith randomness improve readability and allow for dynamic frame evaluations, aligning with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts (4 hunks)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/core/src/particle/ParticleGenerator.ts
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
- packages/core/src/shaderlib/extra/particle.vs.glsl (2 hunks)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/core/src/particle/ParticleGenerator.ts
- packages/core/src/shaderlib/extra/particle.vs.glsl
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/particle/ParticleGenerator.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts (4 hunks)
- packages/core/src/shaderlib/extra/particle.vs.glsl (2 hunks)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/core/src/particle/ParticleGenerator.ts
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts
- packages/core/src/shaderlib/extra/particle.vs.glsl
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2333 +/- ##
==========================================
+ Coverage 69.23% 69.52% +0.28%
==========================================
Files 524 524
Lines 27396 27409 +13
Branches 4100 4101 +1
==========================================
+ Hits 18968 19055 +87
+ Misses 6937 6854 -83
- Partials 1491 1500 +9 ☔ View full report in Codecov by Sentry. |
packages/core/src/particle/modules/TextureSheetAnimationModule.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts (3 hunks)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1 hunks)
Additional comments not posted (7)
packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (3)
6-8: Conditional Compilation Optimization.The conditional compilation adjustment ensures that
renderer_TSAFrameMinCurveis only declared whenRENDERER_TSA_FRAME_RANDOM_CURVESis defined, optimizing shader resource usage.
16-16: Improved Frame Calculation Logic.The refined calculation of
normalizedFrameenhances clarity and maintains the intended functionality of the frame calculations.
Line range hint
17-27: Streamlined UV Calculation Logic.The UV calculation logic has been streamlined by directly using
normalizedFrame, reducing unnecessary variable assignments and enhancing shader performance.packages/core/src/particle/modules/TextureSheetAnimationModule.ts (4)
22-24: Repositioning of Properties.The repositioning of
_cycleCountPropertyand_tillingParamsPropertyimproves code organization without affecting functionality.
42-44: Introduction of_startFrameRand.The new
_startFrameRandproperty enhances the variability in animation start points, aligning with the PR's objectives.
49-49: Static Readonly_frameCurveMacro.The change to make
_frameCurveMacrostatic readonly ensures consistent access and prevents unintended modifications.
92-93: Update to_resetRandomSeedMethod.The inclusion of
_startFrameRandin the_resetRandomSeedmethod ensures consistent random seed usage across frame randomizations.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts (3 hunks)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/core/src/particle/ParticleGenerator.ts
- packages/core/src/particle/modules/TextureSheetAnimationModule.ts
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl
Additional context used
GitHub Check: codecov/patch
packages/core/src/particle/ParticleGenerator.ts
[warning] 810-811: packages/core/src/particle/ParticleGenerator.ts#L810-L811
Added lines #L810 - L811 were not covered by tests
[warning] 813-813: packages/core/src/particle/ParticleGenerator.ts#L813
Added line #L813 was not covered by tests
[warning] 815-815: packages/core/src/particle/ParticleGenerator.ts#L815
Added line #L815 was not covered by tests
[warning] 818-818: packages/core/src/particle/ParticleGenerator.ts#L818
Added line #L818 was not covered by tests
[warning] 820-823: packages/core/src/particle/ParticleGenerator.ts#L820-L823
Added lines #L820 - L823 were not covered by tests
Additional comments not posted (1)
packages/core/src/particle/ParticleGenerator.ts (1)
809-811: Destructuring improves readability.The destructuring of
_tillingInfoandframeOverTimeenhances code readability and reduces repetitive property access.Tools
GitHub Check: codecov/patch
[warning] 810-811: packages/core/src/particle/ParticleGenerator.ts#L810-L811
Added lines #L810 - L811 were not covered by tests
| let tileRow = 0; | ||
| if (frameOverTime.mode === ParticleCurveMode.Constant || frameOverTime.mode === ParticleCurveMode.TwoConstants) { | ||
| tileRow = | ||
| Math.floor(frameOverTime.evaluate(undefined, textureSheetAnimation._frameOverTimeRand.random()) * z) * x; | ||
| } | ||
| const tileRowIndex = Math.floor(tileRow); |
There was a problem hiding this comment.
Ensure test coverage for new logic.
The logic for calculating tileRow and handling frameOverTime modes requires test coverage to ensure correctness.
Would you like me to help generate test cases for this new logic or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 813-813: packages/core/src/particle/ParticleGenerator.ts#L813
Added line #L813 was not covered by tests
[warning] 815-815: packages/core/src/particle/ParticleGenerator.ts#L815
Added line #L815 was not covered by tests
[warning] 818-818: packages/core/src/particle/ParticleGenerator.ts#L818
Added line #L818 was not covered by tests
| instanceVertices[offset + ParticleBufferUtils.simulationUVOffset] = x; | ||
| instanceVertices[offset + 35] = y; | ||
| instanceVertices[offset + 36] = tileRow - tileRowIndex; | ||
| instanceVertices[offset + 37] = tileRowIndex * y; |
There was a problem hiding this comment.
Ensure test coverage for instanceVertices assignments.
The assignments to instanceVertices need test coverage to verify the calculations and ensure they function as expected in different scenarios.
Would you like assistance in creating test cases for these assignments or opening a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 820-823: packages/core/src/particle/ParticleGenerator.ts#L820-L823
Added lines #L820 - L823 were not covered by tests
| instanceVertices[offset + 37] = 0; | ||
| if (textureSheetAnimation.enabled) { | ||
| const { _tillingInfo, frameOverTime } = textureSheetAnimation; | ||
| const { x, y, z } = _tillingInfo; |
There was a problem hiding this comment.
const { frameOverTime } = textureSheetAnimation;
const { x, y, z } = textureSheetAnimation._tillingInfo;
GuoLei1990
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-textureSheetAnimation.jpgis excluded by!**/*.jpg
Files selected for processing (3)
- e2e/case/particleRenderer-textureSheetAnimation.ts (1 hunks)
- e2e/config.ts (1 hunks)
- packages/core/src/particle/ParticleGenerator.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/particle/ParticleGenerator.ts
Additional comments not posted (6)
e2e/case/particleRenderer-textureSheetAnimation.ts (5)
5-20: Imports look good.All imported modules are relevant and necessary for the implementation.
22-36: Engine setup and scene configuration are correct.The WebGLEngine is properly initialized, and the scene is configured with ambient light and background color.
37-41: Camera setup is correct.The camera is properly positioned and configured with a field of view.
45-59: Particle system setup is correct.The texture is loaded correctly, and the particle renderer and material are configured appropriately.
63-75: Texture sheet animation configuration is correct.The tiling, frame over time, and enabling of the animation are set up correctly.
e2e/config.ts (1)
193-196: New configuration entry is correct.The
textureSheetAnimationentry is consistent with other entries and correctly formatted.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-textureSheetAnimation.jpgis excluded by!**/*.jpg
Files selected for processing (1)
- e2e/case/particleRenderer-textureSheetAnimation.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/case/particleRenderer-textureSheetAnimation.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
e2e/fixtures/originImage/Particle_particleRenderer-textureSheetAnimation.jpgis excluded by!**/*.jpg
Files selected for processing (1)
- e2e/case/particleRenderer-textureSheetAnimation.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/case/particleRenderer-textureSheetAnimation.ts
* feat: enable startFrame
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Enable startFrame in constant and twoConstants modes:
Now supports using the startFrame parameter in constant and twoConstants modes, allowing users to specify the initial frame of the particle animation.
Enable frameOverLifetime in constant and twoConstants modes:
The frameOverLifetime parameter is now available in constant and twoConstants modes, providing finer control over animation frames throughout the particle's lifetime.
What is the current behavior? (You can also link to an open issue here)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes