Refactor sprite mask system #2369
Conversation
WalkthroughThe recent changes introduce functionality for managing sprite masks with custom stencil operations in a WebGL environment. A new file, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
packages/core/src/2d/index.ts (1)
Potential Issues Due to Removal of
SpriteMaskLayerExportThe removal of the
SpriteMaskLayerexport frompackages/core/src/2d/index.tsmay impact several parts of the codebase that rely on it. The following files have been identified as usingSpriteMaskLayer:
examples/sprite-mask.tspackages/core/src/2d/sprite/SpriteMask.tspackages/core/src/Renderer.tstests/src/core/2d/text/TextRenderer.test.tstests/src/core/SpriteRenderer.test.tstests/src/core/SpriteMask.test.tsPlease review these files to ensure that they are updated to use an alternative import path or consider retaining the export if necessary.
Analysis chain
Line range hint
1-10: Verify the impact of removing theSpriteMaskLayerexport.The export of
SpriteMaskLayerfrom./enums/SpriteMaskLayerhas been removed. This change may impact dependent modules or components that relied on this export.Run the following script to verify the usage of
SpriteMaskLayerin the codebase:If there are any occurrences of
SpriteMaskLayerin the codebase, please ensure that they are updated to use an alternative approach or discuss with the team if the export needs to be retained.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `SpriteMaskLayer` in the codebase. # Test 1: Search for import statements. Expect: No import statements for `SpriteMaskLayer`. rg --type typescript $'import \{ SpriteMaskLayer \} from \'@oasis-engine/core\'' # Test 2: Search for usage of `SpriteMaskLayer`. Expect: No usage of `SpriteMaskLayer`. rg --type typescript $'SpriteMaskLayer'Length of output: 194
Script:
#!/bin/bash # Description: Verify the usage of `SpriteMaskLayer` in the codebase with corrected file type. # Test 1: Search for import statements. Expect: No import statements for `SpriteMaskLayer`. rg --type ts $'import \\{ SpriteMaskLayer \\} from \'@oasis-engine/core\'' # Test 2: Search for usage of `SpriteMaskLayer`. Expect: No usage of `SpriteMaskLayer`. rg --type ts $'SpriteMaskLayer'Length of output: 1853
packages/core/src/2d/sprite/SpriteMask.ts (1)
207-209: Potential Performance Implications of Not Culling Sprite MasksThe
_isCulledByCameramethod inSpriteMask.tsalways returnsfalse, meaning sprite masks will not be culled by the camera. This could lead to performance issues by rendering elements that are not visible, especially in complex scenes with many sprite masks. Consider verifying the impact on rendering efficiency and exploring optimization strategies if necessary.
- File:
packages/core/src/2d/sprite/SpriteMask.ts- Usage:
packages/core/src/RenderPipeline/BasicRenderPipeline.tsAnalysis chain
Verify the performance impact of not culling the sprite mask by the camera.
The updated
_isCulledByCameramethod always returnsfalse, which implies that the sprite mask will not be culled by the camera. This change might have performance implications that need to be verified.Run the following script to verify the performance impact:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the performance impact of not culling the sprite mask by the camera. # Test: Search for potential performance issues related to not culling the sprite mask. Expect: No significant performance issues. rg --type typescript -i -C 5 $'_isCulledByCamera'Length of output: 83
Script:
#!/bin/bash # Corrected script to search for the _isCulledByCamera method in TypeScript files. # Test: Search for potential performance issues related to not culling the sprite mask. Expect: No significant performance issues. rg -t ts -i -C 5 '_isCulledByCamera'Length of output: 2066
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
342-344: The change improves readability and maintainability by encapsulating the culling logic within the_isCulledByCameramethod. Looks good!To ensure the correctness of the culling logic after this refactor, consider adding a unit test for the
_isCulledByCameramethod that covers various scenarios (visible, culled, edge cases, etc.).A robust unit test will serve as a safety net to catch any potential regressions and verify that the culling behavior remains consistent with the previous implementation.
packages/core/src/Renderer.ts (1)
357-359: Approve the_isCulledByCameramethod with a minor refactor suggestion.The
_isCulledByCameramethod enhances the rendering logic by allowing for more granular control over which entities are rendered based on camera settings. This can potentially improve performance by culling entities that are not visible to the camera.Consider using the bitwise AND assignment operator (
&=) for conciseness:-return !(camera.cullingMask & this._entity.layer); +return (camera.cullingMask &= this._entity.layer) === 0;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- packages/core/src/2d/index.ts (1 hunks)
- packages/core/src/2d/sprite/SpriteMask.ts (3 hunks)
- packages/core/src/2d/sprite/SpriteRenderer.ts (4 hunks)
- packages/core/src/2d/text/TextRenderer.ts (5 hunks)
- packages/core/src/Engine.ts (7 hunks)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/MaskManager.ts (1 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (7 hunks)
- packages/core/src/Renderer.ts (2 hunks)
Additional comments not posted (14)
packages/core/src/RenderPipeline/MaskManager.ts (2)
57-57: LGTM!The code change is approved.
60-60: LGTM!The code change is approved.
packages/core/src/RenderPipeline/RenderQueue.ts (4)
15-15: LGTM!The code change is approved.
57-57: LGTM!The code change is approved.
76-77: LGTM!The code changes are approved.
Also applies to: 170-192
231-237: LGTM!The code changes are approved.
packages/core/src/2d/sprite/SpriteMask.ts (1)
2-2: LGTM!The import statement is necessary to use the
Cameraclass in theSpriteMaskclass.packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
172-172: Please provide more context on the purpose of settingscene._maskManager.preMaskLayerto0.It's unclear what the implications of this change are without more information about the
_maskManagerobject and how thepreMaskLayerproperty is used in the rendering pipeline.To better understand the purpose and impact of this change, please provide answers to the following questions:
- What is the role of the
_maskManagerobject in the rendering pipeline?- How is the
preMaskLayerproperty used by the_maskManagerand other parts of the rendering system?- What are the consequences of setting
preMaskLayerto0at this point in the rendering process?Providing this additional context will help ensure the change is appropriate and doesn't introduce any unintended side effects.
packages/core/src/2d/sprite/SpriteRenderer.ts (1)
335-335: LGTM!The code change simplifies the material assignment logic by directly using
_spriteDefaultMaterialinstead of relying on_maskInteractionto select from_spriteDefaultMaterials. This change reduces complexity and is unlikely to introduce any issues.packages/core/src/Renderer.ts (1)
4-4: LGTM!The import statement changes are approved:
- The
Cameraimport has been added, indicating its usage in this file.- The
SpriteMaskLayerimport has been relocated, possibly to improve clarity and maintainability.Also applies to: 10-10
packages/core/src/Engine.ts (2)
238-238: LGTM!The code changes are approved. The initialization of
_spriteDefaultMaterialhas been simplified by calling_createSpriteMaterialwithout any arguments, which aligns with the refactoring objective to streamline the sprite material creation process.
Line range hint
571-584: LGTM!The code changes are approved. The
_createSpriteMaterialmethod has been simplified by removing themaskInteractionparameter and the associated logic for handling different sprite mask interactions. The method now creates a default material with a fixed set of render states, which reduces the complexity of the sprite rendering behavior and aligns with the refactoring objective.packages/core/src/2d/text/TextRenderer.ts (2)
Line range hint
6-7: Verify that removing theDirtyFlag.MaskInteractionenumeration value does not cause any issues.Ensure that the removal of the
DirtyFlag.MaskInteractionenumeration value does not break any functionality or cause any compile-time errors.Run the following script to verify the usage of the
DirtyFlag.MaskInteractionenumeration value:Verification successful
Removal of
DirtyFlag.MaskInteractionis Safe: TheDirtyFlag.MaskInteractionenumeration value is not referenced anywhere in the codebase, indicating that its removal does not cause any issues or break functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `DirtyFlag.MaskInteraction` enumeration value. # Test: Search for references to the `DirtyFlag.MaskInteraction` enumeration value. # Expect: No results found. rg --type typescript 'DirtyFlag.MaskInteraction'Length of output: 83
Script:
#!/bin/bash # Description: Verify the usage of the `DirtyFlag.MaskInteraction` enumeration value. # Test: Search for references to the `DirtyFlag.MaskInteraction` enumeration value. # Expect: No results found. rg --type ts 'DirtyFlag.MaskInteraction'Length of output: 40
Line range hint
32-38: Verify the impact of removing the_updateStencilStatemethod.Ensure that removing the
_updateStencilStatemethod does not break any functionality or negatively impact the visual output.Run the following script to verify the usage of the
_updateStencilStatemethod:Verification successful
Removal of
_updateStencilStateMethod VerifiedThe
_updateStencilStatemethod has been removed from theTextRendererclass, and there are no references to it elsewhere in the codebase. This confirms that its removal does not impact the functionality or integrity of the code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `_updateStencilState` method. # Test: Search for references to the `_updateStencilState` method. # Expect: No results found. rg --type typescript '_updateStencilState'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of the `_updateStencilState` method. # Test: Search for references to the `_updateStencilState` method. # Expect: No results found. rg --type ts '_updateStencilState'Length of output: 34
|
The use of Mask in some special situations must be considered, such as Shadow. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/RenderPipeline/RenderQueue.ts (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/RenderQueue.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/shadow/CascadedShadowCasterPass.ts (2 hunks)
Additional comments not posted (3)
packages/core/src/shadow/CascadedShadowCasterPass.ts (3)
5-5: Review the import order change.The change in the import order of
RenderContextandContextRendererUpdateFlagis purely cosmetic and does not impact functionality.
158-158: Approve the addition but suggest further investigation into its implications.The addition of
scene._maskManager.preMaskLayer = 0is approved as it appears to be part of the refactoring effort to better manage mask layers. However, it's important to investigate further how this change interacts with shadow rendering and other visual elements, especially considering the concerns raised in the PR comments.Run the following script to investigate further into the implications of this change:
Verification successful
The addition of
scene._maskManager.preMaskLayer = 0is consistent with existing practices.The initialization of
preMaskLayerto0is a common pattern observed in the rendering pipeline, ensuring that mask states are reset before rendering operations. This addition aligns with the existing codebase practices and is unlikely to introduce any issues with shadow rendering or mask interactions.
BasicRenderPipeline.tsandMaskManager.tsalso resetpreMaskLayerto0, indicating a deliberate design choice.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Investigate the implications of setting `preMaskLayer` to `0` on mask and shadow interactions. # Test: Search for any related discussions or code comments about `preMaskLayer` and its usage. rg --type ts -A 5 $'preMaskLayer'Length of output: 3147
154-154: Approve the change in clear flags but verify its impact.The change from
CameraClearFlags.DepthtoCameraClearFlags.DepthStencilis approved as it seems to be a deliberate enhancement to include stencil buffer clearing. However, given the concerns raised in the PR comments about shadows, it's crucial to verify that this change does not adversely affect shadow rendering or other related functionalities.Run the following script to verify the impact on shadow rendering:
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/core/src/2d/sprite/SpriteMask.ts (1)
207-209: Potential performance impact due to_isCulledByCameraalways returningfalse.The
_isCulledByCameramethod inSpriteMask.tsis overridden to always returnfalse, which means sprite masks will never be culled by the camera. This could lead to performance issues, especially in scenes with many sprite masks or complex visibility conditions, as they will always be processed regardless of their visibility.
- File:
packages/core/src/2d/sprite/SpriteMask.ts- Usage:
packages/core/src/RenderPipeline/BasicRenderPipeline.tsConsider reviewing this change to ensure it does not negatively impact rendering performance.
Analysis chain
Potential impact on rendering performance due to
_isCulledByCamerachanges.The method now always returns
false, which means sprite masks will not be culled by the camera. This could potentially lead to performance issues in scenes with many sprite masks or complex visibility conditions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the `_isCulledByCamera` change on rendering performance. # Test: Search for performance metrics or logs related to rendering times before and after the change. rg --type typescript -A 5 $'performance metrics'Length of output: 84
Script:
#!/bin/bash # Description: Search for references to the `_isCulledByCamera` method to understand its usage and impact. # Correct file type for TypeScript is `ts`. rg '_isCulledByCamera' --type ts -A 5Length of output: 1189
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/core/src/2d/sprite/SpriteMask.ts (3 hunks)
- packages/core/src/Renderer.ts (2 hunks)
- packages/core/src/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/core/src/index.ts
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Renderer.ts
Additional comments not posted (1)
packages/core/src/2d/sprite/SpriteMask.ts (1)
2-2: Review the new import statement forCamera.The addition of the
Cameraimport suggests that camera-related functionality is now utilized within theSpriteMaskclass. This aligns with the changes in the_isCulledByCameramethod, enhancing the class's integration with camera logic.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
packages/core/src/RenderPipeline/RenderQueue.ts (1)
175-176: Improved render state applicationThe addition of
material.shaderDataandcustomStatesto the_applyStatescall enhances the flexibility of render state management. This change allows for more granular control over rendering, incorporating material-specific and custom states.To improve maintainability:
- Consider adding a comment explaining the purpose and impact of these new parameters.
- Update the documentation for the
_applyStatesmethod to reflect these changes.packages/core/src/shader/state/BlendState.ts (2)
175-178: Improve readability of custom state handlingThe new code block correctly handles the potential override of the color write mask from
customStates. However, the use of assignment within an expression on line 177 may reduce readability.Consider refactoring the code to separate the condition from the assignment for improved clarity:
if (customStates) { const colorWriteMaskState = customStates[RenderStateElementKey.BlendStateColorWriteMask0]; - colorWriteMaskState !== undefined && (colorWriteMask = <number>colorWriteMaskState); + if (colorWriteMaskState !== undefined) { + colorWriteMask = <number>colorWriteMaskState; + } }This change will make the code more explicit and easier to read without changing its functionality.
🧰 Tools
🪛 Biome
[error] 177-177: 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)
Line range hint
1-255: Summary: Positive enhancements toBlendStatewith minor improvement suggestionThe changes to the
BlendStateclass successfully introduce more flexibility in managing blend states, which aligns well with the PR objective of refactoring mask functionality. The addition of thecustomStatesparameter allows for dynamic overrides of blend state properties, which could be particularly useful in scenarios involving shadows or other complex rendering situations mentioned in the PR comments.Key improvements:
- Added support for custom state overrides through the new
customStatesparameter.- Updated both
_applyand_platformApplymethods consistently.- Maintained backward compatibility by making
customStatesoptional.While the changes are generally positive, there's a minor suggestion to improve readability in the custom state handling block.
To further address the PR objectives and comments:
- Consider adding documentation or comments explaining how these changes might affect the interaction between Mask and Shadows.
- If not already done, ensure that unit tests are updated or added to cover the new custom state override functionality.
- Consider creating examples or documentation that demonstrate how to use the new
customStatesparameter effectively, especially in scenarios involving shadows or other complex rendering situations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/core/src/BasicResources.ts (6 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (5 hunks)
- packages/core/src/shader/state/BlendState.ts (3 hunks)
- packages/core/src/shader/state/DepthState.ts (2 hunks)
- packages/core/src/shader/state/RasterState.ts (2 hunks)
- packages/core/src/shader/state/RenderState.ts (2 hunks)
- packages/core/src/shader/state/StencilState.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/shader/state/RenderState.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/BasicResources.ts
[warning] 36-36: packages/core/src/BasicResources.ts#L36
Added line #L36 was not covered by tests
[warning] 41-41: packages/core/src/BasicResources.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: packages/core/src/BasicResources.ts#L43
Added line #L43 was not covered by tests
[warning] 46-46: packages/core/src/BasicResources.ts#L46
Added line #L46 was not covered by tests
[warning] 48-48: packages/core/src/BasicResources.ts#L48
Added line #L48 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 53-53: packages/core/src/BasicResources.ts#L53
Added line #L53 was not covered by tests
[warning] 56-60: packages/core/src/BasicResources.ts#L56-L60
Added lines #L56 - L60 were not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 66-66: packages/core/src/BasicResources.ts#L66
Added line #L66 was not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 73-73: packages/core/src/BasicResources.ts#L73
Added line #L73 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 78-78: packages/core/src/BasicResources.ts#L78
Added line #L78 was not covered by tests
[warning] 80-80: packages/core/src/BasicResources.ts#L80
Added line #L80 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-96: packages/core/src/BasicResources.ts#L86-L96
Added lines #L86 - L96 were not covered by tests
[warning] 98-98: packages/core/src/BasicResources.ts#L98
Added line #L98 was not covered by tests
🪛 Biome
packages/core/src/RenderPipeline/RenderQueue.ts
[error] 39-39: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 87-87: 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/shader/state/BlendState.ts
[error] 177-177: 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/shader/state/DepthState.ts
[error] 83-83: 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/shader/state/RasterState.ts
[error] 68-68: 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/shader/state/StencilState.ts
[error] 181-181: 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] 183-183: 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] 185-185: 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] 187-187: 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] 189-189: 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] 191-191: 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] 193-193: 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] 195-195: 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] 197-197: 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] 199-199: 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] 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)
🔇 Additional comments (16)
packages/core/src/RenderPipeline/RenderQueue.ts (3)
1-11: Imports updated to support new functionalityThe new imports for
SpriteMaskInteraction,BasicResources, andStencilAccessalign with the changes in mask handling and rendering states. These additions support the refactoring of theRenderQueueclass.
60-67: Enhanced mask handling in render methodThe addition of
_maskManagerfrom thesceneobject and the introduction ofmaskInteractionimprove the mask handling capabilities of therendermethod. These changes align with the overall refactoring of the mask management system.
82-95: Verify the new mask interaction logicThe new mask interaction logic improves handling of different mask scenarios. However, the complexity of the conditional checks might affect readability and maintainability. Consider the following suggestions:
- Extract the mask interaction logic into a separate method for better readability.
- Add comments explaining the different mask interaction scenarios.
- Verify that all possible combinations of
maskInteractionandmaskTypeare correctly handled.To ensure the correctness of the mask interaction logic, please run the following script:
This script will help identify potential edge cases and ensure consistent usage of mask interactions and types across the codebase.
✅ Verification successful
Mask interaction logic verified successfully
The new mask interaction logic has been thoroughly reviewed and no issues were found. All combinations of
maskInteractionandmaskTypeare consistently handled across the codebase, ensuring improved readability and maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of mask interaction and mask types in the codebase. # Test: Search for usages of SpriteMaskInteraction and RenderQueueMaskType echo "Usages of SpriteMaskInteraction:" rg --type typescript "SpriteMaskInteraction\." -A 5 echo "\nUsages of RenderQueueMaskType:" rg --type typescript "RenderQueueMaskType\." -A 5 # Test: Check for potential edge cases in mask handling echo "\nPotential edge cases in mask handling:" rg --type typescript "(maskInteraction|maskType).*(None|No)" -A 5Length of output: 519
Script:
#!/bin/bash # Description: Verify the usage of mask interaction and mask types in the codebase. # Test: Search for usages of SpriteMaskInteraction and RenderQueueMaskType echo "Usages of SpriteMaskInteraction:" rg "SpriteMaskInteraction\." --glob "*.ts" --glob "*.tsx" -A 5 echo "\nUsages of RenderQueueMaskType:" rg "RenderQueueMaskType\." --glob "*.ts" --glob "*.tsx" -A 5 # Test: Check for potential edge cases in mask handling echo "\nPotential edge cases in mask handling:" rg "(maskInteraction|maskType).*(None|No)" --glob "*.ts" --glob "*.tsx" -A 5Length of output: 15906
🧰 Tools
🪛 Biome
[error] 87-87: 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/shader/state/BlendState.ts (3)
3-3: LGTM: New import added forRenderStateElementMapThe new import is correctly added and is necessary for the
customStatesparameter introduced in the updated methods.
152-157: LGTM: Updated_applymethod withcustomStatesparameterThe
_applymethod has been correctly updated to include the optionalcustomStatesparameter. This change enhances flexibility in state management while maintaining backward compatibility. ThecustomStatesparameter is correctly passed to the_platformApplymethod.
160-160: LGTM: Updated_platformApplymethod signatureThe
_platformApplymethod signature has been correctly updated to include the optionalcustomStatesparameter, maintaining consistency with the_applymethod and allowing for platform-specific handling of custom states.packages/core/src/BasicResources.ts (4)
1-3: LGTM: New imports and type alias enhance code clarity and functionality.The added imports and the new
RenderStateElementMaptype alias are well-structured and contribute to the enhanced functionality of theBasicResourcesclass. These changes improve code readability and type safety.Also applies to: 14-21, 263-263
113-115: LGTM: New default material properties enhance performance and convenience.The addition of
spriteDefaultMaterial,textDefaultMaterial, andspriteMaskDefaultMaterialproperties is a good optimization. These shared materials can improve performance by reducing the need to create new materials for each sprite or text element.
162-165: LGTM: Constructor updated to initialize new default materials.The constructor has been appropriately updated to initialize the new default material properties using the newly added private methods. This ensures that the materials are properly set up when a
BasicResourcesinstance is created.
30-99: LGTM: Comprehensive stencil state management, but needs test coverage.The new static properties and methods for managing stencil states are well-implemented and provide a structured approach to handling different mask interactions and types. However, the static analysis hints indicate a lack of test coverage for these new additions.
To ensure the reliability of these new features, please add unit tests covering the following scenarios:
- Different mask interactions in
getMaskInteractionRenderStates- Different mask types in
getMaskTypeRenderStates- Caching behavior of the render states
Here's a script to verify the current test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: packages/core/src/BasicResources.ts#L36
Added line #L36 was not covered by tests
[warning] 41-41: packages/core/src/BasicResources.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: packages/core/src/BasicResources.ts#L43
Added line #L43 was not covered by tests
[warning] 46-46: packages/core/src/BasicResources.ts#L46
Added line #L46 was not covered by tests
[warning] 48-48: packages/core/src/BasicResources.ts#L48
Added line #L48 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 53-53: packages/core/src/BasicResources.ts#L53
Added line #L53 was not covered by tests
[warning] 56-60: packages/core/src/BasicResources.ts#L56-L60
Added lines #L56 - L60 were not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 66-66: packages/core/src/BasicResources.ts#L66
Added line #L66 was not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 73-73: packages/core/src/BasicResources.ts#L73
Added line #L73 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 78-78: packages/core/src/BasicResources.ts#L78
Added line #L78 was not covered by tests
[warning] 80-80: packages/core/src/BasicResources.ts#L80
Added line #L80 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-96: packages/core/src/BasicResources.ts#L86-L96
Added lines #L86 - L96 were not covered by tests
[warning] 98-98: packages/core/src/BasicResources.ts#L98
Added line #L98 was not covered by testspackages/core/src/shader/state/StencilState.ts (2)
154-159: LGTM: Enhanced flexibility with custom statesThe addition of the
customStatesparameter to the_applymethod and its propagation to_platformApplyimproves the flexibility of the stencil state management. This change allows for dynamic overriding of stencil states, which can be useful in scenarios where temporary state modifications are needed.
233-237: LGTM: Proper state management with overridden valuesThe updates to
lastStateproperties with potentially overridden values fromcustomStatesare correct and important. This ensures that the stencil state remains consistent across render calls, even when custom states are applied. Good job on maintaining state integrity!packages/core/src/shader/state/DepthState.ts (2)
69-74: Ensure method signature changes are consistently applied across the codebase.The
_applymethod now includes an additional optional parametercustomStates. Verify that all overrides and calls to this method are updated to match the new signature to prevent any runtime errors.Use the following script to find all occurrences of the
_applymethod:#!/bin/bash # Description: Find all implementations and calls of the _apply method to ensure they match the new signature. # Test: Search for _apply method usage. Expect: All usages include the customStates parameter. rg --type ts '_apply\s*\('Review the results to ensure consistency in the method's usage throughout the codebase.
2-2: Verify the import path and existence ofRenderStateElementMap.Ensure that
RenderStateElementMapis correctly exported from"../../BasicResources"and that the import path is accurate.Run the following script to confirm the export:
✅ Verification successful
Import Path and
RenderStateElementMapExport VerifiedThe import path
"../../BasicResources"is correct, andRenderStateElementMapis properly exported as a type frompackages/core/src/BasicResources.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that RenderStateElementMap is exported from BasicResources.ts # Test: Search for the export statement. Expect: Export of RenderStateElementMap. rg --type ts 'export .*RenderStateElementMap' ../../BasicResources.tsLength of output: 790
Script:
#!/bin/bash # Search the entire codebase for the export of RenderStateElementMap rg --type ts 'export .*RenderStateElementMap'Length of output: 168
Script:
#!/bin/bash # Search for usage of RenderStateElementMap in DepthState.ts rg 'RenderStateElementMap' packages/core/src/shader/state/DepthState.tsLength of output: 292
packages/core/src/shader/state/RasterState.ts (2)
48-54: Function signature updates to_applymethod are appropriateThe addition of the optional
customStatesparameter enhances flexibility by allowing custom rendering states to be passed during the raster state application. The implementation is correct and aligns with the desired functionality.
57-62: Function signature updates to_platformApplymethod are appropriateThe updated method signature correctly incorporates the optional
customStatesparameter, enabling the handling of custom rendering states within the platform-specific application. This change is well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
packages/core/src/RenderPipeline/MaskManager.ts (4)
2-6: LGTM! Improved encapsulation and added new functionality.The changes to the imports and class properties look good. Making
_preMaskLayerand_allSpriteMasksprivate improves encapsulation. The newhasWrittenToStencilproperty adds useful state tracking.Consider adding a brief comment explaining the purpose of
hasWrittenToStencilfor better code documentation.Also applies to: 28-30
32-41: Improve readability inremoveSpriteMaskmethodThe changes to
addSpriteMasklook good. However, inremoveSpriteMask, the assignment in the expression can be improved for better readability.Consider refactoring the
removeSpriteMaskmethod as follows:removeSpriteMask(mask: SpriteMask): void { const replaced = this._allSpriteMasks.deleteByIndex(mask._maskIndex); if (replaced) { replaced._maskIndex = mask._maskIndex; } mask._maskIndex = -1; }This change makes the code more explicit and easier to understand.
🧰 Tools
🪛 Biome
[error] 39-39: 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)
Line range hint
123-157: LGTM! Improved_buildMaskRenderElementand newStencilAccessenumThe changes to the
_buildMaskRenderElementmethod align well with the new class structure, using the private properties effectively. The newStencilAccessenum provides clear definitions for stencil access types, which enhances code readability and type safety.Consider adding a brief comment above the
StencilAccessenum to explain its purpose and usage within theMaskManagerclass. This would improve code documentation and make it easier for other developers to understand the enum's role in the stencil management process.
Line range hint
1-157: Summary: Solid refactoring with room for improvementOverall, this refactoring of the
MaskManagerclass is well-implemented and aligns with the PR objectives. The changes improve encapsulation, add new functionality for mask management, and introduce better type safety with theStencilAccessenum.Key improvements:
- Enhanced encapsulation by making
_preMaskLayerand_allSpriteMasksprivate.- New methods
drawMask,clearMask, andcheckStencilAccessthat provide more granular control over mask operations.- Improved
destroymethod with proper memory management.Areas for further improvement:
- Add test coverage for new and modified methods, especially
drawMask,clearMask, andcheckStencilAccess.- Improve readability in some areas, such as the
removeSpriteMaskmethod.- Enhance documentation with comments explaining the purpose of new properties and enums.
Regarding the PR objectives and comments:
- The refactoring addresses the concern raised by
cptbtptpbcptdtptpabout the use of Mask in specific scenarios, particularly its interaction with Shadows. The newcheckStencilAccessmethod provides a way to verify stencil operations, which can help in managing these interactions.- The suggestions from GuoLei1990 have been partially addressed:
- The
checkStencilAccessmethod investigates the scope of stencil operations, but further work may be needed to fully address the override mechanism.- The potential conflicts during continuous rendering of Stencil between objects with and without Mask are not explicitly handled in this refactoring. This may require additional implementation or testing.
- The improvement of the override mechanism for better compatibility, particularly regarding how users set
StencilStateReferenceValue, is not directly addressed in this refactoring. This might be an area for future improvement.To fully address the PR objectives and comments:
- Consider adding a mechanism to handle potential conflicts between objects rendered with and without Mask during continuous Stencil rendering.
- Explore ways to improve the override mechanism for better compatibility with user-set
StencilStateReferenceValue.- Ensure thorough testing of the new mask management functionality, especially in scenarios involving shadows and different stencil operations.
🧰 Tools
🪛 Biome
[error] 25-25: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 39-39: 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)
🪛 GitHub Check: codecov/patch
[warning] 44-47: packages/core/src/RenderPipeline/MaskManager.ts#L44-L47
Added lines #L44 - L47 were not covered by tests
[warning] 49-49: packages/core/src/RenderPipeline/MaskManager.ts#L49
Added line #L49 was not covered by tests
[warning] 51-58: packages/core/src/RenderPipeline/MaskManager.ts#L51-L58
Added lines #L51 - L58 were not covered by tests
[warning] 65-66: packages/core/src/RenderPipeline/MaskManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 68-71: packages/core/src/RenderPipeline/MaskManager.ts#L68-L71
Added lines #L68 - L71 were not covered by tests
[warning] 75-78: packages/core/src/RenderPipeline/MaskManager.ts#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-81: packages/core/src/RenderPipeline/MaskManager.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 84-84: packages/core/src/RenderPipeline/MaskManager.ts#L84
Added line #L84 was not covered by tests
[warning] 93-93: packages/core/src/RenderPipeline/MaskManager.ts#L93
Added line #L93 was not covered by tests
[warning] 98-98: packages/core/src/RenderPipeline/MaskManager.ts#L98
Added line #L98 was not covered by testspackages/core/src/RenderPipeline/RenderQueue.ts (1)
82-95: Improve readability of mask handling logicThe new implementation for handling mask interactions and types is more explicit and separates concerns well. However, the condition on line 87 contains an assignment within an expression, which can be confusing and error-prone.
Consider separating the assignment from the condition for better readability:
const stencilAccess = maskManager.checkStencilAccess(material); if (stencilAccess & StencilAccess.Writable) { maskManager.hasWrittenToStencil = true; }This change will make the code more readable and reduce the risk of unintended behavior.
🧰 Tools
🪛 Biome
[error] 87-87: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/MaskManager.ts (3 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🧰 Additional context used
🪛 Biome
packages/core/src/RenderPipeline/MaskManager.ts
[error] 39-39: 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/RenderPipeline/RenderQueue.ts
[error] 39-39: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 87-87: 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)
🪛 GitHub Check: codecov/patch
packages/core/src/RenderPipeline/MaskManager.ts
[warning] 44-47: packages/core/src/RenderPipeline/MaskManager.ts#L44-L47
Added lines #L44 - L47 were not covered by tests
[warning] 49-49: packages/core/src/RenderPipeline/MaskManager.ts#L49
Added line #L49 was not covered by tests
[warning] 51-58: packages/core/src/RenderPipeline/MaskManager.ts#L51-L58
Added lines #L51 - L58 were not covered by tests
[warning] 65-66: packages/core/src/RenderPipeline/MaskManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 68-71: packages/core/src/RenderPipeline/MaskManager.ts#L68-L71
Added lines #L68 - L71 were not covered by tests
[warning] 75-78: packages/core/src/RenderPipeline/MaskManager.ts#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-81: packages/core/src/RenderPipeline/MaskManager.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 84-84: packages/core/src/RenderPipeline/MaskManager.ts#L84
Added line #L84 was not covered by tests
[warning] 93-93: packages/core/src/RenderPipeline/MaskManager.ts#L93
Added line #L93 was not covered by tests
[warning] 98-98: packages/core/src/RenderPipeline/MaskManager.ts#L98
Added line #L98 was not covered by tests
🔇 Additional comments (4)
packages/core/src/RenderPipeline/MaskManager.ts (4)
117-121: LGTM! ImproveddestroymethodThe changes to the
destroymethod look good. Using the private_allSpriteMasksproperty aligns with the earlier encapsulation improvements. The addition ofgarbageCollection()is a good practice for proper memory management.
61-86: 🛠️ Refactor suggestion
⚠️ Potential issueImprove
clearMaskmethod and add test coverageThe new
clearMaskmethod effectively handles different scenarios for mask clearing. However, there are a few areas that could be improved:
- Direct access to
_elementsofDisorderedArraymight violate encapsulation.- The use of logical AND for control flow could be improved for readability.
- The method lacks test coverage.
Consider the following improvements:
- Use a public method or iterator of
DisorderedArrayinstead of accessing_elementsdirectly.- Replace the logical AND control flow with an explicit if statement.
Here's a suggested refactor:
clearMask(context: RenderContext, pipelineStageTagValue: string): void { const preMaskLayer = this._preMaskLayer; if (preMaskLayer !== SpriteMaskLayer.Nothing) { if (this.hasWrittenToStencil) { const decrementMaskQueue = MaskManager.getMaskDecrementRenderQueue(); decrementMaskQueue.clear(); this._allSpriteMasks.forEach(mask => { if (mask.influenceLayers & preMaskLayer) { decrementMaskQueue.pushRenderElement(mask._renderElement); } }); const batcherManager = context.camera.engine._batcherManager; decrementMaskQueue.batch(batcherManager); batcherManager.uploadBuffer(); decrementMaskQueue.render(context, pipelineStageTagValue, RenderQueueMaskType.Decrement); } else { const engine = context.camera.engine; engine._hardwareRenderer.clearRenderTarget(engine, CameraClearFlags.Stencil, null); } this._preMaskLayer = SpriteMaskLayer.Nothing; } }Please add unit tests for the
clearMaskmethod to ensure its reliability. Consider testing different scenarios, including:
- Clearing with different pre-mask layers
- Both branches of the
hasWrittenToStencilcondition- Interaction with the decrement queue and batcher manager
Here's a script to verify the current test coverage:
#!/bin/bash # Description: Check test coverage for the clearMask method # Test: Search for test files related to MaskManager echo "Searching for MaskManager test files:" fd -e spec.ts -e test.ts MaskManager # Test: Look for clearMask method in test files echo "\nSearching for clearMask tests:" rg -i "test.*clearMask" -t tsThis script will help identify if there are any existing tests for the
clearMaskmethod and where new tests should be added.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 65-66: packages/core/src/RenderPipeline/MaskManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 68-71: packages/core/src/RenderPipeline/MaskManager.ts#L68-L71
Added lines #L68 - L71 were not covered by tests
[warning] 75-78: packages/core/src/RenderPipeline/MaskManager.ts#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-81: packages/core/src/RenderPipeline/MaskManager.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 84-84: packages/core/src/RenderPipeline/MaskManager.ts#L84
Added line #L84 was not covered by tests
88-115: LGTM! NewcheckStencilAccessmethod is well-implementedThe new
checkStencilAccessmethod provides a comprehensive check for stencil access based on material properties. The use of bitwise operations for flag manipulation is appropriate and efficient.However, the static analysis hints indicate that some parts of this method are not covered by tests. To ensure the reliability of this new functionality, please add unit tests for the
checkStencilAccessmethod. Consider testing different scenarios, including:
- Materials with different stencil state configurations
- Edge cases where only one of
ReadableorWritableaccess is set- Cases where
NoneandAllaccess should be returnedHere's a script to verify the current test coverage:
This script will help identify if there are any existing tests for the
checkStencilAccessmethod and where new tests should be added.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-93: packages/core/src/RenderPipeline/MaskManager.ts#L93
Added line #L93 was not covered by tests
[warning] 98-98: packages/core/src/RenderPipeline/MaskManager.ts#L98
Added line #L98 was not covered by tests
43-59: LGTM! NewdrawMaskmethod looks good, but needs test coverage.The new
drawMaskmethod effectively encapsulates the mask drawing logic and uses the batcher manager for optimized rendering. This is a good addition to the class.However, the static analysis hints indicate that this method is not covered by tests. To ensure the reliability of this new functionality, please add unit tests for the
drawMaskmethod. Consider testing different scenarios, including:
- Drawing with different mask layers
- Verifying the correct usage of increment and decrement queues
- Checking the interaction with the batcher manager
Here's a script to verify the current test coverage:
This script will help identify if there are any existing tests for the
drawMaskmethod and where new tests should be added.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-47: packages/core/src/RenderPipeline/MaskManager.ts#L44-L47
Added lines #L44 - L47 were not covered by tests
[warning] 49-49: packages/core/src/RenderPipeline/MaskManager.ts#L49
Added line #L49 was not covered by tests
[warning] 51-58: packages/core/src/RenderPipeline/MaskManager.ts#L51-L58
Added lines #L51 - L58 were not covered by tests
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
packages/core/src/RenderPipeline/enums/RenderQueueMaskType.ts (1)
1-4: Consider enhancing the documentation.While the current documentation provides a basic description, it would be beneficial to add more details about each enum member. This would help developers understand the specific use case for each mask type.
Consider expanding the documentation like this:
/** * @internal * Render queue mask type. * * - No: No masking operation. * - Increment: Increments the stencil value. * - Decrement: Decrements the stencil value. */examples/sprite-mask.ts (1)
Line range hint
1-164: Overall, these changes enhance code quality, but further considerations are needed.The modifications in this file improve code consistency, readability, and type safety, which aligns with the general objective of refactoring the mask functionality. However, they don't directly address the concerns raised in the PR comments regarding:
- The interaction between Mask and Shadows.
- The clear scope and rationality of the override mechanism for "producing" and "consuming" Stencil attributes.
- Potential conflicts during continuous rendering of Stencil between objects rendered with and without Mask.
- Improving the override mechanism for better compatibility with user-set
StencilStateReferenceValue.Consider the following next steps:
- Implement test cases that specifically examine the behavior of Mask with Shadows.
- Review and document the Stencil attribute override mechanism.
- Analyze and optimize the rendering process for objects with and without Mask.
- Evaluate the current override mechanism and consider improvements for better user control over Stencil states.
These steps will help ensure that the refactoring comprehensively addresses all aspects of the Mask functionality.
packages/core/src/RenderPipeline/MaskManager.ts (5)
27-30: LGTM: New properties added with appropriate initializationsThe new properties
hasWrittenStencil,_preMaskLayer, and_allSpriteMasksare well-initialized and consistent with the class's purpose. However, consider adding JSDoc comments to explain the purpose and usage of thehasWrittenStencilproperty, as it's public and its role may not be immediately clear to users of the class.Would you like me to generate a sample JSDoc comment for the
hasWrittenStencilproperty?
43-58: LGTM: Well-structureddrawMaskmethodThe new
drawMaskmethod effectively manages the rendering process for masks, properly utilizing thebatcherManagerand following a clear sequence of operations.Consider a minor optimization: You could move the
batcherManagerinitialization to the beginning of the method to avoid repeatingcontext.camera.engine._batcherManager:drawMask(context: RenderContext, pipelineStageTagValue: string, maskLayer: SpriteMaskLayer): void { + const batcherManager = context.camera.engine._batcherManager; const incrementMaskQueue = MaskManager.getMaskIncrementRenderQueue(); incrementMaskQueue.clear(); const decrementMaskQueue = MaskManager.getMaskDecrementRenderQueue(); decrementMaskQueue.clear(); this._buildMaskRenderElement(maskLayer, incrementMaskQueue, decrementMaskQueue); - const batcherManager = context.camera.engine._batcherManager; incrementMaskQueue.batch(batcherManager); batcherManager.uploadBuffer(); incrementMaskQueue.render(context, pipelineStageTagValue, RenderQueueMaskType.Increment); decrementMaskQueue.batch(batcherManager); batcherManager.uploadBuffer(); decrementMaskQueue.render(context, pipelineStageTagValue, RenderQueueMaskType.Decrement); }This change reduces redundancy and slightly improves readability.
60-85: LGTM: ComprehensiveclearMaskmethod with room for optimizationThe
clearMaskmethod effectively handles different scenarios for clearing masks based on the current pre-mask layer andhasWrittenStencilproperty. The use of bitwise operations for mask layers is efficient.Consider optimizing the loop that processes mask elements:
if (this.hasWrittenStencil) { const decrementMaskQueue = MaskManager.getMaskDecrementRenderQueue(); decrementMaskQueue.clear(); const masks = this._allSpriteMasks; const maskElements = masks._elements; - for (let i = 0, n = masks.length; i < n; i++) { - const mask = maskElements[i]; - mask.influenceLayers & preMaskLayer && decrementMaskQueue.pushRenderElement(mask._renderElement); - } + masks.forEach(mask => { + if (mask.influenceLayers & preMaskLayer) { + decrementMaskQueue.pushRenderElement(mask._renderElement); + } + }); const batcherManager = context.camera.engine._batcherManager; decrementMaskQueue.batch(batcherManager); batcherManager.uploadBuffer(); decrementMaskQueue.render(context, pipelineStageTagValue, RenderQueueMaskType.Decrement); } else { // ... (rest of the method) }This change improves readability and uses the built-in
forEachmethod, which can be more efficient in modern JavaScript engines.
87-103: LGTM: ComprehensiveisStencilWrittenmethod with potential for simplificationThe
isStencilWrittenmethod thoroughly checks various stencil operations to determine if stencil has been written. The logic is comprehensive and correct.Consider simplifying the logic for better readability:
isStencilWritten(material: Material): boolean { const stencilState = material.renderState.stencilState; const stencilOperation = StencilOperation.Keep; - if ( - stencilState.enabled && - stencilState.writeMask !== 0x00 && - (stencilState.passOperationFront !== stencilOperation || - stencilState.passOperationBack !== stencilOperation || - stencilState.failOperationFront !== stencilOperation || - stencilState.failOperationBack !== stencilOperation || - stencilState.zFailOperationFront !== stencilOperation || - stencilState.zFailOperationBack !== stencilOperation) - ) { - return true; - } - return false; + return ( + stencilState.enabled && + stencilState.writeMask !== 0x00 && + [ + stencilState.passOperationFront, + stencilState.passOperationBack, + stencilState.failOperationFront, + stencilState.failOperationBack, + stencilState.zFailOperationFront, + stencilState.zFailOperationBack + ].some(operation => operation !== stencilOperation) + ); }This change simplifies the logic, making it more concise and easier to read while maintaining the same functionality.
Line range hint
111-137: LGTM: Well-implemented_buildMaskRenderElementmethodThe changes to the
_buildMaskRenderElementmethod align well with the new mask management approach. The use of bitwise operations for mask layers is efficient, and the method effectively handles different scenarios for incrementing and decrementing mask queues.Consider a minor optimization to reduce redundant property access:
private _buildMaskRenderElement( curMaskLayer: SpriteMaskLayer, incrementMaskQueue: RenderQueue, decrementMaskQueue: RenderQueue ): void { const preMaskLayer = this._preMaskLayer; if (preMaskLayer !== curMaskLayer) { const masks = this._allSpriteMasks; const commonLayer = preMaskLayer & curMaskLayer; const reduceLayer = preMaskLayer & ~curMaskLayer; const maskElements = masks._elements; + const n = masks.length; - for (let i = 0, n = masks.length; i < n; i++) { + for (let i = 0; i < n; i++) { const mask = maskElements[i]; const influenceLayers = mask.influenceLayers; if (influenceLayers & commonLayer) { continue; } if (influenceLayers & curMaskLayer) { incrementMaskQueue.pushRenderElement(mask._renderElement); } else if (influenceLayers & reduceLayer) { decrementMaskQueue.pushRenderElement(mask._renderElement); } } this._preMaskLayer = curMaskLayer; } }This change slightly improves performance by avoiding repeated access to
masks.lengthin each iteration.🧰 Tools
🪛 Biome
[error] 24-24: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 39-39: 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/RenderPipeline/RenderQueue.ts (1)
170-171: LGTM: Enhanced render state application.The addition of
material.shaderDataandcustomStatesto the_applyStatesmethod improves the flexibility of render state application. This change allows for more granular control over the rendering process, potentially enhancing the handling of custom render states for different materials and mask interactions.Consider updating the documentation for the
_applyStatesmethod to reflect these new parameters and their impact on the rendering process.packages/core/src/shader/state/StencilState.ts (1)
Line range hint
164-202: Refactor assignments for improved readabilityThe logic for applying custom stencil states is correct and comprehensive. However, the current implementation using assignments within logical expressions can be less readable and has been flagged by static analysis tools. Consider refactoring these lines to use explicit
ifstatements to enhance clarity and maintainability.For example, replace:
enabledState !== undefined && (enabled = <boolean>enabledState);with:
if (enabledState !== undefined) { enabled = <boolean>enabledState; }Applying this change to all similar lines will improve code readability and address the static analysis warnings about assignments in expressions.
🧰 Tools
🪛 Biome
[error] 181-181: 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] 183-183: 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] 185-185: 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] 187-187: 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] 189-189: 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] 191-191: 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] 193-193: 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] 195-195: 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] 197-197: 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] 199-199: 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] 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- examples/sprite-mask.ts (3 hunks)
- packages/core/src/2d/sprite/SpriteMask.ts (4 hunks)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/MaskManager.ts (3 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (5 hunks)
- packages/core/src/RenderPipeline/enums/RenderQueueMaskType.ts (1 hunks)
- packages/core/src/shader/state/BlendState.ts (3 hunks)
- packages/core/src/shader/state/RasterState.ts (2 hunks)
- packages/core/src/shader/state/StencilState.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🧰 Additional context used
🪛 Biome
packages/core/src/RenderPipeline/MaskManager.ts
[error] 39-39: 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/RenderPipeline/RenderQueue.ts
[error] 83-83: 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/shader/state/BlendState.ts
[error] 177-177: 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/shader/state/RasterState.ts
[error] 68-68: 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/shader/state/StencilState.ts
[error] 181-181: 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] 183-183: 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] 185-185: 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] 187-187: 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] 189-189: 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] 191-191: 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] 193-193: 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] 195-195: 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] 197-197: 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] 199-199: 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] 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)
🔇 Additional comments (22)
packages/core/src/RenderPipeline/enums/RenderQueueMaskType.ts (2)
5-9: LGTM: Enum declaration looks good.The
RenderQueueMaskTypeenum is well-defined with clear and concise member names that seem to represent different mask operations. The use of TypeScript's enum is appropriate for this scenario.
1-4: Correct implementation of @internal decorator.The @internal decorator has been correctly added to the JSDoc comment as requested in the previous review. This appropriately marks the enum for internal use within the module.
packages/core/src/shader/state/RasterState.ts (3)
2-2: LGTM: Import statement added correctly.The new import for
RenderStateElementMapis necessary for thecustomStatesparameter and is placed appropriately with other imports.
48-54: LGTM:_applymethod updated correctly.The addition of the optional
customStatesparameter enhances flexibility while maintaining backward compatibility. The parameter is correctly passed to the_platformApplymethod, ensuring consistent behavior.
57-62: LGTM:_platformApplymethod signature updated correctly.The addition of the
customStatesparameter to the_platformApplymethod signature is consistent with the changes in the_applymethod. This allows for platform-specific implementations to handle custom render states.examples/sprite-mask.ts (2)
61-61: Excellent typo correction!The renaming of
maksSprite1tomaskSprite1improves code consistency and readability. This change aligns the variable name with the established naming convention in the file, enhancing overall code quality.
117-117: Great improvement in type safety!Changing the
influenceLayersparameter type fromnumbertoSpriteMaskLayerenhances type safety and code clarity. This modification explicitly indicates the expected values for mask layers, aligning with the usage ofSpriteMaskLayerenum elsewhere in the code.To ensure consistency across the codebase, please run the following script to verify all
addMaskfunction calls:This script will help identify any inconsistencies in
addMaskfunction calls that might need updating due to this type change.✅ Verification successful
Please try running the following script to verify all
addMaskfunction calls are using theSpriteMaskLayerenum:
Verification Successful!
All
addMaskfunction calls are using theSpriteMaskLayerenum, ensuring type safety and consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `addMask` function use `SpriteMaskLayer` enum. # Test: Search for `addMask` function calls. Expect: All calls use `SpriteMaskLayer` enum. rg --type typescript 'addMask\s*\([^)]*,' -A 1Length of output: 83
Script:
#!/bin/bash # Description: Verify all calls to `addMask` function use `SpriteMaskLayer` enum. # Test: Search for `addMask` function calls in TypeScript files. Expect: All calls use `SpriteMaskLayer` enum. rg 'addMask\s*\([^)]*,' --glob '*.ts' --glob '*.tsx' -A 1Length of output: 310
packages/core/src/RenderPipeline/MaskManager.ts (2)
2-8: LGTM: New imports align with added functionalitiesThe newly added imports are consistent with the changes made to the
MaskManagerclass, supporting the introduction of new methods and properties.
105-109: LGTM: Improved memory management indestroymethodThe changes to the
destroymethod enhance memory management by properly clearing the_allSpriteMasksarray and triggering garbage collection. This approach is consistent with theDisorderedArrayAPI and helps prevent memory leaks.packages/core/src/RenderPipeline/RenderQueue.ts (2)
2-2: LGTM: Import statements updated appropriately.The changes in import statements reflect the new dependencies and potential reorganization of the codebase. The separation of
RenderQueueMaskTypeimport enhances code readability.Also applies to: 4-4, 10-10
93-93: LGTM: Shader program handling remains consistent.The changes in the shader program handling section are minimal and maintain the existing logic. The code structure remains consistent with the previous version, ensuring that the rendering process continues to function as expected.
packages/core/src/shader/state/BlendState.ts (4)
3-3: LGTM: New import added forRenderStateElementMapThe new import is correctly added and is necessary for the
customStatesparameter introduced in the modified methods.
152-157: LGTM: Updated_applymethod signatureThe
_applymethod has been correctly updated to include the optionalcustomStatesparameter. This change enhances the flexibility of the blend state application by allowing custom states to be passed through to the_platformApplymethod.
160-160: LGTM: Updated_platformApplymethod signatureThe
_platformApplymethod signature has been correctly updated to include the optionalcustomStatesparameter, maintaining consistency with the_applymethod changes.
Line range hint
1-265: Summary of BlendState.ts changesThe modifications to the
BlendStateclass successfully introduce support for custom blend states, particularly for color write masks. These changes enhance the flexibility of the blend state application process, aligning well with the PR objectives of refactoring the mask implementation.Key improvements:
- Added support for custom states in
_applyand_platformApplymethods.- Introduced handling for custom color write mask states.
The changes are well-integrated into the existing code structure and maintain consistency with the class's overall functionality. With the suggested refactoring of the custom state handling block, these modifications will significantly improve the mask functionality while maintaining code clarity and readability.
packages/core/src/2d/sprite/SpriteMask.ts (3)
29-29: Improved type safety forinfluenceLayerspropertyThe type change from
numbertoSpriteMaskLayerfor theinfluenceLayersproperty enhances type safety. This ensures that only validSpriteMaskLayervalues can be assigned, reducing the potential for errors and improving code clarity.
Line range hint
1-365: Summary of SpriteMask.ts changesThe changes to this file appear to be part of a larger refactoring effort:
- Import paths have been updated, suggesting a project structure change.
- The
influenceLayersproperty now uses a more specific type, improving type safety.- Access to the default sprite mask material has been modified, indicating a change in resource management.
These changes seem to enhance the overall code quality and structure. However, it's crucial to ensure that these modifications are consistent across the entire codebase to prevent any potential issues.
180-180: Verify consistency of default material accessThe access path for the default sprite mask material has been changed from
_engine._spriteMaskDefaultMaterialto_engine._basicResources.spriteMaskDefaultMaterial. This change appears in both the constructor and the_rendermethod.
- Ensure that this change is consistent across the entire codebase where the default sprite mask material is accessed.
- Verify that
_engine._basicResources.spriteMaskDefaultMaterialis correctly initialized and accessible at the time these methods are called.- Consider the potential impact on performance if
_basicResourcesis a getter that performs additional operations.Run the following script to check for any remaining occurrences of the old material access pattern:
#!/bin/bash # Description: Check for any remaining occurrences of the old default material access # Test: Search for the old material access pattern rg --type typescript '_engine\._spriteMaskDefaultMaterial'Also applies to: 264-264
packages/core/src/shader/state/StencilState.ts (4)
2-2: LGTM: Import and method signature updatesThe addition of the
RenderStateElementMapimport and the update to the_applymethod signature to include thecustomStatesparameter are consistent with the implementation changes. These modifications provide the necessary type information and flexibility for custom stencil states.Also applies to: 154-158
162-162: LGTM: Updated_platformApplymethod signatureThe addition of the
customStatesparameter to the_platformApplymethod signature is consistent with the implementation changes and allows for custom stencil states to be applied at the platform level.
233-233: LGTM: Last state updatesThe updates to the last state values (
lastState.compareFunctionBack,lastState.referenceValue, andlastState.mask) are correct and ensure that the last state reflects the most recent changes, including those from custom states. These updates are consistent with the overall implementation and maintain the integrity of the stencil state tracking.Also applies to: 236-237
Line range hint
1-279: Summary: Effective implementation of custom stencil states with minor improvement suggestionsThe changes to the
StencilStateclass successfully implement the ability to apply custom stencil states. The newcustomStatesparameter in both_applyand_platformApplymethods, along with the logic to override default values, provides flexibility in stencil state management.While the implementation is functionally correct, consider the suggested refactoring of assignments within logical expressions to improve readability. Overall, these changes enhance the capabilities of the
StencilStateclass while maintaining its structural integrity.🧰 Tools
🪛 Biome
[error] 181-181: 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] 183-183: 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] 185-185: 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] 187-187: 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] 189-189: 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] 191-191: 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] 193-193: 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] 195-195: 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] 197-197: 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] 199-199: 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] 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)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/core/src/RenderPipeline/MaskManager.ts (2)
Line range hint
118-151: Approve changes todestroy, consider encapsulation in_buildMaskRenderElement.The changes to the
destroymethod improve encapsulation by using the private_allSpriteMasksproperty. This change is good and aligns with best practices.For the
_buildMaskRenderElementmethod:
The use of private properties
_preMaskLayerand_allSpriteMasksimproves encapsulation.However, directly accessing
_elementsofDisorderedArray(line 134) might violate the encapsulation of theDisorderedArrayclass. Consider using a public method or iterator provided byDisorderedArrayto access its elements, if available.🧰 Tools
🪛 Biome
[error] 25-25: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 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)
🪛 GitHub Check: codecov/patch
[warning] 45-48: packages/core/src/RenderPipeline/MaskManager.ts#L45-L48
Added lines #L45 - L48 were not covered by tests
[warning] 50-50: packages/core/src/RenderPipeline/MaskManager.ts#L50
Added line #L50 was not covered by tests
[warning] 52-58: packages/core/src/RenderPipeline/MaskManager.ts#L52-L58
Added lines #L52 - L58 were not covered by tests
[warning] 65-66: packages/core/src/RenderPipeline/MaskManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 68-70: packages/core/src/RenderPipeline/MaskManager.ts#L68-L70
Added lines #L68 - L70 were not covered by tests
[warning] 74-77: packages/core/src/RenderPipeline/MaskManager.ts#L74-L77
Added lines #L74 - L77 were not covered by tests
[warning] 79-80: packages/core/src/RenderPipeline/MaskManager.ts#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 83-83: packages/core/src/RenderPipeline/MaskManager.ts#L83
Added line #L83 was not covered by tests
[warning] 88-89: packages/core/src/RenderPipeline/MaskManager.ts#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 100-100: packages/core/src/RenderPipeline/MaskManager.ts#L100
Added line #L100 was not covered by tests
Line range hint
1-151: Summary of MaskManager changesThe refactoring of the
MaskManagerclass has significantly improved encapsulation and added new functionality for handling mask operations. The changes are generally well-implemented, but there are a few areas for improvement:
Test Coverage: Many of the new methods lack test coverage. Consider adding unit tests to ensure the reliability of the new functionality.
Documentation: Some of the more complex methods (e.g.,
isStencilWritten,isReadStencil) could benefit from additional comments explaining the logic.Readability: A few instances of complex expressions (e.g., in
removeSpriteMaskandclearMask) could be refactored for better readability.Encapsulation: While generally improved, there are still a few places where encapsulation could be further enhanced (e.g., accessing
_batcherManagerand_elements).Usage Clarity: The purpose and usage of the new
hasStencilWrittenproperty need clarification.Addressing these points will further improve the quality and maintainability of the code.
🧰 Tools
🪛 Biome
[error] 25-25: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 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)
🪛 GitHub Check: codecov/patch
[warning] 45-48: packages/core/src/RenderPipeline/MaskManager.ts#L45-L48
Added lines #L45 - L48 were not covered by tests
[warning] 50-50: packages/core/src/RenderPipeline/MaskManager.ts#L50
Added line #L50 was not covered by tests
[warning] 52-58: packages/core/src/RenderPipeline/MaskManager.ts#L52-L58
Added lines #L52 - L58 were not covered by tests
[warning] 65-66: packages/core/src/RenderPipeline/MaskManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 68-70: packages/core/src/RenderPipeline/MaskManager.ts#L68-L70
Added lines #L68 - L70 were not covered by tests
[warning] 74-77: packages/core/src/RenderPipeline/MaskManager.ts#L74-L77
Added lines #L74 - L77 were not covered by tests
[warning] 79-80: packages/core/src/RenderPipeline/MaskManager.ts#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 83-83: packages/core/src/RenderPipeline/MaskManager.ts#L83
Added line #L83 was not covered by tests
[warning] 88-89: packages/core/src/RenderPipeline/MaskManager.ts#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 100-100: packages/core/src/RenderPipeline/MaskManager.ts#L100
Added line #L100 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/MaskManager.ts (3 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
🧰 Additional context used
🪛 Biome
packages/core/src/RenderPipeline/MaskManager.ts
[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)
packages/core/src/RenderPipeline/RenderQueue.ts
[error] 85-85: 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)
🪛 GitHub Check: codecov/patch
packages/core/src/RenderPipeline/MaskManager.ts
[warning] 45-48: packages/core/src/RenderPipeline/MaskManager.ts#L45-L48
Added lines #L45 - L48 were not covered by tests
[warning] 50-50: packages/core/src/RenderPipeline/MaskManager.ts#L50
Added line #L50 was not covered by tests
[warning] 52-58: packages/core/src/RenderPipeline/MaskManager.ts#L52-L58
Added lines #L52 - L58 were not covered by tests
[warning] 65-66: packages/core/src/RenderPipeline/MaskManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 68-70: packages/core/src/RenderPipeline/MaskManager.ts#L68-L70
Added lines #L68 - L70 were not covered by tests
[warning] 74-77: packages/core/src/RenderPipeline/MaskManager.ts#L74-L77
Added lines #L74 - L77 were not covered by tests
[warning] 79-80: packages/core/src/RenderPipeline/MaskManager.ts#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 83-83: packages/core/src/RenderPipeline/MaskManager.ts#L83
Added line #L83 was not covered by tests
[warning] 88-89: packages/core/src/RenderPipeline/MaskManager.ts#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 100-100: packages/core/src/RenderPipeline/MaskManager.ts#L100
Added line #L100 was not covered by tests
🔇 Additional comments (6)
packages/core/src/RenderPipeline/MaskManager.ts (6)
2-7: LGTM: New imports are appropriate for the added functionality.The newly added imports for
CameraClearFlags,CompareFunction, andStencilOperationare necessary and correctly used in the new methods of theMaskManagerclass.
61-85: 🛠️ Refactor suggestionImprove readability and test coverage in
clearMaskmethod.The
clearMaskmethod seems to handle clearing masks correctly, but there are a few points to consider:
Readability: The use of
&&for control flow in the loop on line 71 could be improved. Consider using an explicitifstatement for better readability.Test Coverage: This method is not covered by tests according to the static analysis hints. Consider adding unit tests to ensure its functionality.
Encapsulation: Similar to the
drawMaskmethod, this method accesses private properties of the engine. Consider if there's a more appropriate way to access this functionality.Here's a suggestion for improving the readability of the loop:
- mask.influenceLayers & preMaskLayer && decrementMaskQueue.pushRenderElement(mask._renderElement); + if (mask.influenceLayers & preMaskLayer) { + decrementMaskQueue.pushRenderElement(mask._renderElement); + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 65-66: packages/core/src/RenderPipeline/MaskManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 68-70: packages/core/src/RenderPipeline/MaskManager.ts#L68-L70
Added lines #L68 - L70 were not covered by tests
[warning] 74-77: packages/core/src/RenderPipeline/MaskManager.ts#L74-L77
Added lines #L74 - L77 were not covered by tests
[warning] 79-80: packages/core/src/RenderPipeline/MaskManager.ts#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 83-83: packages/core/src/RenderPipeline/MaskManager.ts#L83
Added line #L83 was not covered by tests
44-59: 🛠️ Refactor suggestionConsider test coverage and encapsulation in
drawMaskmethod.The new
drawMaskmethod seems to handle the mask rendering process correctly. However, there are two points to consider:
Test Coverage: This method is not covered by tests according to the static analysis hints. Consider adding unit tests to ensure its functionality.
Encapsulation: The method uses
context.camera.engine._batcherManager, which accesses a private property of the engine. Consider if there's a more appropriate way to access this functionality that doesn't break encapsulation.To help verify the test coverage, you can run the following script:
#!/bin/bash # Search for test files related to MaskManager rg --type typescript 'MaskManager' test/🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-48: packages/core/src/RenderPipeline/MaskManager.ts#L45-L48
Added lines #L45 - L48 were not covered by tests
[warning] 50-50: packages/core/src/RenderPipeline/MaskManager.ts#L50
Added line #L50 was not covered by tests
[warning] 52-58: packages/core/src/RenderPipeline/MaskManager.ts#L52-L58
Added lines #L52 - L58 were not covered by tests
87-103: Add tests and improve documentation forisStencilWrittenmethod.The
isStencilWrittenmethod seems to correctly check if a stencil has been written based on the material's stencil state. However, there are a couple of points to consider:
Test Coverage: This method is not covered by tests according to the static analysis hints. Consider adding unit tests to ensure its functionality and to serve as documentation for the complex logic.
Documentation: The logic in this method is quite complex. Consider adding comments to explain the purpose of each condition, especially the bitwise operations and comparisons with
StencilOperation.Keep.To help verify the test coverage, you can run the following script:
#!/bin/bash # Search for test files related to stencil operations rg --type typescript 'isStencilWritten|StencilOperation' test/🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-89: packages/core/src/RenderPipeline/MaskManager.ts#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 100-100: packages/core/src/RenderPipeline/MaskManager.ts#L100
Added line #L100 was not covered by tests
105-116: Add tests and improve documentation forisReadStencilmethod.Similar to the
isStencilWrittenmethod, theisReadStencilmethod seems to correctly check if a stencil is being read based on the material's stencil state. However, the same considerations apply:
Test Coverage: This method is not covered by tests according to the static analysis hints. Consider adding unit tests to ensure its functionality and to serve as documentation for the logic.
Documentation: The logic in this method, while simpler than
isStencilWritten, could still benefit from comments explaining the purpose of each condition, especially the comparisons withCompareFunction.AlwaysandCompareFunction.Never.To help verify the test coverage, you can run the following script:
#!/bin/bash # Search for test files related to stencil operations rg --type typescript 'isReadStencil|CompareFunction' test/
28-28: Clarify usage ofhasStencilWrittenproperty.The new
hasStencilWrittenproperty has been added, but it's not clear where and when this property is updated. Could you please provide more context on how this property is used and where it's being set totrue?To help verify the usage of this property, you can run the following script:
✅ Verification successful
Usage of
hasStencilWrittenproperty verified.The
hasStencilWrittenproperty is properly set and utilized within the codebase:
- Set to
trueinRenderQueue.ts- Reset to
falseinBasicRenderPipeline.ts- Checked in the
clearMaskmethod ofMaskManager.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for occurrences of hasStencilWritten being set or used rg --type typescript 'hasStencilWritten' -C 3Length of output: 180
Script:
#!/bin/bash # Corrected script to search for occurrences of hasStencilWritten being set or used in TypeScript files rg -g "*.ts" -g "*.tsx" 'hasStencilWritten' -C 3Length of output: 2549
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
packages/core/src/RenderPipeline/RenderQueue.ts (1)
2-2: Consider using a more specific import forRenderStateElementMap.The import statement has been updated to include
RenderStateElementMap. This is good for explicitly importing the types you need. However, consider if it would be beneficial to importRenderStateElementMapseparately for better clarity and potential tree-shaking.import { BasicResources } from "../BasicResources"; import { RenderStateElementMap } from "../BasicResources";This change could improve code readability and potentially help with module bundling optimizations.
packages/core/src/BasicResources.ts (3)
30-33: Consider adding documentation for new static properties.While the new static properties for stencil states are a good addition for caching and potentially improving performance, their purpose and usage might not be immediately clear to other developers. Consider adding JSDoc comments to explain the role of these properties in managing stencil states.
115-118: Add documentation for new default material properties.The new instance properties
spriteDefaultMaterial,textDefaultMaterial, andspriteMaskDefaultMaterialare good additions for managing default materials. However, to improve code clarity and maintainability, consider adding JSDoc comments to explain the purpose and usage of these properties. For example:/** * The default material used for rendering sprites. * This material is configured with appropriate blend states and render queue settings for sprite rendering. */ readonly spriteDefaultMaterial: Material; /** * The default material used for rendering text. * This material is configured with appropriate blend states and render queue settings for text rendering. */ readonly textDefaultMaterial: Material; /** * The default material used for sprite masking operations. * This material is configured with appropriate stencil states for mask rendering. */ readonly spriteMaskDefaultMaterial: Material;Also applies to: 165-167
Line range hint
1-265: Overall improvements to BasicResources, with some refinements needed.The changes to
BasicResourcessignificantly enhance its capabilities for stencil state management and 2D rendering. The addition of caching mechanisms for render states and default materials for sprites, text, and sprite masks are valuable improvements.To further enhance the code quality and maintainability, consider the following overall recommendations:
- Add comprehensive JSDoc comments for all new properties and methods.
- Implement error handling for edge cases in the new static methods.
- Refactor common code in
getMaskInteractionRenderStatesandgetMaskTypeRenderStatesto reduce duplication.- Complete the implementation of
_createSpriteMaskMaterialwith appropriate stencil state configurations.- Add unit tests to cover the new functionality, addressing the code coverage warnings from the static analysis.
These refinements will make the code more robust, easier to understand, and maintain in the long run.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: packages/core/src/BasicResources.ts#L36
Added line #L36 was not covered by tests
[warning] 41-41: packages/core/src/BasicResources.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: packages/core/src/BasicResources.ts#L43
Added line #L43 was not covered by tests
[warning] 46-46: packages/core/src/BasicResources.ts#L46
Added line #L46 was not covered by tests
[warning] 48-48: packages/core/src/BasicResources.ts#L48
Added line #L48 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 53-53: packages/core/src/BasicResources.ts#L53
Added line #L53 was not covered by tests
[warning] 56-60: packages/core/src/BasicResources.ts#L56-L60
Added lines #L56 - L60 were not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 66-66: packages/core/src/BasicResources.ts#L66
Added line #L66 was not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 73-73: packages/core/src/BasicResources.ts#L73
Added line #L73 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 78-78: packages/core/src/BasicResources.ts#L78
Added line #L78 was not covered by tests
[warning] 80-80: packages/core/src/BasicResources.ts#L80
Added line #L80 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-98: packages/core/src/BasicResources.ts#L86-L98
Added lines #L86 - L98 were not covered by tests
[warning] 100-100: packages/core/src/BasicResources.ts#L100
Added line #L100 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/src/BasicResources.ts (6 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/BasicResources.ts
[warning] 36-36: packages/core/src/BasicResources.ts#L36
Added line #L36 was not covered by tests
[warning] 41-41: packages/core/src/BasicResources.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: packages/core/src/BasicResources.ts#L43
Added line #L43 was not covered by tests
[warning] 46-46: packages/core/src/BasicResources.ts#L46
Added line #L46 was not covered by tests
[warning] 48-48: packages/core/src/BasicResources.ts#L48
Added line #L48 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 53-53: packages/core/src/BasicResources.ts#L53
Added line #L53 was not covered by tests
[warning] 56-60: packages/core/src/BasicResources.ts#L56-L60
Added lines #L56 - L60 were not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 66-66: packages/core/src/BasicResources.ts#L66
Added line #L66 was not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 73-73: packages/core/src/BasicResources.ts#L73
Added line #L73 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 78-78: packages/core/src/BasicResources.ts#L78
Added line #L78 was not covered by tests
[warning] 80-80: packages/core/src/BasicResources.ts#L80
Added line #L80 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-98: packages/core/src/BasicResources.ts#L86-L98
Added lines #L86 - L98 were not covered by tests
[warning] 100-100: packages/core/src/BasicResources.ts#L100
Added line #L100 was not covered by tests
🪛 Biome
packages/core/src/RenderPipeline/RenderQueue.ts
[error] 91-91: 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 (4)
packages/core/src/RenderPipeline/RenderQueue.ts (3)
38-39: LGTM: Newbatchmethod improves modularity.The addition of the
batchmethod is a good improvement. It separates the batching logic from sorting, enhancing modularity and potentially allowing for more flexible use of the class.
172-173: LGTM: Improved flexibility in render state application.The addition of
material.shaderDataandcustomStatesas parameters to the_applyStatesmethod provides more flexibility in applying render states. This change likely enhances the rendering pipeline's ability to handle various rendering scenarios more effectively.
Line range hint
1-186: Overall assessment: Good improvements with some areas for further refinement.The refactoring of the
RenderQueueclass has significantly improved its functionality, particularly in mask handling and rendering logic. The changes enhance modularity and provide more flexibility in the rendering pipeline. However, there are a few areas that could benefit from further refinement:
- Improve type safety in method signatures, especially for comparison functions.
- Enhance readability by avoiding assignments within expressions.
- Consider more specific imports for better code organization.
These minor adjustments would further improve the code quality and maintainability of this class.
🧰 Tools
🪛 Biome
[error] 91-91: 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/BasicResources.ts (1)
1-3: LGTM: New imports and type alias enhance code clarity and functionality.The added imports and the new
RenderStateElementMaptype alias are well-structured and support the new stencil state management features. This improves code readability and type safety.Also applies to: 14-21, 265-265
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
e2e/case/spriteMask-customStencil.ts (1)
11-11: Remove unused import 'Script'After removing the unused generic type parameter from the
addMaskfunction, theScriptimport is no longer needed. To keep the code clean, consider removing this unused import.Apply this diff to remove the unused import:
import { AssetType, Camera, CompareFunction, Layer, - Script, Sprite,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/SpriteMask_spriteMask-customStencil.jpgis excluded by!**/*.jpg
📒 Files selected for processing (3)
- e2e/case/spriteMask-customStencil.ts (1 hunks)
- e2e/config.ts (1 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
e2e/case/spriteMask-customStencil.ts
[warning] 20-21: e2e/case/spriteMask-customStencil.ts#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-25: e2e/case/spriteMask-customStencil.ts#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 28-28: e2e/case/spriteMask-customStencil.ts#L28
Added line #L28 was not covered by tests
[warning] 31-34: e2e/case/spriteMask-customStencil.ts#L31-L34
Added lines #L31 - L34 were not covered by tests
[warning] 37-39: e2e/case/spriteMask-customStencil.ts#L37-L39
Added lines #L37 - L39 were not covered by tests
[warning] 41-44: e2e/case/spriteMask-customStencil.ts#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 46-52: e2e/case/spriteMask-customStencil.ts#L46-L52
Added lines #L46 - L52 were not covered by tests
[warning] 55-56: e2e/case/spriteMask-customStencil.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 59-74: e2e/case/spriteMask-customStencil.ts#L59-L74
Added lines #L59 - L74 were not covered by tests
[warning] 77-93: e2e/case/spriteMask-customStencil.ts#L77-L93
Added lines #L77 - L93 were not covered by tests
[warning] 96-106: e2e/case/spriteMask-customStencil.ts#L96-L106
Added lines #L96 - L106 were not covered by tests
[warning] 109-110: e2e/case/spriteMask-customStencil.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 113-130: e2e/case/spriteMask-customStencil.ts#L113-L130
Added lines #L113 - L130 were not covered by tests
[warning] 132-134: e2e/case/spriteMask-customStencil.ts#L132-L134
Added lines #L132 - L134 were not covered by tests
[warning] 136-136: e2e/case/spriteMask-customStencil.ts#L136
Added line #L136 was not covered by tests
[warning] 141-153: e2e/case/spriteMask-customStencil.ts#L141-L153
Added lines #L141 - L153 were not covered by tests
[warning] 155-160: e2e/case/spriteMask-customStencil.ts#L155-L160
Added lines #L155 - L160 were not covered by tests
[warning] 162-163: e2e/case/spriteMask-customStencil.ts#L162-L163
Added lines #L162 - L163 were not covered by tests
[warning] 168-171: e2e/case/spriteMask-customStencil.ts#L168-L171
Added lines #L168 - L171 were not covered by tests
[warning] 174-178: e2e/case/spriteMask-customStencil.ts#L174-L178
Added lines #L174 - L178 were not covered by testse2e/config.ts
[warning] 216-222: e2e/config.ts#L216-L222
Added lines #L216 - L222 were not covered by tests
🪛 Biome
packages/core/src/RenderPipeline/RenderQueue.ts
[error] 92-92: 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 (1)
e2e/config.ts (1)
216-222: LGTM: New SpriteMask category added correctly.The addition of the
SpriteMaskcategory with theCustomStencilentry is well-structured and consistent with the existing configuration format. ThecaseFileName"spriteMask-customStencil" aligns with the new file mentioned in the PR summary, and the threshold value is in line with similar entries.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-222: e2e/config.ts#L216-L222
Added lines #L216 - L222 were not covered by tests
| SpriteMask: { | ||
| CustomStencil: { | ||
| category: "SpriteMask", | ||
| caseFileName: "spriteMask-customStencil", | ||
| threshold: 0.3 | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for the new SpriteMask configuration.
There are no existing tests covering the newly added E2E_CONFIG.SpriteMask.CustomStencil configuration. Please add appropriate tests to ensure reliability and correctness.
🔗 Analysis chain
Consider adding test coverage for the new configuration.
The static analysis tool indicates that the newly added lines are not covered by tests. To ensure the reliability and correctness of the configuration, it's recommended to add appropriate test coverage for these new lines.
To verify the current test coverage and identify areas that need additional tests, you can run the following command:
Would you like assistance in generating test cases for the new SpriteMask category in the E2E_CONFIG?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the E2E_CONFIG object
# Test: Search for tests related to E2E_CONFIG
rg -i "E2E_CONFIG" --type ts --glob "**/*test*.ts"
# Test: Check if there are any existing tests for SpriteMask category
rg -i "SpriteMask" --type ts --glob "**/*test*.ts"
Length of output: 10608
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-222: e2e/config.ts#L216-L222
Added lines #L216 - L222 were not covered by tests
| const scale = new Vector3(); | ||
|
|
There was a problem hiding this comment.
Avoid reusing 'pos' and 'scale' variables to prevent unintended side effects
The variables pos and scale are reused when setting the position and scale of multiple entities. Since transform.position and transform.scale assign the object references, subsequent modifications to pos and scale will unintentionally affect the previously assigned entities. This can lead to unexpected behavior in the scene.
Consider creating new Vector3 instances for each entity to avoid this issue. Apply the following changes:
In the main block after line 52:
const pos1 = new Vector3(0, 0, 0);
const scale1 = new Vector3(5, 5, 5);
// Use pos1 and scale1 for the first sprite rendererRepeat for each position and scale:
const pos2 = new Vector3(3, 3, 0);
const scale2 = new Vector3(5, 5, 5);
// Use pos2 and scale2 for the second sprite rendererIn the addSpriteRenderer function, modify lines 155-156:
- transform.position = pos;
- transform.scale = scale;
+ transform.position = pos.clone();
+ transform.scale = scale.clone();In the addMask function, modify line 174:
- entity.transform.position = pos;
+ entity.transform.position = pos.clone();Also applies to: 155-156, 174-174
| function addMask<T extends Script>(pos: Vector3, sprite: Sprite, layer: number, influenceLayers: number): void { | ||
| const entity = rootEntity.createChild(`Mask`); | ||
| entity.layer = layer; | ||
| const mask = entity.addComponent(SpriteMask); | ||
|
|
||
| // entity.addComponent(scriptType); | ||
| entity.transform.position = pos; | ||
| mask.sprite = sprite; | ||
| mask.influenceLayers = influenceLayers; | ||
| } |
There was a problem hiding this comment.
Remove unused generic type parameter 'T extends Script' from 'addMask' function
The generic type parameter T extends Script in the addMask function is not used within the function body. This may cause confusion and is unnecessary. Consider removing it to simplify the function signature.
Apply this diff to remove the unused generic type parameter:
- function addMask<T extends Script>(pos: Vector3, sprite: Sprite, layer: number, influenceLayers: number): void {
+ function addMask(pos: Vector3, sprite: Sprite, layer: number, influenceLayers: number): void {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function addMask<T extends Script>(pos: Vector3, sprite: Sprite, layer: number, influenceLayers: number): void { | |
| const entity = rootEntity.createChild(`Mask`); | |
| entity.layer = layer; | |
| const mask = entity.addComponent(SpriteMask); | |
| // entity.addComponent(scriptType); | |
| entity.transform.position = pos; | |
| mask.sprite = sprite; | |
| mask.influenceLayers = influenceLayers; | |
| } | |
| function addMask(pos: Vector3, sprite: Sprite, layer: number, influenceLayers: number): void { | |
| const entity = rootEntity.createChild(`Mask`); | |
| entity.layer = layer; | |
| const mask = entity.addComponent(SpriteMask); | |
| // entity.addComponent(scriptType); | |
| entity.transform.position = pos; | |
| mask.sprite = sprite; | |
| mask.influenceLayers = influenceLayers; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 168-171: e2e/case/spriteMask-customStencil.ts#L168-L171
Added lines #L168 - L171 were not covered by tests
| @@ -47,16 +52,15 @@ | |||
|
|
|||
| const { rendererUpdateFlag, camera } = context; | |||
| const { engine, scene, instanceId: cameraId, shaderData: cameraData } = camera; | |||
| const { instanceId: sceneId, shaderData: sceneData } = scene; | |||
| const { instanceId: sceneId, shaderData: sceneData, _maskManager: maskManager } = scene; | |||
There was a problem hiding this comment.
Avoid accessing private property _maskManager directly
The property _maskManager appears to be a private member of the Scene class (indicated by the underscore prefix). Accessing private properties directly can break encapsulation and lead to maintenance issues. Consider using a public getter or method to access maskManager instead.
| } else { | ||
| maskManager.isReadStencil(material) && maskManager.clearMask(context, pipelineStageTagValue); | ||
| } | ||
| maskManager.isStencilWritten(material) && (maskManager.hasStencilWritten = true); |
There was a problem hiding this comment.
Refactor assignment within expression for clarity
The assignment within an expression on line 92 can be confusing and may lead to maintenance issues. It's recommended to refactor the code for better readability.
Apply this diff to improve clarity:
- maskManager.isStencilWritten(material) && (maskManager.hasStencilWritten = true);
+ if (maskManager.isStencilWritten(material)) {
+ maskManager.hasStencilWritten = true;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| maskManager.isStencilWritten(material) && (maskManager.hasStencilWritten = true); | |
| if (maskManager.isStencilWritten(material)) { | |
| maskManager.hasStencilWritten = true; | |
| } |
🧰 Tools
🪛 Biome
[error] 92-92: 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)
Summary by CodeRabbit
New Features
SpriteMaskwith properties for custom stencil cases.Bug Fixes
Refactor
RenderQueueclass.