Fix the buttons of the current frame are not updated in time
#2506
Conversation
cptbtptpbcptdtptp
added
bug
WalkthroughThe changes introduce updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2506 +/- ##
==========================================
+ Coverage 68.53% 68.56% +0.02%
==========================================
Files 957 957
Lines 100057 100052 -5
Branches 8552 8561 +9
==========================================
+ Hits 68575 68601 +26
+ Misses 31226 31195 -31
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
🧹 Nitpick comments (1)
tests/src/core/input/InputManager.test.ts (1)
Line range hint
133-149: Consider adding edge case tests.While the current pointer state verification is solid, consider adding tests for:
- Rapid pointer events (quick down/up sequences)
- Multiple pointer buttons pressed simultaneously
- Interrupted pointer sequences (e.g., pointercancel during drag)
This would further strengthen the test coverage against timing-related issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/core/input/InputManager.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (2)
tests/src/core/input/InputManager.test.ts (2)
133-135: LGTM! Valuable assertions for pointer state verification.These assertions effectively verify that the pointer state is correctly updated immediately after the pointerdown event, helping prevent timing-related issues with button state updates.
147-149: LGTM! Complete pointer lifecycle verification.These assertions effectively complement the earlier pointer state checks by verifying proper state cleanup after pointer release, ensuring a complete test of the pointer interaction lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/component/UICanvas.ts (1)
Line range hint
233-246: Consider optimizing raycast performance for large UI hierarchies.The implementation is correct, but for large UI hierarchies, consider these optimizations:
- Add early bounds checking before detailed raycasting.
- Consider spatial partitioning for large numbers of renderers.
Example optimization:
_raycast(ray: Ray, out: UIHitResult, distance: number = Number.MAX_SAFE_INTEGER): boolean { const renderers = this._getRenderers(); + // Early bounds check for the entire canvas + if (!this.entity.transform.bounds.intersectsRay(ray)) { + out.reset(); + return false; + } for (let i = renderers.length - 1; i >= 0; i--) { const element = renderers[i]; - if (element.raycastEnabled && element._raycast(ray, out, distance)) { + if (element.raycastEnabled && + element.bounds.intersectsRay(ray) && // Quick bounds check + element._raycast(ray, out, distance)) { return true; } } - out.component = null; - out.entity = null; - out.distance = 0; - out.point.set(0, 0, 0); - out.normal.set(0, 0, 0); + out.reset(); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/component/UICanvas.ts(1 hunks)packages/ui/src/input/UIPointerEventEmitter.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/input/UIPointerEventEmitter.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/ui/src/component/UICanvas.ts (1)
230-233: Breaking change:raycastmethod is now internal.The change to make this method internal is appropriate as it should be an implementation detail of the UI system. However, this is a breaking change that could affect external consumers who might be directly using the
raycastmethod.Let's verify if there are any external usages of the public
raycastmethod that need to be updated:✅ Verification successful
Breaking change verified:
raycastto_raycastchange is safeThe method was only used internally by UI components, and existing usages already follow the internal
_raycastpattern. No external consumers were found to be directly using the publicraycastmethod.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of raycast method on UICanvas instances rg -l "canvas\s*\.\s*raycast\s*\(" --type ts # Search for any documentation or examples that might reference the public method rg -l "UICanvas.*raycast" --type mdLength of output: 84
Script:
#!/bin/bash # Search for any occurrence of raycast in TypeScript files rg "raycast" --type ts -l # Find files importing UICanvas rg "import.*UICanvas" --type ts -l # Look for classes extending UICanvas ast-grep --pattern 'class $_ extends UICanvas'Length of output: 1534
Script:
#!/bin/bash # Check usage context in UI-related files rg "raycast" -B 2 -A 2 packages/ui/src/input/UIPointerEventEmitter.ts packages/ui/src/component/UIRenderer.tsLength of output: 3594
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/ui/UIUtils.ts (1)
55-64: Add documentation and improve type safety for OverlayCamera class.The OverlayCamera class lacks documentation explaining its purpose and usage. Additionally:
- Remove
@ts-ignoreand properly type the instanceId- Consider adding JSDoc comments for the class and its properties
+/** + * Camera used for rendering UI overlays. + * @internal This class is intended for internal use by UIUtils. + */ class OverlayCamera { - // @ts-ignore - instanceId: number = ++EngineObject._instanceIdCounter; + /** Unique identifier for the camera instance */ + readonly instanceId: number = ++EngineObject._instanceIdCounter; + /** Reference to the engine instance */ engine: Engine; + /** Reference to the scene being rendered */ scene: Scene;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/src/Camera.ts(0 hunks)packages/core/src/Engine.ts(1 hunks)packages/core/src/Entity.ts(0 hunks)packages/core/src/Renderer.ts(0 hunks)packages/core/src/ui/UIUtils.ts(2 hunks)packages/ui/src/input/UIPointerEventEmitter.ts(4 hunks)
💤 Files with no reviewable changes (3)
- packages/core/src/Renderer.ts
- packages/core/src/Camera.ts
- packages/core/src/Entity.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/input/UIPointerEventEmitter.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/ui/UIUtils.ts
[error] 23-23: 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] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 24-24: 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] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 25-25: 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] 25-25: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 27-27: 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] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (3)
packages/core/src/ui/UIUtils.ts (1)
38-39: LGTM! Improved readability with destructuring.The change to use destructuring for accessing position makes the code more readable while maintaining the same functionality.
packages/core/src/Engine.ts (2)
578-597: LGTM! Improved camera handling with index tracking.The changes improve the rendering flow by:
- Allowing rendering to continue even without active cameras while maintaining the debug message
- Adding camera index tracking which helps maintain rendering order
603-603: LGTM! Correctly updated renderOverlay call.The call to UIUtils.renderOverlay has been properly updated to include the scene parameter, aligning with the modified method signature.
| static renderOverlay(engine: Engine, scene: Scene, uiCanvases: DisorderedArray<IUICanvas>): void { | ||
| const { canvas, _hardwareRenderer: rhi, _renderContext: renderContext, _batcherManager: batcherManager } = engine; | ||
| const uiRenderQueue = (this._renderQueue ||= new RenderQueue(RenderQueueType.Transparent)); | ||
| const virtualCamera = (this._virtualCamera ||= new VirtualCamera()); | ||
| const viewport = (this._viewport ||= new Vector4(0, 0, 1, 1)); | ||
| const { canvas, _hardwareRenderer: rhi, _renderContext: renderContext, _batcherManager: batcherManager } = engine; | ||
| // @ts-ignore | ||
| const camera = (this._overlayCamera ||= new OverlayCamera()); | ||
| camera.engine = engine; | ||
| camera.scene = scene; | ||
| renderContext.camera = camera as unknown as Camera; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and static method implementation.
The code has several areas that could be improved:
- Remove
@ts-ignoreand properly type the camera assignment - Avoid assignments in expressions (e.g.,
this._overlayCamera ||= new OverlayCamera()) - Use class name instead of
thisin static methods
- // @ts-ignore
- const camera = (this._overlayCamera ||= new OverlayCamera());
+ const camera = UIUtils._overlayCamera || (UIUtils._overlayCamera = new OverlayCamera());
- renderContext.camera = camera as unknown as Camera;
+ renderContext.camera = camera as Camera;📝 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.
| static renderOverlay(engine: Engine, scene: Scene, uiCanvases: DisorderedArray<IUICanvas>): void { | |
| const { canvas, _hardwareRenderer: rhi, _renderContext: renderContext, _batcherManager: batcherManager } = engine; | |
| const uiRenderQueue = (this._renderQueue ||= new RenderQueue(RenderQueueType.Transparent)); | |
| const virtualCamera = (this._virtualCamera ||= new VirtualCamera()); | |
| const viewport = (this._viewport ||= new Vector4(0, 0, 1, 1)); | |
| const { canvas, _hardwareRenderer: rhi, _renderContext: renderContext, _batcherManager: batcherManager } = engine; | |
| // @ts-ignore | |
| const camera = (this._overlayCamera ||= new OverlayCamera()); | |
| camera.engine = engine; | |
| camera.scene = scene; | |
| renderContext.camera = camera as unknown as Camera; | |
| static renderOverlay(engine: Engine, scene: Scene, uiCanvases: DisorderedArray<IUICanvas>): void { | |
| const { canvas, _hardwareRenderer: rhi, _renderContext: renderContext, _batcherManager: batcherManager } = engine; | |
| const uiRenderQueue = (this._renderQueue ||= new RenderQueue(RenderQueueType.Transparent)); | |
| const virtualCamera = (this._virtualCamera ||= new VirtualCamera()); | |
| const viewport = (this._viewport ||= new Vector4(0, 0, 1, 1)); | |
| const camera = UIUtils._overlayCamera || (UIUtils._overlayCamera = new OverlayCamera()); | |
| camera.engine = engine; | |
| camera.scene = scene; | |
| renderContext.camera = camera as Camera; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: 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] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 24-24: 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] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 25-25: 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] 25-25: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 27-27: 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] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
Summary by CodeRabbit
UICanvasclass to restrict external access to the raycasting functionality.OverlayCameraclass to facilitate overlay rendering with scene context.InputManager, ensuring accurate state reflection throughout pointer interactions.