Fix particle rotation bug #2423
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/particle/modules/MainModule.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/particle/modules/MainModule.ts
[error] 296-296: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
packages/core/src/particle/modules/MainModule.ts (1)
15-15: Good fix: Preventing shared state issues by making _tempVector40 instance-level.
Converting _tempVector40 from static to instance-level is a good fix to prevent potential race conditions when multiple particle systems are active simultaneously.
Let's verify if there are other similar static Vector4 variables that might need the same treatment:
✅ Verification successful
✅ The _tempVector40 change is safe and no similar cases need fixing
The search results confirm this is the only temporary Vector4 used in particle system operations. Other Vector4 instances found in the codebase serve different purposes (viewport, fog parameters, atlas regions, etc.) and are correctly scoped as instance variables. This validates that the change from static to instance-level for _tempVector40 is appropriate and no other similar cases need the same treatment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for static Vector4 declarations in particle-related files
rg -U "private\s+static.*Vector4.*=\s*new\s*Vector4" --type typescript
Length of output: 107
Script:
#!/bin/bash
# Search for static Vector4 declarations in all files
rg -U "private\s+static.*Vector4.*=\s*new\s*Vector4"
# Also search for any Vector4 declarations to understand the pattern
rg -U "private.*Vector4.*=\s*new\s*Vector4"
Length of output: 1504
| const worldRotationV4 = this._tempVector40.copyFrom(worldRotation); // Maybe shaderData should support Quaternion | ||
| shaderData.setVector4(MainModule._worldRotation, worldRotationV4); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address switch clause scoping and consider Quaternion support.
- The variable declaration in the switch clause should be wrapped in a block to prevent potential scoping issues.
- The comment suggests a technical limitation with ShaderData not supporting Quaternion directly.
Apply this fix for the switch clause scoping:
case ParticleSimulationSpace.Local: {
shaderData.setVector3(MainModule._worldPosition, transform.worldPosition);
const worldRotation = transform.worldRotationQuaternion;
const worldRotationV4 = this._tempVector40.copyFrom(worldRotation);
shaderData.setVector4(MainModule._worldRotation, worldRotationV4);
break;
}Consider adding Quaternion support to ShaderData to eliminate the need for Vector4 conversion. This would improve type safety and reduce temporary object creation.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 296-296: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
MainModule, allowing for better state handling.Bug Fixes
MainModuleto ensure consistent behavior across instances.