Fix XR camera clear flag error #2281
Conversation
WalkthroughRecent changes enhance the clarity and maintainability of the Changes
Sequence Diagram(s)sequenceDiagram
participant Camera
participant Engine
participant RenderTarget
Camera->>Engine: Access _engine
Engine->>Camera: Provide context
Camera->>RenderTarget: Check conditions for rendering
RenderTarget-->>Camera: Render output
sequenceDiagram
participant XRManager
participant XRManagerExtended
participant XRCameraManager
XRManager->>XRManagerExtended: Call _getCameraIgnoreClearFlags
XRManagerExtended-->>XRManager: Return CameraClearFlags
XRManagerExtended->>XRCameraManager: Call _getIgnoreClearFlags
XRCameraManager-->>XRManagerExtended: Return CameraClearFlags
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 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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/Camera.ts (2 hunks)
- packages/xr/src/input/XRInputManager.ts (6 hunks)
Additional context used
Biome
packages/xr/src/input/XRInputManager.ts
[error] 77-77: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 129-129: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (4)
packages/xr/src/input/XRInputManager.ts (3)
1-3: Approved: Import statements and class property update.The import of
SafeLoopArrayandIXRListeneris necessary for the new listener management system. The_listenersproperty update is appropriate.Also applies to: 26-26
69-69: Approved: Listener addition update.The update to add listeners as objects with a
fnproperty to theSafeLoopArrayis appropriate and enhances listener management.
84-84: Approved: Listener retrieval and invocation update.The update to retrieve listeners via
getLoopArrayand check if a listener is destroyed before invoking it is appropriate and improves robustness.Also applies to: 117-120
packages/core/src/Camera.ts (1)
602-603: Approved: Streamlined engine context access and improved control flow.The introduction of a local variable
engineimproves readability and potentially enhances performance. The additional checks for_renderTargetandindependentCanvasEnabledprovide more nuanced control flow.Also applies to: 634-640, 643-643
| if (index >= 0) { | ||
| listeners.splice(index, 1); | ||
| } | ||
| this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); |
There was a problem hiding this comment.
Refactor: Avoid assignment in expression.
The assignment within the expression is confusing and should be refactored for clarity.
- this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false));
+ this._listeners.findAndRemove((value) => {
+ if (value.fn === listener) {
+ value.destroyed = true;
+ return true;
+ }
+ return false;
+ });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.
| this._listeners.findAndRemove((value) => (value.fn === listener ? (value.destroyed = true) : false)); | |
| this._listeners.findAndRemove((value) => { | |
| if (value.fn === listener) { | |
| value.destroyed = true; | |
| return true; | |
| } | |
| return false; | |
| }); |
Tools
Biome
[error] 77-77: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| @@ -128,7 +126,7 @@ | |||
| * @internal | |||
| */ | |||
| _onDestroy(): void { | |||
| this._listeners.length = 0; | |||
| this._listeners.findAndRemove((value) => (value.destroyed = true)); | |||
There was a problem hiding this comment.
Refactor: Avoid assignment in expression.
The assignment within the expression is confusing and should be refactored for clarity.
- this._listeners.findAndRemove((value) => (value.destroyed = true));
+ this._listeners.findAndRemove((value) => {
+ value.destroyed = true;
+ return true;
+ });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.
| this._listeners.findAndRemove((value) => (value.destroyed = true)); | |
| this._listeners.findAndRemove((value) => { | |
| value.destroyed = true; | |
| return true; | |
| }); |
Tools
Biome
[error] 129-129: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
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/Camera.ts (2 hunks)
- packages/core/src/xr/XRManager.ts (1 hunks)
- packages/xr/src/XRManagerExtended.ts (2 hunks)
- packages/xr/src/feature/camera/XRCameraManager.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Camera.ts
Additional comments not posted (4)
packages/core/src/xr/XRManager.ts (1)
41-41: Method renamed for clarity.The method
_getCameraClearFlagsMaskhas been renamed to_getCameraIgnoreClearFlags. This change improves clarity regarding the method's purpose within the context of camera management in XR scenarios.packages/xr/src/feature/camera/XRCameraManager.ts (1)
128-128: Method renamed for clarity.The method
_getCameraClearFlagsMaskhas been renamed to_getCameraIgnoreClearFlags. This change improves clarity regarding the method's purpose within the context of camera management in XR scenarios.packages/xr/src/XRManagerExtended.ts (2)
147-148: Method renamed for clarity.The method
_getCameraClearFlagsMaskhas been renamed to_getCameraIgnoreClearFlags. This change improves clarity regarding the method's purpose within the context of camera management in XR scenarios.
293-293: Method renamed for clarity.The method
_getCameraClearFlagsMaskhas been renamed to_getCameraIgnoreClearFlagsin the module augmentation for@galacean/engine. This change improves clarity regarding the method's purpose within the context of camera management in XR scenarios.
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/Camera.ts (2 hunks)
- packages/xr/src/XRManagerExtended.ts (2 hunks)
- packages/xr/src/feature/camera/XRCameraManager.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/core/src/Camera.ts
- packages/xr/src/XRManagerExtended.ts
- packages/xr/src/feature/camera/XRCameraManager.ts
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