Fix texture sheet and gravity bug #2422
Conversation
WalkthroughThe changes in this pull request enhance the particle system's functionality by integrating gravity effects into various components. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2422 +/- ##
==========================================
- Coverage 69.50% 69.48% -0.03%
==========================================
Files 522 522
Lines 27365 27366 +1
Branches 4097 4098 +1
==========================================
- Hits 19021 19014 -7
- Misses 6846 6850 +4
- Partials 1498 1502 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/shaderlib/extra/particle.vs.glsl (1)
77-77: Consider adding range validation for the random gravity multiplier.While introducing randomization to gravity is a good approach for natural variation, the current implementation might benefit from ensuring the random multiplier stays within a reasonable range.
Consider clamping or scaling the random value to ensure gravity remains within expected bounds:
-vec3 gravityVelocity = renderer_Gravity * a_Random0.x * age; +// Ensure gravity multiplier is between 0.5 and 1.5 for controlled variation +vec3 gravityVelocity = renderer_Gravity * (0.5 + a_Random0.x) * age;This would prevent extreme variations while still maintaining natural randomness in particle behavior.
packages/core/src/particle/ParticleGenerator.ts (1)
761-761: Clarify the comment for better understandingThe comment
// Gravity, unused, size, rotationmight be confusing or unclear about the code's intent. It would improve readability to make the comment more descriptive.Consider updating the comment to clearly describe the purpose of the following code block, such as:
-// Gravity, unused, size, rotation +// Set gravity modifier based on its mode
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/core/src/particle/ParticleGenerator.ts(3 hunks)packages/core/src/particle/modules/MainModule.ts(2 hunks)packages/core/src/shaderlib/extra/particle.vs.glsl(1 hunks)packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl(1 hunks)
🔇 Additional comments (5)
packages/core/src/shaderlib/particle/texture_sheet_animation_module.glsl (1)
23-25: LGTM! Variable name change improves clarity.
The renaming from floorTotalULength to tileRowIndex better describes the variable's purpose in calculating tile-based UV coordinates.
Let's verify if similar variable names exist in related shader files:
✅ Verification successful
Variable renaming is isolated and safe
The variable renaming from floorTotalULength to tileRowIndex is contained within the texture sheet animation module and doesn't affect other shader files. The search results confirm that this UV calculation logic is unique to texture_sheet_animation_module.glsl and the renamed variable isn't referenced elsewhere in the particle shader system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar variable names in other shader files
# Look for both old and new variable names to ensure consistency
# Search for related shader files
echo "Searching for related shader files..."
rg -t glsl "floorTotalULength|tileRowIndex" --type glsl
# Search specifically in particle-related shaders
echo "Checking particle-specific shaders..."
fd -e glsl -e vert -e frag . "packages/core/src/shaderlib/particle"
Length of output: 1037
Script:
#!/bin/bash
# Let's check the content of these shader files for any related UV calculation or similar variable names
for file in $(fd -e glsl . "packages/core/src/shaderlib/particle"); do
echo "=== $file ==="
# Search for UV-related calculations and variable names
rg -A 3 -B 3 "uv\.|tileRow|floorTotalULength|TSATillingParams" "$file"
echo ""
done
Length of output: 4833
packages/core/src/shaderlib/extra/particle.vs.glsl (1)
77-77: Verify integration with ParticleGenerator.
The gravity calculation change looks correct, but we should ensure the ParticleGenerator is providing appropriate random values.
Let's verify the integration:
✅ Verification successful
Integration with ParticleGenerator is correctly implemented
The verification shows that:
- The gravity calculation in the shader correctly uses
a_Random0.xwhich is populated by the ParticleGenerator - The ParticleGenerator properly initializes gravity values in
_addNewParticle()method based on the MainModule's gravityModifier settings:- For constant mode:
instanceVertices[offset + 19] = main.gravityModifier.constant - For two constants mode:
instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random())
- For constant mode:
- The MainModule has proper random seed handling for gravity via
_gravityModifierRand
The integration is working as expected with proper random value generation and attribute population.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how a_Random0 is populated in ParticleGenerator
ast-grep --pattern 'class ParticleGenerator {
$$$
_addNewParticle($_) {
$$$
a_Random0
$$$
}
$$$
}'
# Also check for any gravity-related configurations
rg -A 3 "renderer_Gravity" --type ts
Length of output: 727
Script:
#!/bin/bash
# Let's check for a_Random0 usage in particle-related files
rg -A 5 "a_Random0" --type glsl
# Check ParticleGenerator implementation for random value generation
ast-grep --pattern 'class ParticleGenerator {
$$$
_addNewParticle($_) {
$$$
}
}'
# Check MainModule for gravity configuration
ast-grep --pattern 'class MainModule {
$$$
gravity
$$$
}'
Length of output: 219
Script:
#!/bin/bash
# Let's check for a_Random0 usage in shader files with different extensions
rg -A 5 "a_Random0"
# Check ParticleGenerator for random attribute setup
rg -A 10 "ParticleGenerator" --type ts
# Check MainModule gravity implementation
rg -A 10 "class MainModule" --type ts
Length of output: 47396
Script:
#!/bin/bash
# Let's check how random values are generated and used in ParticleGenerator
rg -A 10 "_addNewParticle" --type ts
# Check for Random0 attribute initialization
rg -A 5 "Random0" --type ts --type js
Length of output: 3377
Script:
#!/bin/bash
# Let's check for random value generation in ParticleGenerator
rg -B 5 -A 20 "instanceVertices\[offset \+ \d+\]" --type ts
# Check for gravity-related code in MainModule
rg -B 5 -A 10 "gravityModifier" --type ts
Length of output: 33690
packages/core/src/particle/modules/MainModule.ts (1)
323-323:
Verify gravity modifier integration with physics gravity.
The change to use scene's physics gravity directly is architecturally cleaner, making the physics system the single source of truth. However, it's unclear how this integrates with the gravityModifier property that still exists in the class.
Let's verify the gravity modifier usage:
Consider documenting the relationship between physics gravity and gravity modifier in the class documentation, especially since this represents a significant change in how gravity is handled in the particle system.
The PR indicates that tests haven't been added. Given this is a bug fix affecting particle physics behavior, please add tests to verify:
- Particle behavior with different physics gravity values
- Interaction between physics gravity and gravity modifier
- Particle behavior in different simulation spaces (Local vs World)
✅ Verification successful
Let me gather more information about how the gravity modifier is actually applied to the physics gravity.
Let me try a different approach to find how the gravity modifier is applied in the particle system.
Based on the evidence gathered, I can now provide the final response:
The gravity modifier is properly integrated with physics gravity
The gravity modifier is correctly applied in the particle system:
- The modifier value is used to scale the physics gravity vector in
ParticleGenerator.ts - The calculation includes proper lifetime coefficients (
0.5 * maxLifetime * maxLifetime) - Both min/max gravity effects are handled for different curve modes
- Existing tests in
ParticleBoundingBox.test.tsverify the gravity modifier behavior
The architectural change to use scene's physics gravity is sound and maintains compatibility with the existing gravity modifier system. The PR's changes align well with the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gravity modifier usage in particle system
rg -A 5 "gravityModifier" --type ts
Length of output: 8276
Script:
#!/bin/bash
# Search for gravity-related shader data updates
ast-grep --pattern 'setVector3($_gravity, $_)'
# Search for gravity usage in shader
rg -A 5 "gravity" --type glsl
Length of output: 107
Script:
#!/bin/bash
# Search for shader data updates in MainModule
ast-grep --pattern '_updateShaderData() {
$$$
}'
# Look for gravity-related calculations
rg -A 10 "_getExtremeValueFromZero.*gravityModifier" --type ts
Length of output: 1123
packages/core/src/particle/ParticleGenerator.ts (2)
17-17: Import of ParticleBufferUtils is correctly added
The addition of import { ParticleBufferUtils } from "./ParticleBufferUtils"; ensures that the necessary utilities for particle buffer manipulation are available.
27-27: Import of ParticleCompositeCurve is appropriately included
The import statement import { ParticleCompositeCurve } from "./modules/ParticleCompositeCurve"; is necessary for handling gravity modifiers using composite curves.
| switch (main.gravityModifier.mode) { | ||
| case ParticleCurveMode.Constant: | ||
| instanceVertices[offset + 19] = main.gravityModifier.constant; | ||
| break; | ||
| case ParticleCurveMode.TwoConstants: | ||
| instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random()); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Handle all possible ParticleCurveMode cases or add a default case
The switch statement currently handles only ParticleCurveMode.Constant and ParticleCurveMode.TwoConstants. Other possible modes like ParticleCurveMode.Curve and ParticleCurveMode.TwoCurves are not handled, which may result in gravity not being applied correctly when these modes are used.
Consider adding cases for the missing modes or a default case to ensure that all possible ParticleCurveMode values are handled. Here's a suggested update:
switch (main.gravityModifier.mode) {
case ParticleCurveMode.Constant:
instanceVertices[offset + 19] = main.gravityModifier.constant;
break;
case ParticleCurveMode.TwoConstants:
instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random());
break;
+ case ParticleCurveMode.Curve:
+ instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, main._gravityModifierRand.random());
+ break;
+ case ParticleCurveMode.TwoCurves:
+ instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, main._gravityModifierRand.random());
+ break;
+ default:
+ instanceVertices[offset + 19] = 0;
+ break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (main.gravityModifier.mode) { | |
| case ParticleCurveMode.Constant: | |
| instanceVertices[offset + 19] = main.gravityModifier.constant; | |
| break; | |
| case ParticleCurveMode.TwoConstants: | |
| instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random()); | |
| break; | |
| } | |
| switch (main.gravityModifier.mode) { | |
| case ParticleCurveMode.Constant: | |
| instanceVertices[offset + 19] = main.gravityModifier.constant; | |
| break; | |
| case ParticleCurveMode.TwoConstants: | |
| instanceVertices[offset + 19] = main.gravityModifier.evaluate(undefined, main._gravityModifierRand.random()); | |
| break; | |
| case ParticleCurveMode.Curve: | |
| instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, main._gravityModifierRand.random()); | |
| break; | |
| case ParticleCurveMode.TwoCurves: | |
| instanceVertices[offset + 19] = main.gravityModifier.evaluate(0, main._gravityModifierRand.random()); | |
| break; | |
| default: | |
| instanceVertices[offset + 19] = 0; | |
| break; | |
| } |
Please check if the PR fulfills these requirements
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
These changes collectively enhance the particle system's functionality and realism, offering a more dynamic experience for users.