Fixed some GUI issues #2504
Fixed some GUI issues #2504
Conversation
* refactor: update physX wasm without assertions
Fix: Merge code and fix unit test error
refactor: UIRenderer.raycastEnable -> UIRenderer.raycastEnabled
Fix: fix conflicts
WalkthroughThis pull request introduces enhancements to resource deserialization and UI components across multiple files. The changes focus on improving entity transformation handling, adding new properties to UI components, and refining interaction mechanisms. Key modifications include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (4)
Finishing Touches
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 @@
## dev/1.4 #2504 +/- ##
===========================================
+ Coverage 68.50% 68.51% +0.01%
===========================================
Files 956 957 +1
Lines 100007 100068 +61
Branches 8545 8548 +3
===========================================
+ Hits 68506 68560 +54
- Misses 31245 31252 +7
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts(3 hunks)packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(3 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(1 hunks)packages/ui/src/component/UICanvas.ts(7 hunks)packages/ui/src/component/advanced/Text.ts(1 hunks)packages/ui/src/component/interactive/UIInteractive.ts(1 hunks)packages/ui/src/component/interactive/transition/ColorTransition.ts(1 hunks)packages/ui/src/component/interactive/transition/SpriteTransition.ts(1 hunks)packages/ui/src/index.ts(2 hunks)packages/ui/src/input/UIPointerEventEmitter.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui/src/component/UICanvas.ts
[error] 308-308: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
[error] 317-317: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (17)
packages/ui/src/component/interactive/transition/SpriteTransition.ts (1)
47-47: Simplified sprite assignment logic.The removal of the fallback to
this._normalmakes the assignment more direct and predictable.Let's verify that this change doesn't introduce null sprite issues:
✅ Verification successful
Direct sprite assignment is safe and consistent with codebase patterns.
The simplified assignment aligns with the established sprite handling patterns across the codebase, where components are designed to handle null sprites gracefully and proper cleanup is implemented in destroy methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for null checks before sprite assignments rg -A 2 "sprite\s*=\s*(?!null)[^;]+" --type tsLength of output: 291
Script:
#!/bin/bash # Find sprite assignments using ast-grep ast-grep --pattern 'sprite = $_' # Find sprite-related error handling rg -B2 -A2 "sprite.*(?:null|undefined|Error)" --pcre2 --type ts # Look for sprite property access patterns ast-grep --pattern '$_.sprite'Length of output: 19273
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
41-41: Good addition of explicit transform support.Adding a dedicated
transformproperty toIBasicEntityimproves the clarity of entity transformation handling. This aligns well with the PR objective of enhancing UITransform parsing.Let's verify the transform property usage:
✅ Verification successful
Transform property integration verified successfully
The optional transform property in
IBasicEntityis well-integrated into the codebase:
- Used consistently across UI components (UICanvas, Image, Text)
- Properly supports UITransform implementation
- Maintains backward compatibility with existing component system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for transform property usage in entity parsing ast-grep --pattern 'transform?: IComponent'Length of output: 45
Script:
#!/bin/bash # Search for IBasicEntity interface usage and UITransform references echo "=== Searching for IBasicEntity usage ===" rg "IBasicEntity" -A 2 echo -e "\n=== Searching for UITransform references ===" rg "UITransform" -A 2 echo -e "\n=== Searching for transform property handling ===" rg "transform.*IComponent" -A 2Length of output: 9802
packages/ui/src/component/interactive/transition/ColorTransition.ts (1)
36-39: Improved state handling with consistent pattern.The changes enhance state management by:
- Using proper state comparison
- Ensuring
_finalValueis set before update- Using
||=for safe assignment- Maintaining consistent pattern across all state handlers
Let's verify the state handling consistency:
Also applies to: 43-46, 50-53, 57-60
✅ Verification successful
Verified: State handling patterns are consistent across all interactive states
The codebase shows identical state handling patterns for Normal, Hover, Pressed, and Disable states, each following the same structure for state comparison, value assignment, and update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar state handling patterns ast-grep --pattern 'if (this._finalState === $_) { this._finalValue ||= this.$_; this._updateValue(); }'Length of output: 1769
packages/ui/src/index.ts (2)
Line range hint
1-1: Verify distanceForSort calculation fix.The PR objective mentions fixing the
distanceForSortcalculation, but this change is not visible in the provided files.Let's search for the calculation:
124-138: Well-structured GUI component registration.The
registerGUIfunction provides a centralized way to register all UI components, including the newly supported UITransform. This enhances maintainability and ensures consistent component registration.Let's verify the component availability:
✅ Verification successful
Component registration and implementation verified successfully.
All UI components registered in
registerGUI()are properly implemented and follow consistent inheritance patterns. The registration is centralized and matches the component implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for component class definitions ast-grep --pattern 'export class $_ extends $_'Length of output: 49
Script:
#!/bin/bash # Find component files echo "=== Component Files ===" fd -e ts -e tsx # Search for class definitions with more flexible pattern echo -e "\n=== Class Definitions ===" rg "class (Text|Image|Button|UIGroup|UICanvas|UITransform|ScaleTransition|ColorTransition|SpriteTransition)" -A 1 # Check export statements echo -e "\n=== Export Statements ===" rg "export.*(Text|Image|Button|UIGroup|UICanvas|UITransform|ScaleTransition|ColorTransition|SpriteTransition)"Length of output: 123658
Script:
#!/bin/bash # Check component implementations echo "=== Text Component ===" rg "class Text" -A 10 packages/ui/src/component/advanced/Text.ts echo -e "\n=== UITransform Component ===" rg "class UITransform" -A 10 packages/ui/src/component/UITransform.ts echo -e "\n=== UICanvas Component ===" rg "class UICanvas" -A 10 packages/ui/src/component/UICanvas.ts echo -e "\n=== Component Registration ===" rg "registerClass|Loader\.register" packages/ui/src/Length of output: 2504
packages/ui/src/component/interactive/UIInteractive.ts (1)
157-159: LGTM! Safer transition cleanup.The change from forward to backward iteration when destroying transitions is a good practice. It prevents potential issues with array index shifting during element removal.
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
29-38: LGTM! Enhanced transform handling.The new transform handling logic provides better flexibility by:
- Supporting explicit transform configuration through
transformConfig- Maintaining backward compatibility with direct position/rotation/scale properties
171-172: LGTM! Dynamic transform class resolution.Good enhancement to support custom transform classes while defaulting to the standard Transform class when not specified.
packages/ui/src/input/UIPointerEventEmitter.ts (3)
69-72: LGTM! Improved camera transform handling.Good enhancement to extract camera properties once and reuse them, improving code readability and potentially performance.
76-76: LGTM! Enhanced sort distance calculation.The addition of
isOrthographicandcameraForwardparameters provides better context for accurate distance calculations.
86-86: LGTM! More robust render check.Replacing direct camera comparison with
_canRendermethod allows for more complex render eligibility logic.packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (2)
223-224: LGTM! Consistent transform handling.Good alignment with ReflectionParser's transform handling, maintaining consistency across the codebase.
292-302: LGTM! Enhanced entity data application.The changes provide a more flexible approach to applying transform data:
- Support for explicit transform configuration
- Fallback to direct property copying
- Clean handling of the layer property
packages/ui/src/component/advanced/Text.ts (1)
231-231: LGTM! Good default value for raycastEnabled.Setting
raycastEnabledtofalseby default is a good practice as text components are typically not interactive, improving performance by skipping unnecessary raycasting operations.packages/ui/src/component/UICanvas.ts (3)
2-3: LGTM! Required imports for the new functionality.The imports are correctly ordered and necessary for the new bounding box functionality.
96-99: LGTM! Well-structured property declarations.The properties are correctly marked with
@ignoreCloneand initialized appropriately for managing the canvas bounds.
592-605: LGTM! Well-implemented bounds calculation with dirty flag pattern.The method efficiently calculates and caches the canvas bounds, using the dirty flag pattern to avoid unnecessary recalculations.
| @@ -297,7 +305,7 @@ | |||
| /** | |||
| * @internal | |||
| */ | |||
| _updateSortDistance(cameraPosition: Vector3): void { | |||
| _updateSortDistance(isOrthographic: Boolean, cameraPosition: Vector3, cameraForward: Vector3): void { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use lowercase 'boolean' type instead of 'Boolean'.
For consistency with TypeScript conventions, use the primitive type boolean instead of the object type Boolean.
Apply this diff:
- _updateSortDistance(isOrthographic: Boolean, cameraPosition: Vector3, cameraForward: Vector3): void {
+ _updateSortDistance(isOrthographic: boolean, cameraPosition: Vector3, cameraForward: Vector3): void {📝 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.
| _updateSortDistance(isOrthographic: Boolean, cameraPosition: Vector3, cameraForward: Vector3): void { | |
| _updateSortDistance(isOrthographic: boolean, cameraPosition: Vector3, cameraForward: Vector3): void { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 308-308: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
@cptbtptpbcptdtptp Thanks for making the change! 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| const boundsCenter = this._getBounds().getCenter(UICanvas._tempVec3); | ||
| if (isOrthographic) { | ||
| Vector3.subtract(boundsCenter, cameraPosition, boundsCenter); | ||
| this._sortDistance = Vector3.dot(boundsCenter, cameraForward); | ||
| } else { | ||
| this._sortDistance = Vector3.distanceSquared(boundsCenter, cameraPosition); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap switch case declarations in a block.
To prevent accidental access to the declarations from other switch cases, wrap the code in a block.
Apply this diff:
case CanvasRenderMode.WorldSpace: {
const boundsCenter = this._getBounds().getCenter(UICanvas._tempVec3);
if (isOrthographic) {
Vector3.subtract(boundsCenter, cameraPosition, boundsCenter);
this._sortDistance = Vector3.dot(boundsCenter, cameraForward);
} else {
this._sortDistance = Vector3.distanceSquared(boundsCenter, cameraPosition);
}
break;
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 317-317: 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)
UITransformdistanceForSortcalculation errorTransitiondestruction errorSummary by CodeRabbit
Release Notes
New Features
transformconfiguration for entities.raycastEnabledfor Text components.Improvements
Bug Fixes
These updates enhance the UI framework's flexibility, interaction handling, and rendering capabilities.