Gizmo ignore particle bounds. #305
Conversation
|
Warning Rate limit exceeded@cptbtptpbcptdtptp has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve the addition of new imports from the Changes
Sequence Diagram(s)sequenceDiagram
participant Group
participant ParticleRenderer
Group->>ParticleRenderer: Check instance
alt Is ParticleRenderer?
Group-->>Group: Ignore bounding box
else
Group-->>Group: Include bounding box
end
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: 0
🧹 Outside diff range and nitpick comments (3)
packages/gizmo/src/Group.ts (3)
1-9: Remove unused importParticleSimulationSpace.The
ParticleSimulationSpaceimport is not used in the visible changes. Consider removing it to keep the imports clean and avoid potential confusion.Apply this diff to remove the unused import:
import { BoundingBox, Renderer, Vector3, Matrix, Entity, - ParticleRenderer, - ParticleSimulationSpace + ParticleRenderer } from "@galacean/engine";
344-350: LGTM. Consider updating documentation for this behavior change.The changes to ignore the bounding box of
ParticleRendererinstances in the_getCentermethod look good. This modification will exclude particle systems from affecting the group's center calculation, which can be beneficial in many scenarios.However, this change introduces a new behavior that might not be immediately obvious to users of the
Groupclass.Consider adding a comment explaining why particle renderers are ignored, and update the class or method documentation to reflect this new behavior. For example:
/** * Calculates the center of the group. * Note: ParticleRenderer instances are ignored in this calculation to prevent their potentially large or dynamic bounds from skewing the group's center. * @param out The vector to store the calculated center. */ private _getCenter(out: Vector3): void { // ... existing code ... // Ignore particle bounding box to prevent skewing the group's center if (renderer instanceof ParticleRenderer) { continue; } // ... rest of the method ... }🧰 Tools
🪛 Biome
[error] 349-349: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
1-9: Summary: Particle renderer exclusion from group center calculationsThe changes in this PR introduce a new behavior in the
Groupclass, specifically in the_getCentermethod. Particle renderers are now excluded from the bounding box calculations used to determine the group's center. This modification can potentially affect how groups containing particle systems are positioned or manipulated.Key points:
- The change is well-implemented and achieves the goal of ignoring particle renderer bounds.
- This behavior change might not be immediately obvious to users of the
Groupclass.- An unused import (
ParticleSimulationSpace) was introduced and should be removed.Recommendations:
- Update the class and/or method documentation to clearly explain this new behavior.
- Consider adding a comment in the code to explain why particle renderers are ignored.
- Remove the unused
ParticleSimulationSpaceimport.- If this change is part of a larger feature or bug fix, ensure that it's mentioned in the PR description or release notes.
To ensure this change doesn't introduce unexpected behavior:
- Review any code that relies on the
Groupclass's center calculations, especially in contexts involving particle systems.- Consider adding unit tests to verify the new behavior with and without particle renderers in the group.
- If applicable, update any performance benchmarks that might be affected by this change in bounding box calculations.
Also applies to: 344-350
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/gizmo/src/Group.ts (2 hunks)
🧰 Additional context used
🪛 Biome
packages/gizmo/src/Group.ts
[error] 349-349: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (1)
packages/gizmo/src/Group.ts (1)
344-350: Retain thecontinuestatement for clarity.The static analysis tool flagged the
continuestatement as unnecessary. While technically correct due to theelseblock, I recommend keeping thecontinuestatement for the following reasons:
- It clearly expresses the intent to skip
ParticleRendererinstances.- It makes the code more resilient to future changes (e.g., if more code is added after the
elseblock).- It improves readability by making the flow control explicit.
If you prefer to address the static analysis warning, you could refactor the code as follows:
if (!(renderer instanceof ParticleRenderer)) { isEffective = true; BoundingBox.merge(tempBoundBox, renderers[j].bounds, tempBoundBox); }However, I believe the current implementation with the
continuestatement is more explicit and easier to understand.🧰 Tools
🪛 Biome
[error] 349-349: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
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
Bug Fixes