Add GUI Infrastructure #2375
Add GUI Infrastructure #2375
Conversation
* feat: enable startFrame
feat: change img
Add UI Input
singlecoder
requested review from
GuoLei1990,
cptbtptpbcptdtptp and
johanzhu
WalkthroughThis pull request introduces extensive updates to the UI framework, adding new classes, interfaces, and enums while modifying existing components for enhanced functionality. Key additions include the Changes
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: 6
Outside diff range and nitpick comments (2)
packages/core/src/ui/UITransform.ts (1)
6-53: Add documentation comments.The
UITransformclass could benefit from some documentation comments to explain its purpose and usage.Consider adding a class-level documentation comment to explain what the
UITransformclass represents and how it should be used. Also, consider adding documentation comments for the public properties and methods.packages/core/src/ui/UICanvas.ts (1)
186-196: Consider simplifying the constructor.The constructor is doing too much work by initializing several properties and binding event listeners. Consider extracting some of the initialization logic into separate methods to improve readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (38)
- packages/core/src/2d/assembler/ISpriteAssembler.ts (1 hunks)
- packages/core/src/2d/assembler/SimpleSpriteAssembler.ts (4 hunks)
- packages/core/src/2d/assembler/SlicedSpriteAssembler.ts (6 hunks)
- packages/core/src/2d/assembler/TiledSpriteAssembler.ts (6 hunks)
- packages/core/src/2d/sprite/SpriteMask.ts (3 hunks)
- packages/core/src/2d/sprite/SpriteRenderer.ts (4 hunks)
- packages/core/src/Camera.ts (10 hunks)
- packages/core/src/Component.ts (1 hunks)
- packages/core/src/ComponentsDependencies.ts (1 hunks)
- packages/core/src/ComponentsManager.ts (4 hunks)
- packages/core/src/Engine.ts (13 hunks)
- packages/core/src/Entity.ts (12 hunks)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/BatcherManager.ts (2 hunks)
- packages/core/src/Renderer.ts (2 hunks)
- packages/core/src/Scene.ts (1 hunks)
- packages/core/src/Script.ts (4 hunks)
- packages/core/src/Transform.ts (2 hunks)
- packages/core/src/enums/CameraType.ts (1 hunks)
- packages/core/src/enums/ComponentType.ts (1 hunks)
- packages/core/src/index.ts (1 hunks)
- packages/core/src/input/pointer/Pointer.ts (2 hunks)
- packages/core/src/input/pointer/PointerEvent.ts (1 hunks)
- packages/core/src/input/pointer/PointerEventType.ts (1 hunks)
- packages/core/src/input/pointer/PointerManager.ts (3 hunks)
- packages/core/src/shader/ShaderPool.ts (2 hunks)
- packages/core/src/shaderlib/extra/uiDefault.fs.glsl (1 hunks)
- packages/core/src/shaderlib/extra/uiDefault.vs.glsl (1 hunks)
- packages/core/src/ui/CanvasGroup.ts (1 hunks)
- packages/core/src/ui/UICanvas.ts (1 hunks)
- packages/core/src/ui/UIImage.ts (1 hunks)
- packages/core/src/ui/UIRenderer.ts (1 hunks)
- packages/core/src/ui/UITransform.ts (1 hunks)
- packages/core/src/ui/enums/BlockingObjects.ts (1 hunks)
- packages/core/src/ui/enums/CanvasRenderMode.ts (1 hunks)
- packages/core/src/ui/enums/ResolutionAdaptationStrategy.ts (1 hunks)
- packages/core/src/ui/index.ts (1 hunks)
- tests/src/core/2d/ui/UICanvas.test.ts (1 hunks)
Additional context used
Biome
packages/core/src/RenderPipeline/BatcherManager.ts
[error] 18-18: 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] 22-22: 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] 26-26: 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] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/ui/UIImage.ts
[error] 233-233: 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)
[error] 246-246: 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)
packages/core/src/ComponentsManager.ts
[error] 101-101: 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)
packages/core/src/ui/UICanvas.ts
[error] 201-201: 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] 214-215: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 216-217: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 326-326: 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)
[error] 334-334: 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)
packages/core/src/Entity.ts
[error] 621-621: 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] 622-622: 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] 639-639: 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] 646-646: 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 (124)
packages/core/src/input/pointer/PointerEventType.ts (1)
1-7: LGTM!The
PointerEventTypeenum is well-defined and covers the common pointer events. Using this enum will make the pointer event handling code more readable and maintainable.packages/core/src/ui/enums/BlockingObjects.ts (1)
1-6: LGTM!The
BlockingObjectsenum is well-defined and the members are self-explanatory. The enum is likely used to specify the type of objects that can block UI elements. The export indicates that it is intended to be used in other parts of the codebase.The code changes are approved.
packages/core/src/input/pointer/PointerEvent.ts (1)
3-5: LGTM, but verify the class usage.The code changes are approved. The
PointerEventclass is correctly implemented and follows the naming conventions. However, it's unclear how this class fits into the overall changes described in the PR objectives and the AI-generated summary. Please provide more context on how this class is used and how it relates to the sprite rendering and UI management changes.packages/core/src/ui/enums/CanvasRenderMode.ts (1)
1-8: LGTM!The enum is well-defined, follows naming conventions, and covers the common render modes for a UI canvas. The comments provide a clear description of the purpose. The file is focused on a single responsibility, which is good for maintainability.
packages/core/src/ui/enums/ResolutionAdaptationStrategy.ts (1)
1-10: LGTM!The code changes are approved. The enum is well-defined and follows the naming convention for enums in TypeScript. The enum members are self-explanatory and cover the common resolution adaptation strategies.
Run the following script to verify the enum usage:
Verification successful
Enum Usage Verified
The
ResolutionAdaptationStrategyenum is actively used in the codebase, particularly in theUICanvas.tsfile, where it is utilized to manage resolution adaptation strategies. The enum is correctly exported and imported, confirming its proper integration within the codebase.
- Files Involved:
packages/core/src/ui/index.ts: Enum is exported.packages/core/src/ui/UICanvas.ts: Enum is imported and used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ResolutionAdaptationStrategy` enum in the codebase. # Test: Search for the enum usage. Expect: At least one occurrence. rg --type typescript $'ResolutionAdaptationStrategy'Length of output: 86
Script:
#!/bin/bash # Description: Verify the usage of `ResolutionAdaptationStrategy` enum in the codebase. # Test: Search for the enum usage in all TypeScript files. rg 'ResolutionAdaptationStrategy' --glob '*.ts' -A 2Length of output: 2722
packages/core/src/shaderlib/extra/uiDefault.fs.glsl (3)
1-2: LGTM!The uniform sampler2D variable
renderer_UITextureis correctly declared.
3-5: LGTM!The varying variables
v_uvandv_colorare correctly declared with the appropriate types.
6-9: LGTM!The
mainfunction correctly samples the texture and applies the tint color. The resulting color is correctly assigned togl_FragColor.packages/core/src/enums/CameraType.ts (2)
9-9: LGTM!The addition of the
UIOverlayenum member with a value of0x8is a good change that expands the enum's capability to represent user interface overlays in the camera system. The value is a power of 2, which is a good practice for defining enum values that can be combined using bitwise operations.
10-10: LGTM!The addition of the
XRCameraenum member that is defined as a bitwise combination of the existing camera types is a good change that allows for a more flexible representation of XR camera configurations. The bitwise combination is a good practice for defining enum values that can be combined.packages/core/src/enums/ComponentType.ts (1)
1-14: LGTM!The
ComponentTypeenum looks good:
- The enum values are correctly defined as powers of 2, allowing them to be used as bit flags.
- The enum covers a good range of component types, including the newly introduced UI related types.
- The
@internaltag is correctly used to indicate that this enum should not be part of the public API.The changes are approved.
packages/core/src/shaderlib/extra/uiDefault.vs.glsl (1)
1-15: LGTM!The vertex shader code looks good:
- The uniform, attributes, and varying variables are correctly defined and used.
- The
mainfunction performs the expected vertex transformations and assignments.- The code follows best practices and naming conventions.
The code changes are approved.
packages/core/src/ui/index.ts (1)
1-8: LGTM!The changes in this file are approved. The file serves as a central location for exporting UI-related classes and enums, which aligns with the PR objective and the AI-generated summary. This is a good practice for organizing and exposing a public API.
packages/core/src/2d/assembler/ISpriteAssembler.ts (3)
9-16: LGTM!The changes to the
updatePositionsmethod signature are approved. The additional parameters (width,height,pivot,flipX, andflipY) provide more flexibility and control over sprite positioning and transformations, aligning with the PR objective of enhancing the sprite rendering infrastructure.
18-18: LGTM!The change to the
updateColormethod signature is approved. The optionalalphaparameter provides more control over sprite transparency during color updates, aligning with the PR objective of enhancing the sprite rendering infrastructure.
19-19: LGTM!The introduction of the new
updateAlphamethod is approved. This method provides a dedicated way to set the alpha value, further refining the control over sprite rendering and aligning with the PR objective of enhancing the sprite rendering infrastructure.packages/core/src/ui/CanvasGroup.ts (6)
6-28: LGTM!The
CanvasGroupclass structure and constructor are correctly implemented.
7-8: LGTM!The
ignoreParentGroupproperty is correctly declared and initialized with the@assignmentClonedecorator.
9-10: LGTM!The
_globalAlphaproperty is correctly declared and initialized with an@internalJSDoc comment.
11-12: LGTM!The
_alphaproperty is correctly declared and initialized with the@assignmentClonedecorator.
14-18: LGTM!The
set alphamethod is correctly implemented. It clamps the input value between 0 and 1 and only updates the_alphaproperty if the new value is different.
20-22: LGTM!The
get alphamethod is correctly implemented. It returns the value of the_alphaproperty.tests/src/core/2d/ui/UICanvas.test.ts (1)
4-17: LGTM!The test suite setup looks good. It creates a suitable environment for testing the
UICanvascomponent by setting up a canvas, engine, scene, root entity, and a camera entity. TheUICanvascomponent is correctly added to the root entity.packages/core/src/ui/UITransform.ts (3)
6-53: LGTM!The
UITransformclass is well-structured and follows good practices. The code changes are approved.
32-44: Verify the constructor logic.The constructor performs some unusual setup by destroying the entity's existing transform and assigning itself as the new transform. This may have unintended consequences.
Please verify that this logic is correct and doesn't break any existing functionality. Consider adding comments to explain why this is necessary.
55-64: LGTM!The
UITransformModifyFlagsenum is well-defined and follows good practices. The code changes are approved.packages/core/src/index.ts (7)
1-2: LGTM!The changes are approved. Reordering exports does not affect the functionality.
9-10: LGTM, but verify the usage of the newly added exports.The changes are approved. However, ensure that the introduction of the
BoolUpdateFlagandCameraexports does not cause any issues in the codebase.Run the following script to verify the usage of the newly added exports:
Verification successful
Verification successful: Newly added exports are appropriate and well-integrated.
The
BoolUpdateFlagandCameraexports are widely used across the codebase, indicating that their addition is consistent with the existing structure and necessary for the functionality. No issues were found related to these exports.
- The
BoolUpdateFlagis used in multiple files, includingCamera.ts,Transform.ts, and several others.- The
Camerais extensively used in tests, examples, and core files, confirming its role as a core component.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added `BoolUpdateFlag` and `Camera` exports. # Test: Search for the usage of `BoolUpdateFlag`. Expect: No issues. ast-grep --lang typescript --pattern $'BoolUpdateFlag' # Test: Search for the usage of `Camera`. Expect: No issues. ast-grep --lang typescript --pattern $'Camera'Length of output: 32428
12-14: LGTM, but verify the usage of the newly added exports and explicitly exported types.The changes are approved. However, ensure that the introduction of the
DependentMode,dependentComponents,EngineConfiguration, andEngineSettingsexports does not cause any issues in the codebase.Run the following script to verify the usage of the newly added exports and explicitly exported types:
Verification successful
Verification successful: Newly added exports and explicitly exported types are used correctly.
The introduction of
DependentMode,dependentComponents,EngineConfiguration, andEngineSettingsexports does not cause any issues in the codebase. Their usage is consistent and correct across multiple files. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added `DependentMode`, `dependentComponents`, `EngineConfiguration`, and `EngineSettings` exports. # Test: Search for the usage of `DependentMode`. Expect: No issues. ast-grep --lang typescript --pattern $'DependentMode' # Test: Search for the usage of `dependentComponents`. Expect: No issues. ast-grep --lang typescript --pattern $'dependentComponents' # Test: Search for the usage of `EngineConfiguration`. Expect: No issues. ast-grep --lang typescript --pattern $'EngineConfiguration' # Test: Search for the usage of `EngineSettings`. Expect: No issues. ast-grep --lang typescript --pattern $'EngineSettings'Length of output: 5516
22-27: LGTM, but verify the usage of the newly added exports and reorganized asset management exports.The changes are approved. However, ensure that the introduction of the
ContentRestorer,Loader, andrequestexports and the reorganization of the asset management exports do not cause any issues in the codebase.Run the following script to verify the usage of the newly added exports and reorganized asset management exports:
Verification successful
Verification Successful: No issues found with the newly added and reorganized exports.
The newly added exports
ContentRestorer,Loader, andrequest, along with the reorganized asset management exports, are being used extensively and appropriately across the codebase. There are no indications of issues or conflicts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added `ContentRestorer`, `Loader`, and `request` exports and the reorganized asset management exports. # Test: Search for the usage of `ContentRestorer`. Expect: No issues. ast-grep --lang typescript --pattern $'ContentRestorer' # Test: Search for the usage of `Loader`. Expect: No issues. ast-grep --lang typescript --pattern $'Loader' # Test: Search for the usage of `request`. Expect: No issues. ast-grep --lang typescript --pattern $'request' # Test: Search for the usage of `LoadItem`. Expect: No issues. ast-grep --lang typescript --pattern $'LoadItem' # Test: Search for the usage of `ResourceManager`. Expect: No issues. ast-grep --lang typescript --pattern $'ResourceManager' # Test: Search for the usage of `resourceLoader`. Expect: No issues. ast-grep --lang typescript --pattern $'resourceLoader'Length of output: 20396
32-37: LGTM, but verify the usage of the newly added exports from various modules.The changes are approved. However, ensure that the introduction of the exports from the
2d,Layer,Utils,animation, andclonemodules does not cause any issues in the codebase.Run the following script to verify the usage of the newly added exports from various modules:
59-66: LGTM, but verify the usage of the newly added exports from various modules.The changes are approved. However, ensure that the introduction of the exports from the
renderingHardwareInterface,shader,shaderlib,shadow,sky,texture, andtrailmodules does not cause any issues in the codebase.Run the following script to verify the usage of the newly added exports from various modules:
Verification successful
Verification Successful: Newly added exports are being utilized appropriately.
The newly added exports from the
renderingHardwareInterface,shader,shaderlib,shadow, andtexturemodules are being used across various files in the codebase. However, there are no import statements found for theskyandtrailmodules, which may require further manual verification to ensure they are used correctly.
Modules with confirmed usage:
renderingHardwareInterfaceshadershaderlibshadowtextureModules without confirmed usage:
skytrailScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added exports from the `renderingHardwareInterface`, `shader`, `shaderlib`, `shadow`, `sky`, `texture`, and `trail` modules. # Test: Search for the usage of exports from the `renderingHardwareInterface` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./renderingHardwareInterface' # Test: Search for the usage of exports from the `shader` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./shader' # Test: Search for the usage of exports from the `shaderlib` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./shaderlib' # Test: Search for the usage of exports from the `shadow` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./shadow' # Test: Search for the usage of exports from the `sky` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./sky' # Test: Search for the usage of exports from the `texture` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./texture' # Test: Search for the usage of exports from the `trail` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./trail'Length of output: 3113
Script:
#!/bin/bash # Description: Verify the usage of the newly added exports from the `renderingHardwareInterface`, `shader`, `shaderlib`, `shadow`, `sky`, `texture`, and `trail` modules. # Corrected pattern to search for import statements from the specified modules. # Test: Search for the usage of exports from the `renderingHardwareInterface` module. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern 'import $_ from "../renderingHardwareInterface"' # Test: Search for the usage of exports from the `shader` module. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern 'import $_ from "../shader"' # Test: Search for the usage of exports from the `shaderlib` module. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern 'import $_ from "../shaderlib"' # Test: Search for the usage of exports from the `shadow` module. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern 'import $_ from "../shadow"' # Test: Search for the usage of exports from the `sky` module. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern 'import $_ from "../sky"' # Test: Search for the usage of exports from the `texture` module. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern 'import $_ from "../texture"' # Test: Search for the usage of exports from the `trail` module. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern 'import $_ from "../trail"'Length of output: 6441
67-68: LGTM, but verify the usage of the newly added exports from theuimodule and theXRManagercomponent.The changes are approved. However, ensure that the introduction of the exports from the
uimodule and theXRManagercomponent does not cause any issues in the codebase.Run the following script to verify the usage of the newly added exports from the
uimodule and theXRManagercomponent:Verification successful
Verification Successful: Newly added exports are correctly integrated.
The newly added exports from the
uimodule and theXRManagercomponent are being used in the codebase without any issues.
XRManageris used in multiple files, confirming its integration.CanvasRenderModefrom theuimodule is used inpackages/core/src/RenderPipeline/BasicRenderPipeline.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added exports from the `ui` module and the `XRManager` component. # Test: Search for the usage of exports from the `ui` module. Expect: No issues. fd --type file --extension ts | xargs ast-grep --lang typescript --pattern $'from \"\\.\\./ui' # Test: Search for the usage of the `XRManager` component. Expect: No issues. ast-grep --lang typescript --pattern $'XRManager'Length of output: 1015
Script:
#!/bin/bash # Corrected script to verify the usage of the newly added exports from the `ui` module and the `XRManager` component. # Test: Correct the search pattern for the usage of exports from the `ui` module. ast-grep --lang typescript --pattern $'from "../ui"' # Test: Verify the usage of the `XRManager` component again for completeness. ast-grep --lang typescript --pattern $'XRManager'Length of output: 1066
Script:
#!/bin/bash # Refined script to verify the usage of the newly added exports from the `ui` module. # Test: Refine the search pattern for the usage of exports from the `ui` module. ast-grep --lang typescript --pattern $'import $_ from "../ui"'Length of output: 164
packages/core/src/RenderPipeline/BatcherManager.ts (5)
11-13: LGTM!The code changes are approved.
15-16: LGTM!The code changes are approved.
17-26: LGTM!The code changes are approved. The use of assignments in expressions is a common pattern for lazy initialization and is not a concern in this context.
Skipping the static analysis hints as they are false positives.
Tools
Biome
[error] 18-18: 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] 22-22: 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] 26-26: 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)
30-41: LGTM!The code changes are approved.
86-88: LGTM!The code changes are approved. The use of
&&operator is a common pattern for conditional method invocation and is not a concern in this context.Skipping the static analysis hints as they are false positives.
Tools
Biome
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/input/pointer/Pointer.ts (2)
5-5: LGTM!The code changes are approved.
28-29: LGTM!The code changes are approved. The addition of the
hitResultproperty to thePointerclass enhances its functionality by enabling it to capture and manage hit results from physics interactions. The initialization ofhitResultwith a new instance ofHitResultensures that eachPointerinstance has its ownHitResultobject to store the raycasting results.packages/core/src/2d/assembler/SimpleSpriteAssembler.ts (6)
1-3: LGTM!The code changes are approved.
Line range hint
16-23: LGTM!The code changes are approved.
Line range hint
25-64: LGTM!The code changes are approved. The refactored
updatePositionsmethod improves the flexibility and control of sprite rendering by accepting parameters for width, height, pivot, and flip states. This allows for more dynamic rendering based on the sprite's properties rather than relying solely on the renderer's attributes.
85-96: LGTM!The code changes are approved. The modified
updateColormethod enhances the visual output by allowing for more nuanced control over the final color's transparency. The final alpha value is now calculated based on the renderer's color and the provided alpha parameter.
98-106: LGTM!The code changes are approved. The new
updateAlphamethod improves code clarity and maintainability by separating the concern of updating the alpha value from theupdateColormethod. This allows for more focused and targeted updates to the alpha value of the vertices.Tools
Biome
[error] 12-106: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
Line range hint
1-106: Overall, the changes to theSimpleSpriteAssemblerclass are approved!The modifications made to the
resetData,updatePositions, andupdateColormethods, along with the introduction of theupdateAlphamethod, significantly enhance the functionality and flexibility of sprite rendering. These changes allow for more dynamic rendering based on sprite properties, improved control over color transparency, and better code organization.The code changes are well-structured, maintainable, and align with the project's objectives of enhancing the sprite rendering capabilities. Great work!
Tools
Biome
[error] 12-106: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/ComponentsDependencies.ts (1)
26-33: LGTM!The refactoring of the
ifstatement to aswitchstatement enhances code readability and maintainability by clearly delineating the different behaviors based on theDependentMode. The changes are consistent with the previous implementation and do not introduce any new bugs or issues.The code changes are approved.
packages/core/src/shader/ShaderPool.ts (2)
28-29: LGTM!The code changes are approved. The imports for the new UI shaders are correctly added.
74-74: LGTM, but verify the shader usage.The code changes are approved. The new "ui" shader is correctly created using the newly imported
uiDefaultVsanduiDefaultFsshaders, and is associated with the forward rendering pass. This change is consistent with the existing shader creation process in the file.However, please ensure that this new shader is being used correctly in other parts of the codebase. You can run the following script to verify the shader usage:
Verification successful
Shader usage verified successfully.
The "ui" shader is not only created but also used in the codebase, specifically in
Engine.ts. This confirms that the shader is integrated and utilized as intended. No further action is required.
- Shader Creation:
packages/core/src/shader/ShaderPool.ts- Shader Usage:
packages/core/src/Engine.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly created "ui" shader. # Test 1: Search for the shader creation. Expect: Only one occurrence in this file. rg --type typescript -w 'Shader.create("ui",' # Test 2: Search for the shader usage. Expect: At least one occurrence in other files. rg --type typescript -w '"ui"' -g '!ShaderPool.ts'Length of output: 219
Script:
#!/bin/bash # Description: Verify the usage of the newly created "ui" shader. # Test 1: Search for the shader creation. Expect: Only one occurrence in this file. rg --type ts -w 'Shader.create\("ui",' # Test 2: Search for the shader usage. Expect: At least one occurrence in other files. rg --type ts -w '"ui"' -g '!ShaderPool.ts'Length of output: 309
packages/core/src/Component.ts (2)
6-6: LGTM!The code changes are approved.
13-15: LGTM!The code changes are approved. The new
_componentTypeproperty is appropriately decorated, typed, and initialized. It enhances theComponentclass by categorizing the component type, which aligns with the provided summary.packages/core/src/2d/assembler/SlicedSpriteAssembler.ts (5)
Line range hint
19-26: LGTM!The code changes are approved. The method has been correctly updated to handle the new
UIImageparameter type.
Line range hint
28-127: LGTM!The code changes are approved. The method has been correctly refactored to use the new parameters for width, height, pivot, and flip flags. The changes are localized and do not introduce any new issues.
Line range hint
130-141: LGTM!The code changes are approved. The method has been correctly updated to handle the new
UIImageparameter type.
143-154: LGTM!The code changes are approved. The method has been correctly updated to handle the new
alphaparameter andUIImageparameter type. The final alpha value is correctly calculated using thealphaparameter.
156-163: LGTM!The code changes are approved. The new
updateAlphamethod is a good addition to the class and provides a more efficient way to update the alpha value. The method is correctly implemented and handles thealphaparameter.packages/core/src/Script.ts (3)
35-37: LGTM!The new
_onPointerEventIndexArrayproperty is appropriately named and decorated with@ignoreClone. It follows the internal naming convention and is likely used to manage the indices of registered pointer event handlers.
130-130: LGTM!The modification to the
onPointerDownmethod signature to accept aPointerevent parameter instead of a genericPointertype is a good improvement. It provides more specific information about the pointer event and follows common naming conventions for event handlers.
212-228: LGTM!The changes in the
_onEnableInScenemethod introduce a structured approach to registering pointer event handlers during the script's initialization. By checking if the pointer event methods are overridden and using thePointerEventTypeenum, it ensures that only the necessary event handlers are registered in a type-safe manner. Adding the script to the entity using_addScriptis also a necessary step for proper association.packages/core/src/ui/UIImage.ts (7)
35-58: LGTM!The code changes are approved.
63-74: LGTM!The code changes are approved.
79-91: LGTM!The code changes are approved.
96-117: LGTM!The code changes are approved.
122-131: LGTM!The code changes are approved.
148-195: LGTM!The code changes are approved.
227-262: LGTM!The code changes are approved.
Tools
Biome
[error] 233-233: 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)
[error] 246-246: 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)
packages/core/src/ComponentsManager.ts (5)
80-90: LGTM!The code changes are approved. The function correctly adds the
UICanvasto the appropriate array based on theCanvasRenderModeand updates the necessary properties.
92-103: LGTM!The code changes are approved. The function correctly removes the
UICanvasfrom the appropriate array based on theCanvasRenderMode, updates the_canvasIndexof the removed and replaced canvases, and sets the necessary flags.Tools
Biome
[error] 101-101: 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)
105-114: LGTM!The code changes are approved. The function correctly sorts the overlay canvases based on their
sortOrderwhen necessary, updates the_canvasIndexof each overlay canvas, and resets the_overlayCanvasesSortingFlagafter sorting.
20-25: LGTM!The code changes are approved. The new internal properties are correctly typed and initialized, and the garbage collection for the new canvas arrays is properly integrated into the existing
_gcmethod.Also applies to: 314-315
101-101: Skipped static analysis hint.The static analysis hint is a false positive. The assignment in the expression is intentional and does not cause any confusion or side effects. It is a common pattern in JavaScript and TypeScript to conditionally assign a value to a property.
Tools
Biome
[error] 101-101: 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)
packages/core/src/2d/sprite/SpriteMask.ts (2)
241-243: LGTM!The changes to the
_updateBoundsmethod enhance its functionality by extracting thespriteproperty and passing additional parameters toSimpleSpriteAssembler.updatePositions. This ensures that the sprite's dimensions, pivot, and flip transformations are considered during the update process, improving the accuracy of the bounds calculation.
254-255: LGTM!The changes to the
_rendermethod improve the integrity and accuracy of the rendering process:
Using the
_spriteproperty directly in the texture check ensures that the correct sprite instance is evaluated before proceeding with rendering.Passing additional parameters to
updatePositionsaligns the functionality with the changes made in the_updateBoundsmethod, ensuring consistency and utilizing more sprite properties for accurate positioning.Also applies to: 271-271
packages/core/src/input/pointer/PointerManager.ts (2)
130-135: LGTM!The changes to the
_pointerRayCastmethod call align with the updated method signature and the overall refactoring of the raycasting logic. Passingpointer.hitResultas theoutparameter allows for direct population of the hit result, which is an improvement.
240-308: Excellent refactoring of the_pointerRayCastmethod!The changes made to the
_pointerRayCastmethod significantly enhance its functionality and maintainability:
- Using the
outparameter to store the raycast results directly streamlines the process and eliminates the need for the static_tempHitResultinstance.- The refined logic for handling overlay canvases, iterating through cameras and canvases, and considering the camera's viewport and rendering order improves the accuracy and performance of the raycasting.
- Separating the physics raycasting and canvas raycasting logic enhances clarity and ensures the correct entity is returned based on the raycast results.
Overall, these changes align with the AI-generated summary and contribute to a more efficient and maintainable implementation of the pointer raycasting functionality.
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (4)
24-24: LGTM!The import statement is approved as it is necessary to support the changes related to canvas rendering.
334-335: LGTM!The destructuring changes are approved as they improve code readability and potentially performance.
337-353: LGTM!The changes to the renderer filtering logic are approved as they streamline the checks against the camera's culling mask and frustum, improving the rendering process.
355-365: LGTM!The changes related to canvas processing are approved as they enhance the rendering pipeline by allowing for more flexible rendering scenarios and better integration of UI elements.
packages/core/src/2d/sprite/SpriteRenderer.ts (5)
21-21: LGTM!The import is necessary for assigning the component type in the constructor.
271-271: LGTM!Assigning the component type in the constructor establishes a clearer identification of the component type for the renderer.
320-322: LGTM!The changes improve the code's readability and maintainability:
- Destructuring the
_spriteproperty improves readability and potentially performance by reducing repeated property access.- Using the destructured
spritevariable to update positions is a more concise approach.
330-331: LGTM!Using the destructured
spritevariable enhances clarity in the rendering logic.
346-346: LGTM!Passing the sprite's dimensions and pivot point to the
_assembler.updatePositionsmethod ensures that the rendering takes into account the sprite's properties.packages/core/src/Scene.ts (1)
330-330: LGTM, but verify the impact of this change.The code change looks good to me. However, please ensure that replacing
entity._setTransformDirty()withentity._setParentChange()is thoroughly tested to confirm it behaves as expected and does not introduce any regressions or unintended side effects in the system.To verify the impact of this change, consider running the following script to search for potential usage or references to the
_setTransformDirtyand_setParentChangemethods across the codebase:The expected results are:
_setTransformDirtyshould have few to no occurrences, confirming it's not widely used._setParentChangeshould only occur in relevant places, such as entity relationship management.If there are unexpected results or too many occurrences, please carefully review those instances to ensure this change does not introduce issues.
Verification successful
Change from
_setTransformDirtyto_setParentChangeis appropriate and consistent.The
_setTransformDirtymethod is not used elsewhere, confirming its removal does not impact other parts of the codebase. The_setParentChangemethod is well-integrated and used appropriately for managing entity relationships. The change aligns with the intended functionality without introducing issues.
- Locations of
_setParentChangeusage:
packages/core/src/Entity.ts: Multiple instances.packages/core/src/Scene.ts: Line under review.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage or references to `_setTransformDirty` and `_setParentChange` methods. # Test 1: Search for `_setTransformDirty`. Expect: Only a few occurrences, if any. echo "Searching for _setTransformDirty..." rg --type typescript '_setTransformDirty' # Test 2: Search for `_setParentChange`. Expect: Only occurrences in relevant places. echo "Searching for _setParentChange..." rg --type typescript '_setParentChange'Length of output: 309
Script:
#!/bin/bash # Description: Search for usage or references to `_setTransformDirty` and `_setParentChange` methods. # Test 1: Search for `_setTransformDirty`. Expect: Only a few occurrences, if any. echo "Searching for _setTransformDirty..." rg '_setTransformDirty' # Test 2: Search for `_setParentChange`. Expect: Only occurrences in relevant places. echo "Searching for _setParentChange..." rg '_setParentChange'Length of output: 433
packages/core/src/Renderer.ts (1)
171-171: LGTM!The code change is approved. Assigning the
ComponentType.Renderertothis._componentTypewithin the constructor of theRendererclass is a good practice. It enhances the classification of components within the rendering pipeline and may facilitate better management and identification of component types during rendering operations.packages/core/src/2d/assembler/TiledSpriteAssembler.ts (5)
Line range hint
22-34: LGTM!The code changes are approved.
Line range hint
150-182: LGTM!The code changes are approved.
184-196: LGTM!The code changes are approved.
198-206: LGTM!The code changes are approved.
Line range hint
37-148: LGTM, but verify the method usage.The code changes are approved. However, ensure that this change is thoroughly tested to confirm that it behaves as expected and that all method calls to
updatePositionshave been updated to match the new signature.Run the following script to verify the method usage:
packages/core/src/ui/UICanvas.ts (2)
198-209: LGTM!The
_prepareRendermethod is well-structured and easy to understand. It prepares the renderers for rendering and updates the sort distance.Tools
Biome
[error] 201-201: 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)
247-256: LGTM!The
rayCastmethod is straightforward and uses a reverse loop for efficiency. It iterates over the renderers and performs ray casting on each one that is ray castable.packages/core/src/Engine.ts (8)
9-9: LGTM!The code changes are approved. The new imports are necessary for the UI rendering logic.
18-18: LGTM!The code changes are approved. The new import is necessary for the UI rendering logic.
20-20: LGTM!The code changes are approved. The new imports are necessary for the UI rendering logic.
25-25: LGTM!The code changes are approved. The new import is necessary for the UI rendering logic.
49-49: LGTM!The code changes are approved. The new import is necessary for the UI rendering logic.
107-108: LGTM!The code changes are approved. The new private member
_uiDefaultMaterialis necessary for the UI rendering logic. It will store the default material used for rendering UI elements.
135-136: LGTM!The code changes are approved. The new private member
_uiRenderQueueand its initialization withRenderQueueType.Transparentare necessary for the UI rendering logic. It will store the render queue used for rendering UI elements.Also applies to: 269-271
260-260: LGTM!The code changes are approved. The new logic for rendering UI elements is implemented correctly and follows a logical flow:
- The default UI material is created with the appropriate render states for transparency and culling.
- The UI canvases are sorted and retrieved from the
componentsManager.- The UI canvases are prepared for rendering and added to the
_uiRenderQueue.- The
_uiRenderQueueis batched and rendered using the_batcherManager.The changes are consistent with the existing rendering logic for other elements.
Also applies to: 366-366, 530-582, 685-701
packages/core/src/Entity.ts (8)
11-11: LGTM!The import statement for
UpdateFlagManageris approved.
16-18: LGTM!The import statements for
PointerandPointerEventTypeare approved. They are necessary for handling pointer events in theEntityclass.
101-103: LGTM!The addition of the
_updateFlagManagerproperty is approved. It is used to manage update flags related to the entity's state.
107-115: LGTM!The refactoring of the
transformproperty is approved. It encapsulates the transform logic more effectively and aligns with the new structure of the class.
110-111: LGTM!The addition of the
_onPointerCallBacksArrayproperty is approved. It is used to store callbacks for pointer events, enhancing the interactivity of entities within the scene.
277-287: LGTM!The addition of the
getComponentsInParentmethod is approved. It enhances component management by allowing the retrieval of components from the entity and its parent, which is consistent with the AI-generated summary.
826-831: LGTM!The addition of the
EntityModifyFlagsenum is approved. It provides a structured way to define modification flags related to the entity's state, which is consistent with the AI-generated summary.
Line range hint
1-831: Code changes are consistent with the AI-generated summary.The code changes in the
Entityclass are consistent with the AI-generated summary. The summary accurately captures the key modifications, such as the introduction of theUpdateFlagManager, the refactoring of thetransformproperty, the addition of thegetComponentsInParentmethod, the addition of pointer event handling capabilities, and the addition of theEntityModifyFlagsenum.Tools
Biome
[error] 621-621: 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] 622-622: 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] 639-639: 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] 646-646: 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)
packages/core/src/Camera.ts (6)
129-131: LGTM!The addition of the
_updateFlagManagerinstance variable of typeUpdateFlagManagerto theCameraclass looks good. It aligns with the AI-generated summary about enhancing theCameraclass with update flag management capabilities.
287-296: LGTM!The modifications to the
isOrthographicproperty setter look good. Dispatching theCameraModifyFlags.Typeflag using the_updateFlagManagerwhen the value changes aligns with the AI-generated summary about tracking camera property modifications. Updating the shader data macro based on theisOrthographicvalue ensures the correct projection type is used in the shaders.
308-312: LGTM!The modifications to the
orthographicSizeproperty setter look good. Dispatching theCameraModifyFlags.OrthographicSizeflag using the_updateFlagManagerwhen the value changes aligns with the AI-generated summary about tracking camera property modifications.
426-427: LGTM!The modifications to the
renderTargetproperty setter look good. Dispatching theCameraModifyFlags.RenderTargetflag using the_updateFlagManagerwhen the value changes aligns with the AI-generated summary about tracking camera property modifications. Calling the_checkMainCanvasAntialiasWastemethod ensures that any potential waste of resources related to antialiasing is checked when the render target changes.
841-841: LGTM!The modification to the
_onPixelViewportChangedmethod looks good. Dispatching theCameraModifyFlags.ViewPortflag using the_updateFlagManageraligns with the AI-generated summary about tracking camera property modifications.
856-868: LGTM!The addition of the
CameraModifyFlagsenum looks good. It aligns with the AI-generated summary about introducing flags to track modifications to camera properties. The enum provides a structured way to manage and track changes to specific camera properties, enhancing the clarity and maintainability of the code.packages/core/src/Transform.ts (9)
23-23: LGTM!The code change is approved. Changing the access modifier of the
_positionproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
25-25: LGTM!The code change is approved. Changing the access modifier of the
_rotationproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
27-27: LGTM!The code change is approved. Changing the access modifier of the
_rotationQuaternionproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
29-29: LGTM!The code change is approved. Changing the access modifier of the
_scaleproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
31-31: LGTM!The code change is approved. Changing the access modifier of the
_worldPositionproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
33-33: LGTM!The code change is approved. Changing the access modifier of the
_worldRotationproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
35-35: LGTM!The code change is approved. Changing the access modifier of the
_worldRotationQuaternionproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
39-39: LGTM!The code change is approved. Changing the access modifier of the
_localMatrixproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
41-41: LGTM!The code change is approved. Changing the access modifier of the
_worldMatrixproperty fromprivatetoprotectedenhances the extensibility of theTransformclass by allowing subclasses to access and customize this property directly.
| it("WorldSpace", () => { | ||
| const imageEntity = root.createChild("image"); | ||
| const image = imageEntity.addComponent(UIImage); | ||
| const uiTransform = <UITransform>imageEntity.transform; | ||
| uiTransform.rect.x = 100; | ||
| uiTransform.rect.y = 100; | ||
|
|
||
| uiCanvas.renderMode = CanvasRenderMode.WorldSpace; | ||
| cameraEntity.transform.position.set(0, 0, 100); | ||
| }); |
There was a problem hiding this comment.
The test case is incomplete and needs improvements.
The test case has the following issues:
- It does not have any assertions to test the expected behavior.
- The purpose of the test case is unclear as it's not testing any specific behavior or functionality.
- The test case name
WorldSpacedoes not clearly describe what the test case is testing.
To improve the test case, consider the following suggestions:
- Add assertions to test the expected behavior. For example, you could assert that the
UIImageis rendered correctly in world space by checking its position on the screen. - Clarify the purpose of the test case by adding comments or a more descriptive test case name. For example, the test case could be renamed to
should_render_UIImage_in_world_spaceto clearly describe what it's testing. - If the test case is not needed, consider removing it.
packages/core/src/ui/UIRenderer.ts
Outdated
| override _prepareRender(context: RenderContext): void { | ||
| // Update once per frame per renderer, not influenced by batched | ||
| if (this._renderFrameCount !== this.engine.time.frameCount) { | ||
| this._update(context); | ||
| } | ||
|
|
||
| this._render(context); | ||
|
|
||
| // union camera global macro and renderer macro. | ||
| ShaderMacroCollection.unionCollection( | ||
| context.camera._globalShaderMacro, | ||
| this.shaderData._macroCollection, | ||
| this._globalShaderMacro | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| override _onEnableInScene(): void { | ||
| const componentsManager = this.scene._componentsManager; | ||
| if (this._overrideUpdate) { | ||
| componentsManager.addOnUpdateRenderers(this); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| override _onDisableInScene(): void { | ||
| const componentsManager = this.scene._componentsManager; | ||
| if (this._overrideUpdate) { | ||
| componentsManager.removeOnUpdateRenderers(this); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| _setGroup(group: CanvasGroup): void { | ||
| this._group = group; | ||
| const alpha = group ? group._globalAlpha : 1; | ||
| if (this._alpha !== alpha) { | ||
| this._alpha = alpha; | ||
| this._dirtyUpdateFlag |= UIRendererUpdateFlags.Alpha; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| _getChunkManager(): PrimitiveChunkManager { | ||
| return this.engine._batcherManager.primitiveChunkManagerUI; | ||
| } | ||
|
|
||
| /** @internal */ | ||
| _raycast(ray: Ray, out: HitResult, distance: number = Number.MAX_SAFE_INTEGER): boolean { | ||
| const entity = this._entity; | ||
| const plane = UIRenderer._tempPlane; | ||
| const transform = entity.transform; | ||
| const normal = plane.normal.copyFrom(transform.worldForward); | ||
| plane.distance = -Vector3.dot(normal, transform.worldPosition); | ||
| ray.intersectPlane(plane); | ||
| const curDistance = ray.intersectPlane(plane); | ||
| if (curDistance >= 0 && curDistance < distance) { | ||
| const hitPointWorld = ray.getPoint(curDistance, UIRenderer._tempVec30); | ||
| const worldMatrixInv = UIRenderer._tempMat; | ||
| Matrix.invert(this.entity.transform.worldMatrix, worldMatrixInv); | ||
| const hitPointLocal = UIRenderer._tempVec31; | ||
| Vector3.transformCoordinate(hitPointWorld, worldMatrixInv, hitPointLocal); | ||
| if (this.isRaycastLocationValid(hitPointLocal)) { | ||
| out.distance = curDistance; | ||
| out.entity = entity; | ||
| out.normal.copyFrom(normal); | ||
| out.point.copyFrom(hitPointWorld); | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| protected isRaycastLocationValid(hitPoint: Vector3): boolean { | ||
| const { x, y } = hitPoint; | ||
| const uiTransform = <UITransform>this._transform; | ||
| const { x: width, y: height } = uiTransform.size; | ||
| const { x: pivotX, y: pivotY } = uiTransform.pivot; | ||
| return x >= -width * pivotX && x <= width * (1 - pivotX) && y >= -height * pivotY && y <= height * (1 - pivotY); | ||
| } | ||
|
|
||
| protected override _onDestroy(): void { | ||
| if (this._subChunk) { | ||
| this._getChunkManager().freeSubChunk(this._subChunk); | ||
| this._subChunk = null; | ||
| } | ||
|
|
||
| super._onDestroy(); | ||
| } | ||
|
|
||
| protected override _onTransformChanged(flag: TransformModifyFlags): void {} | ||
| } |
There was a problem hiding this comment.
LGTM! Consider adding JSDoc comments and unit tests.
The UIRenderer class is well-structured and follows best practices. The use of decorators, bitwise flags, and static properties for optimization is commendable. The raycasting functionality is also implemented correctly.
Consider adding JSDoc comments to the public properties and methods to improve the documentation and make it easier for other developers to understand and use the class.
Also, consider adding unit tests to cover the raycasting functionality to ensure that it works as expected and to catch any potential bugs. Do you want me to generate the unit testing code or open a GitHub issue to track this task?
| } | ||
| break; | ||
| case SpriteModifyFlags.border: | ||
| this._drawMode === SpriteDrawMode.Sliced && (this._dirtyUpdateFlag |= ImageUpdateFlags.VertexData); |
There was a problem hiding this comment.
Avoid using assignments in expressions.
The use of assignments in expressions can be confusing as expressions are often considered side-effect free.
Consider extracting the assignment to a separate statement:
-this._drawMode === SpriteDrawMode.Sliced && (this._dirtyUpdateFlag |= ImageUpdateFlags.VertexData);
+if (this._drawMode === SpriteDrawMode.Sliced) {
+ this._dirtyUpdateFlag |= ImageUpdateFlags.VertexData;
+}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._drawMode === SpriteDrawMode.Sliced && (this._dirtyUpdateFlag |= ImageUpdateFlags.VertexData); | |
| if (this._drawMode === SpriteDrawMode.Sliced) { | |
| this._dirtyUpdateFlag |= ImageUpdateFlags.VertexData; | |
| } |
Tools
Biome
[error] 246-246: 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)
packages/core/src/ui/UICanvas.ts
Outdated
| switch (component._componentType) { | ||
| case ComponentType.CanvasGroup: | ||
| const alpha = group ? group.alpha : 1; | ||
| group = <CanvasGroup>component; | ||
| group._globalAlpha = group.ignoreParentGroup ? 1 : alpha * group.alpha; | ||
| for (let j = offset, m = renderers.length; j < m; j++) { | ||
| renderers[j]._setGroup(group); | ||
| } | ||
| break; | ||
| case ComponentType.UIRenderer: | ||
| const uiRenderer = <UIRenderer>component; | ||
| uiRenderer._canvas = this; | ||
| uiRenderer._setGroup(group); | ||
| renderers.push(uiRenderer); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrap the variable declarations in blocks.
The static analysis tool has correctly identified two variable declarations in switch cases that are accessible to other cases. Wrap the declarations in blocks to restrict their scope.
Apply this diff to fix the issues:
switch (component._componentType) {
case ComponentType.CanvasGroup:
- const alpha = group ? group.alpha : 1;
- group = <CanvasGroup>component;
+ {
+ const alpha = group ? group.alpha : 1;
+ group = <CanvasGroup>component;
+ }
group._globalAlpha = group.ignoreParentGroup ? 1 : alpha * group.alpha;
for (let j = offset, m = renderers.length; j < m; j++) {
renderers[j]._setGroup(group);
}
break;
case ComponentType.UIRenderer:
- const uiRenderer = <UIRenderer>component;
+ {
+ const uiRenderer = <UIRenderer>component;
+ }
uiRenderer._canvas = this;
uiRenderer._setGroup(group);
renderers.push(uiRenderer);
break;Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 326-326: 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)
[error] 334-334: 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)
packages/core/src/Entity.ts
Outdated
| /** @internal */ | ||
| _addOnPointerEvent( | ||
| type: PointerEventType, | ||
| script: Script, | ||
| callback: (event: Pointer) => void, | ||
| useCapture: boolean = false | ||
| ): void { | ||
| const onPointerCallBacksArray = (this._onPointerCallBacksArray ||= []); | ||
| const onPointerCallBacks = (onPointerCallBacksArray[type] ||= new DisorderedArray<(event: Pointer) => void>()); | ||
| script._onPointerEventIndexArray[type] = onPointerCallBacks.length; | ||
| onPointerCallBacks.add(callback); | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| _setTransformDirty() { | ||
| if (this.transform) { | ||
| this.transform._parentChange(); | ||
| } else { | ||
| for (let i = 0, len = this._children.length; i < len; i++) { | ||
| this._children[i]._setTransformDirty(); | ||
| } | ||
| } | ||
| _setParentChange() { | ||
| this._transform._parentChange(); | ||
| this._dispatchModify(EntityModifyFlags.Parent); | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| _registerModifyListener(onChange: (flag: EntityModifyFlags) => void): void { | ||
| (this._updateFlagManager ||= new UpdateFlagManager()).addListener(onChange); | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| _removeModifyListener(onChange: (flag: EntityModifyFlags) => void): void { | ||
| (this._updateFlagManager ||= new UpdateFlagManager()).addListener(onChange); | ||
| } |
There was a problem hiding this comment.
Approve the new methods, but address the static analysis hints.
The addition of the _addOnPointerEvent method and the modification listener methods (_registerModifyListener, _removeModifyListener, and _dispatchModify) is approved. They enhance the interactivity and state management of entities.
However, the static analysis hints raise valid concerns about the use of assignments in expressions:
The assignments in lines 621, 622, 639, and 646 should be moved out of the expressions to improve code clarity. Apply this diff to fix the issues:
- const onPointerCallBacksArray = (this._onPointerCallBacksArray ||= []);
+ this._onPointerCallBacksArray = this._onPointerCallBacksArray || [];
+ const onPointerCallBacksArray = this._onPointerCallBacksArray;
- const onPointerCallBacks = (onPointerCallBacksArray[type] ||= new DisorderedArray<(event: Pointer) => void>());
+ onPointerCallBacksArray[type] = onPointerCallBacksArray[type] || new DisorderedArray<(event: Pointer) => void>();
+ const onPointerCallBacks = onPointerCallBacksArray[type];
- (this._updateFlagManager ||= new UpdateFlagManager()).addListener(onChange);
+ this._updateFlagManager = this._updateFlagManager || new UpdateFlagManager();
+ this._updateFlagManager.addListener(onChange);
- (this._updateFlagManager ||= new UpdateFlagManager()).addListener(onChange);
+ this._updateFlagManager = this._updateFlagManager || new UpdateFlagManager();
+ this._updateFlagManager.addListener(onChange);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.
| /** @internal */ | |
| _addOnPointerEvent( | |
| type: PointerEventType, | |
| script: Script, | |
| callback: (event: Pointer) => void, | |
| useCapture: boolean = false | |
| ): void { | |
| const onPointerCallBacksArray = (this._onPointerCallBacksArray ||= []); | |
| const onPointerCallBacks = (onPointerCallBacksArray[type] ||= new DisorderedArray<(event: Pointer) => void>()); | |
| script._onPointerEventIndexArray[type] = onPointerCallBacks.length; | |
| onPointerCallBacks.add(callback); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _setTransformDirty() { | |
| if (this.transform) { | |
| this.transform._parentChange(); | |
| } else { | |
| for (let i = 0, len = this._children.length; i < len; i++) { | |
| this._children[i]._setTransformDirty(); | |
| } | |
| } | |
| _setParentChange() { | |
| this._transform._parentChange(); | |
| this._dispatchModify(EntityModifyFlags.Parent); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _registerModifyListener(onChange: (flag: EntityModifyFlags) => void): void { | |
| (this._updateFlagManager ||= new UpdateFlagManager()).addListener(onChange); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _removeModifyListener(onChange: (flag: EntityModifyFlags) => void): void { | |
| (this._updateFlagManager ||= new UpdateFlagManager()).addListener(onChange); | |
| } | |
| /** @internal */ | |
| _addOnPointerEvent( | |
| type: PointerEventType, | |
| script: Script, | |
| callback: (event: Pointer) => void, | |
| useCapture: boolean = false | |
| ): void { | |
| this._onPointerCallBacksArray = this._onPointerCallBacksArray || []; | |
| const onPointerCallBacksArray = this._onPointerCallBacksArray; | |
| onPointerCallBacksArray[type] = onPointerCallBacksArray[type] || new DisorderedArray<(event: Pointer) => void>(); | |
| const onPointerCallBacks = onPointerCallBacksArray[type]; | |
| script._onPointerEventIndexArray[type] = onPointerCallBacks.length; | |
| onPointerCallBacks.add(callback); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _setParentChange() { | |
| this._transform._parentChange(); | |
| this._dispatchModify(EntityModifyFlags.Parent); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _registerModifyListener(onChange: (flag: EntityModifyFlags) => void): void { | |
| this._updateFlagManager = this._updateFlagManager || new UpdateFlagManager(); | |
| this._updateFlagManager.addListener(onChange); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| _removeModifyListener(onChange: (flag: EntityModifyFlags) => void): void { | |
| this._updateFlagManager = this._updateFlagManager || new UpdateFlagManager(); | |
| this._updateFlagManager.addListener(onChange); | |
| } |
Tools
Biome
[error] 621-621: 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] 622-622: 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] 639-639: 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] 646-646: 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: 3
Outside diff range and nitpick comments (6)
packages/core/src/ui/CanvasGroup.ts (1)
6-28: LGTM, but consider adding JSDoc comments and making minor improvements.The
CanvasGroupclass implementation looks good overall. Just a few suggestions for improvement:
- Add JSDoc comments to explain the purpose of the
ignoreParentGroupand_globalAlphaproperties.- Add a private accessor to the
_globalAlphaproperty for consistency with_alpha.- Add a type annotation for the
entityparameter in the constructor.Apply this diff to implement the suggestions:
export class CanvasGroup extends Component { @assignmentClone + /** Indicates whether to ignore the parent group's properties. */ ignoreParentGroup = false; + /** @internal */ - _globalAlpha = 1; + private _globalAlpha = 1; + @assignmentClone private _alpha = 1; // ... - constructor(entity: Entity) { + constructor(entity: Entity) { super(entity); this._componentType = ComponentType.CanvasGroup; } }packages/core/src/ui/UITransform.ts (1)
6-53: Add documentation comments for better maintainability.Consider adding documentation comments for the
UITransformclass, constructor, and private methods to improve maintainability and make the code easier to understand for developers not familiar with the codebase.packages/core/src/2d/assembler/SlicedSpriteAssembler.ts (1)
Line range hint
28-127: LGTM, but consider adding JSDoc comments for the new parameters.The code changes are approved. The refactoring improves the method's flexibility and allows for more granular control over sprite positioning and transformations. The changes also enable support for
SpriteMask, which is great.To improve the maintainability and readability of the code, consider adding JSDoc comments to document the new parameters and their usage. This will help future developers understand the purpose and expected values of each parameter.
packages/core/src/ui/UIRenderer.ts (1)
168-174: LGTM, but consider adding padding support.The
isRaycastLocationValidmethod is correctly checking if the hit point is within the bounds of the UI element based on its size and pivot. However, it's not considering the_rayCastPaddingproperty, which could be used to add some padding to the bounds.Consider adding support for the
_rayCastPaddingproperty to allow some flexibility in the raycasting bounds. You can modify the method like this:protected isRaycastLocationValid(hitPoint: Vector3): boolean { const { x, y } = hitPoint; const uiTransform = <UITransform>this._transform; const { x: width, y: height } = uiTransform.size; const { x: pivotX, y: pivotY } = uiTransform.pivot; + const { x: paddingLeft, y: paddingBottom, z: paddingRight, w: paddingTop } = this._rayCastPadding; - return x >= -width * pivotX && x <= width * (1 - pivotX) && y >= -height * pivotY && y <= height * (1 - pivotY); + return x >= -width * pivotX + paddingLeft && x <= width * (1 - pivotX) - paddingRight && + y >= -height * pivotY + paddingBottom && y <= height * (1 - pivotY) - paddingTop; }packages/core/src/2d/assembler/TiledSpriteAssembler.ts (1)
198-206: LGTM, but consider updating the method to handleUIImagetype.The code changes for the new
updateAlphamethod are approved. The method correctly updates the alpha value of the vertices using thealphaparameter, and the logic for calculating the final alpha value is correct.However, for consistency with other methods in this class, consider updating the method signature to accept both
SpriteRendererandUIImagetypes:-static updateAlpha(renderer: SpriteRenderer, alpha: number = 1): void { +static updateAlpha(renderer: SpriteRenderer | UIImage, alpha: number = 1): void {packages/core/src/Transform.ts (1)
6-6: LGTM!The change in the import statement order is a good nitpick to improve the readability and organization of the code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (38)
- packages/core/src/2d/assembler/ISpriteAssembler.ts (1 hunks)
- packages/core/src/2d/assembler/SimpleSpriteAssembler.ts (4 hunks)
- packages/core/src/2d/assembler/SlicedSpriteAssembler.ts (6 hunks)
- packages/core/src/2d/assembler/TiledSpriteAssembler.ts (6 hunks)
- packages/core/src/2d/sprite/SpriteMask.ts (3 hunks)
- packages/core/src/2d/sprite/SpriteRenderer.ts (4 hunks)
- packages/core/src/Camera.ts (10 hunks)
- packages/core/src/Component.ts (1 hunks)
- packages/core/src/ComponentsDependencies.ts (1 hunks)
- packages/core/src/ComponentsManager.ts (4 hunks)
- packages/core/src/Engine.ts (13 hunks)
- packages/core/src/Entity.ts (12 hunks)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/BatcherManager.ts (2 hunks)
- packages/core/src/Renderer.ts (2 hunks)
- packages/core/src/Scene.ts (1 hunks)
- packages/core/src/Script.ts (4 hunks)
- packages/core/src/Transform.ts (2 hunks)
- packages/core/src/enums/CameraType.ts (1 hunks)
- packages/core/src/enums/ComponentType.ts (1 hunks)
- packages/core/src/index.ts (1 hunks)
- packages/core/src/input/pointer/Pointer.ts (2 hunks)
- packages/core/src/input/pointer/PointerEvent.ts (1 hunks)
- packages/core/src/input/pointer/PointerEventType.ts (1 hunks)
- packages/core/src/input/pointer/PointerManager.ts (3 hunks)
- packages/core/src/shader/ShaderPool.ts (2 hunks)
- packages/core/src/shaderlib/extra/uiDefault.fs.glsl (1 hunks)
- packages/core/src/shaderlib/extra/uiDefault.vs.glsl (1 hunks)
- packages/core/src/ui/CanvasGroup.ts (1 hunks)
- packages/core/src/ui/UICanvas.ts (1 hunks)
- packages/core/src/ui/UIImage.ts (1 hunks)
- packages/core/src/ui/UIRenderer.ts (1 hunks)
- packages/core/src/ui/UITransform.ts (1 hunks)
- packages/core/src/ui/enums/BlockingObjects.ts (1 hunks)
- packages/core/src/ui/enums/CanvasRenderMode.ts (1 hunks)
- packages/core/src/ui/enums/ResolutionAdaptationStrategy.ts (1 hunks)
- packages/core/src/ui/index.ts (1 hunks)
- tests/src/core/2d/ui/UICanvas.test.ts (1 hunks)
Additional context used
Biome
packages/core/src/RenderPipeline/BatcherManager.ts
[error] 18-18: 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] 22-22: 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] 26-26: 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] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/ui/UIImage.ts
[error] 233-233: 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)
[error] 246-246: 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)
packages/core/src/ComponentsManager.ts
[error] 101-101: 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)
packages/core/src/ui/UICanvas.ts
[error] 201-201: 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] 214-215: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 216-217: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 326-326: 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)
[error] 334-334: 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)
packages/core/src/Entity.ts
[error] 621-621: 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] 622-622: 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] 639-639: 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] 646-646: 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 (149)
packages/core/src/input/pointer/PointerEventType.ts (1)
1-7: LGTM!The code changes are approved. The enum is well-defined, follows the TypeScript syntax, and the enum members are well-named and cover the common pointer events. The enum is also exported and defined in a file and directory that follows good code organization practices.
packages/core/src/ui/enums/BlockingObjects.ts (1)
1-6: LGTM!The
BlockingObjectsenum is defined correctly and the member names are appropriate. The enum covers all the necessary cases for specifying the types of objects that can block UI elements.packages/core/src/input/pointer/PointerEvent.ts (1)
3-5: LGTM!The code changes are approved. The
PointerEventclass is correctly implemented and the property name is descriptive.packages/core/src/ui/enums/CanvasRenderMode.ts (1)
1-8: LGTM!The
CanvasRenderModeenum is well-defined and follows best practices:
- The enum values are appropriately named to represent different rendering modes for a UI canvas.
- The comments provide a clear description of the purpose of the enum.
- The values are assigned explicit numbers, which is a good practice for enums.
The code changes are approved.
packages/core/src/ui/enums/ResolutionAdaptationStrategy.ts (1)
1-10: LGTM, but verify the enum usage.The code changes are approved. The enum is well-defined and follows the naming convention for enums. The enum members are self-explanatory and cover the common resolution adaptation strategies.
However, ensure that this enum is being used correctly in the codebase. Verify that the enum values are being used in the appropriate places and that there are no missing or unused values.
Run the following script to verify the enum usage:
Verification successful
Enum usage verified and correct.
The
ResolutionAdaptationStrategyenum is used correctly across the codebase. All enum values are utilized in theUICanvas.tsfile, and the enum is properly exported and imported. No issues found with the enum usage.
- Enum defined in:
packages/core/src/ui/enums/ResolutionAdaptationStrategy.ts- Enum exported in:
packages/core/src/ui/index.ts- Enum used in:
packages/core/src/ui/UICanvas.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `ResolutionAdaptationStrategy` enum. # Test: Search for the enum usage. Expect: At least one occurrence of each enum value. rg --type typescript -w $'ResolutionAdaptationStrategy' | rg --passthru $'WidthAdaptation|HeightAdaptation|BothAdaptation|ExpandAdaptation|ShrinkAdaptation'Length of output: 189
Script:
#!/bin/bash # Description: Verify the usage of the `ResolutionAdaptationStrategy` enum. # Test: Search for the enum usage. Expect: At least one occurrence of each enum value. rg -w 'ResolutionAdaptationStrategy' | rg --passthru 'WidthAdaptation|HeightAdaptation|BothAdaptation|ExpandAdaptation|ShrinkAdaptation'Length of output: 1250
packages/core/src/shaderlib/extra/uiDefault.fs.glsl (3)
1-2: LGTM!The uniform sampler2D variable is correctly declared and follows the naming convention.
3-5: LGTM!The varying variables are correctly declared and follow the common naming convention. The purpose of each variable is clear based on their names.
6-9: LGTM!The
mainfunction is correctly implemented. It samples the UI texture using thev_uvvarying variable, multiplies the sampled color with thev_colorvarying variable to apply a color tint, and assigns the final color togl_FragColor. The logic is clear and concise.packages/core/src/enums/CameraType.ts (2)
9-9: LGTM!The new enum value
UIOverlayis added with a value of0x8, which is consistent with the existing enum values. The name suggests that it is used for rendering UI elements on top of the camera view.
10-10: LGTM!The new enum value
XRCamerais added as a bitwise OR combination of the existing XR camera types. This allows for a unified representation of XR camera types, which can be useful for checking if a camera is an XR camera. The bitwise OR operation is a common pattern for creating composite enum values.packages/core/src/enums/ComponentType.ts (1)
1-14: LGTM!The
ComponentTypeenum looks good:
- The enum values are assigned using bitwise operators, which allows for efficient bitwise operations and comparisons.
- The enum contains various component types related to rendering, such as
Renderer,MeshRenderer,SpriteRenderer,UIRenderer, andCanvasGroup, which aligns with the PR objectives.- The
@internalannotation correctly suggests that this enum is intended for internal use within the package.The code changes are approved.
packages/core/src/shaderlib/extra/uiDefault.vs.glsl (1)
1-15: LGTM!The vertex shader code looks good to me:
- The shader follows the standard structure and naming conventions.
- It correctly transforms the vertex position using the
renderer_MVPMatuniform matrix.- The texture coordinates and color attributes are passed to the fragment shader as expected.
- The shader is minimal and optimized, without any unnecessary computations.
Great job!
packages/core/src/ui/index.ts (1)
1-8: LGTM!The code changes in this file are approved for the following reasons:
- The file is an index file that exports various UI-related classes and enums, which is consistent with the PR objective of introducing GUI infrastructure.
- The exports include classes like
CanvasGroup,UICanvas,UIImage,UIRenderer, andUITransform, and enums likeCanvasRenderModeandResolutionAdaptationStrategy. This aligns with the AI summary that mentions the addition of new classes to facilitate UI rendering.- The file structure and naming conventions follow best practices.
Great job!
packages/core/src/2d/assembler/ISpriteAssembler.ts (3)
9-16: LGTM!The changes to the
updatePositionsmethod signature are approved. The additional parameterswidth,height,pivot,flipX, andflipYprovide more granular control over sprite positioning and transformations, which aligns with the PR objective of enhancing the rendering capabilities.
18-18: LGTM!The changes to the
updateColormethod signature are approved. The optionalalphaparameter provides a way to manage transparency when updating color, which enhances the sprite rendering capabilities.
19-19: LGTM!The addition of the
updateAlphamethod is approved. It provides a more explicit way to manage transparency in sprite rendering, which complements theupdateColormethod and enhances the overall sprite rendering capabilities.tests/src/core/2d/ui/UICanvas.test.ts (3)
1-3: LGTM!The imports are correct and necessary for the test file.
4-17: LGTM!The test setup looks good and follows the necessary steps to create a test environment for the UICanvas.
29-29: LGTM!The test suite is closed correctly.
packages/core/src/ui/UITransform.ts (3)
6-53: LGTM!The
UITransformclass is well-structured and follows TypeScript best practices. The use of getters and setters forsizeandpivotproperties is a good encapsulation technique. The private methods_onSizeChangeand_onPivotChangeare dispatching events using an_updateFlagManager, which is a good way to decouple the transform from other parts of the system that might need to react to changes.
55-64: LGTM!The
UITransformModifyFlagsenum is well-documented with JSDoc comments and the flag values are using bitwise operators to ensure unique values.
32-44: Verify the constructor logic and its impact on the rest of the codebase.The constructor is doing some non-obvious setup by destroying the entity's existing transform and replacing it with this
UITransforminstance. This might be confusing for developers not familiar with the codebase.Please run the following script to verify the constructor logic and its impact on the rest of the codebase:
Verification successful
Constructor Logic is Localized and Does Not Impact the Rest of the Codebase
The constructor logic in the
UITransformclass, which modifiesentity._transformandentity._inverseWorldMatFlag, is confined to theUITransform.tsfile. There are no additional occurrences of these properties elsewhere in the codebase, indicating that the changes are localized and do not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constructor logic and its impact on the rest of the codebase. # Test 1: Search for usage of `entity._transform` and `entity._inverseWorldMatFlag`. # Expect: Only occurrences within `UITransform` class. rg --type typescript $'entity\\._transform|entity\\._inverseWorldMatFlag' # Test 2: Search for instantiation of `UITransform` class. # Expect: Instantiation should always pass an `Entity` instance. ast-grep --lang typescript --pattern $'new UITransform($_)'Length of output: 168
Script:
#!/bin/bash # Corrected script to verify the constructor logic and its impact on the rest of the codebase. # Test 1: Search for usage of `entity._transform` and `entity._inverseWorldMatFlag` across all files. rg 'entity\._transform|entity\._inverseWorldMatFlag' -A 3 # Test 2: Search for instantiation of `UITransform` class. ast-grep --lang typescript --pattern $'new UITransform($_)'Length of output: 673
packages/core/src/index.ts (1)
1-1: LGTM! The export changes align with the PR objectives, but verify the impact of the removed exports.The changes to the export statements in this file are approved for the following reasons:
- The export statements follow a consistent pattern and are grouped by categories, improving the overall organization of the file.
- The addition of new exports, such as
UICanvas,UIImage, andCanvasGroup, aligns with the PR objective of introducing GUI infrastructure.- The consolidation of asset-related exports and the addition of exports from the
2d,animation,clone, andgraphicdirectories suggest an expansion of the module's rendering and animation capabilities, which is consistent with the PR objective.However, please verify the impact of the removed exports, such as
ScriptandXRManager, to ensure that their removal doesn't break any existing functionality. Also, update the code that imports the entities from this file to match the new export locations and names.To verify the impact of the removed exports, run the following script:
Also applies to: 3-3, 9-10, 12-15, 17-17, 22-24, 26-27, 32-32, 34-37, 39-39, 42-43, 48-51, 59-59, 61-68
packages/core/src/RenderPipeline/BatcherManager.ts (5)
11-13: LGTM!The code changes are approved. The use of private properties for the
PrimitiveChunkManagerinstances is a good practice for encapsulation and lazy initialization.
17-19: LGTM!The code changes are approved. The use of a getter method for lazy initialization of the
primitiveChunkManager2Dproperty is a good practice for improving memory management and performance.The static analysis hint about the assignment in the expression can be safely ignored, as it is a common pattern for lazy initialization.
Tools
Biome
[error] 18-18: 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)
21-23: LGTM!The code changes are approved. The use of a getter method for lazy initialization of the
primitiveChunkManagerMaskproperty is a good practice for improving memory management and performance.The static analysis hint about the assignment in the expression can be safely ignored, as it is a common pattern for lazy initialization.
Tools
Biome
[error] 22-22: 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)
25-26: LGTM!The code changes are approved. The introduction of the
primitiveChunkManagerUIproperty and its corresponding getter method for lazy initialization is a good practice for improving memory management and performance.The static analysis hint about the assignment in the expression can be safely ignored, as it is a common pattern for lazy initialization.
Tools
Biome
[error] 26-26: 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)
30-41: LGTM!The code changes are approved. The updates to the
destroymethod to handle the new private properties are a good practice for ensuring robustness and proper memory management.packages/core/src/input/pointer/Pointer.ts (2)
5-5: LGTM!The code changes are approved.
28-29: LGTM, but verify the usage of thehitResultproperty.The code changes are approved. The addition of the
hitResultproperty to thePointerclass is a good enhancement to capture and manage collision detection results.However, ensure that this property is being properly utilized in the codebase to store the raycasting results and that the stored data is being used effectively in the relevant parts of the engine, such as in the physics system or for handling pointer interactions.
Run the following script to verify the usage of the
hitResultproperty:packages/core/src/2d/assembler/SimpleSpriteAssembler.ts (4)
Line range hint
16-23: LGTM!The code changes are approved. The method has been correctly updated to handle the new
UIImageparameter type.
Line range hint
25-64: LGTM!The code changes are approved. The method has been correctly updated to handle the new parameters for
width,height,pivot,flipX, andflipY. The changes improve the method's versatility and flexibility.
85-96: LGTM!The code changes are approved. The method has been correctly updated to handle the new
alphaparameter. The changes provide a way to adjust the overall transparency of the sprite's color dynamically.
98-105: LGTM!The code changes are approved. The new
updateAlphamethod correctly updates the alpha value of the sprite. The method provides a way to separately handle the alpha value of the sprite, allowing for more granular control over transparency.packages/core/src/ComponentsDependencies.ts (1)
26-33: LGTM!The introduction of the
switchstatement to handle differentDependentModevalues is a great improvement. It enhances readability and maintainability by clearly delineating the behavior based on the mode. The cases forAutoAddandCheckOnlymodes are implemented correctly, and thedefaultcase provides a safe fallback for unknown modes.The code changes are approved.
packages/core/src/shader/ShaderPool.ts (2)
28-29: LGTM!The code changes are approved. The imports are necessary for the subsequent changes and follow the existing naming convention.
74-74: LGTM, but verify the shader usage.The code changes are approved. The addition of the "ui" shader to the
ShaderPoolis consistent with the PR objectives and follows the existing pattern. The shader name is descriptive and appropriate.However, please ensure that the newly added "ui" shader is being used correctly in other parts of the codebase. Run the following script to verify the shader usage:
Verification successful
Shader usage verified successfully.
The "ui" shader is being used correctly in the codebase. It is utilized in the
Engine.tsfile to create aMaterialobject, which is consistent with its intended purpose for UI rendering.
packages/core/src/Engine.ts: The "ui" shader is used to create a material and configure its render state.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the "ui" shader. # Test: Search for the shader usage. Expect: Occurrences of the shader being used for UI rendering. rg --type typescript -A 5 $'"ui"'Length of output: 69
Script:
#!/bin/bash # Description: Verify the usage of the "ui" shader. # Test: Search for the shader usage. Expect: Occurrences of the shader being used for UI rendering. rg --type ts -A 5 $'"ui"'Length of output: 1090
packages/core/src/Component.ts (2)
6-6: LGTM!The code changes are approved.
13-15: LGTM, but verify the usage of_componentType.The code changes are approved. The addition of the
_componentTypeproperty is a good way to categorize or identify the type of component being instantiated. The@internaland@ignoreClonedecorators are used appropriately.However, ensure that this new property is being used correctly throughout the codebase. Verify that the
_componentTypeis being set to the correct value when instantiating different types of components and that it's being used consistently in any internal logic or processing that relies on the component type.Run the following script to verify the usage of
_componentType:packages/core/src/2d/assembler/SlicedSpriteAssembler.ts (4)
Line range hint
19-26: LGTM!The code changes are approved. The updated method signature allows
resetDatato work with bothSpriteRendererandUIImagetypes, improving its flexibility and consistency with the rest of the class.
Line range hint
130-141: LGTM!The code changes are approved. The updated method signature allows
updateUVsto work with bothSpriteRendererandUIImagetypes, improving its consistency with the rest of the class.
143-154: LGTM!The code changes are approved. The updated method signature allows
updateColorto work with bothSpriteRendererandUIImagetypes, improving its consistency with the rest of the class. The newalphaparameter provides more flexibility in adjusting the final alpha value used in rendering, which is a nice addition.
156-163: LGTM!The code changes are approved. The new
updateAlphamethod separates the concern of updating the alpha value from theupdateColormethod, which improves the separation of responsibilities within the class. This is a good addition to the class.packages/core/src/ui/UIRenderer.ts (15)
21-21: LGTM!The code is approved.
23-23: LGTM!The code is approved.
Also applies to: 25-25, 27-27
29-29: LGTM!The code is approved.
31-38: LGTM!The code is approved.
40-42: LGTM!The code is approved.
44-60: LGTM!The code is approved.
65-68: LGTM!The constructor is correctly initializing the
UIRendererinstance. It calls the base class constructor and sets the_componentTypeproperty toComponentType.UIRenderer.
106-111: LGTM!The
_onEnableInSceneand_onDisableInScenemethods are correctly adding and removing the renderer from thecomponentsManager'sonUpdateRendererslist if the_overrideUpdateflag is set. This ensures that the renderer is updated when it's enabled in the scene and stops updating when it's disabled.Also applies to: 116-121
126-133: LGTM!The
_setGroupmethod is correctly setting the_groupproperty and updating the_alphaproperty based on the group's global alpha value. It also sets theUIRendererUpdateFlags.Alphaflag if the alpha value has changed, indicating that the alpha needs to be updated.
138-140: LGTM!The
_getChunkManagermethod is correctly returning theprimitiveChunkManagerUIfrom the engine's_batcherManager. This ensures that the correct chunk manager is used for UI rendering.
176-183: LGTM!The
_onDestroymethod is correctly freeing the_subChunkif it exists by calling thefreeSubChunkmethod of the chunk manager. It then calls the base class_onDestroymethod to perform any necessary cleanup.
87-101: LGTM, but verify the logic for preparing render.The
_prepareRendermethod is overriding the base class method. It checks if the current frame is different from the last rendered frame and calls the_updatemethod if so. It then calls the_rendermethod and updates the global shader macro by combining the camera's global shader macro and the renderer's shader macro.The logic seems correct, but it's worth verifying a few things:
Verify the following using the provided script:
- The
_updatemethod is correctly updating the necessary properties and states.- The
_rendermethod is correctly rendering the UI elements.- The global shader macro is being correctly updated and used.
Verification successful
Verification Successful: Logic for Preparing Render is Correct
The
_prepareRendermethod inUIRenderer.tscorrectly implements the logic for updating and rendering. The_updateand_rendermethods are properly invoked, and the global shader macro is correctly updated by combining the camera's global shader macro and the renderer's shader macro.
- The
_updatemethod is correctly updating necessary properties and states.- The
_rendermethod is correctly rendering the UI elements.- The global shader macro is being correctly updated and used.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the _update method rg --type typescript -A 10 $'_update\\(' # Search for the _render method rg --type typescript -A 10 $'_render\\(' # Search for the usage of _globalShaderMacro rg --type typescript -A 5 $'_globalShaderMacro'Length of output: 233
Script:
#!/bin/bash # Search for the _update method in TypeScript files rg --type ts -A 10 $'_update\\(' # Search for the _render method in TypeScript files rg --type ts -A 10 $'_render\\(' # Search for the usage of _globalShaderMacro in TypeScript files rg --type ts -A 5 $'_globalShaderMacro'Length of output: 49830
185-185: Verify the logic for handling transform changes.The
_onTransformChangedmethod is overriding the base class method but has an empty implementation. This suggests that theUIRendererdoesn't need to perform any specific actions when the transform changes.Verify that not handling transform changes in this method doesn't lead to any issues. Use the following script to search for relevant code:
Verification successful
No issues found with the empty
_onTransformChangedmethod inUIRenderer.The
_onTransformChangedmethod inUIRendereris intentionally left empty, suggesting that it does not need to handle transform changes in the same way as other renderers likeParticleRendererorTextRenderer. This is likely by design and does not lead to any issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of _onTransformChanged in the base class rg --type typescript -A 10 $'_onTransformChanged' # Search for other places where transform changes are handled rg --type typescript -A 5 $'_transform.on'Length of output: 160
Script:
#!/bin/bash # Search for the usage of _onTransformChanged in the base class rg -A 10 '_onTransformChanged' # Search for other places where transform changes are handled rg -A 5 '_transform.on'Length of output: 5015
143-166: Verify the raycasting logic.The
_raycastmethod is implementing the raycasting logic for UI elements. It performs the following steps:
- Gets the entity and creates a plane using the entity's world forward direction and position.
- Intersects the ray with the plane and gets the distance.
- If the distance is valid and less than the provided distance, it gets the hit point in world space and transforms it to local space using the entity's inverse world matrix.
- Calls the
isRaycastLocationValidmethod to check if the hit point is within the valid bounds of the UI element.- If the hit point is valid, it sets the
HitResultproperties and returnstrue, otherwise it returnsfalse.The logic seems correct, but it's worth verifying a few things:
Verify the following using the provided script:
- The plane is being correctly created using the entity's world forward direction and position.
- The ray intersection with the plane is being correctly calculated.
- The hit point is being correctly transformed from world space to local space.
- The
isRaycastLocationValidmethod is correctly checking the bounds of the UI element.Verification successful
Raycasting Logic Verified Successfully
The raycasting logic in the
_raycastmethod ofUIRenderer.tsis correctly implemented. The plane is created using the entity's world forward direction and position, the ray intersection with the plane is calculated accurately, the hit point is transformed from world space to local space correctly, and theisRaycastLocationValidmethod properly checks the bounds of the UI element. No issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of entity's world forward direction and position rg --type typescript -A 5 $'transform.worldForward|transform.worldPosition' # Search for the usage of ray intersection with plane rg --type typescript -A 5 $'ray.intersectPlane' # Search for the usage of transforming hit point from world space to local space rg --type typescript -A 5 $'Vector3.transformCoordinate' # Search for the isRaycastLocationValid method rg --type typescript -A 10 $'isRaycastLocationValid'Length of output: 371
Script:
#!/bin/bash # Search for the usage of entity's world forward direction and position rg --type ts -A 5 $'transform.worldForward|transform.worldPosition' # Search for the usage of ray intersection with plane rg --type ts -A 5 $'ray.intersectPlane' # Search for the usage of transforming hit point from world space to local space rg --type ts -A 5 $'Vector3.transformCoordinate' # Search for the isRaycastLocationValid method rg --type ts -A 10 $'isRaycastLocationValid'Length of output: 27922
73-82: Verify the logic for updating shader data.The
_updateTransformShaderDatamethod is overriding the base class method. It has some commented out code that calls the base class method. Instead, it directly updates the shader data based on theonlyMVPflag.Verify that the logic for updating shader data is correct and that not calling the base class method doesn't lead to any issues. Use the following script to search for relevant code:
packages/core/src/Script.ts (7)
35-37: LGTM!The new
_onPointerEventIndexArrayproperty looks good. The naming convention and@ignoreClonedecorator are used appropriately.
130-130: LGTM!The modification to the
onPointerDownmethod signature to accept aPointerevent parameter is a good improvement. It enhances the specificity of the event handling.
136-136: LGTM!The modification to the
onPointerUpmethod signature to accept aPointerevent parameter is a good improvement. It enhances the specificity of the event handling, similar to the change made to theonPointerDownmethod.
142-142: LGTM!The modification to the
onPointerClickmethod signature to accept aPointerevent parameter is a good improvement. It enhances the specificity of the event handling, similar to the changes made to theonPointerDownandonPointerUpmethods.
148-148: LGTM!The modification to the
onPointerEntermethod signature to accept aPointerevent parameter is a good improvement. It enhances the specificity of the event handling, similar to the changes made to the other pointer event methods.
154-154: LGTM!The modification to the
onPointerExitmethod signature to accept aPointerevent parameter is a good improvement. It enhances the specificity of the event handling, similar to the changes made to the other pointer event methods.
212-228: LGTM!The updates to the
_onEnableInScenemethod to include checks for the existence of pointer event methods and register the corresponding event handlers are well-implemented. This registration process enhances the interactivity of theScriptclass and ensures that it can respond to pointer events appropriately. The checks for the existence of pointer event methods prevent unnecessary registrations and improve performance.packages/core/src/ui/UIImage.ts (9)
18-263: TheUIImageclass is well-structured and follows best practices.The class is well-organized and follows best practices for managing sprite rendering. The use of getters and setters for properties, the constructor for initialization, the
_rendermethod for rendering, the_canBatchand_batchmethods for batching, the_onDestroymethod for cleanup, and the_onSpriteChangemethod for handling sprite changes are all good practices.Tools
Biome
[error] 233-233: 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)
[error] 246-246: 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)
35-58: ThedrawModegetter and setter are well-implemented.The getter and setter follow best practices. The setter updates the
_assemblerproperty based on the value of_drawMode, which keeps the assembler in sync with the draw mode. The setter also sets a dirty flag for vertex data, which triggers a re-render when the draw mode changes.
63-74: ThetileModegetter and setter are well-implemented.The getter and setter follow best practices. The setter sets a dirty flag for vertex data if the draw mode is tiled, which triggers a re-render when the tile mode changes in tiled mode.
79-91: ThetiledAdaptiveThresholdgetter and setter are well-implemented.The getter and setter follow best practices. The setter clamps the value between 0 and 1, which ensures that the threshold is within a valid range. The setter also sets a dirty flag for vertex data if the draw mode is tiled, which triggers a re-render when the threshold changes in tiled mode.
96-117: Thespritegetter and setter are well-implemented.The getter and setter follow best practices. The setter removes the listener for the previous sprite and adds a listener for the new sprite, which keeps the sprite in sync with the renderer. The setter also sets a dirty flag for all update flags, which triggers a full re-render when the sprite changes. The setter updates the texture in the shader data, which ensures that the correct texture is used for rendering.
122-131: Thecolorgetter and setter are well-implemented.The getter and setter follow best practices. The setter copies the value to the
_colorproperty, which ensures that the color is updated correctly. The setter also sets a dirty flag for color, which triggers a re-render when the color changes.
227-262: The_onSpriteChangemethod is well-implemented.The method follows best practices for handling sprite changes. The use of a switch statement to handle different types of sprite changes keeps the code organized and readable. The method sets dirty flags based on the type of sprite change, which triggers a re-render when the sprite changes.
Tools
Biome
[error] 233-233: 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)
[error] 246-246: 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)
233-233: Address the static analysis hint about the switch statement.The static analysis tool suggests wrapping the declaration in a block to restrict its access to the switch clause. However, this is a false positive because the declaration is not accessed outside the switch clause.
Tools
Biome
[error] 233-233: 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)
246-246: Address the static analysis hint about the assignment in an expression.The static analysis tool suggests avoiding assignments in expressions because they can be confusing. In this case, the assignment is used as a condition for the ternary operator, which is a common pattern and is not confusing.
Tools
Biome
[error] 246-246: 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)
packages/core/src/ComponentsManager.ts (6)
20-25: LGTM!The code changes are approved. The new internal properties are used to manage UI canvases based on their rendering mode and are marked as
@internal, indicating they are not part of the public API.
80-90: LGTM!The code changes are approved. The
addUICanvasmethod correctly adds theuiCanvasto the appropriate array based on themodeand updates the_canvasIndexand_overlayCanvasesSortingFlagaccordingly.
92-103: LGTM!The code changes are approved. The
removeUICanvasmethod correctly removes theuiCanvasfrom the appropriate array based on themodeand updates the_canvasIndexand_overlayCanvasesSortingFlagaccordingly.Tools
Biome
[error] 101-101: 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)
105-114: LGTM!The code changes are approved. The
sortUICanvasesmethod correctly sorts the_overlayCanvasesarray based on thesortOrderproperty of the canvases and updates the_canvasIndexof each canvas accordingly. The sorting is performed only when needed, as indicated by the_overlayCanvasesSortingFlag.
314-314: LGTM!The code changes are approved. Adding the
_overlayCanvasesarray to the_gcmethod ensures proper memory management for the overlay canvases.
315-315: LGTM!The code changes are approved. Adding the
_canvasesarray to the_gcmethod ensures proper memory management for the canvases.packages/core/src/2d/sprite/SpriteMask.ts (2)
241-243: LGTM!The code changes are approved. The destructuring of the
spriteproperty enhances readability and reduces redundancy. The additional parameters passed toSimpleSpriteAssembler.updatePositionslikely provide more context for position updates.
Line range hint
254-271: LGTM!The code changes are approved. The destructuring of the
spriteproperty and the adjusted condition checking for the sprite's texture improve the clarity of the sprite handling within the rendering process. The update in the_dirtyUpdateFlagsection reflects the new parameter usage in theupdatePositionsmethod, which is consistent with the changes made in the_updateBoundsfunction.packages/core/src/input/pointer/PointerManager.ts (5)
130-135: LGTM!The code changes are approved. Passing the
outparameter to the_pointerRayCastmethod aligns with the updated method signature and allows for directly populating the hit result, streamlining the raycasting process.
240-243: LGTM!The code changes are approved. The updated
_pointerRayCastmethod signature and usage of theoutparameter align with the AI-generated summary and improve the raycasting process.
249-259: LGTM!The code changes are approved. The raycasting process now correctly handles overlay canvases, improving the accuracy of hit detection against UI elements. The implementation aligns with the AI-generated summary and follows the expected logic.
261-310: LGTM!The code changes are approved. The raycasting process now correctly handles the rendering order of canvases and their distance from the camera, improving the accuracy of hit detection against UI elements. The control flow has been adjusted to handle hits on canvases, physics, and camera clear flags appropriately. The implementation aligns with the AI-generated summary and follows the expected logic.
277-282: LGTM!The code changes are approved. Performing raycasting against the scene's physics and updating the hit distance based on the result enhances the accuracy of hit detection. The usage of the
outparameter aligns with the updated method signature.packages/core/src/RenderPipeline/BasicRenderPipeline.ts (4)
24-24: LGTM!The import statement is necessary and placed correctly.
334-335: LGTM!Extracting the
enableFrustumCulling,cullingMask, and_frustumproperties from thecameraobject is a good optimization. It streamlines the rendering checks by avoiding repeated access to thecameraobject.
337-353: LGTM!The filtering logic for renderers based on the camera's culling mask and frustum intersection is implemented correctly. It ensures that only relevant renderers are processed, improving the rendering performance.
355-365: LGTM!The new loop for handling canvas elements is a great addition to the rendering pipeline. It integrates canvas management and filters the canvases based on the rendering mode, associated camera, and culling mask before preparing them for rendering. This enhances the flexibility and efficiency of the rendering process.
packages/core/src/2d/sprite/SpriteRenderer.ts (6)
21-21: LGTM!The code changes are approved.
271-271: LGTM!The code changes are approved. Setting the
_componentTypeproperty toComponentType.SpriteRendererin the constructor helps identify the type of component being rendered.
320-322: LGTM!The code changes are approved. Using destructuring for the
_spriteproperty improves code readability. The additional parameters passed toupdatePositionslikely enhance the precision of position updates by incorporating the sprite's dimensions and pivot point.
330-331: LGTM!The code changes are approved. Using destructuring for the
_spriteproperty improves code readability. The additional checks forsprite,sprite.texture,this.width, andthis.heightensure that the rendering process only occurs when valid sprite data is available.
346-346: LGTM!The code changes are approved. The additional parameters passed to
updatePositionslikely enhance the precision of position updates by incorporating the sprite's dimensions and pivot point.
Line range hint
1-500: Overall, the code changes look good!The changes made to the
SpriteRendererclass align with the AI-generated summary and the list of alterations. The code changes improve the handling of sprite rendering and component management. There are no obvious missing requirements or potential improvements based on the provided context.Great job with the enhancements!
packages/core/src/Scene.ts (1)
330-330: LGTM, but verify the impact of this change.The change from
entity._setTransformDirty()toentity._setParentChange()suggests an improvement in managing the state of an entity when it is added to the scene hierarchy. However, it would be beneficial to verify that this change does not introduce any unintended side effects.To gain more clarity on the impact of this change, please run the following script to search for usages of
_setParentChangeand_setTransformDirtyin the codebase:If
_setParentChangeis used consistently across the codebase and_setTransformDirtyhas been fully replaced, then this change should be safe to merge. Otherwise, please investigate any discrepancies and ensure that the change does not introduce any issues.Verification successful
Change from
_setTransformDirtyto_setParentChangeis consistent and safe.The transition from
_setTransformDirtyto_setParentChangeis complete, with no occurrences of the former in the codebase. The_setParentChangemethod is used consistently across relevant files, indicating that the change aligns with the intended update to manage parent-related changes in the entity lifecycle. No issues were found.
- The
_setParentChangemethod is implemented inEntity.tsand is used in bothScene.tsandEntity.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of `_setParentChange` and `_setTransformDirty` methods. # Test: Search for `_setParentChange` method. # Expect: Occurrences in files other than `Scene.ts`, indicating the impact of this change. rg --type ts -A 5 $'_setParentChange' # Test: Search for `_setTransformDirty` method. # Expect: No occurrences, confirming that it has been replaced by `_setParentChange`. rg --type ts -A 5 $'_setTransformDirty'Length of output: 1330
packages/core/src/Renderer.ts (1)
171-171: LGTM!The code change is approved. Assigning
ComponentType.Renderertothis._componentTypein the constructor of theRendererclass explicitly categorizes the renderer, which can facilitate type checking, improve organization of component types, and potentially affect how instances ofRendererinteract with other components in the system.packages/core/src/2d/assembler/TiledSpriteAssembler.ts (5)
Line range hint
22-34: LGTM!The code changes to the
resetDatamethod are approved. The method correctly handles the vertex data reset for bothSpriteRendererandUIImagetypes.
Line range hint
37-148: LGTM!The code changes to the
updatePositionsmethod are approved. The method correctly updates the vertex positions using the additional parameterswidth,height,pivot,flipX, andflipY. The logic for calculating the transformation matrix and updating the vertex positions is correct, and the method handles bothSpriteRendererandUIImagetypes.
Line range hint
150-181: LGTM!The code changes to the
updateUVsmethod are approved. The method correctly updates the vertex UVs for bothSpriteRendererandUIImagetypes.
184-196: LGTM!The code changes to the
updateColormethod are approved. The method correctly updates the vertex colors using the optionalalphaparameter. The logic for calculating the final alpha value is correct, and the method handles bothSpriteRendererandUIImagetypes.
Line range hint
208-495: Skipped review.The private methods
_calculateAdaptiveDividingand_calculateContinuousDividinghave not been modified in this change. Since they are not part of the public API of the class, they do not require a detailed review.packages/core/src/ui/UICanvas.ts (1)
1-16: LGTM!The import statements are correctly defined.
packages/core/src/Engine.ts (10)
9-9: LGTM!The new imports are required for the UI rendering changes introduced in this PR.
18-18: LGTM!The
RenderQueueimport is required for the UI rendering changes introduced in this PR.
20-20: LGTM!The
ContextRendererUpdateFlagimport is required for the UI rendering changes introduced in this PR.
25-25: LGTM!The
VirtualCameraimport is required for the UI rendering changes introduced in this PR.
49-49: LGTM!The
UITransformimport is required for the UI rendering changes introduced in this PR.
107-107: LGTM!The
_uiDefaultMaterialvariable is required for the UI rendering changes introduced in this PR. It is correctly initialized in the constructor.
135-136: LGTM!The
_uiRenderQueueand_virtualCameravariables are required for the UI rendering changes introduced in this PR. They are correctly initialized in the constructor.
269-271: LGTM!The
_uiRenderQueueis correctly initialized for rendering transparent UI elements.
366-366: LGTM!The changes in the
updatemethod correctly set up the rendering of UI elements using the_virtualCamera,_uiRenderQueue, and_uiDefaultMaterial. The code adjusts the projection and view matrices based on the canvas dimensions and UI element positions, ensuring that the UI is rendered correctly in relation to the scene. The addition ofsortUICanvases()indicates a new sorting mechanism for UI elements, which should enhance the rendering order and efficiency.Also applies to: 380-380, 530-582
685-701: LGTM!The
_createUIMaterial()method correctly sets up a material for rendering transparent UI elements. It enables blending with the appropriate blend factors and sets the render queue type toRenderQueueType.Transparent. The material is also marked asisGCIgnoredto prevent it from being garbage collected.packages/core/src/Entity.ts (15)
112-114: LGTM!The code change is approved. Adding a getter for the
transformproperty is a good practice to improve encapsulation.
277-287: LGTM!The code change is approved. The new
getComponentsInParentmethod is a useful addition to retrieve components from the parent hierarchy.
351-351: LGTM!The code change is approved. Calling
_setParentChangeon the child entity after adding it as a child is necessary to propagate the parent change.
421-421: LGTM!The code change is approved. Adding a
UITransformcomponent to the child entity if the parent entity has one is a good practice to ensure consistency in the UI hierarchy.
442-442: LGTM!The code change is approved. Decrementing the
childrenarray length after removing all child entities is necessary to keep the array length in sync with the actual number of child entities.
544-544: LGTM!The code change is approved. Setting the
_updateFlagManagerproperty tonullwhen the entity is destroyed is a good practice to avoid memory leaks.
614-625: LGTM!The code change is approved. The new
_addOnPointerEventmethod is a useful addition to register callbacks for pointer events. The use of theDisorderedArraydata structure is appropriate for storing the callbacks.The static analysis hint about the assignments in the expressions is a false positive in this case because the assignments are intentional and necessary to initialize the arrays if they don't exist.
Tools
Biome
[error] 621-621: 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] 622-622: 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)
630-633: LGTM!The code change is approved. The new
_setParentChangemethod is necessary to propagate the parent change to the entity's transform and dispatch a modify event with the appropriate flag.
635-640: LGTM!The code change is approved. The new
_registerModifyListenermethod is a useful addition to register listeners for entity modify events. The initialization of the_updateFlagManagerproperty is necessary to ensure that it exists before adding the listener.The static analysis hint about the assignment in the expression is a false positive in this case because the assignment is intentional and necessary to initialize the
_updateFlagManagerproperty if it doesn't exist.Tools
Biome
[error] 639-639: 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)
642-647: LGTM!The code change is approved. The new
_removeModifyListenermethod is a useful addition to remove listeners for entity modify events. The initialization of the_updateFlagManagerproperty is necessary to ensure that it exists before removing the listener.The static analysis hint about the assignment in the expression is a false positive in this case because the assignment is intentional and necessary to initialize the
_updateFlagManagerproperty if it doesn't exist.Tools
Biome
[error] 646-646: 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)
649-653: LGTM!The code change is approved. The new
_dispatchModifymethod is a useful addition to dispatch entity modify events with a specified flag and optional parameter.
722-722: LGTM!The code change is approved. Calling the
_setParentChangemethod after setting the parent of the entity is necessary to propagate the parent change to the entity's transform and dispatch a modify event with the appropriate flag.
726-734: LGTM!The code change is approved. The new private
_getComponentsInParentmethod is a useful addition to retrieve components from the parent hierarchy.
819-819: LGTM!The code change is approved. Inverting the entity's world matrix and storing it in the
_invModelMatrixproperty is necessary for thegetInvModelMatrixmethod to work correctly.
826-830: LGTM!The code change is approved. The new
EntityModifyFlagsenum is a useful addition to provide flags for entity modify events.packages/core/src/Camera.ts (10)
129-131: LGTM!The addition of the
_updateFlagManagerproperty and its initialization with a newUpdateFlagManagerinstance is a good approach to manage update flags for camera properties. The@ignoreClonedecorator is also correctly used to prevent cloning of this instance-specific manager.
219-223: LGTM!Dispatching the
CameraModifyFlags.FOVflag when thefieldOfViewproperty changes is a good way to notify other components about the update. This enhances the reactivity and communication within the camera system.
287-296: LGTM!Dispatching the
CameraModifyFlags.Typeflag when theisOrthographicproperty changes is a good way to notify other components about the camera projection type update. Enabling or disabling the "CAMERA_ORTHOGRAPHIC" shader macro based on theisOrthographicvalue ensures consistency between the camera and the rendering system.
308-312: LGTM!Dispatching the
CameraModifyFlags.OrthographicSizeflag when theorthographicSizeproperty changes is a good way to notify other components about the update. This enhances the reactivity and communication within the camera system.
426-427: LGTM!Calling the
_checkMainCanvasAntialiasWastemethod when therenderTargetproperty changes is a good practice to monitor and warn about potential resource waste related to antialiasing. Dispatching theCameraModifyFlags.RenderTargetflag ensures that other components are notified about the render target update.
Line range hint
621-652: LGTM!The modifications to the
rendermethod improve the integration between the camera and the rendering pipeline. Setting the necessary camera-related properties on thecontextobject ensures that the pipeline has access to the required data. Computing the view-projection matrix and updating the camera's position and forward direction based on the transform keeps the camera's orientation and position in sync with the scene.
654-657: LGTM!Retrieving the
ignoreClearFlagsfrom thexrManagerwhen the camera type isCameraType.XRCameraand certain conditions are met is a good way to handle special clearing behavior in an XR context. This change improves the integration between the camera and the XR system, ensuring that the clearing flags are properly handled based on the XR manager's configuration.
841-842: LGTM!Dispatching the
CameraModifyFlags.ViewPortflag in the_onPixelViewportChangedmethod is a good way to notify other components about the pixel viewport update. This enhances the communication between the camera and other parts of the system that rely on the pixel viewport information.
856-858: LGTM!Adding an empty line at the end of the
Cameraclass definition improves code readability and adheres to common coding style guidelines.
857-868: LGTM!The addition of the
CameraModifyFlagsenum is a great way to define and organize the different types of camera modifications. Each flag represents a specific camera property or aspect that can be modified, making the code more readable and maintainable. Using an enum provides a clear and consistent way to refer to these modification flags throughout the codebase.packages/core/src/Transform.ts (9)
23-23: LGTM!The change of the access modifier for
_positionfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
25-25: LGTM!The change of the access modifier for
_rotationfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
27-27: LGTM!The change of the access modifier for
_rotationQuaternionfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
29-29: LGTM!The change of the access modifier for
_scalefromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
31-31: LGTM!The change of the access modifier for
_worldPositionfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
33-33: LGTM!The change of the access modifier for
_worldRotationfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
35-35: LGTM!The change of the access modifier for
_worldRotationQuaternionfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
39-39: LGTM!The change of the access modifier for
_localMatrixfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
41-41: LGTM!The change of the access modifier for
_worldMatrixfromprivatetoprotectedis approved. It allows subclasses ofTransformto access the property directly, enhancing the extensibility of the class. This change aligns with the overall goal of the PR to improve the rendering capabilities of the engine.
| // Check default properties | ||
| it("WorldSpace", () => { | ||
| const imageEntity = root.createChild("image"); | ||
| const image = imageEntity.addComponent(UIImage); | ||
| const uiTransform = <UITransform>imageEntity.transform; | ||
| uiTransform.rect.x = 100; | ||
| uiTransform.rect.y = 100; | ||
|
|
||
| uiCanvas.renderMode = CanvasRenderMode.WorldSpace; | ||
| cameraEntity.transform.position.set(0, 0, 100); | ||
| }); |
There was a problem hiding this comment.
Add assertions and rename the test case.
The test case is missing assertions to verify the expected behavior. Please add assertions to ensure that the UIImage is rendered correctly in the WorldSpace render mode.
Also, please rename the test case to be more descriptive, such as "should render UIImage correctly in WorldSpace render mode".
| this._primitiveChunkManager2D && this._primitiveChunkManager2D.uploadBuffer(); | ||
| this._primitiveChunkManagerMask && this._primitiveChunkManagerMask.uploadBuffer(); | ||
| this._primitiveChunkManagerUI && this._primitiveChunkManagerUI.uploadBuffer(); |
There was a problem hiding this comment.
LGTM, but consider using optional chaining.
The code changes are approved. The updates to the uploadBuffer method to conditionally call the uploadBuffer method on the private properties are a good practice for ensuring robustness.
However, consider using optional chaining to simplify the conditional checks and improve readability. Here's a suggested refactor:
- this._primitiveChunkManager2D && this._primitiveChunkManager2D.uploadBuffer();
- this._primitiveChunkManagerMask && this._primitiveChunkManagerMask.uploadBuffer();
- this._primitiveChunkManagerUI && this._primitiveChunkManagerUI.uploadBuffer();
+ this._primitiveChunkManager2D?.uploadBuffer();
+ this._primitiveChunkManagerMask?.uploadBuffer();
+ this._primitiveChunkManagerUI?.uploadBuffer();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._primitiveChunkManager2D && this._primitiveChunkManager2D.uploadBuffer(); | |
| this._primitiveChunkManagerMask && this._primitiveChunkManagerMask.uploadBuffer(); | |
| this._primitiveChunkManagerUI && this._primitiveChunkManagerUI.uploadBuffer(); | |
| this._primitiveChunkManager2D?.uploadBuffer(); | |
| this._primitiveChunkManagerMask?.uploadBuffer(); | |
| this._primitiveChunkManagerUI?.uploadBuffer(); |
Tools
Biome
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
packages/core/src/ui/UICanvas.ts
Outdated
| } | ||
|
|
||
| private _registerParentListener(): void { | ||
| const { _parents: parents } = this; | ||
| let curParent = this.entity.parent; | ||
| let index = 0; | ||
| while (curParent) { | ||
| const preParent = parents[index]; | ||
| if (preParent !== curParent) { | ||
| preParent?._removeModifyListener(this._onEntityModify); | ||
| parents[index] = curParent; | ||
| curParent._registerModifyListener(this._onEntityModify); | ||
| } | ||
| curParent = curParent.parent; | ||
| index++; | ||
| } | ||
| } | ||
|
|
||
| private _onEntityModify(flag: EntityModifyFlags, param?: any): void { | ||
| switch (flag) { | ||
| case EntityModifyFlags.Parent: | ||
| this._removeParentListener(); | ||
| this._registerParentListener(); | ||
| this._setIsRootCanvas(this._checkIsRootCanvas()); | ||
| break; | ||
| case EntityModifyFlags.UICanvasEnableInScene: | ||
| this._setIsRootCanvas(false); | ||
| break; | ||
| case EntityModifyFlags.UICanvasDisableInScene: | ||
| this._setIsRootCanvas(this._checkIsRootCanvas()); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| private _onReferenceResolutionChanged(): void { | ||
| this._adapterSizeInScreenSpace(); | ||
| } | ||
|
|
||
| private _checkIsRootCanvas(): boolean { | ||
| const canvases = this.entity.getComponentsInParent(UICanvas, []); | ||
| for (let i = 0, n = canvases.length; i < n; i++) { | ||
| if (canvases[i].enabled) return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| private _setIsRootCanvas(value: boolean): void { | ||
| if (this._isRootCanvas !== value) { | ||
| this._isRootCanvas = value; | ||
| const { _renderMode: renderMode } = this; | ||
| if (value) { | ||
| switch (renderMode) { | ||
| case CanvasRenderMode.ScreenSpaceCamera: | ||
| if (this._renderCamera) { | ||
| this._addCameraListener(this._renderCamera); | ||
| } else { | ||
| this._addCanvasListener(); | ||
| } | ||
| // @ts-ignore | ||
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | ||
| this._adapterPoseInScreenSpace(); | ||
| this._adapterSizeInScreenSpace(); | ||
| break; | ||
| case CanvasRenderMode.ScreenSpaceOverlay: | ||
| this._addCanvasListener(); | ||
| // @ts-ignore | ||
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | ||
| this._adapterPoseInScreenSpace(); | ||
| this._adapterSizeInScreenSpace(); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| this.scene._componentsManager.addUICanvas(renderMode, this); | ||
| } else { | ||
| switch (renderMode) { | ||
| case CanvasRenderMode.ScreenSpaceCamera: | ||
| if (this._renderCamera) { | ||
| this._removeCameraListener(this._renderCamera); | ||
| } else { | ||
| this._removeCanvasListener(); | ||
| } | ||
| // @ts-ignore | ||
| this._referenceResolution._onValueChanged = null; | ||
| break; | ||
| case CanvasRenderMode.ScreenSpaceOverlay: | ||
| this._removeCanvasListener(); | ||
| // @ts-ignore | ||
| this._referenceResolution._onValueChanged = null; | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| this.scene._componentsManager.removeUICanvas(renderMode, this); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM, but address the issues identified by static analysis.
The UICanvas class and its members are correctly implemented. However, please address the following issues identified by static analysis:
- The assignment in the expression at line 201 is confusing and should be avoided.
- (referenceResolution.x !== val.x || referenceResolution.y !== val.y) && referenceResolution.copyFrom(val);
+ if (referenceResolution.x !== val.x || referenceResolution.y !== val.y) {
+ referenceResolution.copyFrom(val);
+ }- The switch cases at lines 214-215 and 216-217 are falling through to the next case. Add a
breakorreturnstatement to prevent fallthrough.
switch (this._renderMode) {
case CanvasRenderMode.ScreenSpaceOverlay:
this._sortDistance = 0;
+ break;
case CanvasRenderMode.ScreenSpaceCamera:
this._sortDistance = this._distance;
+ break;
case CanvasRenderMode.WorldSpace:
this._sortDistance = Vector3.distance(cameraPosition, (this._transform as UITransform).worldPosition);
+ break;
}- The declarations at lines 326 and 334 can be erroneously accessed by other switch clauses. Wrap the declarations in a block to restrict their access.
case ComponentType.CanvasGroup:
- const alpha = group ? group.alpha : 1;
- group = <CanvasGroup>component;
+ {
+ const alpha = group ? group.alpha : 1;
+ group = <CanvasGroup>component;
+ }
group._globalAlpha = group.ignoreParentGroup ? 1 : alpha * group.alpha;
for (let j = offset, m = renderers.length; j < m; j++) {
renderers[j]._setGroup(group);
}
break;
case ComponentType.UIRenderer:
- const uiRenderer = <UIRenderer>component;
+ {
+ const uiRenderer = <UIRenderer>component;
+ }
uiRenderer._canvas = this;
uiRenderer._setGroup(group);
renderers.push(uiRenderer);
break;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.
| @dependentComponents(UITransform, DependentMode.AutoAdd) | |
| export class UICanvas extends Component { | |
| /** @internal */ | |
| @ignoreClone | |
| _canvasIndex: number = -1; | |
| /** @internal */ | |
| @ignoreClone | |
| _isRootCanvas: boolean = false; | |
| /** @internal */ | |
| @ignoreClone | |
| _renderElement: RenderElement; | |
| /** @internal */ | |
| @ignoreClone | |
| _sortDistance: number = 0; | |
| @assignmentClone | |
| private _renderMode = CanvasRenderMode.WorldSpace; | |
| @assignmentClone | |
| private _renderCamera: Camera; | |
| @assignmentClone | |
| private _resolutionAdaptationStrategy = ResolutionAdaptationStrategy.BothAdaptation; | |
| @assignmentClone | |
| private _sortOrder: number = 0; | |
| @assignmentClone | |
| private _distance: number = 10; | |
| private _renderers: UIRenderer[] = []; | |
| private _transform: UITransform; | |
| private _referenceResolution: Vector2 = new Vector2(800, 600); | |
| private _enableBlocked: boolean = true; | |
| private _parents: Entity[] = []; | |
| /** @internal */ | |
| get renderers(): UIRenderer[] { | |
| const renderers = this._renderers; | |
| renderers.length = 0; | |
| this._walk(this.entity, renderers); | |
| return renderers; | |
| } | |
| get enableBlocked(): boolean { | |
| return this._enableBlocked; | |
| } | |
| set enableBlocked(value: boolean) { | |
| this._enableBlocked = value; | |
| } | |
| get referenceResolution(): Vector2 { | |
| return this._referenceResolution; | |
| } | |
| set referenceResolution(val: Vector2) { | |
| const { _referenceResolution: referenceResolution } = this; | |
| if (referenceResolution === val) return; | |
| (referenceResolution.x !== val.x || referenceResolution.y !== val.y) && referenceResolution.copyFrom(val); | |
| } | |
| get renderMode(): CanvasRenderMode { | |
| return this._renderMode; | |
| } | |
| set renderMode(mode: CanvasRenderMode) { | |
| let preMode = this._renderMode; | |
| if (preMode !== mode) { | |
| this._renderMode = mode; | |
| if (this._isRootCanvas) { | |
| const camera = this._renderCamera; | |
| preMode = | |
| preMode === CanvasRenderMode.ScreenSpaceCamera && !camera ? CanvasRenderMode.ScreenSpaceOverlay : preMode; | |
| mode = mode === CanvasRenderMode.ScreenSpaceCamera && !camera ? CanvasRenderMode.ScreenSpaceOverlay : mode; | |
| if (preMode !== mode) { | |
| if (preMode === CanvasRenderMode.ScreenSpaceCamera) { | |
| this._removeCameraListener(camera); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| } else if (preMode === CanvasRenderMode.ScreenSpaceOverlay) { | |
| this._removeCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| } | |
| if (mode === CanvasRenderMode.ScreenSpaceCamera) { | |
| this._addCameraListener(camera); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| } else if (mode === CanvasRenderMode.ScreenSpaceOverlay) { | |
| this._addCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| } | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| const { _componentsManager: componentsManager } = this.scene; | |
| componentsManager.removeUICanvas(preMode, this); | |
| componentsManager.addUICanvas(mode, this); | |
| } | |
| } | |
| } | |
| } | |
| get renderCamera(): Camera { | |
| return this._renderCamera; | |
| } | |
| set renderCamera(val: Camera) { | |
| const preCamera = this._renderCamera; | |
| if (preCamera !== val) { | |
| this._renderCamera = val; | |
| if (this._isRootCanvas && this._renderMode === CanvasRenderMode.ScreenSpaceCamera) { | |
| preCamera ? this._removeCameraListener(preCamera) : this._removeCanvasListener(); | |
| const preMode = preCamera ? CanvasRenderMode.ScreenSpaceCamera : CanvasRenderMode.ScreenSpaceOverlay; | |
| const curMode = val ? CanvasRenderMode.ScreenSpaceCamera : CanvasRenderMode.ScreenSpaceOverlay; | |
| if (val) { | |
| this._addCameraListener(val); | |
| } else { | |
| this._addCanvasListener(); | |
| } | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| if (preMode !== curMode) { | |
| const { _componentsManager: componentsManager } = this.scene; | |
| componentsManager.removeUICanvas(preMode, this); | |
| componentsManager.addUICanvas(curMode, this); | |
| } | |
| } | |
| } | |
| } | |
| get resolutionAdaptationStrategy(): ResolutionAdaptationStrategy { | |
| return this._resolutionAdaptationStrategy; | |
| } | |
| set resolutionAdaptationStrategy(val: ResolutionAdaptationStrategy) { | |
| if (this._resolutionAdaptationStrategy !== val) { | |
| this._resolutionAdaptationStrategy = val; | |
| if (this._isRootCanvas && this._renderMode !== CanvasRenderMode.WorldSpace) { | |
| this._adapterSizeInScreenSpace(); | |
| } | |
| } | |
| } | |
| get sortOrder(): number { | |
| return this._sortOrder; | |
| } | |
| set sortOrder(val: number) { | |
| if (this._sortOrder !== val) { | |
| this._sortOrder = val; | |
| if (this._isRootCanvas && this._renderMode === CanvasRenderMode.ScreenSpaceOverlay) { | |
| this.scene._componentsManager._overlayCanvasesSortingFlag = true; | |
| } | |
| } | |
| } | |
| get distance(): number { | |
| return this._distance; | |
| } | |
| set distance(val: number) { | |
| if (this._distance !== val) { | |
| const { _isRootCanvas: isRootCanvas, _renderMode: renderMode } = this; | |
| this._distance = val; | |
| if (this._isRootCanvas) { | |
| if (renderMode === CanvasRenderMode.ScreenSpaceCamera && this._renderCamera) { | |
| this._adapterPoseInScreenSpace(); | |
| } | |
| } | |
| } | |
| } | |
| constructor(entity: Entity) { | |
| super(entity); | |
| this._transform = <UITransform>entity.transform; | |
| this._onEntityModify = this._onEntityModify.bind(this); | |
| this._onCanvasSizeListener = this._onCanvasSizeListener.bind(this); | |
| this._onCameraPropertyListener = this._onCameraPropertyListener.bind(this); | |
| this._onCameraTransformListener = this._onCameraTransformListener.bind(this); | |
| this._onReferenceResolutionChanged = this._onReferenceResolutionChanged.bind(this); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| } | |
| _prepareRender(context: RenderContext): void { | |
| const { renderers } = this; | |
| const { frameCount } = this.engine.time; | |
| const renderElement = (this._renderElement = this.engine._renderElementPool.get()); | |
| this._updateSortDistance(context.virtualCamera.position); | |
| renderElement.set(this.sortOrder, this._sortDistance); | |
| for (let i = 0, n = renderers.length; i < n; i++) { | |
| const renderer = renderers[i]; | |
| renderer._renderFrameCount = frameCount; | |
| renderer._prepareRender(context); | |
| } | |
| } | |
| /** @internal */ | |
| _updateSortDistance(cameraPosition: Vector3): void { | |
| switch (this._renderMode) { | |
| case CanvasRenderMode.ScreenSpaceOverlay: | |
| this._sortDistance = 0; | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| this._sortDistance = this._distance; | |
| case CanvasRenderMode.WorldSpace: | |
| this._sortDistance = Vector3.distance(cameraPosition, (this._transform as UITransform).worldPosition); | |
| } | |
| } | |
| /** | |
| * @internal | |
| */ | |
| override _onEnableInScene(): void { | |
| this._entity._dispatchModify(EntityModifyFlags.UICanvasEnableInScene); | |
| this._entity._registerModifyListener(this._onEntityModify); | |
| this._registerParentListener(); | |
| this._setIsRootCanvas(this._checkIsRootCanvas()); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| override _onDisableInScene(): void { | |
| this._removeParentListener(); | |
| this._entity._removeModifyListener(this._onEntityModify); | |
| this._entity._dispatchModify(EntityModifyFlags.UICanvasDisableInScene); | |
| this._setIsRootCanvas(false); | |
| this._renderers.length = 0; | |
| } | |
| /** | |
| * @internal | |
| */ | |
| rayCast(ray: Ray, out: HitResult, distance: number = Number.MAX_SAFE_INTEGER): boolean { | |
| const { renderers } = this; | |
| for (let i = renderers.length - 1; i >= 0; i--) { | |
| const renderer = renderers[i]; | |
| if (renderer.rayCastAble && renderer._raycast(ray, out, distance)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| private _adapterPoseInScreenSpace(): void { | |
| const { _renderCamera: renderCamera, _transform: transform } = this; | |
| if (renderCamera) { | |
| const { transform: cameraTransform } = renderCamera.entity; | |
| const { worldPosition: cameraWorldPosition, worldForward: cameraWorldForward } = cameraTransform; | |
| const { _distance: distance } = this; | |
| transform.setWorldPosition( | |
| cameraWorldPosition.x + cameraWorldForward.x * distance, | |
| cameraWorldPosition.y + cameraWorldForward.y * distance, | |
| cameraWorldPosition.z + cameraWorldForward.z * distance | |
| ); | |
| transform.worldRotationQuaternion.copyFrom(cameraTransform.worldRotationQuaternion); | |
| } else { | |
| const { canvas } = this.engine; | |
| transform.setWorldPosition(canvas.width / 2, canvas.height / 2, 0); | |
| transform.worldRotationQuaternion.set(0, 0, 0, 1); | |
| } | |
| } | |
| private _adapterSizeInScreenSpace(): void { | |
| const { _renderCamera: renderCamera } = this; | |
| const { x: width, y: height } = this._referenceResolution; | |
| let curWidth: number; | |
| let curHeight: number; | |
| if (renderCamera) { | |
| curHeight = renderCamera.isOrthographic | |
| ? renderCamera.orthographicSize * 2 | |
| : 2 * (Math.tan(MathUtil.degreeToRadian(renderCamera.fieldOfView / 2)) * this._distance); | |
| curWidth = renderCamera.aspectRatio * curHeight; | |
| } else { | |
| const canvas = this.engine.canvas; | |
| curHeight = canvas.height; | |
| curWidth = canvas.width; | |
| } | |
| let expectX: number, expectY: number, expectZ: number; | |
| switch (this._resolutionAdaptationStrategy) { | |
| case ResolutionAdaptationStrategy.WidthAdaptation: | |
| expectX = expectY = expectZ = curWidth / width; | |
| break; | |
| case ResolutionAdaptationStrategy.HeightAdaptation: | |
| expectX = expectY = expectZ = curHeight / height; | |
| break; | |
| case ResolutionAdaptationStrategy.BothAdaptation: | |
| expectX = curWidth / width; | |
| expectY = curHeight / height; | |
| expectZ = (expectX + expectY) / 2; | |
| break; | |
| case ResolutionAdaptationStrategy.ExpandAdaptation: | |
| expectX = expectY = expectZ = Math.min(curWidth / width, curHeight / height); | |
| break; | |
| case ResolutionAdaptationStrategy.ShrinkAdaptation: | |
| expectX = expectY = expectZ = Math.max(curWidth / width, curHeight / height); | |
| break; | |
| default: | |
| break; | |
| } | |
| this.entity.transform.setScale(expectX, expectY, expectZ); | |
| this._transform.size.set(curWidth / expectX, curHeight / expectY); | |
| } | |
| private _walk(entity: Entity, renderers: UIRenderer[], group?: CanvasGroup): void { | |
| const components = entity._components; | |
| const offset = renderers.length; | |
| for (let i = 0, n = components.length; i < n; i++) { | |
| const component = components[i]; | |
| if (component.enabled) { | |
| switch (component._componentType) { | |
| case ComponentType.CanvasGroup: | |
| const alpha = group ? group.alpha : 1; | |
| group = <CanvasGroup>component; | |
| group._globalAlpha = group.ignoreParentGroup ? 1 : alpha * group.alpha; | |
| for (let j = offset, m = renderers.length; j < m; j++) { | |
| renderers[j]._setGroup(group); | |
| } | |
| break; | |
| case ComponentType.UIRenderer: | |
| const uiRenderer = <UIRenderer>component; | |
| uiRenderer._canvas = this; | |
| uiRenderer._setGroup(group); | |
| renderers.push(uiRenderer); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| } | |
| const children = entity._children; | |
| for (let i = 0, n = children.length; i < n; i++) { | |
| const child = children[i]; | |
| child.isActive && this._walk(child, renderers, group); | |
| } | |
| } | |
| private _addCameraListener(camera: Camera): void { | |
| camera.entity.transform._updateFlagManager.addListener(this._onCameraTransformListener); | |
| camera._updateFlagManager.addListener(this._onCameraPropertyListener); | |
| } | |
| private _removeCameraListener(camera: Camera): void { | |
| camera.entity.transform._updateFlagManager.removeListener(this._onCameraTransformListener); | |
| camera._updateFlagManager.removeListener(this._onCameraPropertyListener); | |
| } | |
| private _onCameraPropertyListener(flag: CameraModifyFlags): void { | |
| switch (flag) { | |
| case CameraModifyFlags.NearPlane: | |
| case CameraModifyFlags.FarPlane: | |
| break; | |
| default: | |
| this._adapterSizeInScreenSpace(); | |
| break; | |
| } | |
| } | |
| private _onCameraTransformListener(): void { | |
| this._adapterPoseInScreenSpace(); | |
| } | |
| private _addCanvasListener(): void { | |
| this.engine.canvas._sizeUpdateFlagManager.addListener(this._onCanvasSizeListener); | |
| } | |
| private _removeCanvasListener(): void { | |
| this.engine.canvas._sizeUpdateFlagManager.removeListener(this._onCanvasSizeListener); | |
| } | |
| private _onCanvasSizeListener(): void { | |
| const { canvas } = this.engine; | |
| this._transform.setWorldPosition(canvas.width / 2, canvas.height / 2, 0); | |
| this._adapterSizeInScreenSpace(); | |
| } | |
| private _removeParentListener(offset: number = 0): void { | |
| const { _parents: parents } = this; | |
| for (let i = 0, n = parents.length; i < n; i++) { | |
| parents[i]._removeModifyListener(this._onEntityModify); | |
| } | |
| parents.length = 0; | |
| } | |
| private _registerParentListener(): void { | |
| const { _parents: parents } = this; | |
| let curParent = this.entity.parent; | |
| let index = 0; | |
| while (curParent) { | |
| const preParent = parents[index]; | |
| if (preParent !== curParent) { | |
| preParent?._removeModifyListener(this._onEntityModify); | |
| parents[index] = curParent; | |
| curParent._registerModifyListener(this._onEntityModify); | |
| } | |
| curParent = curParent.parent; | |
| index++; | |
| } | |
| } | |
| private _onEntityModify(flag: EntityModifyFlags, param?: any): void { | |
| switch (flag) { | |
| case EntityModifyFlags.Parent: | |
| this._removeParentListener(); | |
| this._registerParentListener(); | |
| this._setIsRootCanvas(this._checkIsRootCanvas()); | |
| break; | |
| case EntityModifyFlags.UICanvasEnableInScene: | |
| this._setIsRootCanvas(false); | |
| break; | |
| case EntityModifyFlags.UICanvasDisableInScene: | |
| this._setIsRootCanvas(this._checkIsRootCanvas()); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| private _onReferenceResolutionChanged(): void { | |
| this._adapterSizeInScreenSpace(); | |
| } | |
| private _checkIsRootCanvas(): boolean { | |
| const canvases = this.entity.getComponentsInParent(UICanvas, []); | |
| for (let i = 0, n = canvases.length; i < n; i++) { | |
| if (canvases[i].enabled) return false; | |
| } | |
| return true; | |
| } | |
| private _setIsRootCanvas(value: boolean): void { | |
| if (this._isRootCanvas !== value) { | |
| this._isRootCanvas = value; | |
| const { _renderMode: renderMode } = this; | |
| if (value) { | |
| switch (renderMode) { | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| if (this._renderCamera) { | |
| this._addCameraListener(this._renderCamera); | |
| } else { | |
| this._addCanvasListener(); | |
| } | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| break; | |
| case CanvasRenderMode.ScreenSpaceOverlay: | |
| this._addCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| break; | |
| default: | |
| break; | |
| } | |
| this.scene._componentsManager.addUICanvas(renderMode, this); | |
| } else { | |
| switch (renderMode) { | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| if (this._renderCamera) { | |
| this._removeCameraListener(this._renderCamera); | |
| } else { | |
| this._removeCanvasListener(); | |
| } | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| break; | |
| case CanvasRenderMode.ScreenSpaceOverlay: | |
| this._removeCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| break; | |
| default: | |
| break; | |
| } | |
| this.scene._componentsManager.removeUICanvas(renderMode, this); | |
| } | |
| } | |
| } | |
| } | |
| @dependentComponents(UITransform, DependentMode.AutoAdd) | |
| export class UICanvas extends Component { | |
| /** @internal */ | |
| @ignoreClone | |
| _canvasIndex: number = -1; | |
| /** @internal */ | |
| @ignoreClone | |
| _isRootCanvas: boolean = false; | |
| /** @internal */ | |
| @ignoreClone | |
| _renderElement: RenderElement; | |
| /** @internal */ | |
| @ignoreClone | |
| _sortDistance: number = 0; | |
| @assignmentClone | |
| private _renderMode = CanvasRenderMode.WorldSpace; | |
| @assignmentClone | |
| private _renderCamera: Camera; | |
| @assignmentClone | |
| private _resolutionAdaptationStrategy = ResolutionAdaptationStrategy.BothAdaptation; | |
| @assignmentClone | |
| private _sortOrder: number = 0; | |
| @assignmentClone | |
| private _distance: number = 10; | |
| private _renderers: UIRenderer[] = []; | |
| private _transform: UITransform; | |
| private _referenceResolution: Vector2 = new Vector2(800, 600); | |
| private _enableBlocked: boolean = true; | |
| private _parents: Entity[] = []; | |
| /** @internal */ | |
| get renderers(): UIRenderer[] { | |
| const renderers = this._renderers; | |
| renderers.length = 0; | |
| this._walk(this.entity, renderers); | |
| return renderers; | |
| } | |
| get enableBlocked(): boolean { | |
| return this._enableBlocked; | |
| } | |
| set enableBlocked(value: boolean) { | |
| this._enableBlocked = value; | |
| } | |
| get referenceResolution(): Vector2 { | |
| return this._referenceResolution; | |
| } | |
| set referenceResolution(val: Vector2) { | |
| const { _referenceResolution: referenceResolution } = this; | |
| if (referenceResolution === val) return; | |
| if (referenceResolution.x !== val.x || referenceResolution.y !== val.y) { | |
| referenceResolution.copyFrom(val); | |
| } | |
| } | |
| get renderMode(): CanvasRenderMode { | |
| return this._renderMode; | |
| } | |
| set renderMode(mode: CanvasRenderMode) { | |
| let preMode = this._renderMode; | |
| if (preMode !== mode) { | |
| this._renderMode = mode; | |
| if (this._isRootCanvas) { | |
| const camera = this._renderCamera; | |
| preMode = | |
| preMode === CanvasRenderMode.ScreenSpaceCamera && !camera ? CanvasRenderMode.ScreenSpaceOverlay : preMode; | |
| mode = mode === CanvasRenderMode.ScreenSpaceCamera && !camera ? CanvasRenderMode.ScreenSpaceOverlay : mode; | |
| if (preMode !== mode) { | |
| if (preMode === CanvasRenderMode.ScreenSpaceCamera) { | |
| this._removeCameraListener(camera); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| } else if (preMode === CanvasRenderMode.ScreenSpaceOverlay) { | |
| this._removeCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| } | |
| if (mode === CanvasRenderMode.ScreenSpaceCamera) { | |
| this._addCameraListener(camera); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| } else if (mode === CanvasRenderMode.ScreenSpaceOverlay) { | |
| this._addCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| } | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| const { _componentsManager: componentsManager } = this.scene; | |
| componentsManager.removeUICanvas(preMode, this); | |
| componentsManager.addUICanvas(mode, this); | |
| } | |
| } | |
| } | |
| } | |
| get renderCamera(): Camera { | |
| return this._renderCamera; | |
| } | |
| set renderCamera(val: Camera) { | |
| const preCamera = this._renderCamera; | |
| if (preCamera !== val) { | |
| this._renderCamera = val; | |
| if (this._isRootCanvas && this._renderMode === CanvasRenderMode.ScreenSpaceCamera) { | |
| preCamera ? this._removeCameraListener(preCamera) : this._removeCanvasListener(); | |
| const preMode = preCamera ? CanvasRenderMode.ScreenSpaceCamera : CanvasRenderMode.ScreenSpaceOverlay; | |
| const curMode = val ? CanvasRenderMode.ScreenSpaceCamera : CanvasRenderMode.ScreenSpaceOverlay; | |
| if (val) { | |
| this._addCameraListener(val); | |
| } else { | |
| this._addCanvasListener(); | |
| } | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| if (preMode !== curMode) { | |
| const { _componentsManager: componentsManager } = this.scene; | |
| componentsManager.removeUICanvas(preMode, this); | |
| componentsManager.addUICanvas(curMode, this); | |
| } | |
| } | |
| } | |
| } | |
| get resolutionAdaptationStrategy(): ResolutionAdaptationStrategy { | |
| return this._resolutionAdaptationStrategy; | |
| } | |
| set resolutionAdaptationStrategy(val: ResolutionAdaptationStrategy) { | |
| if (this._resolutionAdaptationStrategy !== val) { | |
| this._resolutionAdaptationStrategy = val; | |
| if (this._isRootCanvas && this._renderMode !== CanvasRenderMode.WorldSpace) { | |
| this._adapterSizeInScreenSpace(); | |
| } | |
| } | |
| } | |
| get sortOrder(): number { | |
| return this._sortOrder; | |
| } | |
| set sortOrder(val: number) { | |
| if (this._sortOrder !== val) { | |
| this._sortOrder = val; | |
| if (this._isRootCanvas && this._renderMode === CanvasRenderMode.ScreenSpaceOverlay) { | |
| this.scene._componentsManager._overlayCanvasesSortingFlag = true; | |
| } | |
| } | |
| } | |
| get distance(): number { | |
| return this._distance; | |
| } | |
| set distance(val: number) { | |
| if (this._distance !== val) { | |
| const { _isRootCanvas: isRootCanvas, _renderMode: renderMode } = this; | |
| this._distance = val; | |
| if (this._isRootCanvas) { | |
| if (renderMode === CanvasRenderMode.ScreenSpaceCamera && this._renderCamera) { | |
| this._adapterPoseInScreenSpace(); | |
| } | |
| } | |
| } | |
| } | |
| constructor(entity: Entity) { | |
| super(entity); | |
| this._transform = <UITransform>entity.transform; | |
| this._onEntityModify = this._onEntityModify.bind(this); | |
| this._onCanvasSizeListener = this._onCanvasSizeListener.bind(this); | |
| this._onCameraPropertyListener = this._onCameraPropertyListener.bind(this); | |
| this._onCameraTransformListener = this._onCameraTransformListener.bind(this); | |
| this._onReferenceResolutionChanged = this._onReferenceResolutionChanged.bind(this); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| } | |
| _prepareRender(context: RenderContext): void { | |
| const { renderers } = this; | |
| const { frameCount } = this.engine.time; | |
| const renderElement = (this._renderElement = this.engine._renderElementPool.get()); | |
| this._updateSortDistance(context.virtualCamera.position); | |
| renderElement.set(this.sortOrder, this._sortDistance); | |
| for (let i = 0, n = renderers.length; i < n; i++) { | |
| const renderer = renderers[i]; | |
| renderer._renderFrameCount = frameCount; | |
| renderer._prepareRender(context); | |
| } | |
| } | |
| /** @internal */ | |
| _updateSortDistance(cameraPosition: Vector3): void { | |
| switch (this._renderMode) { | |
| case CanvasRenderMode.ScreenSpaceOverlay: | |
| this._sortDistance = 0; | |
| break; | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| this._sortDistance = this._distance; | |
| break; | |
| case CanvasRenderMode.WorldSpace: | |
| this._sortDistance = Vector3.distance(cameraPosition, (this._transform as UITransform).worldPosition); | |
| break; | |
| } | |
| } | |
| /** | |
| * @internal | |
| */ | |
| override _onEnableInScene(): void { | |
| this._entity._dispatchModify(EntityModifyFlags.UICanvasEnableInScene); | |
| this._entity._registerModifyListener(this._onEntityModify); | |
| this._registerParentListener(); | |
| this._setIsRootCanvas(this._checkIsRootCanvas()); | |
| } | |
| /** | |
| * @internal | |
| */ | |
| override _onDisableInScene(): void { | |
| this._removeParentListener(); | |
| this._entity._removeModifyListener(this._onEntityModify); | |
| this._entity._dispatchModify(EntityModifyFlags.UICanvasDisableInScene); | |
| this._setIsRootCanvas(false); | |
| this._renderers.length = 0; | |
| } | |
| /** | |
| * @internal | |
| */ | |
| rayCast(ray: Ray, out: HitResult, distance: number = Number.MAX_SAFE_INTEGER): boolean { | |
| const { renderers } = this; | |
| for (let i = renderers.length - 1; i >= 0; i--) { | |
| const renderer = renderers[i]; | |
| if (renderer.rayCastAble && renderer._raycast(ray, out, distance)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| private _adapterPoseInScreenSpace(): void { | |
| const { _renderCamera: renderCamera, _transform: transform } = this; | |
| if (renderCamera) { | |
| const { transform: cameraTransform } = renderCamera.entity; | |
| const { worldPosition: cameraWorldPosition, worldForward: cameraWorldForward } = cameraTransform; | |
| const { _distance: distance } = this; | |
| transform.setWorldPosition( | |
| cameraWorldPosition.x + cameraWorldForward.x * distance, | |
| cameraWorldPosition.y + cameraWorldForward.y * distance, | |
| cameraWorldPosition.z + cameraWorldForward.z * distance | |
| ); | |
| transform.worldRotationQuaternion.copyFrom(cameraTransform.worldRotationQuaternion); | |
| } else { | |
| const { canvas } = this.engine; | |
| transform.setWorldPosition(canvas.width / 2, canvas.height / 2, 0); | |
| transform.worldRotationQuaternion.set(0, 0, 0, 1); | |
| } | |
| } | |
| private _adapterSizeInScreenSpace(): void { | |
| const { _renderCamera: renderCamera } = this; | |
| const { x: width, y: height } = this._referenceResolution; | |
| let curWidth: number; | |
| let curHeight: number; | |
| if (renderCamera) { | |
| curHeight = renderCamera.isOrthographic | |
| ? renderCamera.orthographicSize * 2 | |
| : 2 * (Math.tan(MathUtil.degreeToRadian(renderCamera.fieldOfView / 2)) * this._distance); | |
| curWidth = renderCamera.aspectRatio * curHeight; | |
| } else { | |
| const canvas = this.engine.canvas; | |
| curHeight = canvas.height; | |
| curWidth = canvas.width; | |
| } | |
| let expectX: number, expectY: number, expectZ: number; | |
| switch (this._resolutionAdaptationStrategy) { | |
| case ResolutionAdaptationStrategy.WidthAdaptation: | |
| expectX = expectY = expectZ = curWidth / width; | |
| break; | |
| case ResolutionAdaptationStrategy.HeightAdaptation: | |
| expectX = expectY = expectZ = curHeight / height; | |
| break; | |
| case ResolutionAdaptationStrategy.BothAdaptation: | |
| expectX = curWidth / width; | |
| expectY = curHeight / height; | |
| expectZ = (expectX + expectY) / 2; | |
| break; | |
| case ResolutionAdaptationStrategy.ExpandAdaptation: | |
| expectX = expectY = expectZ = Math.min(curWidth / width, curHeight / height); | |
| break; | |
| case ResolutionAdaptationStrategy.ShrinkAdaptation: | |
| expectX = expectY = expectZ = Math.max(curWidth / width, curHeight / height); | |
| break; | |
| default: | |
| break; | |
| } | |
| this.entity.transform.setScale(expectX, expectY, expectZ); | |
| this._transform.size.set(curWidth / expectX, curHeight / expectY); | |
| } | |
| private _walk(entity: Entity, renderers: UIRenderer[], group?: CanvasGroup): void { | |
| const components = entity._components; | |
| const offset = renderers.length; | |
| for (let i = 0, n = components.length; i < n; i++) { | |
| const component = components[i]; | |
| if (component.enabled) { | |
| switch (component._componentType) { | |
| case ComponentType.CanvasGroup: | |
| { | |
| const alpha = group ? group.alpha : 1; | |
| group = <CanvasGroup>component; | |
| group._globalAlpha = group.ignoreParentGroup ? 1 : alpha * group.alpha; | |
| for (let j = offset, m = renderers.length; j < m; j++) { | |
| renderers[j]._setGroup(group); | |
| } | |
| } | |
| break; | |
| case ComponentType.UIRenderer: | |
| { | |
| const uiRenderer = <UIRenderer>component; | |
| uiRenderer._canvas = this; | |
| uiRenderer._setGroup(group); | |
| renderers.push(uiRenderer); | |
| } | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| } | |
| const children = entity._children; | |
| for (let i = 0, n = children.length; i < n; i++) { | |
| const child = children[i]; | |
| child.isActive && this._walk(child, renderers, group); | |
| } | |
| } | |
| private _addCameraListener(camera: Camera): void { | |
| camera.entity.transform._updateFlagManager.addListener(this._onCameraTransformListener); | |
| camera._updateFlagManager.addListener(this._onCameraPropertyListener); | |
| } | |
| private _removeCameraListener(camera: Camera): void { | |
| camera.entity.transform._updateFlagManager.removeListener(this._onCameraTransformListener); | |
| camera._updateFlagManager.removeListener(this._onCameraPropertyListener); | |
| } | |
| private _onCameraPropertyListener(flag: CameraModifyFlags): void { | |
| switch (flag) { | |
| case CameraModifyFlags.NearPlane: | |
| case CameraModifyFlags.FarPlane: | |
| break; | |
| default: | |
| this._adapterSizeInScreenSpace(); | |
| break; | |
| } | |
| } | |
| private _onCameraTransformListener(): void { | |
| this._adapterPoseInScreenSpace(); | |
| } | |
| private _addCanvasListener(): void { | |
| this.engine.canvas._sizeUpdateFlagManager.addListener(this._onCanvasSizeListener); | |
| } | |
| private _removeCanvasListener(): void { | |
| this.engine.canvas._sizeUpdateFlagManager.removeListener(this._onCanvasSizeListener); | |
| } | |
| private _onCanvasSizeListener(): void { | |
| const { canvas } = this.engine; | |
| this._transform.setWorldPosition(canvas.width / 2, canvas.height / 2, 0); | |
| this._adapterSizeInScreenSpace(); | |
| } | |
| private _removeParentListener(offset: number = 0): void { | |
| const { _parents: parents } = this; | |
| for (let i = 0, n = parents.length; i < n; i++) { | |
| parents[i]._removeModifyListener(this._onEntityModify); | |
| } | |
| parents.length = 0; | |
| } | |
| private _registerParentListener(): void { | |
| const { _parents: parents } = this; | |
| let curParent = this.entity.parent; | |
| let index = 0; | |
| while (curParent) { | |
| const preParent = parents[index]; | |
| if (preParent !== curParent) { | |
| preParent?._removeModifyListener(this._onEntityModify); | |
| parents[index] = curParent; | |
| curParent._registerModifyListener(this._onEntityModify); | |
| } | |
| curParent = curParent.parent; | |
| index++; | |
| } | |
| } | |
| private _onEntityModify(flag: EntityModifyFlags, param?: any): void { | |
| switch (flag) { | |
| case EntityModifyFlags.Parent: | |
| this._removeParentListener(); | |
| this._registerParentListener(); | |
| this._setIsRootCanvas(this._checkIsRootCanvas()); | |
| break; | |
| case EntityModifyFlags.UICanvasEnableInScene: | |
| this._setIsRootCanvas(false); | |
| break; | |
| case EntityModifyFlags.UICanvasDisableInScene: | |
| this._setIsRootCanvas(this._checkIsRootCanvas()); | |
| break; | |
| default: | |
| break; | |
| } | |
| } | |
| private _onReferenceResolutionChanged(): void { | |
| this._adapterSizeInScreenSpace(); | |
| } | |
| private _checkIsRootCanvas(): boolean { | |
| const canvases = this.entity.getComponentsInParent(UICanvas, []); | |
| for (let i = 0, n = canvases.length; i < n; i++) { | |
| if (canvases[i].enabled) return false; | |
| } | |
| return true; | |
| } | |
| private _setIsRootCanvas(value: boolean): void { | |
| if (this._isRootCanvas !== value) { | |
| this._isRootCanvas = value; | |
| const { _renderMode: renderMode } = this; | |
| if (value) { | |
| switch (renderMode) { | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| if (this._renderCamera) { | |
| this._addCameraListener(this._renderCamera); | |
| } else { | |
| this._addCanvasListener(); | |
| } | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| break; | |
| case CanvasRenderMode.ScreenSpaceOverlay: | |
| this._addCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = this._onReferenceResolutionChanged; | |
| this._adapterPoseInScreenSpace(); | |
| this._adapterSizeInScreenSpace(); | |
| break; | |
| default: | |
| break; | |
| } | |
| this.scene._componentsManager.addUICanvas(renderMode, this); | |
| } else { | |
| switch (renderMode) { | |
| case CanvasRenderMode.ScreenSpaceCamera: | |
| if (this._renderCamera) { | |
| this._removeCameraListener(this._renderCamera); | |
| } else { | |
| this._removeCanvasListener(); | |
| } | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| break; | |
| case CanvasRenderMode.ScreenSpaceOverlay: | |
| this._removeCanvasListener(); | |
| // @ts-ignore | |
| this._referenceResolution._onValueChanged = null; | |
| break; | |
| default: | |
| break; | |
| } | |
| this.scene._componentsManager.removeUICanvas(renderMode, this); | |
| } | |
| } | |
| } | |
| } |
Tools
Biome
[error] 201-201: 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] 214-215: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 216-217: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 326-326: 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)
[error] 334-334: 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)
| import { Renderer } from "../../Renderer"; | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| export interface ISpriteAssembler { | ||
| resetData(renderer: Renderer, vertexCount?: number): void; | ||
| updatePositions?(renderer: Renderer): void; | ||
| updatePositions?( |
There was a problem hiding this comment.
After adding parameters like width, height, pivot, and flip, it seems that the function is no longer just updating the position. Should we consider renaming the function, or adding a new method for these updates？
There was a problem hiding this comment.
After adding parameters like width, height, pivot, and flip, it seems that the function is no longer just updating the position. Should we consider renaming the function, or adding a new method for these updates？
This method just for update vertex position, can you give a other case?
There was a problem hiding this comment.
This is my problem. I saw that the input parameters changed, and I thought the function accusation also changed.
Please revolve this comment.
| @@ -12,7 +13,7 @@ export class SimpleSpriteAssembler { | |||
| static _rectangleTriangles = [0, 1, 2, 2, 1, 3]; | |||
| static _worldMatrix = new Matrix(); | |||
|
|
|||
| static resetData(renderer: SpriteRenderer | SpriteMask): void { | |||
| static resetData(renderer: SpriteRenderer | SpriteMask | UIImage): void { | |||
There was a problem hiding this comment.
I noticed that we now have three related types (SpriteRenderer, SpriteMask, and UIImage). To improve readability and maintainability, would it be better to define a single type for these renderable components, such as Renderable? This way, we can avoid redundancy and make future updates easier.
There was a problem hiding this comment.
Good idea, you should try to pr
| const sx = renderer.flipX ? -width : width; | ||
| const sy = renderer.flipY ? -height : height; | ||
| const sx = flipX ? -width : width; | ||
| const sy = flipY ? -height : height; |
There was a problem hiding this comment.
I ran a performance test comparing the ternary operator version and a simplified multiplication version for sx and sy. The results showed that the multiplication approach is consistently faster. Since both versions produce the same output, I recommend switching to the multiplication method to improve performance, especially in cases where this function is called frequently. Here’s the updated version:
const sx = width * (flipX ? -1 : 1);
const sy = height * (flipY ? -1 : 1);
There was a problem hiding this comment.
理论上 -x 应该是比 -1 * x 性能更好
另外，-1 * x 和 x 对比怎么样呢
| @@ -14,7 +16,7 @@ export class SlicedSpriteAssembler { | |||
| ]; | |||
| static _worldMatrix = new Matrix(); | |||
|
|
|||
| static resetData(renderer: SpriteRenderer): void { | |||
| static resetData(renderer: SpriteRenderer | UIImage): void { | |||
There was a problem hiding this comment.
Same as the previous comment regarding SpriteRenderer | SpriteMask | UIImage. To avoid redundancy, consider using the same Renderable type here as well. See the comment in packages/core/src/2d/assembler/SimpleSpriteAssembler.ts
| static updateAlpha(renderer: SpriteRenderer, alpha: number = 1): void { | ||
| const subChunk = renderer._subChunk; | ||
| const finalAlpha = renderer.color.a * alpha; | ||
| const vertices = subChunk.chunk.vertices; |
There was a problem hiding this comment.
Above three lines can be optimized using destructuring to make it more concise and avoid repeated references to renderer and subChunk. Here's a suggested improvement:
const { _subChunk: { chunk: { vertices } }, color: { a } } = renderer;
const finalAlpha = a * alpha;
There was a problem hiding this comment.
I personally think this is not good for readability.
| @@ -395,6 +418,7 @@ export class Entity extends EngineObject { | |||
| */ | |||
| createChild(name?: string): Entity { | |||
| const child = new Entity(this.engine, name); | |||
| this._transform instanceof UITransform && child.addComponent(UITransform); | |||
There was a problem hiding this comment.
- If performance is a primary concern and there is no need to check through inheritance chains, here can use constructor instead of instanceof. This may give a slight performance improvement in certain scenarios:
this._transform.constructor === UITransform && && child.addComponent(UITransform);
2.It feels a little tricky here.
|
|
||
| const elements = renderers._elements; | ||
| let rendererElements = renderers._elements; |
There was a problem hiding this comment.
Since the variable isn't modified, we can use const instead of let.
| continue; | ||
| } | ||
| } | ||
| renderer._prepareRender(context); | ||
| renderer._renderFrameCount = engine.time.frameCount; | ||
| } | ||
|
|
||
| let canvasesElements = canvases._elements; |
There was a problem hiding this comment.
Since the variable isn't modified, we can use const instead of let.
| this._primitiveChunkManager2D && this._primitiveChunkManager2D.uploadBuffer(); | ||
| this._primitiveChunkManagerMask && this._primitiveChunkManagerMask.uploadBuffer(); | ||
| this._primitiveChunkManagerUI && this._primitiveChunkManagerUI.uploadBuffer(); |
| if (this.onPointerExit !== prototype.onPointerExit) { | ||
| entity._addOnPointerEvent(PointerEventType.Exit, this, this.onPointerExit); | ||
| } | ||
| entity._addScript(this); |
There was a problem hiding this comment.
We can optimize the code by destructuring this and prototype to reduce redundant references to this.onPointerDown, etc. Here’s an example:
const { _entity: entity, onPointerDown, onPointerUp, onPointerClick, onPointerEnter, onPointerExit } = this;
const { onPointerDown: protoOnPointerDown, onPointerUp: protoOnPointerUp, onPointerClick: protoOnPointerClick, onPointerEnter: protoOnPointerEnter, onPointerExit: protoOnPointerExit } = prototype;
|
|
||
| export class PointerEvent { | ||
| pointer: Pointer; | ||
| } |
There was a problem hiding this comment.
The PointerEvent class currently only has a single pointer property. Is this the intended design, or should there be additional properties or methods associated with the event?
| break; | ||
| case CanvasRenderMode.ScreenSpaceOverlay: | ||
| this._addCanvasListener(); | ||
| // @ts-ignore |
| } else { | ||
| this._removeCanvasListener(); | ||
| } | ||
| // @ts-ignore |
| break; | ||
| case CanvasRenderMode.ScreenSpaceOverlay: | ||
| this._removeCanvasListener(); | ||
| // @ts-ignore |
| const { _sprite: sprite } = this; | ||
| const transform = this._transform as UITransform; | ||
| const { x: width, y: height } = transform.size; | ||
| if (!sprite?.texture || !width || !height) { |
There was a problem hiding this comment.
We can optimize the code by destructuring this._sprite and this._transform together, and simplifying the sprite?.texture check. Here's a example:
const { _sprite: sprite, _transform: transform } = this;
const { x: width, y: height } = (transform as UITransform).size;
const texture = sprite?.texture;
if (!texture || !width || !height) {
return;
}
| const { engine } = context.camera; | ||
| const canvas = this._canvas; | ||
| const subRenderElement = engine._subRenderElementPool.get(); | ||
| const subChunk = this._subChunk; |
There was a problem hiding this comment.
We can further optimize by destructuring primitive from subChunk.chunk to reduce the nested property access. Here's an improved version:
const { _canvas: canvas, _subChunk: subChunk, sprite } = this;
const { primitive } = subChunk.chunk;
const subRenderElement = engine._subRenderElementPool.get();
const texture = sprite.texture;
subRenderElement.set(this, material, primitive, subChunk.subMesh, texture, subChunk);
| @assignmentClone | ||
| private _resolutionAdaptationStrategy = ResolutionAdaptationStrategy.BothAdaptation; | ||
| @assignmentClone | ||
| private _sortOrder: number = 0; |
There was a problem hiding this comment.
The number type in private _sortOrder: number = 0; can be omitted, as TypeScript will automatically infer the type from the initial value. We can simplify this to：
private _sortOrder = 0;
| @assignmentClone | ||
| private _sortOrder: number = 0; | ||
| @assignmentClone | ||
| private _distance: number = 10; |
There was a problem hiding this comment.
The number type in private _sortOrder: number = 0; can be omitted, as TypeScript will automatically infer the type from the initial value. We can simplify this to：
private _sortOrder = 0;
| private _distance: number = 10; | ||
| private _renderers: UIRenderer[] = []; | ||
| private _transform: UITransform; | ||
| private _referenceResolution: Vector2 = new Vector2(800, 600); |
There was a problem hiding this comment.
Same as commented before. The Vector2 type in _referenceResolution: Vector2 = new Vector2(800, 600); can be omitted.
packages/core/src/ui/UICanvas.ts
Outdated
| private _renderers: UIRenderer[] = []; | ||
| private _transform: UITransform; | ||
| private _referenceResolution: Vector2 = new Vector2(800, 600); | ||
| private _enableBlocked: boolean = true; |
There was a problem hiding this comment.
Same as commented before. Can be omitted too.
| _renderElement: RenderElement; | ||
| /** @internal */ | ||
| @ignoreClone | ||
| _sortDistance: number = 0; |
There was a problem hiding this comment.
The number type can be omitted.
| protected isRaycastLocationValid(hitPoint: Vector3): boolean { | ||
| const { x, y } = hitPoint; | ||
| const uiTransform = <UITransform>this._transform; | ||
| const { x: width, y: height } = uiTransform.size; |
There was a problem hiding this comment.
Maybe you can name x and y of size directly as width and height? Is this more semantically appropriate?
| const { x, y } = hitPoint; | ||
| const uiTransform = <UITransform>this._transform; | ||
| const { x: width, y: height } = uiTransform.size; | ||
| const { x: pivotX, y: pivotY } = uiTransform.pivot; |
There was a problem hiding this comment.
Above two lines can be optimize like this:
const { x, y } = hitPoint;
const { size: { x: width, y: height }, pivot: { x: pivotX, y: pivotY } } = this._transform;
|
|
||
| export class UITransform extends Transform { | ||
| @deepClone | ||
| private _size: Vector2 = new Vector2(100, 100); |
There was a problem hiding this comment.
The Vector2 type in _size: Vector2 = new Vector2(100, 100); can be omitted
| @deepClone | ||
| private _size: Vector2 = new Vector2(100, 100); | ||
| @deepClone | ||
| private _pivot: Vector2 = new Vector2(0.5, 0.5); |
There was a problem hiding this comment.
The Vector2 type in _pivot: Vector2 = new Vector2(0.5, 0.5); can be omitted
|
|
||
| constructor(entity: Entity) { | ||
| super(entity); | ||
| // @ts-ignore |
There was a problem hiding this comment.
Too much ignore here.Can it be optimized?
| * @internal | ||
| */ | ||
| export enum CameraModifyFlags { | ||
| Type = 0x1, |
There was a problem hiding this comment.
Change to CameraType, the semantics is clearer.
| @deepClone | ||
| private _worldMatrix: Matrix = new Matrix(); | ||
| protected _worldMatrix: Matrix = new Matrix(); |
There was a problem hiding this comment.
Above 18 line, the type declaration can be omitted.
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/ui/UICanvas.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/ui/UICanvas.ts
[error] 201-201: 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] 329-329: 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)
[error] 337-337: 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)
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/RenderPipeline/BatcherManager.ts (2 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/RenderPipeline/BatcherManager.ts
[error] 18-18: 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] 22-22: 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] 26-26: 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 (2)
packages/core/src/RenderPipeline/BatcherManager.ts (2)
11-13: Good practice: Making properties private enhances encapsulation.Changing
primitiveChunkManager2DandprimitiveChunkManagerMaskto private properties and introducing_primitiveChunkManagerUIas a private property improves the encapsulation of the class and hides internal implementation details.
86-88: Good use of optional chaining to simplify method calls.The use of optional chaining (
?.) when callinguploadBuffer()on potentially undefined properties improves code safety and readability.
| return (this._primitiveChunkManager2D ||= new PrimitiveChunkManager(this.engine)); | ||
| } | ||
|
|
||
| get primitiveChunkManagerMask(): PrimitiveChunkManager { | ||
| return (this._primitiveChunkManagerMask ||= new PrimitiveChunkManager(this.engine, 128)); | ||
| } | ||
|
|
||
| get primitiveChunkManagerUI(): PrimitiveChunkManager { | ||
| return (this._primitiveChunkManagerUI ||= new PrimitiveChunkManager(this.engine)); |
There was a problem hiding this comment.
Avoid assignments within return statements for better readability.
Assigning within a return statement can be confusing and may lead to unintended side effects. Separating the assignment from the return statement enhances code clarity.
Consider refactoring the getters as follows:
get primitiveChunkManager2D(): PrimitiveChunkManager {
if (!this._primitiveChunkManager2D) {
this._primitiveChunkManager2D = new PrimitiveChunkManager(this.engine);
}
return this._primitiveChunkManager2D;
}
get primitiveChunkManagerMask(): PrimitiveChunkManager {
if (!this._primitiveChunkManagerMask) {
this._primitiveChunkManagerMask = new PrimitiveChunkManager(this.engine, 128);
}
return this._primitiveChunkManagerMask;
}
get primitiveChunkManagerUI(): PrimitiveChunkManager {
if (!this._primitiveChunkManagerUI) {
this._primitiveChunkManagerUI = new PrimitiveChunkManager(this.engine);
}
return this._primitiveChunkManagerUI;
}🧰 Tools
🪛 Biome
[error] 18-18: 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] 22-22: 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] 26-26: 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)
* feat: ui input
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (18)
packages/core/src/enums/ComponentType.ts (1)
15-15: Good use of bitwise combination for UIElementThe bitwise OR operation to define
UIElementas a combination ofUICanvasandUIRendereris a good approach. This allowsUIElementto represent both types simultaneously, which can be useful for type checking or filtering.For improved readability, consider adding a comment explaining the purpose of this combination:
UIElement = UICanvas | UIRenderer // Represents elements that are both UICanvas and UIRendererpackages/core/src/ui/index.ts (2)
5-8: LGTM! Consider adding a comment for improved readability.The UI component exports are well-organized, with each component in its own file and following a consistent naming convention. To further improve readability, consider adding a comment above this section to clearly indicate that these are UI component exports.
Here's a suggested change to improve readability:
export { ResolutionAdaptationStrategy } from "./enums/ResolutionAdaptationStrategy"; +// UI Components export { UICanvas } from "./UICanvas"; export { UIImage } from "./UIImage"; export { UIRenderer } from "./UIRenderer"; export { UITransform } from "./UITransform";
1-8: Excellent organization. Consider adding a file-level comment.The overall structure of this index file is well-organized, promoting easy imports for other parts of the application. The exports are logically grouped, and there's a clear separation between different types of exports.
To further enhance the file's clarity and maintainability, consider adding a file-level comment at the beginning to describe the purpose of this index file. Here's a suggested addition:
+/** + * UI Module Exports + * + * This file re-exports UI-related components, enums, and utilities + * to provide a centralized entry point for the UI module. + */ + export { UIGroup } from "./UIGroup"; export { CanvasRenderMode } from "./enums/CanvasRenderMode"; export { ResolutionAdaptationStrategy } from "./enums/ResolutionAdaptationStrategy"; export { UICanvas } from "./UICanvas"; export { UIImage } from "./UIImage"; export { UIRenderer } from "./UIRenderer"; export { UITransform } from "./UITransform";packages/core/src/input/pointer/PointerEventData.ts (2)
12-14: Consider resetting the position property in the dispose method.The
disposemethod correctly nullifiespointer,target, andcurrentTarget. However, it doesn't reset thepositionproperty. To ensure complete cleanup, consider resetting thepositionproperty as well.You could modify the
disposemethod like this:dispose() { this.pointer = this.target = this.currentTarget = null; this.position.set(0, 0, 0); // Or this._position = null; if using lazy initialization }This ensures all properties are reset when the object is disposed.
16-21: Implement or add TODO comments for empty methods.The
_postDispatch,_preDispatch, and_dispatchmethods are currently empty. While their presence suggests a well-structured event dispatching mechanism, the lack of implementation or explanatory comments may lead to confusion.Consider either implementing these methods or adding TODO comments to explain their intended functionality. For example:
// Dispatch logic to be executed after the event is handled protected _postDispatch() { // TODO: Implement post-dispatch logic } // Dispatch logic to be executed before the event is handled protected _preDispatch() { // TODO: Implement pre-dispatch logic } // Main dispatch logic private _dispatch() { // TODO: Implement main dispatch logic }This will provide clarity for future development and maintenance.
packages/core/src/ui/interface/IUIElement.ts (2)
8-18: Consider adding explicit type declarations for properties.While the property names are descriptive, adding explicit type declarations would improve code clarity and maintainability. For example:
depth: number; raycastEnable: boolean; raycastPadding: Vector4; _entity: Entity; _parents: Entity[]; _canvas: UICanvas; _indexInCanvas: number; _group: UIGroup; _indexInGroup: number;This change would make the interface more self-documenting and help prevent potential type-related issues.
20-22: LGTM: Method declarations are well-defined. Consider adding JSDoc comments.The method declarations are clear and type-safe. To further improve documentation, consider adding JSDoc comments for each method. For example:
/** * Performs a raycast check on the UI element. * @param ray - The ray to check against. * @param out - The result of the hit check. * @param distance - The maximum distance to check. * @returns True if the ray intersects with the UI element, false otherwise. */ _raycast(ray: Ray, out: UIHitResult, distance: number): boolean; /** * Handles modifications to the associated entity. * @param flag - Flags indicating the type of modification. */ _onEntityModify(flag: EntityModifyFlags): void;This addition would provide more context for developers using or maintaining this interface.
packages/core/src/input/pointer/emitter/UIHitResult.ts (3)
5-8: Documentation is clear and concise.The class documentation effectively explains the purpose of
UIHitResult. It's great that it mentions both raycast and sweep operations.Consider adding a brief example of how this class might be used to provide even more context for developers.
9-19: Class properties are well-defined and initialized.The
UIHitResultclass properties are comprehensive, covering essential information for hit results in a 3D UI context. The use of appropriate types, includingVector3for spatial properties, is commendable. It's also good practice to initialize all properties to prevent undefined errors.For the
componentproperty, consider using a union type that includesUIImageas well, since it's imported but not used. IfUIImageis not needed, you may want to remove it from the imports.
1-19: Overall, excellent implementation of theUIHitResultclass.The
UIHitResultclass is well-structured, properly documented, and follows good coding practices. It provides a solid foundation for handling hit results in a 3D UI context. The use of appropriate types and initializations for all properties helps prevent potential runtime errors.As this class seems to be part of a larger UI system within a 3D engine, ensure that it integrates well with other components of the system, such as event handling and rendering pipelines. Consider creating unit tests to verify the behavior of this class in various scenarios.
packages/core/src/input/pointer/emitter/PointerEventEmitter.ts (1)
44-44: Remove or document commented-out codeThe line
// data.position.copyFrom(pointer.position);is commented out. If this code is no longer needed, consider removing it to keep the codebase clean. If it's intended for future use or requires further implementation, please add a comment explaining why it's commented out to provide context for other developers.Apply this diff to remove the commented-out code:
- // data.position.copyFrom(pointer.position);packages/core/src/input/pointer/Pointer.ts (1)
53-54: Method name should be singularThe method
_addEmittersadds a single emitter to the_emittersarray. For clarity and consistency, consider renaming the method to_addEmitterto accurately reflect that it adds one emitter at a time.packages/core/src/ui/UIGroup.ts (1)
156-189: Simplify_registryToParentGroupmethod for better readabilityThe
_registryToParentGroupmethod is complex and may benefit from refactoring to enhance clarity and maintainability.Consider breaking the method into smaller helper functions or adding comments to explain the logic flow. This will make it easier for others to understand and maintain the code.
🧰 Tools
🪛 Biome
[error] 174-174: 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] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/ui/UIImage.ts (1)
160-160: Address the TODO commentThe code contains a TODO comment:
// @todo: This question needs to be raised rather than hidden.This indicates that there is an outstanding issue or consideration that needs attention. Please address this TODO by providing the necessary implementation or clarification.
Would you like assistance in resolving this TODO or opening a GitHub issue to track it?
packages/core/src/Engine.ts (3)
269-269: Translate the comment to English for consistencyThe comment at line 269 is currently in Chinese. To maintain consistency and ensure all team members can understand, please translate it to English.
Apply this diff:
-// 为 overlay 的 UI 准备的 +// Preparing for overlay UI
566-566: Improve readability by separating assignmentsConsider splitting the multiple assignments into separate lines to enhance readability and maintainability.
Apply this diff:
-(projectE[0] = 2 / canvas.width), (projectE[5] = 2 / canvas.height), (projectE[10] = 0); +projectE[0] = 2 / canvas.width; +projectE[5] = 2 / canvas.height; +projectE[10] = 0;
572-572: Improve readability by separating assignmentsSimilarly, splitting the assignments at line 572 into separate lines will improve clarity.
Apply this diff:
-(viewE[12] = -transform.position.x), (viewE[13] = -transform.position.y); +viewE[12] = -transform.position.x; +viewE[13] = -transform.position.y;packages/core/src/input/pointer/PointerManager.ts (1)
110-110: Use explicitifstatement for better readabilityUsing short-circuit evaluation with
&&for method invocation can reduce code clarity. It's recommended to use an explicitifstatement to improve readability and maintainability.Current code:
physicsInitialized && pointer._addEmitters(PointerPhysicsEventEmitter);Suggested change:
if (physicsInitialized) { pointer._addEmitters(PointerPhysicsEventEmitter); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- packages/core/src/Engine.ts (12 hunks)
- packages/core/src/Entity.ts (12 hunks)
- packages/core/src/Script.ts (4 hunks)
- packages/core/src/enums/ComponentType.ts (1 hunks)
- packages/core/src/input/index.ts (1 hunks)
- packages/core/src/input/pointer/Pointer.ts (2 hunks)
- packages/core/src/input/pointer/PointerCallbackType.ts (1 hunks)
- packages/core/src/input/pointer/PointerEventData.ts (1 hunks)
- packages/core/src/input/pointer/PointerManager.ts (6 hunks)
- packages/core/src/input/pointer/emitter/PointerEventEmitter.ts (1 hunks)
- packages/core/src/input/pointer/emitter/PointerPhysicsEventEmitter.ts (1 hunks)
- packages/core/src/input/pointer/emitter/PointerUIEventEmitter.ts (1 hunks)
- packages/core/src/input/pointer/emitter/UIHitResult.ts (1 hunks)
- packages/core/src/ui/UICanvas.ts (1 hunks)
- packages/core/src/ui/UIGroup.ts (1 hunks)
- packages/core/src/ui/UIImage.ts (1 hunks)
- packages/core/src/ui/UIRenderer.ts (1 hunks)
- packages/core/src/ui/UIUtil.ts (1 hunks)
- packages/core/src/ui/index.ts (1 hunks)
- packages/core/src/ui/interface/IUIElement.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/Entity.ts
[error] 621-621: 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] 628-628: 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)
packages/core/src/input/pointer/emitter/PointerPhysicsEventEmitter.ts
[error] 73-73: 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] 73-73: 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)
packages/core/src/input/pointer/emitter/PointerUIEventEmitter.ts
[error] 85-85: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 98-98: 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] 98-98: 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] 212-212: 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)
packages/core/src/ui/UICanvas.ts
[error] 237-237: 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] 351-351: 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)
packages/core/src/ui/UIGroup.ts
[error] 174-174: 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] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/core/src/ui/UIImage.ts
[error] 243-243: 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)
[error] 256-256: 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)
packages/core/src/ui/UIRenderer.ts
[error] 134-134: 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] 148-148: 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)
packages/core/src/ui/UIUtil.ts
[error] 7-114: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 40-40: 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] 59-59: 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 (25)
packages/core/src/input/index.ts (1)
4-6: LGTM! Verify impact on dependent modules.The addition of the
PointerEventDataexport is consistent with the existing structure and naming conventions. It's appropriately placed with other pointer-related exports.To ensure this change doesn't unexpectedly impact other parts of the codebase, please run the following script:
This script will help identify any existing usage of
PointerEventDatain the codebase, which might be affected by this new export. If there are any matches, please review them to ensure this change doesn't introduce any unintended side effects.✅ Verification successful
Please run the following updated script to verify the impact of adding
PointerEventData:This updated script removes the incorrect
--type typescriptflag and instead uses--globpatterns to target TypeScript files (.tsand.tsx). Please execute these scripts to gather the necessary information for verifying the impact of the newPointerEventDataexport.
Verified the impact of adding
PointerEventDataexport. All existing imports and usages remain consistent, and no unintended side effects were detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing usage of PointerEventData and potential impact of the new export. # Test 1: Check for existing imports of PointerEventData echo "Checking for existing imports of PointerEventData:" rg --type typescript 'import.*PointerEventData' -g '!**/input/index.ts' # Test 2: Check for usage of PointerEventData without explicit import echo "Checking for usage of PointerEventData without explicit import:" rg --type typescript '\bPointerEventData\b' -g '!**/input/**' # Test 3: Check for any TODOs or FIXMEs related to PointerEventData echo "Checking for TODOs or FIXMEs related to PointerEventData:" rg --type typescript '(TODO|FIXME).*PointerEventData'Length of output: 671
Script:
#!/bin/bash # Description: Check for existing usage of PointerEventData and potential impact of the new export. # Test 1: Check for existing imports of PointerEventData echo "Checking for existing imports of PointerEventData:" rg 'import.*PointerEventData' --glob '!**/input/index.ts' --glob '*.ts' --glob '*.tsx' # Test 2: Check for usage of PointerEventData without explicit import echo "Checking for usage of PointerEventData without explicit import:" rg '\bPointerEventData\b' --glob '!**/input/**' --glob '*.ts' --glob '*.tsx' # Test 3: Check for any TODOs or FIXMEs related to PointerEventData echo "Checking for TODOs or FIXMEs related to PointerEventData:" rg '(TODO|FIXME).*PointerEventData' --glob '*.ts' --glob '*.tsx'Length of output: 1960
packages/core/src/enums/ComponentType.ts (2)
1-3: Good use of@internaldecoratorThe
@internaldecorator is correctly used to indicate that this enum is intended for internal use within the package. This is a good practice for encapsulation and preventing external dependencies on internal structures.
5-14: Well-structured enum with appropriate naming and valuesThe enum members follow the correct PascalCase naming convention for TypeScript enums. The use of hexadecimal values that are powers of 2 is a good choice, as it allows for efficient bitwise operations. This can be particularly useful for flags or combining types in the future.
packages/core/src/ui/index.ts (1)
1-3: Verify the nature of UIGroup and consider reorganizing if necessary.The export of
UIGroupis grouped with enum exports, but it's not located in the "enums" subfolder likeCanvasRenderModeandResolutionAdaptationStrategy. IfUIGroupis indeed an enum, consider moving it to the "enums" subfolder for consistency. If it's not an enum, it might be more appropriate to group it with the other UI component exports.To verify the nature of UIGroup, please run the following script:
Based on the result, we can determine if any reorganization is necessary.
packages/core/src/input/pointer/PointerEventData.ts (1)
1-6: LGTM: Imports and class declaration are well-structured.The imports are appropriate for the class implementation, and implementing the
IPoolElementinterface suggests good performance considerations for object pooling in event handling.packages/core/src/ui/interface/IUIElement.ts (3)
1-6: LGTM: Imports are appropriate and well-organized.The imports cover all necessary types and components for the
IUIElementinterface. The use of relative paths and the external math package suggests a well-structured project.
7-7: LGTM: Interface declaration follows best practices.The
IUIElementinterface is properly declared and exported, following the common naming convention for interfaces.
1-22: Overall, the IUIElement interface is well-designed and provides a solid foundation for UI elements.The interface covers essential properties and methods for UI element management, including rendering, raycasting, and entity modification handling. The structure is clear and follows TypeScript conventions. The suggested improvements (explicit type declarations and JSDoc comments) would further enhance the code's clarity and maintainability, but the current implementation is already of good quality.
packages/core/src/input/pointer/emitter/UIHitResult.ts (1)
1-3: LGTM: Imports are appropriate and well-organized.The imports are relevant to the
UIHitResultclass being defined. The use of the@galacean/engine-mathpackage and relative imports shows good project structure and organization.packages/core/src/input/pointer/emitter/PointerEventEmitter.ts (1)
8-54: Well-structured abstract class for pointer event handlingThe
PointerEventEmitterclass is effectively designed, defining essential abstract methods for pointer event processing. The inclusion of the helper method_createEventDatapromotes code reuse and encapsulates event data creation efficiently. The code is clean, follows good TypeScript practices, and adheres to object-oriented principles.packages/core/src/input/pointer/Pointer.ts (5)
5-5: ImportingPointerEventEmitterThe import statement correctly brings in
PointerEventEmitter, which is necessary for emitter management within thePointerclass.
46-47: Constructor correctly initializes theidpropertyThe constructor properly assigns the
idparameter to the readonlyidproperty, ensuring eachPointerinstance has a unique identifier.
60-62: Method_resetOnFrameBegineffectively resets pointer stateThe
_resetOnFrameBeginmethod appropriately resets_frameEventsand clears related event arrays to prepare for the new frame, ensuring the pointer state is correctly managed between frames.
77-80: Addition ofEmitterTypeenum enhances emitter managementThe
EmitterTypeenum clearly defines emitter types (PhysicsandUI), which can improve type safety and readability when managing different emitter categories.
82-88:PointerEventTypeenum uses bitwise flags effectivelyThe
PointerEventTypeenum assigns bitwise values to event types, allowing for efficient combination and checking of multiple event states using bitwise operations.packages/core/src/ui/UIUtil.ts (1)
96-105: Verify the compatibility of component casting within the switch statement.In the switch case for
ComponentType.UIRenderer, the component is cast toUICanvas:_meetRootCanvas = (<UICanvas>component)._isRootCanvas;Please verify that a component of type
UIRenderercan be safely cast toUICanvas. Casting incompatible types may lead to runtime errors.Run the following script to check the component hierarchy and confirm the casting is valid:
packages/core/src/ui/UIRenderer.ts (3)
59-59: Consider omitting the explicit type annotationThe type
numberinprotected _alpha: number = 1;can be omitted since it can be inferred from the initial value.
165-166: Avoid redundant calls toray.intersectPlane(plane)The method
ray.intersectPlane(plane)is called twice. Store the result in a variable to prevent redundant computation.
170-170: Consider destructuring for cleaner codeTo enhance readability and reduce repetitive property access, consider destructuring
this.entity.transform.worldMatrix.packages/core/src/Script.ts (1)
231-257: Optimize repetitive conditional checks using destructuring.A previous review comment suggested optimizing this block of code by destructuring
thisandprototypeto reduce redundant references tothis.onPointerDown,this.onPointerUp, etc. This can make the code cleaner and more efficient.Consider applying the optimization as previously suggested:
const { _entity: entity, onPointerDown, onPointerUp, onPointerClick, onPointerEnter, onPointerExit, onPointerBeginDrag, onPointerDrag, onPointerEndDrag, onPointerDrop } = this; const { onPointerDown: protoOnPointerDown, onPointerUp: protoOnPointerUp, onPointerClick: protoOnPointerClick, onPointerEnter: protoOnPointerEnter, onPointerExit: protoOnPointerExit, onPointerBeginDrag: protoOnPointerBeginDrag, onPointerDrag: protoOnPointerDrag, onPointerEndDrag: protoOnPointerEndDrag, onPointerDrop: protoOnPointerDrop } = prototype; // Then use the destructured variables in your conditional checks if (onPointerDown !== protoOnPointerDown) { this._pointerOverrideFlag |= PointerCallbackType.onPointerDown; } // ... and so on for the other event methodspackages/core/src/input/pointer/emitter/PointerUIEventEmitter.ts (1)
212-212: 🛠️ Refactor suggestionAvoid multiple assignments in a single expression for clarity
Similar to the previous instance, assigning multiple variables in a single expression can affect code readability. Consider splitting the assignments into separate statements for better clarity.
Apply this diff to improve readability:
- this._enteredElement = this._pressedElement = this._draggedElement = null; + this._enteredElement = null; + this._pressedElement = null; + this._draggedElement = null;Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 212-212: 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)
packages/core/src/ui/UICanvas.ts (1)
303-303:⚠️ Potential issueRemove Redundant Method Call to
ray.intersectPlaneThe method
ray.intersectPlane(plane)is called twice consecutively. The first call doesn't affect any state or variables and can be removed to improve efficiency.Apply this diff to eliminate the redundant call:
- ray.intersectPlane(plane);Likely invalid or redundant comment.
packages/core/src/Entity.ts (2)
104-109: Encapsulation of thetransformproperty enhances maintainabilityChanging the
transformproperty to a private member with a public getter improves encapsulation and allows for better control over howTransformis accessed and modified.
272-283: Addition ofgetComponentsInParentmethod enhances component retrievalThe new
getComponentsInParentmethod provides a convenient way to retrieve components from parent entities, enhancing the flexibility of component management within the entity hierarchy.packages/core/src/input/pointer/PointerManager.ts (1)
250-256: Differentiate handling betweenpointerleaveandpointercanceleventsIn the
_updatePointerInfomethod, both"pointerleave"and"pointercancel"events setpointer.phasetoPointerPhase.Leave. Verify if this unified handling is appropriate, as"pointercancel"might require distinct processing.
| export enum PointerCallbackType { | ||
| None = 0x0, | ||
| onPointerDown = 0x1, | ||
| onPointerUp = 0x2, | ||
| onPointerClick = 0x4, | ||
| onPointerEnter = 0x8, | ||
| onPointerExit = 0x10, | ||
| onPointerBeginDrag = 0x20, | ||
| onPointerDrag = 0x40, | ||
| onPointerEndDrag = 0x80, | ||
| onPointerDrop = 0x100 | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
PointerCallbackType enum is missing the onPointerMove member.
This omission may lead to unhandled pointer move events, potentially causing incomplete functionality in areas where pointer movement is expected.
- Consider adding
onPointerMove = 0x200to thePointerCallbackTypeenum to ensure comprehensive event handling.
🔗 Analysis chain
LGTM! Well-structured enum for pointer callback types.
The PointerCallbackType enum is well-implemented, using hexadecimal values that follow a binary progression. This approach allows for efficient bitwise operations when combining multiple event types.
Consider adding JSDoc comments to describe the purpose of the enum and each member. This would improve code readability and maintainability. For example:
/**
* Enum representing various pointer callback types.
* Each type is assigned a unique bit flag for efficient combination and checking.
*/
export enum PointerCallbackType {
/** No pointer callback type */
None = 0x0,
/** Callback for pointer down event */
onPointerDown = 0x1,
// ... (add comments for other members)
}Is the enum complete, or are there any other pointer event types that should be included? For instance, onPointerMove is a common event that's not present in this enum. Let's check if it's defined elsewhere:
Consider removing the "on" prefix from the enum members, as it's somewhat redundant in an enum specifically for callback types. This would result in more concise names like PointerDown, PointerUp, etc. However, this decision should be consistent with the naming conventions used in the rest of the codebase. Let's check the naming conventions used in other similar enums:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for onPointerMove in TypeScript files
rg --type typescript 'onPointerMove'
Length of output: 71
Script:
#!/bin/bash
# Search for other enums with similar naming patterns
rg --type typescript 'enum.*on\w+' -g '!*PointerCallbackType.ts'
Length of output: 101
Script:
#!/bin/bash
# Search for onPointerMove in TypeScript files
rg --type ts 'onPointerMove'
# Search for other enums with similar naming patterns in TypeScript files, excluding PointerCallbackType.ts
rg --type ts 'enum.*on\w+' -g '!PointerCallbackType.ts'
Length of output: 6380
| export enum ComponentType { | ||
| Component = 0x1, | ||
| Script = 0x2, | ||
| Renderer = 0x4, | ||
| MeshRenderer = 0x8, | ||
| SkinnedMeshRenderer = 0x10, | ||
| SpriteRenderer = 0x20, | ||
| TextRenderer = 0x40, | ||
| UICanvas = 0x80, | ||
| UIRenderer = 0x100, | ||
| UIGroup = 0x200, | ||
| UIElement = UICanvas | UIRenderer | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider grouping related enum members
The enum covers a wide range of component types, which is good for extensibility. However, to improve clarity and maintainability, consider grouping related enum members. For example:
export enum ComponentType {
// Base components
Component = 0x1,
Script = 0x2,
// Renderers
Renderer = 0x4,
MeshRenderer = 0x8,
SkinnedMeshRenderer = 0x10,
SpriteRenderer = 0x20,
TextRenderer = 0x40,
// UI components
UICanvas = 0x80,
UIRenderer = 0x100,
UIGroup = 0x200,
UIElement = UICanvas | UIRenderer
}This grouping makes it easier to understand the relationships between different component types and may help with future additions to the enum.
| target: Entity; | ||
| currentTarget: Entity; | ||
| pointer: Pointer; | ||
| position: Vector3 = new Vector3(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider lazy initialization for the position property.
The position property is always initialized with a new Vector3 instance. If this property isn't used in every event, consider lazy initialization to optimize memory usage and reduce unnecessary object creation.
You could implement it like this:
private _position: Vector3 | null = null;
get position(): Vector3 {
if (!this._position) {
this._position = new Vector3();
}
return this._position;
}This way, the Vector3 instance is only created when it's first accessed.
| protected static _tempRay: Ray = new Ray(); | ||
| protected static _tempPoint: Vector2 = new Vector2(); | ||
| protected static _tempHitResult: HitResult = new HitResult(); |
There was a problem hiding this comment.
Potential concurrency issues with static mutable state
Using protected static variables _tempRay, _tempPoint, and _tempHitResult can lead to unintended side effects in concurrent scenarios due to shared state across all instances of the class. If multiple pointers are processed simultaneously, this shared state might result in data races or incorrect behavior. Consider making these instance variables to ensure thread safety.
Apply this diff to change the static variables to instance variables:
- protected static _tempRay: Ray = new Ray();
- protected static _tempPoint: Vector2 = new Vector2();
- protected static _tempHitResult: HitResult = new HitResult();
+ protected _tempRay: Ray = new Ray();
+ protected _tempPoint: Vector2 = new Vector2();
+ protected _tempHitResult: HitResult = new HitResult();📝 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.
| protected static _tempRay: Ray = new Ray(); | |
| protected static _tempPoint: Vector2 = new Vector2(); | |
| protected static _tempHitResult: HitResult = new HitResult(); | |
| protected _tempRay: Ray = new Ray(); | |
| protected _tempPoint: Vector2 = new Vector2(); | |
| protected _tempHitResult: HitResult = new HitResult(); |
| /** @internal */ | ||
| _frameEvents: PointerEventType = PointerEventType.None; | ||
| /** @internal */ | ||
| _emitters: PointerEventEmitter[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding access modifiers to internal properties
To enhance encapsulation and clarity, consider explicitly marking internal properties like _frameEvents and _emitters as private or protected. This makes their intended usage clearer and prevents unintended access from outside the class.
| @@ -415,8 +434,8 @@ export class Entity extends EngineObject { | |||
| activeChangeFlag && child._processInActive(activeChangeFlag); | |||
|
|
|||
| Entity._traverseSetOwnerScene(child, null); // Must after child._processInActive(). | |||
| children.length--; | |||
There was a problem hiding this comment.
Avoid modifying array length within a loop in clearChildren
Directly decrementing children.length inside the loop can lead to unexpected behavior, such as skipping elements or runtime errors. It's safer to avoid modifying the array while iterating over it.
Consider refactoring the loop to prevent side effects:
clearChildren(): void {
const children = this._children;
- for (let i = children.length - 1; i >= 0; i--) {
+ while (children.length > 0) {
+ const child = children[children.length - 1];
child._parent = null;
let activeChangeFlag = ActiveChangeFlag.None;
child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy);
child._isActiveInScene && (activeChangeFlag |= ActiveChangeFlag.Scene);
activeChangeFlag && child._processInActive(activeChangeFlag);
Entity._traverseSetOwnerScene(child, null); // Must after child._processInActive().
- children.length--;
+ children.pop();
}
}📝 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.
| children.length--; | |
| clearChildren(): void { | |
| const children = this._children; | |
| while (children.length > 0) { | |
| const child = children[children.length - 1]; | |
| child._parent = null; | |
| let activeChangeFlag = ActiveChangeFlag.None; | |
| child._isActiveInHierarchy && (activeChangeFlag |= ActiveChangeFlag.Hierarchy); | |
| child._isActiveInScene && (activeChangeFlag |= ActiveChangeFlag.Scene); | |
| activeChangeFlag && child._processInActive(activeChangeFlag); | |
| Entity._traverseSetOwnerScene(child, null); // Must after child._processInActive(). | |
| children.pop(); | |
| } | |
| } |
| const { width, height } = this._canvas; | ||
| const { clientWidth, clientHeight } = htmlCanvas; | ||
| const { left, top } = htmlCanvas.getBoundingClientRect(); | ||
| const widthDPR = width / clientWidth; | ||
| const heightDPR = height / clientHeight; | ||
|
|
||
| const physicsInitialized = engine._physicsInitialized; |
There was a problem hiding this comment.
Accessing internal property _physicsInitialized directly
The _physicsInitialized property is accessed directly from the Engine instance. If this property is intended to be private or protected, consider using a public getter method or refactoring to maintain proper encapsulation.
| const { _events: events, _emitters: emitters } = pointer; | ||
| emitters.forEach((emitter) => { | ||
| emitter._processRaycast(scenes, pointer); | ||
| }); |
There was a problem hiding this comment.
Ensure _emitters are initialized before usage
Before invoking methods on _emitters, confirm that the _emitters array is properly initialized to prevent potential undefined or null reference errors.
Suggested code:
if (emitters && emitters.length > 0) {
emitters.forEach((emitter) => {
emitter._processRaycast(scenes, pointer);
});
}| pointer._frameEvents |= PointerEventType.Move; | ||
| pointer.phase = PointerPhase.Move; | ||
| break; | ||
| case "pointerleave": | ||
| pointer._frameEvents |= PointerEventType.Leave; | ||
| pointer.phase = PointerPhase.Leave; | ||
| break; | ||
| case "pointercancel": | ||
| pointer._frameEvents |= PointerEventType.Cancel; | ||
| pointer.phase = PointerPhase.Leave; | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor pointer.phase assignment for consistency
The pointer.phase is assigned multiple times within different event cases. Consider refactoring the switch-case block to consolidate pointer.phase assignments, enhancing code clarity and reducing the risk of incorrect phase settings.
| pointer._addEmitters(PointerUIEventEmitter); | ||
| physicsInitialized && pointer._addEmitters(PointerPhysicsEventEmitter); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistency in emitter addition
Currently, PointerUIEventEmitter is added using an explicit method call, while PointerPhysicsEventEmitter is added using short-circuit evaluation. For consistency, both emitters should be added using the same pattern.
Suggested change:
pointer._addEmitters(PointerUIEventEmitter);
if (physicsInitialized) {
pointer._addEmitters(PointerPhysicsEventEmitter);
}
| import { Renderer } from "../../Renderer"; | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| export interface ISpriteAssembler { | ||
| resetData(renderer: Renderer, vertexCount?: number): void; | ||
| updatePositions?(renderer: Renderer): void; | ||
| updatePositions?( |
There was a problem hiding this comment.
This is my problem. I saw that the input parameters changed, and I thought the function accusation also changed.
Please revolve this comment.
| flipX: boolean = false, | ||
| flipY: boolean = false | ||
| ): void { | ||
| const { sprite } = renderer; | ||
| const { border } = sprite; |
There was a problem hiding this comment.
Regarding the destructuring style, it’s indeed a subjective choice that depends on individual coding style and readability. If the original style aligns with project guidelines or personal preference, then let’s keep it as it is.
| const subChunk = renderer._subChunk; | ||
| const { r, g, b, a } = renderer.color; | ||
| const finalAlpha = a * alpha; | ||
| const vertices = subChunk.chunk.vertices; |
There was a problem hiding this comment.
👌 Thanks for the insight! Since destructuring is a subjective choice, let’s keep the original approach as it aligns with your style.
| SimpleSpriteAssembler.updatePositions(this); | ||
| const { sprite } = this; | ||
| if (sprite) { | ||
| SimpleSpriteAssembler.updatePositions(this, this.width, this.height, sprite.pivot, this._flipX, this._flipY); |
There was a problem hiding this comment.
👌 Agreed. We can use this.xx here directly~
| const component = components[i]; | ||
| if (component.enabled && component._componentType & ComponentType.UIElement) { | ||
| (component as unknown as IUIElement).depth = depth++; | ||
| elements.push(component as unknown as IUIElement); |
There was a problem hiding this comment.
Can this type conversion be optimized? I feel like Guolei might have comments on it.
Summary by CodeRabbit
New Features
UIGroupandUIImageclasses for better organization and rendering of UI elements.Enhancements
Engineclass.getComponentsInParentmethod.Bug Fixes
Documentation