Refactor mask #2369
Refactor mask #2369
Conversation
WalkthroughThe recent changes enhance the 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/core/src/RenderPipeline/RenderQueue.ts (6 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/RenderPipeline/RenderQueue.ts
[error] 88-88: 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)
🔇 Additional comments (1)
packages/core/src/RenderPipeline/RenderQueue.ts (1)
16-19: Initialization of static stencil state propertiesThe addition of the static properties
_insideStencilStates,_outsideStencilStates,_incrementStencilStates, and_decrementStencilStatesprovides a structured approach to manage stencil states efficiently.
| private static _getMaskInteractionStencilStates( | ||
| maskInteraction: SpriteMaskInteraction | ||
| ): Record<number, number | boolean> | null { | ||
| if (maskInteraction === SpriteMaskInteraction.None) { | ||
| return null; | ||
| } | ||
|
|
||
| const stencilStates = | ||
| maskInteraction === SpriteMaskInteraction.VisibleInsideMask | ||
| ? RenderQueue._insideStencilStates | ||
| : RenderQueue._outsideStencilStates; | ||
| if (stencilStates[RenderStateElementKey.StencilStateEnabled] === undefined) { | ||
| stencilStates[RenderStateElementKey.StencilStateEnabled] = true; | ||
| stencilStates[RenderStateElementKey.StencilStateWriteMask] = 0x00; | ||
| stencilStates[RenderStateElementKey.StencilStateReferenceValue] = 1; | ||
| const compareFunction = | ||
| maskInteraction === SpriteMaskInteraction.VisibleInsideMask | ||
| ? CompareFunction.LessEqual | ||
| : CompareFunction.Greater; | ||
| stencilStates[RenderStateElementKey.StencilStateCompareFunctionFront] = compareFunction; | ||
| stencilStates[RenderStateElementKey.StencilStateCompareFunctionBack] = compareFunction; | ||
| } | ||
| return stencilStates; | ||
| } | ||
|
|
||
| private static _getMaskTypeStencilStates(maskType: RenderQueueMaskType): Record<number, number | boolean> | null { | ||
| if (maskType === RenderQueueMaskType.No) { | ||
| return null; | ||
| } | ||
|
|
||
| const stencilStates = | ||
| maskType === RenderQueueMaskType.Increment | ||
| ? RenderQueue._incrementStencilStates | ||
| : RenderQueue._decrementStencilStates; | ||
| const passOperation = | ||
| maskType === RenderQueueMaskType.Increment | ||
| ? StencilOperation.IncrementSaturate | ||
| : StencilOperation.DecrementSaturate; | ||
| if (stencilStates[RenderStateElementKey.StencilStatePassOperationFront] === undefined) { | ||
| stencilStates[RenderStateElementKey.StencilStatePassOperationFront] = passOperation; | ||
| stencilStates[RenderStateElementKey.StencilStatePassOperationBack] = passOperation; | ||
| } | ||
| return stencilStates; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication in stencil state initialization
The methods _getMaskInteractionStencilStates and _getMaskTypeStencilStates contain similar logic for initializing stencil states. Extracting the common initialization code into a shared helper method can enhance readability and maintainability.
Consider applying this refactor:
+ private static _initializeStencilStates(
+ stencilStates: Record<number, number | boolean>,
+ states: Record<number, number | boolean>
+ ): void {
+ for (const key in states) {
+ stencilStates[key] = states[key];
+ }
+ }
+
private static _getMaskInteractionStencilStates(
maskInteraction: SpriteMaskInteraction
): Record<number, number | boolean> | null {
if (maskInteraction === SpriteMaskInteraction.None) {
return null;
}
const stencilStates =
maskInteraction === SpriteMaskInteraction.VisibleInsideMask
? RenderQueue._insideStencilStates
: RenderQueue._outsideStencilStates;
if (stencilStates[RenderStateElementKey.StencilStateEnabled] === undefined) {
- stencilStates[RenderStateElementKey.StencilStateEnabled] = true;
- stencilStates[RenderStateElementKey.StencilStateWriteMask] = 0x00;
- stencilStates[RenderStateElementKey.StencilStateReferenceValue] = 1;
const compareFunction =
maskInteraction === SpriteMaskInteraction.VisibleInsideMask
? CompareFunction.LessEqual
: CompareFunction.Greater;
- stencilStates[RenderStateElementKey.StencilStateCompareFunctionFront] = compareFunction;
- stencilStates[RenderStateElementKey.StencilStateCompareFunctionBack] = compareFunction;
+ this._initializeStencilStates(stencilStates, {
+ [RenderStateElementKey.StencilStateEnabled]: true,
+ [RenderStateElementKey.StencilStateWriteMask]: 0x00,
+ [RenderStateElementKey.StencilStateReferenceValue]: 1,
+ [RenderStateElementKey.StencilStateCompareFunctionFront]: compareFunction,
+ [RenderStateElementKey.StencilStateCompareFunctionBack]: compareFunction
+ });
}
return stencilStates;
}
private static _getMaskTypeStencilStates(
maskType: RenderQueueMaskType
): Record<number, number | boolean> | null {
if (maskType === RenderQueueMaskType.No) {
return null;
}
const stencilStates =
maskType === RenderQueueMaskType.Increment
? RenderQueue._incrementStencilStates
: RenderQueue._decrementStencilStates;
const passOperation =
maskType === RenderQueueMaskType.Increment
? StencilOperation.IncrementSaturate
: StencilOperation.DecrementSaturate;
if (stencilStates[RenderStateElementKey.StencilStatePassOperationFront] === undefined) {
- stencilStates[RenderStateElementKey.StencilStatePassOperationFront] = passOperation;
- stencilStates[RenderStateElementKey.StencilStatePassOperationBack] = passOperation;
+ this._initializeStencilStates(stencilStates, {
+ [RenderStateElementKey.StencilStatePassOperationFront]: passOperation,
+ [RenderStateElementKey.StencilStatePassOperationBack]: passOperation
+ });
}
return stencilStates;
}📝 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.
| private static _getMaskInteractionStencilStates( | |
| maskInteraction: SpriteMaskInteraction | |
| ): Record<number, number | boolean> | null { | |
| if (maskInteraction === SpriteMaskInteraction.None) { | |
| return null; | |
| } | |
| const stencilStates = | |
| maskInteraction === SpriteMaskInteraction.VisibleInsideMask | |
| ? RenderQueue._insideStencilStates | |
| : RenderQueue._outsideStencilStates; | |
| if (stencilStates[RenderStateElementKey.StencilStateEnabled] === undefined) { | |
| stencilStates[RenderStateElementKey.StencilStateEnabled] = true; | |
| stencilStates[RenderStateElementKey.StencilStateWriteMask] = 0x00; | |
| stencilStates[RenderStateElementKey.StencilStateReferenceValue] = 1; | |
| const compareFunction = | |
| maskInteraction === SpriteMaskInteraction.VisibleInsideMask | |
| ? CompareFunction.LessEqual | |
| : CompareFunction.Greater; | |
| stencilStates[RenderStateElementKey.StencilStateCompareFunctionFront] = compareFunction; | |
| stencilStates[RenderStateElementKey.StencilStateCompareFunctionBack] = compareFunction; | |
| } | |
| return stencilStates; | |
| } | |
| private static _getMaskTypeStencilStates(maskType: RenderQueueMaskType): Record<number, number | boolean> | null { | |
| if (maskType === RenderQueueMaskType.No) { | |
| return null; | |
| } | |
| const stencilStates = | |
| maskType === RenderQueueMaskType.Increment | |
| ? RenderQueue._incrementStencilStates | |
| : RenderQueue._decrementStencilStates; | |
| const passOperation = | |
| maskType === RenderQueueMaskType.Increment | |
| ? StencilOperation.IncrementSaturate | |
| : StencilOperation.DecrementSaturate; | |
| if (stencilStates[RenderStateElementKey.StencilStatePassOperationFront] === undefined) { | |
| stencilStates[RenderStateElementKey.StencilStatePassOperationFront] = passOperation; | |
| stencilStates[RenderStateElementKey.StencilStatePassOperationBack] = passOperation; | |
| } | |
| return stencilStates; | |
| } | |
| private static _initializeStencilStates( | |
| stencilStates: Record<number, number | boolean>, | |
| states: Record<number, number | boolean> | |
| ): void { | |
| for (const key in states) { | |
| stencilStates[key] = states[key]; | |
| } | |
| } | |
| private static _getMaskInteractionStencilStates( | |
| maskInteraction: SpriteMaskInteraction | |
| ): Record<number, number | boolean> | null { | |
| if (maskInteraction === SpriteMaskInteraction.None) { | |
| return null; | |
| } | |
| const stencilStates = | |
| maskInteraction === SpriteMaskInteraction.VisibleInsideMask | |
| ? RenderQueue._insideStencilStates | |
| : RenderQueue._outsideStencilStates; | |
| if (stencilStates[RenderStateElementKey.StencilStateEnabled] === undefined) { | |
| const compareFunction = | |
| maskInteraction === SpriteMaskInteraction.VisibleInsideMask | |
| ? CompareFunction.LessEqual | |
| : CompareFunction.Greater; | |
| this._initializeStencilStates(stencilStates, { | |
| [RenderStateElementKey.StencilStateEnabled]: true, | |
| [RenderStateElementKey.StencilStateWriteMask]: 0x00, | |
| [RenderStateElementKey.StencilStateReferenceValue]: 1, | |
| [RenderStateElementKey.StencilStateCompareFunctionFront]: compareFunction, | |
| [RenderStateElementKey.StencilStateCompareFunctionBack]: compareFunction | |
| }); | |
| } | |
| return stencilStates; | |
| } | |
| private static _getMaskTypeStencilStates(maskType: RenderQueueMaskType): Record<number, number | boolean> | null { | |
| if (maskType === RenderQueueMaskType.No) { | |
| return null; | |
| } | |
| const stencilStates = | |
| maskType === RenderQueueMaskType.Increment | |
| ? RenderQueue._incrementStencilStates | |
| : RenderQueue._decrementStencilStates; | |
| const passOperation = | |
| maskType === RenderQueueMaskType.Increment | |
| ? StencilOperation.IncrementSaturate | |
| : StencilOperation.DecrementSaturate; | |
| if (stencilStates[RenderStateElementKey.StencilStatePassOperationFront] === undefined) { | |
| this._initializeStencilStates(stencilStates, { | |
| [RenderStateElementKey.StencilStatePassOperationFront]: passOperation, | |
| [RenderStateElementKey.StencilStatePassOperationBack]: passOperation | |
| }); | |
| } | |
| return stencilStates; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
packages/core/src/RenderPipeline/RenderQueue.ts (2)
3-3: LGTM! Consider adding JSDoc comments for the new static properties.The new imports and static properties for stencil states are well-structured and align with the added functionality for stencil and mask management. This approach of caching stencil states can potentially improve performance.
Consider adding JSDoc comments to the new static properties to explain their purpose and usage. For example:
/** Stencil states for inside mask rendering */ private static _insideStencilStates: Record<number, number | boolean> = {};Also applies to: 5-5, 9-9, 17-20
Line range hint
1-255: Overall approval with suggestions for further improvementsThe changes to the
RenderQueueclass successfully refactor the mask functionality and introduce new stencil state management, aligning well with the PR objectives. The separation of concerns in methods likesortandbatchimproves code structure and maintainability.Key improvements:
- Introduction of stencil state caching and management.
- Enhanced mask interaction handling in the
rendermethod.- Refactoring of sorting and batching operations.
Suggestions for further improvement:
- Add JSDoc comments to new static properties and methods for better documentation.
- Consider extracting common logic in stencil state initialization to reduce duplication.
- Refactor the
rendermethod to improve readability by extracting complex logic into separate methods.- Improve test coverage, especially for the new stencil state and mask interaction functionality.
- Address the
Functiontype issue in thesortmethod for better type safety.To further improve the code architecture, consider:
- Introducing a
StencilStateManagerclass to encapsulate the stencil state logic, which could help in managing the complexity of stencil operations across the rendering pipeline.- Implementing a strategy pattern for different mask types and interactions, which could make it easier to add or modify mask behaviors in the future.
These changes would enhance the modularity and extensibility of the rendering system, making it more robust for future developments.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-44: packages/core/src/RenderPipeline/RenderQueue.ts#L42-L44
Added lines #L42 - L44 were not covered by tests
[warning] 49-50: packages/core/src/RenderPipeline/RenderQueue.ts#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 52-52: packages/core/src/RenderPipeline/RenderQueue.ts#L52
Added line #L52 was not covered by tests
[warning] 69-75: packages/core/src/RenderPipeline/RenderQueue.ts#L69-L75
Added lines #L69 - L75 were not covered by tests
[warning] 77-77: packages/core/src/RenderPipeline/RenderQueue.ts#L77
Added line #L77 was not covered by testspackages/core/src/Scene.ts (2)
Line range hint
332-332: Consider initializing _stencilManager in the constructor.The
_stencilManagerproperty is not initialized in the constructor. To ensure it's always available when needed, consider initializing it in the constructor along with other managers.
Line range hint
1-568: Overall implementation looks good, but some concerns need addressing.The integration of
StencilManagerinto theSceneclass is well-implemented and consistent with existing patterns. However, there are a few points to consider:
The initialization of
_stencilManagerin the constructor should be added for consistency and to prevent potential issues.The PR objectives mentioned concerns about the interaction of Mask with Shadows and potential conflicts in continuous rendering of Stencil. These concerns are not directly addressed in the current changes. Consider adding comments or documentation explaining how these concerns are mitigated or if they will be addressed in future updates.
It might be beneficial to add some usage examples or documentation for the new stencil management functionality to help users understand how to utilize these new capabilities effectively.
To address the concerns raised in the PR objectives:
- Consider adding unit tests that specifically check the interaction between Mask and Shadows.
- Implement safeguards or checks in the
StencilManagerto prevent conflicts during continuous rendering of Stencil between objects rendered with and without Mask.- Review and possibly enhance the override mechanism for better compatibility, especially regarding how users set
StencilStateReferenceValue.Would you like assistance in implementing any of these suggestions?
packages/core/src/RenderPipeline/StencilManager.ts (2)
30-57: Add unit tests forcheckStencilAccessmethodThe
checkStencilAccessmethod is crucial for determining stencil access levels but is not covered by unit tests. Adding tests will help ensure it behaves correctly across different scenarios.Would you like assistance in creating unit tests for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 35-35: packages/core/src/RenderPipeline/StencilManager.ts#L35
Added line #L35 was not covered by tests
[warning] 40-40: packages/core/src/RenderPipeline/StencilManager.ts#L40
Added line #L40 was not covered by tests
[warning] 43-43: packages/core/src/RenderPipeline/StencilManager.ts#L43
Added line #L43 was not covered by tests
[warning] 52-52: packages/core/src/RenderPipeline/StencilManager.ts#L52
Added line #L52 was not covered by tests
69-94: Increase test coverage forresumeStencilmethodThe
resumeStencilmethod handles resuming stencil operations and the rendering of stencil write sub-elements. However, it lacks unit test coverage. Comprehensive tests can help verify its correctness, especially with various stencil configurations.Would you like assistance in developing unit tests for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 70-71: packages/core/src/RenderPipeline/StencilManager.ts#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 73-73: packages/core/src/RenderPipeline/StencilManager.ts#L73
Added line #L73 was not covered by tests
[warning] 76-78: packages/core/src/RenderPipeline/StencilManager.ts#L76-L78
Added lines #L76 - L78 were not covered by tests
[warning] 80-83: packages/core/src/RenderPipeline/StencilManager.ts#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 85-89: packages/core/src/RenderPipeline/StencilManager.ts#L85-L89
Added lines #L85 - L89 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (6 hunks)
- packages/core/src/RenderPipeline/StencilManager.ts (1 hunks)
- packages/core/src/Scene.ts (3 hunks)
- packages/core/src/shader/state/StencilState.ts (3 hunks)
- packages/core/src/shadow/CascadedShadowCasterPass.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/shadow/CascadedShadowCasterPass.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/RenderPipeline/RenderQueue.ts
[warning] 42-44: packages/core/src/RenderPipeline/RenderQueue.ts#L42-L44
Added lines #L42 - L44 were not covered by tests
[warning] 49-50: packages/core/src/RenderPipeline/RenderQueue.ts#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 52-52: packages/core/src/RenderPipeline/RenderQueue.ts#L52
Added line #L52 was not covered by tests
[warning] 69-75: packages/core/src/RenderPipeline/RenderQueue.ts#L69-L75
Added lines #L69 - L75 were not covered by tests
[warning] 77-77: packages/core/src/RenderPipeline/RenderQueue.ts#L77
Added line #L77 was not covered by tests
[warning] 146-146: packages/core/src/RenderPipeline/RenderQueue.ts#L146
Added line #L146 was not covered by tests
[warning] 152-152: packages/core/src/RenderPipeline/RenderQueue.ts#L152
Added line #L152 was not covered by testspackages/core/src/RenderPipeline/StencilManager.ts
[warning] 23-23: packages/core/src/RenderPipeline/StencilManager.ts#L23
Added line #L23 was not covered by tests
[warning] 35-35: packages/core/src/RenderPipeline/StencilManager.ts#L35
Added line #L35 was not covered by tests
[warning] 40-40: packages/core/src/RenderPipeline/StencilManager.ts#L40
Added line #L40 was not covered by tests
[warning] 43-43: packages/core/src/RenderPipeline/StencilManager.ts#L43
Added line #L43 was not covered by tests
[warning] 52-52: packages/core/src/RenderPipeline/StencilManager.ts#L52
Added line #L52 was not covered by tests
[warning] 60-61: packages/core/src/RenderPipeline/StencilManager.ts#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 65-66: packages/core/src/RenderPipeline/StencilManager.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 70-71: packages/core/src/RenderPipeline/StencilManager.ts#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 73-73: packages/core/src/RenderPipeline/StencilManager.ts#L73
Added line #L73 was not covered by tests
[warning] 76-78: packages/core/src/RenderPipeline/StencilManager.ts#L76-L78
Added lines #L76 - L78 were not covered by tests
[warning] 80-83: packages/core/src/RenderPipeline/StencilManager.ts#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 85-89: packages/core/src/RenderPipeline/StencilManager.ts#L85-L89
Added lines #L85 - L89 were not covered by tests
🪛 Biome
packages/core/src/RenderPipeline/RenderQueue.ts
[error] 94-94: 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)
packages/core/src/RenderPipeline/StencilManager.ts
[error] 14-14: 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] 184-184: 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] 186-186: 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] 188-188: 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] 190-190: 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] 192-192: 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] 194-194: 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] 196-196: 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] 198-198: 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] 200-200: 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] 202-202: 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] 204-204: 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 (9)
packages/core/src/RenderPipeline/RenderQueue.ts (2)
Line range hint
102-245: Approve changes torendermethod, suggest improvements, and remind about test coverage.The updates to the
rendermethod significantly enhance the handling of masks and stencils in the rendering pipeline. However, there are a few points to consider:
The increased complexity of the method might affect readability and maintainability. Consider extracting some of the new logic into separate private methods to improve clarity.
Some parts of the new code are not covered by tests according to the static analysis hints. It's crucial to add unit tests for the new functionality, especially for the mask interaction and stencil management logic.
The changes improve the handling of masks and stencils in the rendering pipeline, which aligns with the PR objectives.
Consider extracting the mask interaction and stencil management logic into separate private methods. For example:
private _handleMaskInteraction(context: RenderContext, pipelineStageTagValue: string, subElement: SubRenderElement): void { // ... mask interaction logic ... } private _handleStencilAccess(context: RenderContext, pipelineStageTagValue: string, subElement: SubRenderElement): void { // ... stencil access logic ... }This would make the
rendermethod more readable and easier to maintain.To verify the test coverage for the new functionality, you can run:
npm run test:coveragePlease ensure that the new mask interaction and stencil management logic is adequately covered by unit tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 146-146: packages/core/src/RenderPipeline/RenderQueue.ts#L146
Added line #L146 was not covered by tests
[warning] 152-152: packages/core/src/RenderPipeline/RenderQueue.ts#L152
Added line #L152 was not covered by tests
30-78: 🛠️ Refactor suggestionConsider refactoring stencil state initialization and improving test coverage.
The new static methods
_getMaskInteractionStencilStatesand_getMaskTypeStencilStatesare well-structured and handle different mask interactions and types appropriately. However, there are a few points to consider:
There's some code duplication in the initialization of stencil states. Consider extracting the common initialization logic into a separate private method to improve maintainability.
The static analysis hints indicate that these methods are not covered by tests. It's important to add unit tests to ensure the correct behavior of these methods.
Here's a suggestion to reduce duplication:
private static _initializeStencilStates(states: Record<number, number | boolean>, initialValues: Record<number, number | boolean>): void { Object.assign(states, initialValues); } // Use this method in both _getMaskInteractionStencilStates and _getMaskTypeStencilStatesTo verify the test coverage, you can run the following command:
This will generate a coverage report. Please ensure that these new methods are adequately covered by unit tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-44: packages/core/src/RenderPipeline/RenderQueue.ts#L42-L44
Added lines #L42 - L44 were not covered by tests
[warning] 49-50: packages/core/src/RenderPipeline/RenderQueue.ts#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 52-52: packages/core/src/RenderPipeline/RenderQueue.ts#L52
Added line #L52 was not covered by tests
[warning] 69-75: packages/core/src/RenderPipeline/RenderQueue.ts#L69-L75
Added lines #L69 - L75 were not covered by tests
[warning] 77-77: packages/core/src/RenderPipeline/RenderQueue.ts#L77
Added line #L77 was not covered by testspackages/core/src/RenderPipeline/BasicRenderPipeline.ts (3)
169-169: LGTM: Simplified engine referenceThe change from
camera.enginetoenginesimplifies the code without altering its functionality. This is a good refactoring step that improves code readability.
Line range hint
1-371: Summary of BasicRenderPipeline.ts changesThe changes in this file contribute positively to the refactoring of mask functionality:
- Simplified engine reference in the
rendermethod.- Added stencil reset and mask clearing in the
_drawRenderPassmethod.- Introduced
RenderQueueFlagsenum for better organization.These modifications align well with the PR objectives and address concerns raised in the comments. The suggested verification steps should be performed to ensure the effectiveness and consistency of these changes across the codebase.
Line range hint
365-371: Verify consistent usage of RenderQueueFlagsThe addition of the
RenderQueueFlagsenum is a good improvement for managing render queue flags. This should enhance code readability and maintainability.To ensure consistent usage of this new enum throughout the codebase, please run the following verification:
#!/bin/bash # Description: Check for consistent usage of RenderQueueFlags # Test 1: Search for usage of RenderQueueFlags echo "Searching for usage of RenderQueueFlags:" rg "RenderQueueFlags" -g "*.ts" # Test 2: Check for any remaining hardcoded render queue flag values echo "Checking for hardcoded render queue flag values:" rg -e "renderQueueFlags\s*=\s*\d+" -g "*.ts" # Test 3: Verify that RenderQueueFlags is imported where used echo "Verifying imports of RenderQueueFlags:" rg "import.*RenderQueueFlags" -g "*.ts"These tests will help ensure that the new
RenderQueueFlagsenum is used consistently across the codebase and that there are no remaining hardcoded flag values.packages/core/src/Scene.ts (3)
6-7: LGTM: New imports for enhanced functionality.The addition of
MaskManagerandStencilManagerimports suggests an enhancement in the scene's rendering capabilities, particularly in mask and stencil management.
61-61: LGTM: New StencilManager property added.The
_stencilManagerproperty is correctly declared as internal and initialized with a newStencilManagerinstance, aligning with the new import and enhancing the scene's stencil management capabilities.
518-518: LGTM: Proper cleanup of StencilManager.The addition of
this._stencilManager.destroy();in the_onDestroymethod ensures proper cleanup of stencil management resources when the Scene is destroyed, consistent with the handling of other managers.packages/core/src/shader/state/StencilState.ts (1)
161-165: Verify updates to_platformApplyinvocationsThe
_platformApplymethod signature has been updated to include the optionalcustomStatesparameter. Please ensure all internal calls to this method are updated to reflect this change.Use the following script to find all usages of
_platformApply:#!/bin/bash # Description: Find all calls to `_platformApply` in TypeScript files. # Test: Search for calls to `_platformApply`. Expect: Ensure the `customStates` parameter is correctly handled. rg --type typescript --pcre2 --multiline --context 5 '\b_platformApply\('
| sortBatch(compareFunc: Function, batcherManager: BatcherManager): void { | ||
| this._sort(compareFunc); | ||
| this._batch(batcherManager); | ||
| this.sort(compareFunc); | ||
| this.batch(batcherManager); | ||
| } | ||
|
|
||
| sort(compareFunc: Function): void { | ||
| Utils._quickSort(this.elements, 0, this.elements.length, compareFunc); | ||
| } | ||
|
|
||
| batch(batcherManager: BatcherManager): void { | ||
| batcherManager.batch(this); |
There was a problem hiding this comment.
Approve refactoring and suggest fixing the Function type.
The refactoring of sortBatch, sort, and batch methods improves code readability by separating concerns. However, there's an issue with the sort method that needs to be addressed:
The separation of sort and batch operations is a good practice and improves the flexibility of the code.
The sort method uses Function as a type for compareFunc, which is flagged as an error by the static analysis tool. This can lead to potential bugs and reduced type safety.
To fix this, please update the sort method signature as follows:
sort(compareFunc: (a: RenderElement, b: RenderElement) => number): void {
Utils._quickSort(this.elements, 0, this.elements.length, compareFunc);
}This change will provide better type safety and make the expected function signature explicit.
🧰 Tools
🪛 Biome
[error] 89-89: 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] 94-94: 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)
| // Reset stencil | ||
| scene._maskManager.clearMask(context, PipelineStage.Forward); | ||
| scene._stencilManager.clearStencil(); |
There was a problem hiding this comment.
💡 Codebase verification
Stencil state manipulations found outside of StencilManager
The verification process revealed several instances where stencil state operations are performed outside of the StencilManager. This breaks the centralized management of stencil states and could lead to inconsistent rendering behaviors.
Please refactor these instances to ensure all stencil operations are handled through StencilManager.
🔗 Analysis chain
Verify stencil reset and mask clearing
The addition of stencil reset and mask clearing after rendering the transparent queue is a good step towards addressing the concerns raised in the PR comments. This change should help manage stencil states more effectively.
To ensure this change resolves the issues mentioned in the PR comments, please run the following verification:
These tests will help ensure that the stencil and mask management is now centralized and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining stencil or mask-related issues after these changes
# Test 1: Search for any remaining stencil-related TODOs or FIXMEs
echo "Searching for stencil-related TODOs or FIXMEs:"
rg -i "TODO|FIXME" -g "*.ts" | rg -i "stencil"
# Test 2: Search for any remaining mask-related TODOs or FIXMEs
echo "Searching for mask-related TODOs or FIXMEs:"
rg -i "TODO|FIXME" -g "*.ts" | rg -i "mask"
# Test 3: Check for any direct stencil state manipulations outside of StencilManager
echo "Checking for direct stencil state manipulations:"
rg "stencil" -g "*.ts" | rg -v "StencilManager"
# Test 4: Verify that MaskManager is used consistently
echo "Verifying consistent use of MaskManager:"
rg "MaskManager" -g "*.ts"
Length of output: 17524
| for (let i = 0; i < stencilLen; ++i) { | ||
| const subElement = stencilWriteSubElements.get(i); | ||
| stencilRenderQueue.batchedSubElements.push(subElement); | ||
| subElement.material.renderState.blendState.targetBlendState.colorWriteMask = ColorWriteMask.None; | ||
| } | ||
| stencilRenderQueue.render(context, pipelineStageTagValue); | ||
| const colorWriteMasks = this._colorWriteMasks; | ||
| for (let i = 0; i < stencilLen; ++i) { | ||
| const subElement = stencilWriteSubElements.get(i); | ||
| subElement.material.renderState.blendState.targetBlendState.colorWriteMask = colorWriteMasks.get(i); | ||
| } |
There was a problem hiding this comment.
Avoid modifying shared material properties during rendering
Directly changing material properties like colorWriteMask on shared materials can cause unintended side effects if the material is used elsewhere in the application. This can lead to hard-to-debug rendering issues.
Consider creating a copy of the material or adjusting the render state in a way that doesn't affect shared materials.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-83: packages/core/src/RenderPipeline/StencilManager.ts#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 85-89: packages/core/src/RenderPipeline/StencilManager.ts#L85-L89
Added lines #L85 - L89 were not covered by tests
| private static _stencilRenderQueue: RenderQueue; | ||
|
|
||
| static getStencilRenderQueue(): RenderQueue { | ||
| return (StencilManager._stencilRenderQueue ||= new RenderQueue(RenderQueueType.Transparent)); |
There was a problem hiding this comment.
Avoid assignments within return statements for better readability
Assigning values within a return statement can reduce code clarity and make the code harder to maintain. It's recommended to separate the assignment from the return statement.
Apply this diff to improve code readability:
-static getStencilRenderQueue(): RenderQueue {
- return (StencilManager._stencilRenderQueue ||= new RenderQueue(RenderQueueType.Transparent));
+static getStencilRenderQueue(): RenderQueue {
+ if (!StencilManager._stencilRenderQueue) {
+ StencilManager._stencilRenderQueue = new RenderQueue(RenderQueueType.Transparent);
+ }
+ return StencilManager._stencilRenderQueue;
+}📝 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.
| return (StencilManager._stencilRenderQueue ||= new RenderQueue(RenderQueueType.Transparent)); | |
| static getStencilRenderQueue(): RenderQueue { | |
| if (!StencilManager._stencilRenderQueue) { | |
| StencilManager._stencilRenderQueue = new RenderQueue(RenderQueueType.Transparent); | |
| } | |
| return StencilManager._stencilRenderQueue; | |
| } |
🧰 Tools
🪛 Biome
[error] 14-14: 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)
| enabledState !== undefined && (enabled = <boolean>enabledState); | ||
| const writeMaskState = customStates[RenderStateElementKey.StencilStateWriteMask]; | ||
| writeMaskState !== undefined && (writeMask = <number>writeMaskState); | ||
| const referenceValueState = customStates[RenderStateElementKey.StencilStateReferenceValue]; | ||
| referenceValueState !== undefined && (referenceValue = <number>referenceValueState); | ||
| const compareFunctionFrontState = customStates[RenderStateElementKey.StencilStateCompareFunctionFront]; | ||
| compareFunctionFrontState !== undefined && (compareFunctionFront = <number>compareFunctionFrontState); | ||
| const compareFunctionBackState = customStates[RenderStateElementKey.StencilStateCompareFunctionBack]; | ||
| compareFunctionBackState !== undefined && (compareFunctionBack = <number>compareFunctionBackState); | ||
| const passOperationFrontState = customStates[RenderStateElementKey.StencilStatePassOperationFront]; | ||
| passOperationFrontState !== undefined && (passOperationFront = <number>passOperationFrontState); | ||
| const passOperationBackState = customStates[RenderStateElementKey.StencilStatePassOperationBack]; | ||
| passOperationBackState !== undefined && (passOperationBack = <number>passOperationBackState); | ||
| const failOperationFrontState = customStates[RenderStateElementKey.StencilStateFailOperationFront]; | ||
| failOperationFrontState !== undefined && (failOperationFront = <number>failOperationFrontState); | ||
| const failOperationBackState = customStates[RenderStateElementKey.StencilStateFailOperationBack]; | ||
| failOperationBackState !== undefined && (failOperationBack = <number>failOperationBackState); | ||
| const zFailOperationFrontState = customStates[RenderStateElementKey.StencilStateZFailOperationFront]; | ||
| zFailOperationFrontState !== undefined && (zFailOperationFront = <number>zFailOperationFrontState); | ||
| const zFailOperationBackState = customStates[RenderStateElementKey.StencilStateZFailOperationBack]; | ||
| zFailOperationBackState !== undefined && (zFailOperationBack = <number>zFailOperationBackState); |
There was a problem hiding this comment.
Refactor assignments within expressions for clarity
Lines 184-204 perform assignments within logical expressions using the && operator. This can make the code less readable and may lead to confusion.
Consider refactoring these lines to use explicit if statements 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 adhere to best practices.
🧰 Tools
🪛 Biome
[error] 184-184: 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] 186-186: 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] 188-188: 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] 190-190: 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] 192-192: 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] 194-194: 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] 196-196: 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] 198-198: 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] 200-200: 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] 202-202: 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] 204-204: 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/StencilManager.ts (2)
17-29: Approve private properties and getters with a suggestion for documentationThe use of
DisorderedArrayfor storing elements and the boolean flags for tracking stencil operation states is well-structured. The getter methods provide appropriate read-only access to the private flags.Consider adding JSDoc comments to explain the purpose of each private property and getter, especially the
DisorderedArrayusage and the meaning of the boolean flags. This will enhance code maintainability and make it easier for other developers to understand the class's internal state.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-24: packages/core/src/RenderPipeline/StencilManager.ts#L24
Added line #L24 was not covered by tests
103-119: Approve cleanup methods with a suggestion for optimizationThe
clearStencilanddestroymethods effectively clean up the stored data. However, there's a potential optimization:Consider optimizing the garbage collection calls in
clearStencil. Instead of callinggarbageCollection()on each array immediately after clearing it, you could defer this to a single call at the end of the method. This might be more efficient, especially if the JavaScript engine's garbage collector is already optimized for handling multiple empty arrays.Here's a suggested refactor:
clearStencil(): void { const arrays = [ this._stencilWriteSubElements, this._targetBlendStates, this._colorWriteMasks ]; arrays.forEach(arr => { arr.length = 0; }); // Single garbage collection call arrays.forEach(arr => arr.garbageCollection()); }This change maintains the same functionality while potentially improving performance by reducing the number of explicit garbage collection calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/core/src/RenderPipeline/StencilManager.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/RenderPipeline/StencilManager.ts
[error] 14-14: 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/StencilManager.ts
[warning] 24-24: packages/core/src/RenderPipeline/StencilManager.ts#L24
Added line #L24 was not covered by tests
[warning] 36-36: packages/core/src/RenderPipeline/StencilManager.ts#L36
Added line #L36 was not covered by tests
[warning] 41-41: packages/core/src/RenderPipeline/StencilManager.ts#L41
Added line #L41 was not covered by tests
[warning] 44-44: packages/core/src/RenderPipeline/StencilManager.ts#L44
Added line #L44 was not covered by tests
[warning] 53-53: packages/core/src/RenderPipeline/StencilManager.ts#L53
Added line #L53 was not covered by tests
[warning] 61-63: packages/core/src/RenderPipeline/StencilManager.ts#L61-L63
Added lines #L61 - L63 were not covered by tests
[warning] 67-68: packages/core/src/RenderPipeline/StencilManager.ts#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 72-73: packages/core/src/RenderPipeline/StencilManager.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-75: packages/core/src/RenderPipeline/StencilManager.ts#L75
Added line #L75 was not covered by tests
[warning] 78-80: packages/core/src/RenderPipeline/StencilManager.ts#L78-L80
Added lines #L78 - L80 were not covered by tests
[warning] 83-88: packages/core/src/RenderPipeline/StencilManager.ts#L83-L88
Added lines #L83 - L88 were not covered by tests
[warning] 91-91: packages/core/src/RenderPipeline/StencilManager.ts#L91
Added line #L91 was not covered by tests
🔇 Additional comments (3)
packages/core/src/RenderPipeline/StencilManager.ts (3)
122-127: Approve StencilAccess enumThe
StencilAccessenum is well-designed and uses appropriate bitwise values for easy combination of access levels. This approach allows for efficient checking and combination of different access types.The enum is clear, concise, and follows good practices for bitwise enums. No changes are necessary.
66-101: Approve stencil management methods with suggestions for improvementThe
suspendStencilandresumeStencilmethods work together to manage the stencil rendering process effectively. However, there are some areas for improvement:
The
resumeStencilmethod is quite complex and could benefit from further decomposition into smaller, more focused methods. This would improve readability and maintainability.Both methods lack test coverage, which is crucial for ensuring their reliability.
Consider refactoring
resumeStencilinto smaller methods, each handling a specific part of the process. For example:private prepareStencilRenderQueue(): void { // Logic for preparing the render queue } private closeColorWrite(): void { // Logic for closing color write } private reopenColorWrite(): void { // Logic for reopening color write } resumeStencil(context: RenderContext, pipelineStageTagValue: string): void { if (this._stencilWriteSubElements.length === 0) return; this._isResuming = true; this.prepareStencilRenderQueue(); this.closeColorWrite(); StencilManager.getStencilRenderQueue().render(context, pipelineStageTagValue); this.reopenColorWrite(); this._hasSuspendStencil = false; this._isResuming = false; }To address the lack of test coverage, please add unit tests for both methods. Here's a script to verify the current test coverage:
#!/bin/bash # Description: Check test coverage for suspendStencil and resumeStencil methods # Test: Search for test files related to StencilManager echo "Searching for StencilManager test files..." fd -e test.ts -e spec.ts StencilManager # Test: Check for tests specifically covering suspendStencil and resumeStencil methods echo "Checking for suspendStencil and resumeStencil tests..." rg -i "describe.*(suspendStencil|resumeStencil)" $(fd -e test.ts -e spec.ts StencilManager)If the script doesn't find any relevant test files or specific tests for these methods, consider adding them to improve code reliability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-68: packages/core/src/RenderPipeline/StencilManager.ts#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 72-73: packages/core/src/RenderPipeline/StencilManager.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-75: packages/core/src/RenderPipeline/StencilManager.ts#L75
Added line #L75 was not covered by tests
[warning] 78-80: packages/core/src/RenderPipeline/StencilManager.ts#L78-L80
Added lines #L78 - L80 were not covered by tests
[warning] 83-88: packages/core/src/RenderPipeline/StencilManager.ts#L83-L88
Added lines #L83 - L88 were not covered by tests
[warning] 91-91: packages/core/src/RenderPipeline/StencilManager.ts#L91
Added line #L91 was not covered by tests
1-127: Overall assessment of StencilManager implementationThe
StencilManagerclass provides a robust and well-structured system for managing stencil operations in the rendering pipeline. The implementation makes good use of TypeScript features and object-oriented principles.Key strengths:
- Clear separation of concerns with methods for different aspects of stencil management.
- Efficient use of data structures like
DisorderedArrayfor performance optimization.- Well-designed
StencilAccessenum for bitwise operations.Areas for improvement:
- Lack of test coverage for most methods, which is crucial for ensuring reliability.
- Some methods, particularly
resumeStencil, could benefit from further decomposition.- Minor optimizations possible in garbage collection and static property initialization.
To address the main concern of test coverage, please prioritize adding comprehensive unit tests for all public methods of the
StencilManagerclass. This will significantly improve the reliability and maintainability of this critical component in the rendering pipeline.Here's a script to verify the overall test coverage for the
StencilManagerclass:#!/bin/bash # Description: Check overall test coverage for StencilManager class # Test: Search for test files related to StencilManager echo "Searching for StencilManager test files..." fd -e test.ts -e spec.ts StencilManager # Test: Check for tests covering StencilManager methods echo "Checking for tests covering StencilManager methods..." rg -i "describe.*StencilManager" -A 10 $(fd -e test.ts -e spec.ts StencilManager) | rg -i "it\(.*" # Test: Check for any TODO comments in test files echo "Checking for TODO comments in test files..." rg -i "TODO" $(fd -e test.ts -e spec.ts StencilManager)Run this script to get an overview of the current test coverage and identify areas that need additional tests.
🧰 Tools
🪛 Biome
[error] 14-14: 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] 24-24: packages/core/src/RenderPipeline/StencilManager.ts#L24
Added line #L24 was not covered by tests
[warning] 36-36: packages/core/src/RenderPipeline/StencilManager.ts#L36
Added line #L36 was not covered by tests
[warning] 41-41: packages/core/src/RenderPipeline/StencilManager.ts#L41
Added line #L41 was not covered by tests
[warning] 44-44: packages/core/src/RenderPipeline/StencilManager.ts#L44
Added line #L44 was not covered by tests
[warning] 53-53: packages/core/src/RenderPipeline/StencilManager.ts#L53
Added line #L53 was not covered by tests
[warning] 61-63: packages/core/src/RenderPipeline/StencilManager.ts#L61-L63
Added lines #L61 - L63 were not covered by tests
[warning] 67-68: packages/core/src/RenderPipeline/StencilManager.ts#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 72-73: packages/core/src/RenderPipeline/StencilManager.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-75: packages/core/src/RenderPipeline/StencilManager.ts#L75
Added line #L75 was not covered by tests
[warning] 78-80: packages/core/src/RenderPipeline/StencilManager.ts#L78-L80
Added lines #L78 - L80 were not covered by tests
[warning] 83-88: packages/core/src/RenderPipeline/StencilManager.ts#L83-L88
Added lines #L83 - L88 were not covered by tests
[warning] 91-91: packages/core/src/RenderPipeline/StencilManager.ts#L91
Added line #L91 was not covered by tests
| export class StencilManager { | ||
| private static _stencilRenderQueue: RenderQueue; | ||
|
|
||
| static getStencilRenderQueue(): RenderQueue { | ||
| return (StencilManager._stencilRenderQueue ||= new RenderQueue(RenderQueueType.Transparent)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability of static property initialization
The current implementation of getStencilRenderQueue() uses a concise but potentially less readable nullish coalescing assignment. Consider refactoring for better clarity and to address the static analysis warning about assignments in expressions.
Apply this diff to improve readability:
static getStencilRenderQueue(): RenderQueue {
- return (StencilManager._stencilRenderQueue ||= new RenderQueue(RenderQueueType.Transparent));
+ if (!StencilManager._stencilRenderQueue) {
+ StencilManager._stencilRenderQueue = new RenderQueue(RenderQueueType.Transparent);
+ }
+ return StencilManager._stencilRenderQueue;
}This change makes the initialization more explicit and easier to understand at a glance.
📝 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.
| export class StencilManager { | |
| private static _stencilRenderQueue: RenderQueue; | |
| static getStencilRenderQueue(): RenderQueue { | |
| return (StencilManager._stencilRenderQueue ||= new RenderQueue(RenderQueueType.Transparent)); | |
| } | |
| export class StencilManager { | |
| private static _stencilRenderQueue: RenderQueue; | |
| static getStencilRenderQueue(): RenderQueue { | |
| if (!StencilManager._stencilRenderQueue) { | |
| StencilManager._stencilRenderQueue = new RenderQueue(RenderQueueType.Transparent); | |
| } | |
| return StencilManager._stencilRenderQueue; | |
| } |
🧰 Tools
🪛 Biome
[error] 14-14: 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)
| addStencilWriteSubElement(subElement: SubRenderElement): void { | ||
| this._stencilWriteSubElements.add(subElement); | ||
| this._targetBlendStates.add(subElement.material.renderState.blendState.targetBlendState); | ||
| this._colorWriteMasks.add(subElement.material.renderState.blendState.targetBlendState.colorWriteMask); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add unit tests for addStencilWriteSubElement method
The addStencilWriteSubElement method currently lacks unit tests, which may affect code reliability. Please add unit tests to ensure its functionality is properly validated.
🔗 Analysis chain
Approve addStencilWriteSubElement method with a suggestion for unit tests
The addStencilWriteSubElement method efficiently adds a sub-element to the stencil write queue along with its associated blend state and color write mask. This approach ensures that all necessary information is stored together, which is good for maintaining consistency.
However, the static analysis hints indicate that this method is not covered by tests. To ensure the reliability of this method, please add unit tests. Here's a script to verify the current test coverage:
If the script doesn't find any relevant test files or specific tests for addStencilWriteSubElement, consider adding them to improve code reliability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for addStencilWriteSubElement method
# Test: Search for test files related to StencilManager
echo "Searching for StencilManager test files..."
fd -e test.ts -e spec.ts StencilManager
# Test: Check for tests specifically covering addStencilWriteSubElement method
echo "Checking for addStencilWriteSubElement tests..."
rg -i "describe.*addStencilWriteSubElement" $(fd -e test.ts -e spec.ts StencilManager)
Length of output: 6151
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-63: packages/core/src/RenderPipeline/StencilManager.ts#L61-L63
Added lines #L61 - L63 were not covered by tests
| checkStencilAccess(material: Material): StencilAccess { | ||
| const stencilState = material.renderState.stencilState; | ||
| let stencilAccess = StencilAccess.None; | ||
|
|
||
| if (stencilState.enabled) { | ||
| const { compareFunctionFront, compareFunctionBack } = stencilState; | ||
| if ( | ||
| (compareFunctionFront !== CompareFunction.Always && compareFunctionFront !== CompareFunction.Never) || | ||
| (compareFunctionBack !== CompareFunction.Always && compareFunctionBack !== CompareFunction.Never) | ||
| ) { | ||
| stencilAccess |= StencilAccess.Readable; | ||
| } | ||
|
|
||
| const stencilOperation = StencilOperation.Keep; | ||
| if ( | ||
| stencilState.passOperationFront !== stencilOperation || | ||
| stencilState.passOperationBack !== stencilOperation || | ||
| stencilState.failOperationFront !== stencilOperation || | ||
| stencilState.failOperationBack !== stencilOperation || | ||
| stencilState.zFailOperationFront !== stencilOperation || | ||
| stencilState.zFailOperationBack !== stencilOperation | ||
| ) { | ||
| stencilAccess |= StencilAccess.Writable; | ||
| } | ||
| } | ||
|
|
||
| return stencilAccess; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add unit tests for checkStencilAccess method
The checkStencilAccess method currently lacks test coverage. Please add unit tests to ensure the reliability of this critical method.
🔗 Analysis chain
Approve checkStencilAccess method with a suggestion for unit tests
The checkStencilAccess method provides a comprehensive check of stencil access based on the material's stencil state. The use of bitwise operations for combining access types is efficient and appropriate.
However, the static analysis hints indicate that this method is not covered by tests. To ensure the reliability of this critical method, please add unit tests. Here's a script to verify the current test coverage:
If the script doesn't find any relevant test files or specific tests for checkStencilAccess, consider adding them to improve code reliability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for checkStencilAccess method
# Test: Search for test files related to StencilManager
echo "Searching for StencilManager test files..."
fd -e test.ts -e spec.ts StencilManager
# Test: Check for tests specifically covering checkStencilAccess method
echo "Checking for checkStencilAccess tests..."
rg -i "describe.*checkStencilAccess" $(fd -e test.ts -e spec.ts StencilManager)
Length of output: 309
Script:
#!/bin/bash
# Description: Re-verify test coverage for checkStencilAccess method using ast-grep
# Search for test files related to StencilManager
echo "Searching for StencilManager test files..."
TEST_FILES=$(fd -e test.ts -e spec.ts StencilManager)
if [ -z "$TEST_FILES" ]; then
echo "No test files found for StencilManager."
exit 0
fi
# Check for occurrences of checkStencilAccess in the test files
echo "Checking for checkStencilAccess usage in test files..."
ast-grep --lang typescript --pattern 'describe $_ { it $_ { $_.checkStencilAccess($_) } }' $TEST_FILES
Length of output: 265
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: packages/core/src/RenderPipeline/StencilManager.ts#L36
Added line #L36 was not covered by tests
[warning] 41-41: packages/core/src/RenderPipeline/StencilManager.ts#L41
Added line #L41 was not covered by tests
[warning] 44-44: packages/core/src/RenderPipeline/StencilManager.ts#L44
Added line #L44 was not covered by tests
[warning] 53-53: packages/core/src/RenderPipeline/StencilManager.ts#L53
Added line #L53 was not covered by tests
| const renderState = material.renderState; | ||
| renderState.blendState.targetBlendState.colorWriteMask = ColorWriteMask.None; | ||
| renderState.rasterState.cullMode = CullMode.Off; | ||
| renderState.stencilState.enabled = true; |
| @@ -26,6 +27,24 @@ export class SpriteRenderer extends Renderer { | |||
| /** @internal */ | |||
| static _textureProperty: ShaderProperty = ShaderProperty.getByName("renderer_SpriteTexture"); | |||
|
|
|||
| /** @internal */ | |||
| static _createSpriteMaterial(engine): Material { | |||
| @@ -394,11 +419,6 @@ export class TextRenderer extends Renderer { | |||
| return; | |||
| } | |||
|
|
|||
| if (this._isContainDirtyFlag(DirtyFlag.MaskInteraction)) { | |||
| this._updateStencilState(); | |||
There was a problem hiding this comment.
Before implement is different!
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
packages/core/src/2d/sprite/SpriteMask.ts (1)
264-264: Approve material reference update and suggest test coverageThe update to use
engine._basicResources.spriteMaskDefaultMaterialis consistent with the change in the constructor. This change looks good.However, the static analysis tool indicates that this line is not covered by tests. Consider adding test coverage for this scenario to ensure the fallback mechanism works as expected when the material is destroyed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 264-264: packages/core/src/2d/sprite/SpriteMask.ts#L264
Added line #L264 was not covered by testspackages/core/src/2d/text/TextRenderer.ts (2)
12-15: Consider adjusting import statement formattingThe static analysis tool suggests condensing the import statement to a single line. While this change wouldn't affect functionality, the current multi-line format might be preferred for readability or to comply with line length limits. If there's no specific style guide dictating the format, the current multi-line approach is acceptable.
If you decide to adjust the formatting, you can use the following:
-import { - ShaderData, - ShaderProperty -} from "../../shader"; +import { ShaderData, ShaderProperty } from "../../shader";🧰 Tools
🪛 GitHub Check: lint
[failure] 12-12:
Replace⏎··ShaderData,⏎··ShaderProperty⏎with·ShaderData,·ShaderProperty·
Line range hint
1-741: Summary of TextRenderer.ts changesThe changes in this file appear to be part of a larger refactoring effort related to mask interaction and dirty flags. Here's a summary of the main points:
- Import statements have been consolidated, improving readability.
- The constructor now uses a different method to access the default material, which may indicate a broader change in resource management.
- The
maskInteractionsetter no longer updates a dirty flag, which requires clarification on its impact.- The
DirtyFlagenum has been updated, removingMaskInteractionand adjusting theColorflag value.These changes generally improve code organization and consistency. However, it's crucial to ensure that:
- The new method of accessing the default material is used consistently across the codebase.
- The removal of the dirty flag update in the
maskInteractionsetter doesn't introduce any unintended side effects.- The changes to the
DirtyFlagenum don't negatively impact other parts of the codebase.Please review the verification scripts provided in the comments above to check for any potential issues related to these changes.
🧰 Tools
🪛 GitHub Check: lint
[failure] 12-12:
Replace⏎··ShaderData,⏎··ShaderProperty⏎with·ShaderData,·ShaderProperty·packages/core/src/BasicResources.ts (1)
83-85: Consider adding unit tests for new material creation methods.Adding unit tests for the new materials (
spriteDefaultMaterial,textDefaultMaterial, andspriteMaskDefaultMaterial) will help ensure they are correctly configured and behave as expected in different rendering scenarios.Would you like assistance in creating these unit tests or opening a GitHub issue to track this task?
packages/core/src/Engine.ts (1)
Line range hint
83-86: Ensure consistency in accessinghardwareRenderer.Within the constructor, you've assigned
this._hardwareRenderer = hardwareRenderer;. However, in the conditional check, you usehardwareRenderer.canIUse(...)directly. For consistency and clarity, consider usingthis._hardwareRenderer.canIUse(...)instead of the parameterhardwareRenderer.Apply this diff to improve consistency:
- if (!hardwareRenderer.canIUse(GLCapabilityType.depthTexture)) { + if (!this._hardwareRenderer.canIUse(GLCapabilityType.depthTexture)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/core/src/2d/sprite/SpriteMask.ts (3 hunks)
- packages/core/src/2d/sprite/SpriteRenderer.ts (2 hunks)
- packages/core/src/2d/text/TextRenderer.ts (3 hunks)
- packages/core/src/BasicResources.ts (4 hunks)
- packages/core/src/Engine.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/2d/sprite/SpriteMask.ts
[warning] 264-264: packages/core/src/2d/sprite/SpriteMask.ts#L264
Added line #L264 was not covered by testspackages/core/src/2d/sprite/SpriteRenderer.ts
[warning] 335-335: packages/core/src/2d/sprite/SpriteRenderer.ts#L335
Added line #L335 was not covered by tests
🪛 GitHub Check: lint
packages/core/src/2d/text/TextRenderer.ts
[failure] 12-12:
Replace⏎··ShaderData,⏎··ShaderProperty⏎with·ShaderData,·ShaderProperty·
🔇 Additional comments (11)
packages/core/src/2d/sprite/SpriteMask.ts (1)
180-180: Approve material reference update and verify resource managementThe update to use
_engine._basicResources.spriteMaskDefaultMaterialaligns with a new resource management strategy. This change looks good, but it's important to ensure consistency.To verify the new resource management approach:
- Confirm that
_basicResourcesis properly initialized in the engine setup.- Check for any other occurrences of
_spriteMaskDefaultMaterialthat might need updating:#!/bin/bash # Description: Check for any remaining occurrences of the old material reference # Test: Search for the old material reference rg --type typescript '_spriteMaskDefaultMaterial'packages/core/src/2d/text/TextRenderer.ts (4)
12-15: LGTM: Import statement consolidationThe consolidation of import statements for
ShaderDataandShaderPropertyinto a single block improves code organization and readability. This change aligns with common import formatting practices.🧰 Tools
🪛 GitHub Check: lint
[failure] 12-12:
Replace⏎··ShaderData,⏎··ShaderProperty⏎with·ShaderData,·ShaderProperty·
738-741: LGTM: DirtyFlag enum updateThe removal of the
MaskInteractionflag from theDirtyFlagenum and the adjustment of theColorflag value to0x10is consistent with the changes observed in themaskInteractionsetter. This update looks good, but it's crucial to ensure that these changes don't negatively impact other parts of the codebase that may rely on these flags.To verify the impact of these changes, please run the following script:
#!/bin/bash # Search for usages of DirtyFlag.Color echo "Searching for DirtyFlag.Color usages:" rg "DirtyFlag\.Color" --type ts # Search for any remaining references to DirtyFlag.MaskInteraction echo "Searching for DirtyFlag.MaskInteraction references:" rg "DirtyFlag\.MaskInteraction" --type ts
298-298: LGTM: Updated default material accessThe change from
engine._textDefaultMaterialtoengine._basicResources.textDefaultMaterialsuggests a refactoring of how default materials are accessed within the engine. This change looks good, but it's important to ensure consistency across the codebase.To verify the consistency of this change, please run the following script:
✅ Verification successful
Verified: Consistent Default Material Access
All references to
engine._textDefaultMaterialhave been successfully updated toengine._basicResources.textDefaultMaterial. No remaining instances of_textDefaultMaterialwere found, ensuring consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to _textDefaultMaterial echo "Searching for _textDefaultMaterial references:" rg "_textDefaultMaterial" --type ts # Search for new usage of _basicResources.textDefaultMaterial echo "Searching for _basicResources.textDefaultMaterial usage:" rg "_basicResources\.textDefaultMaterial" --type tsLength of output: 423
Line range hint
398-398: Please clarify the rationale for removing the dirty flag updateThe removal of
_setDirtyFlagTrue(DirtyFlag.MaskInteraction)from themaskInteractionsetter suggests that updates tomaskInteractionno longer trigger a dirty flag update for mask interaction. Could you please explain the reasoning behind this change and its potential impact on the rendering process? This will help ensure that the modification doesn't introduce any unintended side effects.To better understand the impact of this change, please run the following script to check for other usages of
DirtyFlag.MaskInteraction:✅ Verification successful
No Additional Usages Found for
DirtyFlag.MaskInteractionAfter searching the codebase, no additional usages of
DirtyFlag.MaskInteractionwere found. Therefore, the removal of_setDirtyFlagTrue(DirtyFlag.MaskInteraction)appears safe and should not introduce unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of DirtyFlag.MaskInteraction echo "Searching for DirtyFlag.MaskInteraction usages:" rg "DirtyFlag\.MaskInteraction" --type tsLength of output: 547
🧰 Tools
🪛 GitHub Check: lint
[failure] 12-12:
Replace⏎··ShaderData,⏎··ShaderProperty⏎with·ShaderData,·ShaderProperty·packages/core/src/BasicResources.ts (4)
11-11: Imports are correctly updated for new functionalities.The addition of
BlendFactor,BlendOperation,ColorWriteMask,CullMode, andRenderQueueTypeensures that the necessary enums are available for material configuration.
33-35: Addition of default materials enhances resource management.Introducing
spriteDefaultMaterial,textDefaultMaterial, andspriteMaskDefaultMaterialproperties improves the efficiency of material reuse across sprites, text rendering, and sprite masking.
83-85: Materials are properly initialized in the constructor.Initializing the new default materials within the constructor ensures they are available when the
BasicResourcesinstance is created.
187-196: Verify stencil state configuration in_createSpriteMaskMaterial.The
_createSpriteMaskMaterialmethod enables stencil testing and modifies several render states, such as disabling color writes and depth testing. Given the concerns about potential conflicts during continuous rendering between masked and unmasked objects, please ensure that this configuration does not negatively impact rendering in scenes with shadows or other stencil-dependent effects.Consider running integration tests in scenarios where sprite masks interact with shadows or other stencil operations to validate the behavior.
packages/core/src/2d/sprite/SpriteRenderer.ts (1)
270-270: EnsurespriteDefaultMaterialis initialized before useIn the constructor, you're setting the material using
this._engine._basicResources.spriteDefaultMaterial. Please verify thatspriteDefaultMaterialis properly initialized at this point to prevent potentialundefinedornullreferences.packages/core/src/Engine.ts (1)
10-10: Import statement correctly includes new sprite classes.The inclusion of
SpriteMask,SpriteRenderer, andTextRendererfrom./2daligns with the refactored sprite rendering implementation.
| @@ -9,9 +9,9 @@ | |||
| import { SubRenderElement } from "../../RenderPipeline/SubRenderElement"; | |||
| import { Renderer, RendererUpdateFlags } from "../../Renderer"; | |||
| import { assignmentClone, ignoreClone } from "../../clone/CloneManager"; | |||
| import { SpriteMaskLayer } from "../../enums/SpriteMaskLayer"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Residual old import paths detected
The script found an occurrence of the old import path in packages/core/src/RenderPipeline/MaskManager.ts. Please update this reference to ensure consistency across the codebase.
🔗 Analysis chain
Verify import path consistency across the codebase
The import path for SpriteMaskLayer has been updated. Ensure that this change is consistent across the entire codebase to prevent any import-related issues.
Run the following script to check for any remaining occurrences of the old import path:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old import path for SpriteMaskLayer
# Test: Search for the old import path
rg --type typescript 'from "../enums/SpriteMaskLayer"'
Length of output: 178
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old import path for SpriteMaskLayer
# Test: Search for the old import path
rg --type ts "from \"../enums/SpriteMaskLayer\""
Length of output: 156
packages/core/src/BasicResources.ts
Outdated
| const target = renderState.blendState.targetBlendState; | ||
| target.enabled = true; | ||
| target.sourceColorBlendFactor = BlendFactor.SourceAlpha; | ||
| target.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||
| target.sourceAlphaBlendFactor = BlendFactor.One; | ||
| target.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha; | ||
| target.colorBlendOperation = target.alphaBlendOperation = BlendOperation.Add; | ||
| renderState.depthState.writeEnabled = false; | ||
| renderState.rasterState.cullMode = CullMode.Off; | ||
| renderState.renderQueueType = RenderQueueType.Transparent; | ||
| material.isGCIgnored = true; | ||
| return material; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in material setup methods.
The methods _createSpriteMaterial and _createTextMaterial contain identical code for configuring the render state of the materials. Consider extracting this common code into a shared helper method to reduce duplication and improve maintainability.
Example:
private _configureTransparentMaterial(material: Material): void {
const renderState = material.renderState;
const target = renderState.blendState.targetBlendState;
target.enabled = true;
target.sourceColorBlendFactor = BlendFactor.SourceAlpha;
target.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha;
target.sourceAlphaBlendFactor = BlendFactor.One;
target.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha;
target.colorBlendOperation = target.alphaBlendOperation = BlendOperation.Add;
renderState.depthState.writeEnabled = false;
renderState.rasterState.cullMode = CullMode.Off;
renderState.renderQueueType = RenderQueueType.Transparent;
material.isGCIgnored = true;
}Then update the methods as follows:
private _createSpriteMaterial(engine: Engine): Material {
const material = new Material(engine, Shader.find("Sprite"));
this._configureTransparentMaterial(material);
return material;
}
private _createTextMaterial(engine: Engine): Material {
const material = new Material(engine, Shader.find("Text"));
this._configureTransparentMaterial(material);
return material;
}Also applies to: 174-185
packages/core/src/BasicResources.ts
Outdated
| @@ -140,6 +149,51 @@ | |||
| ); | |||
| return texture as T; | |||
| } | |||
|
|
|||
| private _createSpriteMaterial(engine): Material { | |||
There was a problem hiding this comment.
Specify the type of the engine parameter in _createSpriteMaterial.
The engine parameter in the _createSpriteMaterial method is missing a type annotation. For consistency and to leverage TypeScript's type checking, please specify the parameter type as Engine.
Suggested change:
-private _createSpriteMaterial(engine): Material {
+private _createSpriteMaterial(engine: Engine): Material {📝 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.
| private _createSpriteMaterial(engine): Material { | |
| private _createSpriteMaterial(engine: Engine): Material { |
| @@ -334,7 +332,7 @@ | |||
| } | |||
| // @todo: This question needs to be raised rather than hidden. | |||
| if (material.destroyed) { | |||
| material = this._engine._spriteDefaultMaterials[this._maskInteraction]; | |||
| material = this._engine._basicResources.spriteDefaultMaterial; | |||
There was a problem hiding this comment.
Add test coverage for material fallback logic
The line material = this._engine._basicResources.spriteDefaultMaterial; introduces new logic to handle cases where the material has been destroyed. However, static analysis indicates that this line is not covered by existing tests.
Consider adding a unit test to cover this scenario to ensure that the fallback to the default material behaves as expected when the original material is destroyed.
Would you like assistance in creating a test case for this?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 335-335: packages/core/src/2d/sprite/SpriteRenderer.ts#L335
Added line #L335 was not covered by tests
packages/core/src/BasicResources.ts
Outdated
| const renderState = material.renderState; | ||
| renderState.blendState.targetBlendState.colorWriteMask = ColorWriteMask.None; | ||
| renderState.rasterState.cullMode = CullMode.Off; | ||
| renderState.stencilState.enabled = true; |
| this._buildMaskRenderElement(maskLayer, incrementMaskQueue, decrementMaskQueue); | ||
|
|
||
| const engine = context.camera.engine; | ||
| const batcherManager = engine._batcherManager; |
There was a problem hiding this comment.
const batcherManager = context.camera.engine._batcherManager;
| @@ -42,6 +42,7 @@ export { ReplacementFailureStrategy } from "./enums/ReplacementFailureStrategy"; | |||
| export { Downsampling } from "./enums/Downsampling"; | |||
| export { ColorSpace } from "./enums/ColorSpace"; | |||
| export { BackgroundTextureFillMode } from "./enums/BackgroundTextureFillMode"; | |||
| export { SpriteMaskLayer } from "./enums/SpriteMaskLayer"; | |||
There was a problem hiding this comment.
influenceLayers: number = SpriteMaskLayer.Everything;->influenceLayers: SpriteMaskLayer = SpriteMaskLayer.Everything;
| @@ -0,0 +1,8 @@ | |||
| /** | |||
| * Render queue mask type. | |||
| private _platformApply( | ||
| rhi: IHardwareRenderer, | ||
| lastState: StencilState, | ||
| customStates?: Record<number, number | boolean> |
There was a problem hiding this comment.
customStates?: Record<RenderStateElementKey, number | boolean>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/2d/text/TextRenderer.ts (2 hunks)
- packages/core/src/BasicResources.ts (4 hunks)
- packages/core/src/Engine.ts (0 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/Engine.ts
🧰 Additional context used
🔇 Additional comments (6)
packages/core/src/BasicResources.ts (4)
11-11: Imports are appropriate and necessaryThe added imports
BlendFactor,BlendOperation,ColorWriteMask,CullMode, andRenderQueueTypeare required for the new material configurations and are correctly imported from"./shader".
33-35: New readonly properties are correctly declaredThe properties
spriteDefaultMaterial,textDefaultMaterial, andspriteMaskDefaultMaterialare appropriately declared asreadonlyand typed asMaterial, enhancing the material management capabilities of the class.
83-85: Refactored material creation reduces code duplicationThe introduction of
_create2DMaterialand_createSpriteMaskMaterialmethods effectively addresses previous concerns about duplicated code in material setup methods, improving code maintainability.
153-168: Method_create2DMaterialis correctly implemented with proper typingThe
_create2DMaterialmethod is well-defined with correctly typed parametersengine: Engineandshader: Shader. The method appropriately configures the material for 2D rendering.packages/core/src/2d/text/TextRenderer.ts (2)
295-295: Correct use ofengine._basicResources.textDefaultMaterialUpdating the material assignment to
engine._basicResources.textDefaultMaterialensures that theTextRendereruses the appropriate default material provided by the engine's basic resources. This change enhances the consistency and reliability of material handling within theTextRenderer.
735-735: Adjustment ofDirtyFlag.Colorvalue afterMaskInteractionremovalThe update of
DirtyFlag.Colorfrom0x20to0x10reflects the removal of theMaskInteractionflag, which previously occupied the0x10value. This adjustment maintains the integrity of the enum's bitmask values and ensures that there are no overlaps or conflicts between flags. It is a necessary change to keep the dirty flag system consistent after the removal of theMaskInteractionflag.
packages/core/src/BasicResources.ts
Outdated
| private _createSpriteMaskMaterial(engine: Engine): Material { | ||
| const material = new Material(engine, Shader.find("SpriteMask")); | ||
| const renderState = material.renderState; | ||
| renderState.blendState.targetBlendState.colorWriteMask = ColorWriteMask.None; | ||
| renderState.rasterState.cullMode = CullMode.Off; | ||
| renderState.stencilState.enabled = true; | ||
| renderState.depthState.enabled = false; | ||
| material.isGCIgnored = true; | ||
| return material; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Stencil state configuration is incomplete in _createSpriteMaskMaterial
- The stencil state is enabled (
renderState.stencilState.enabled = true) but lacks additional configurations such as stencil function, reference value, mask, and operations. This may lead to unintended interactions with other materials using the stencil buffer.
🔗 Analysis chain
Verify stencil state configuration for sprite masking
In the _createSpriteMaskMaterial method, the stencil state is enabled (renderState.stencilState.enabled = true), but additional stencil configurations (e.g., stencil function, reference value, mask, operations) are not specified. This may lead to unintended interactions with other materials using the stencil buffer. Ensure that the stencil state is fully configured to achieve the desired masking effect and to prevent conflicts.
Run the following script to verify stencil state configurations in other materials:
This script searches for instances where renderState.stencilState.enabled is set to true and displays the following 10 lines to examine how stencil states are typically configured elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for full stencil state configurations in the codebase.
# Expected: Find examples where stencil state is fully configured after being enabled.
rg --type typescript -A 10 'renderState\.stencilState\.enabled\s*=\s*true'
Length of output: 111
Script:
#!/bin/bash
# Description: Search for full stencil state configurations in TypeScript files.
# Expected: Find examples where stencil state is fully configured after being enabled.
rg -g "*.ts" -g "*.tsx" -A 10 'renderState\.stencilState\.enabled\s*=\s*true'
Length of output: 689
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/core/src/RenderPipeline/RenderQueue.ts (2)
60-65: Improved organization with separate mask and stencil managers.The introduction of
_maskManagerand_stencilManagerfrom the scene object indicates a good separation of concerns for mask and stencil management. This change should lead to better organization and maintainability of the code.Consider removing the underscore prefix from these properties if they are intended to be public APIs of the scene object. If they are meant to be private or internal, ensure this is consistently applied and documented across the codebase.
87-110: Enhanced mask interaction and stencil management logic.The refactoring of mask interaction and stencil management logic brings several improvements:
- More explicit handling of different mask interaction scenarios.
- Utilization of specialized managers for mask and stencil operations.
- Centralized retrieval of stencil states from
BasicResources.These changes should lead to more robust and maintainable code.
To further improve readability and maintainability, consider the following suggestions:
Extract the complex conditional logic into separate, well-named methods. For example:
private handleMaskInteraction(context: RenderContext, pipelineStageTagValue: string, subElement: SubRenderElement): void { // Logic from lines 87-103 } private getCustomStates(maskInteraction: SpriteMaskInteraction, maskType: RenderQueueMaskType): Record<number, number | boolean> | null { // Logic from lines 105-110 }Add comments explaining the purpose and expected behavior of each major block of code within these complex operations.
These refactorings would make the code easier to understand and maintain, especially for developers who might not be familiar with the intricacies of mask interactions and stencil management.
packages/core/src/BasicResources.ts (1)
240-265: LGTM: Well-implemented material creation methods with a suggestion for improvement.The new private methods
_create2DMaterialand_createSpriteMaskMaterialare well-implemented and set up the materials with appropriate render states for their intended use. The use of enums for blend factors, operations, and cull modes enhances type safety.Consider extracting the common render state setup logic into a separate private method to reduce code duplication. This would improve maintainability and make it easier to update render state configurations in the future. Here's a suggested refactor:
private _setupCommonRenderState(renderState: RenderState): void { const target = renderState.blendState.targetBlendState; target.enabled = true; target.sourceColorBlendFactor = BlendFactor.SourceAlpha; target.destinationColorBlendFactor = BlendFactor.OneMinusSourceAlpha; target.sourceAlphaBlendFactor = BlendFactor.One; target.destinationAlphaBlendFactor = BlendFactor.OneMinusSourceAlpha; target.colorBlendOperation = target.alphaBlendOperation = BlendOperation.Add; renderState.depthState.writeEnabled = false; renderState.rasterState.cullMode = CullMode.Off; renderState.renderQueueType = RenderQueueType.Transparent; } private _create2DMaterial(engine: Engine, shader: Shader): Material { const material = new Material(engine, shader); this._setupCommonRenderState(material.renderState); material.isGCIgnored = true; return material; } private _createSpriteMaskMaterial(engine: Engine): Material { const material = new Material(engine, Shader.find("SpriteMask")); this._setupCommonRenderState(material.renderState); material.renderState.blendState.targetBlendState.colorWriteMask = ColorWriteMask.None; material.renderState.depthState.enabled = false; material.isGCIgnored = true; return material; }This refactoring would centralize the common render state setup logic, making it easier to maintain and update in the future.
📜 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] 45-45: packages/core/src/BasicResources.ts#L45
Added line #L45 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: packages/core/src/BasicResources.ts#L52
Added line #L52 was not covered by tests
[warning] 55-55: packages/core/src/BasicResources.ts#L55
Added line #L55 was not covered by tests
[warning] 57-57: packages/core/src/BasicResources.ts#L57
Added line #L57 was not covered by tests
[warning] 59-59: packages/core/src/BasicResources.ts#L59
Added line #L59 was not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 65-69: packages/core/src/BasicResources.ts#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 81-81: packages/core/src/BasicResources.ts#L81
Added line #L81 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-86: packages/core/src/BasicResources.ts#L86
Added line #L86 was not covered by tests
[warning] 88-88: packages/core/src/BasicResources.ts#L88
Added line #L88 was not covered by tests
[warning] 90-90: packages/core/src/BasicResources.ts#L90
Added line #L90 was not covered by tests
[warning] 93-93: packages/core/src/BasicResources.ts#L93
Added line #L93 was not covered by tests
[warning] 96-103: packages/core/src/BasicResources.ts#L96-L103
Added lines #L96 - L103 were not covered by tests
[warning] 105-105: packages/core/src/BasicResources.ts#L105
Added line #L105 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)
🔇 Additional comments (8)
packages/core/src/RenderPipeline/RenderQueue.ts (3)
35-37: Improved separation of concerns insortBatchmethod.The refactoring of
sortBatchto call separatesortandbatchmethods enhances code modularity and flexibility. This change allows for independent sorting and batching operations, which can be beneficial for maintenance and potential future optimizations.
43-44: Improved batching strategy withbatchmethod.The introduction of a separate
batchmethod that delegates tobatcherManagerenhances flexibility and adheres to the separation of concerns principle. This change allows for easier maintenance and potential customization of batching strategies in the future.
Line range hint
1-205: Summary of RenderQueue refactoringThe refactoring of the
RenderQueueclass has introduced several improvements:
- Better separation of concerns with distinct
sortandbatchmethods.- Introduction of specialized managers for mask and stencil operations.
- More explicit handling of mask interactions and stencil states.
These changes enhance the modularity, flexibility, and maintainability of the code. However, there are a few areas that could benefit from further refinement:
- Improving type safety, particularly in the
sortmethod.- Enhancing readability in complex sections, such as the mask interaction and stencil management logic.
Overall, this refactoring appears to be a step in the right direction, providing a more robust and extensible implementation of the render queue.
packages/core/src/BasicResources.ts (5)
Line range hint
1-39: LGTM: New imports and static properties for stencil states.The new imports and static properties are well-organized and follow the internal API design pattern. The use of lazy initialization for stencil states is a good approach for optimizing memory usage.
Line range hint
120-172: LGTM: New default material properties and constructor initialization.The addition of default material properties and their initialization in the constructor is well-implemented. This centralized approach to creating default materials enhances maintainability and consistency across the codebase.
Line range hint
1-265: Overall assessment: Well-implemented refactoring of mask functionality with minor suggestions for improvement.The changes in this file effectively refactor the mask functionality by introducing new methods for managing stencil states and default materials. The implementation is generally well-done, with good use of TypeScript features and appropriate encapsulation of functionality.
Key points:
- New static methods for generating stencil states enhance the flexibility and maintainability of mask-related operations.
- The addition of default material properties centralizes material creation, improving consistency across the codebase.
- The new private methods for creating materials are well-implemented but could benefit from further refactoring to reduce code duplication.
Suggestions for improvement:
- Add unit tests for the new static methods to ensure code coverage and prevent regressions.
- Consider refactoring the material creation methods to extract common render state setup logic.
These changes align well with the PR objectives and provide a solid foundation for further development of the mask functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-45: packages/core/src/BasicResources.ts#L45
Added line #L45 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: packages/core/src/BasicResources.ts#L52
Added line #L52 was not covered by tests
[warning] 55-55: packages/core/src/BasicResources.ts#L55
Added line #L55 was not covered by tests
[warning] 57-57: packages/core/src/BasicResources.ts#L57
Added line #L57 was not covered by tests
[warning] 59-59: packages/core/src/BasicResources.ts#L59
Added line #L59 was not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 65-69: packages/core/src/BasicResources.ts#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 81-81: packages/core/src/BasicResources.ts#L81
Added line #L81 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-86: packages/core/src/BasicResources.ts#L86
Added line #L86 was not covered by tests
[warning] 88-88: packages/core/src/BasicResources.ts#L88
Added line #L88 was not covered by tests
[warning] 90-90: packages/core/src/BasicResources.ts#L90
Added line #L90 was not covered by tests
[warning] 93-93: packages/core/src/BasicResources.ts#L93
Added line #L93 was not covered by tests
[warning] 96-103: packages/core/src/BasicResources.ts#L96-L103
Added lines #L96 - L103 were not covered by tests
[warning] 105-105: packages/core/src/BasicResources.ts#L105
Added line #L105 was not covered by tests
74-106: LGTM: Well-implemented _getMaskTypeStencilStates method.The method efficiently generates stencil states based on the RenderQueueMaskType. The use of lazy initialization and enums for stencil operations is a good practice.
As with the previous method, consider adding unit tests to cover this method, as it's currently not covered by tests according to the static analysis report. This will help ensure its correctness and prevent potential regressions.
To verify the test coverage, you can run the following command:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 81-81: packages/core/src/BasicResources.ts#L81
Added line #L81 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-86: packages/core/src/BasicResources.ts#L86
Added line #L86 was not covered by tests
[warning] 88-88: packages/core/src/BasicResources.ts#L88
Added line #L88 was not covered by tests
[warning] 90-90: packages/core/src/BasicResources.ts#L90
Added line #L90 was not covered by tests
[warning] 93-93: packages/core/src/BasicResources.ts#L93
Added line #L93 was not covered by tests
[warning] 96-103: packages/core/src/BasicResources.ts#L96-L103
Added lines #L96 - L103 were not covered by tests
[warning] 105-105: packages/core/src/BasicResources.ts#L105
Added line #L105 was not covered by tests
41-72: LGTM: Efficient implementation of _getMaskInteractionStencilStates.The method effectively generates stencil states based on the SpriteMaskInteraction type. The use of lazy initialization and enums for compare functions is a good practice.
Consider adding unit tests to cover this method, as it's currently not covered by tests according to the static analysis report. This will help ensure its correctness and prevent potential regressions.
To verify the test coverage, you can run the following command:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-45: packages/core/src/BasicResources.ts#L45
Added line #L45 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: packages/core/src/BasicResources.ts#L52
Added line #L52 was not covered by tests
[warning] 55-55: packages/core/src/BasicResources.ts#L55
Added line #L55 was not covered by tests
[warning] 57-57: packages/core/src/BasicResources.ts#L57
Added line #L57 was not covered by tests
[warning] 59-59: packages/core/src/BasicResources.ts#L59
Added line #L59 was not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 65-69: packages/core/src/BasicResources.ts#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/core/src/RenderPipeline/MaskManager.ts (2)
27-27: Fix linting issue: Remove trailing whitespaceThere is trailing whitespace at line 27, which is causing a linting error. Please remove the unnecessary whitespace to adhere to the code style guidelines.
Apply this diff to fix the issue:
🧰 Tools
🪛 GitHub Check: lint
[failure] 27-27:
Delete··⏎
68-68: Enhance readability of conditional expressionThe use of bitwise operations within the conditional expression can be made more explicit for better readability. Consider using an
ifstatement with an explicit comparison to clarify intent.Apply this diff to improve readability:
+ decrementMaskQueue.pushRenderElement(mask._renderElement); + }packages/core/src/Camera.ts (1)
Line range hint
725-749: Address Potential Ambiguity insetReplacementShaderOverloadsThe method
setReplacementShaderhas two overloads that differ only by the type of thereplacementTagparameter—one accepts astring, and the other accepts aShaderTagKey. This could lead to ambiguity, especially ifShaderTagKeycan be instantiated with a string, or when TypeScript cannot definitively resolve which overload to use.Consider the following solutions:
Combine Overloads into a Single Method: Merge the overloads into one method that accepts a union type for
replacementTag./** * Set the replacement shader. * @param shader - Replacement shader * @param replacementTag - Sub shader tag name or key * @param failureStrategy - Replacement failure strategy, @defaultValue `ReplacementFailureStrategy.KeepOriginalShader` */ setReplacementShader( shader: Shader, replacementTag?: string | ShaderTagKey, failureStrategy: ReplacementFailureStrategy = ReplacementFailureStrategy.KeepOriginalShader ): void { // Method implementation }Differentiate Method Names: Use distinct method names for different parameter types to eliminate ambiguity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/Camera.ts (3 hunks)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1 hunks)
- packages/core/src/RenderPipeline/MaskManager.ts (3 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] 36-36: 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: lint
packages/core/src/RenderPipeline/MaskManager.ts
[failure] 27-27:
Delete··⏎
🔇 Additional comments (3)
packages/core/src/Camera.ts (3)
20-20: Validate the Import ofSpriteMaskLayerThe addition of
SpriteMaskLayerimport at line 20 is required for the new stencil and mask management logic. This import is appropriate and correctly referenced in the code.
7-7: Confirm the Necessity of ImportingPipelineStageThe imported
PipelineStagefrom./RenderPipelineis used in the newly added code at line 614. Please ensure that this import is necessary and thatPipelineStageis correctly utilized in the context.Run the following script to verify the usage of
PipelineStagein the codebase:✅ Verification successful
Confirmed: The
PipelineStageimport is necessary and correctly utilized inCamera.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of 'PipelineStage' in 'Camera.ts' # Test: Search for 'PipelineStage' usage. Expect: Should find its usage in the file. rg --type typescript 'PipelineStage' packages/core/src/Camera.tsLength of output: 99
Script:
#!/bin/bash # Description: Re-verify the usage of 'PipelineStage' in 'Camera.ts' without specifying file type # Test: Search for 'PipelineStage' usage in 'Camera.ts'. Expect: Should find its usage in the file. rg 'PipelineStage' packages/core/src/Camera.tsLength of output: 166
611-614:⚠️ Potential issueVerify the Correctness of Clear Flags Bitwise Operation
The condition
if (this.clearFlags & CameraClearFlags.Stencil)uses a bitwise AND operation to check if theStencilflag is set. Ensure thatCameraClearFlags.Stencilis intended to be used as a bitmask and that this operation behaves as expected.Run the following script to confirm that
CameraClearFlags.Stencilis properly defined:✅ Verification successful
Clear Flags Bitwise Operation is Correct
The
CameraClearFlags.Stencilflag is properly defined as0x4, which is suitable for bitwise operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of 'CameraClearFlags.Stencil' # Test: Retrieve the definition of 'CameraClearFlags' rg --type typescript 'export enum CameraClearFlags' -A 5 packages/core/src/enums/CameraClearFlags.ts # Expected: 'CameraClearFlags.Stencil' should be defined with a value suitable for bitwise operations (e.g., powers of two).Length of output: 137
Script:
#!/bin/bash # Description: Verify the definition of 'CameraClearFlags.Stencil' # Test: Retrieve the definition of 'CameraClearFlags' rg 'export enum CameraClearFlags' packages/core/src/enums/CameraClearFlags.ts -A 10 # Expected: 'CameraClearFlags.Stencil' should be defined with a value suitable for bitwise operations (e.g., powers of two).Length of output: 346
packages/core/src/Camera.ts
Outdated
| const lastCamera = context.camera; | ||
| if (lastCamera) { | ||
| const scene = lastCamera.scene; | ||
| if (this.clearFlags & CameraClearFlags.Stencil) { | ||
| scene._maskManager._preMaskLayer = SpriteMaskLayer.Nothing; | ||
| } else { | ||
| scene._maskManager.clearMask(context, PipelineStage.Forward); | ||
| } | ||
| scene._stencilManager.clearStencil(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Direct Modification of Internal Properties
The code directly accesses and modifies internal properties like scene._maskManager._preMaskLayer and calls internal methods such as scene._stencilManager.clearStencil(). Direct manipulation of properties and methods prefixed with an underscore may break encapsulation and lead to maintenance issues.
Consider the following refinements:
- Expose Public Methods or Properties: Update the
MaskManagerandStencilManagerclasses to provide public methods or properties that encapsulate these operations. - Enhance Encapsulation: Modify the code to interact with these managers through their public interfaces to maintain encapsulation and facilitate future maintenance.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
packages/core/src/RenderPipeline/RenderQueue.ts (1)
87-110: Improved mask handling and stencil state managementThe refactoring of the
rendermethod significantly improves the handling of mask interactions and stencil states. The use ofBasicResourcesfor getting render states centralizes this logic, enhancing maintainability. These changes appear to address the concerns raised in the PR objectives about mask and shadow interactions.While the changes are beneficial, the increased complexity might affect readability. Consider adding comments to explain the logic flow, particularly in the following sections:
- Lines 89-103: Explain the conditions for mask interaction and stencil access.
- Lines 105-110: Describe the purpose and impact of setting
customStates.This will help future maintainers understand the reasoning behind these complex operations.
packages/core/src/BasicResources.ts (3)
41-72: LGTM: Efficient implementation of _getMaskInteractionRenderStates.The method efficiently handles both mask interaction types and uses memoization for performance. The stencil state configuration is appropriate for each case.
Consider adding a default case to handle unexpected
maskInteractionvalues:if (visibleInsideMask) { // ... existing code ... } else if (maskInteraction === SpriteMaskInteraction.VisibleOutsideMask) { // ... existing code ... } else { throw new Error(`Unsupported SpriteMaskInteraction: ${maskInteraction}`); }This would make the method more robust against potential future changes or incorrect usage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-45: packages/core/src/BasicResources.ts#L45
Added line #L45 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: packages/core/src/BasicResources.ts#L52
Added line #L52 was not covered by tests
[warning] 55-55: packages/core/src/BasicResources.ts#L55
Added line #L55 was not covered by tests
[warning] 57-57: packages/core/src/BasicResources.ts#L57
Added line #L57 was not covered by tests
[warning] 59-59: packages/core/src/BasicResources.ts#L59
Added line #L59 was not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 65-69: packages/core/src/BasicResources.ts#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
74-107: LGTM: Efficient implementation of _getMaskTypeRenderStates.The method efficiently handles both mask types and uses memoization for performance. The stencil state configuration is appropriate for each case.
Consider adding a default case to handle unexpected
maskTypevalues:if (isIncrement) { // ... existing code ... } else if (maskType === RenderQueueMaskType.Decrement) { // ... existing code ... } else { throw new Error(`Unsupported RenderQueueMaskType: ${maskType}`); }This would make the method more robust against potential future changes or incorrect usage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 81-81: packages/core/src/BasicResources.ts#L81
Added line #L81 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-86: packages/core/src/BasicResources.ts#L86
Added line #L86 was not covered by tests
[warning] 88-88: packages/core/src/BasicResources.ts#L88
Added line #L88 was not covered by tests
[warning] 90-90: packages/core/src/BasicResources.ts#L90
Added line #L90 was not covered by tests
[warning] 93-93: packages/core/src/BasicResources.ts#L93
Added line #L93 was not covered by tests
[warning] 96-104: packages/core/src/BasicResources.ts#L96-L104
Added lines #L96 - L104 were not covered by tests
[warning] 106-106: packages/core/src/BasicResources.ts#L106
Added line #L106 was not covered by tests
241-265: LGTM: New private methods for material creation.The new private methods
_create2DMaterialand_createSpriteMaskMaterialare well-implemented and provide appropriate configurations for 2D and sprite mask rendering.Consider extracting the common material setup logic into a separate private method to reduce code duplication:
private _setupBaseMaterial(material: Material): void { material.isGCIgnored = true; material.renderState.rasterState.cullMode = CullMode.Off; } private _create2DMaterial(engine: Engine, shader: Shader): Material { const material = new Material(engine, shader); this._setupBaseMaterial(material); // ... rest of the 2D material setup ... } private _createSpriteMaskMaterial(engine: Engine): Material { const material = new Material(engine, Shader.find("SpriteMask")); this._setupBaseMaterial(material); // ... rest of the sprite mask material setup ... }This would improve maintainability and reduce the chance of inconsistencies between the two methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/core/src/BasicResources.ts (6 hunks)
- packages/core/src/RenderPipeline/RenderQueue.ts (5 hunks)
- packages/core/src/shader/state/BlendState.ts (2 hunks)
- packages/core/src/shader/state/RenderState.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/BasicResources.ts
[warning] 45-45: packages/core/src/BasicResources.ts#L45
Added line #L45 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: packages/core/src/BasicResources.ts#L52
Added line #L52 was not covered by tests
[warning] 55-55: packages/core/src/BasicResources.ts#L55
Added line #L55 was not covered by tests
[warning] 57-57: packages/core/src/BasicResources.ts#L57
Added line #L57 was not covered by tests
[warning] 59-59: packages/core/src/BasicResources.ts#L59
Added line #L59 was not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 65-69: packages/core/src/BasicResources.ts#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 81-81: packages/core/src/BasicResources.ts#L81
Added line #L81 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-86: packages/core/src/BasicResources.ts#L86
Added line #L86 was not covered by tests
[warning] 88-88: packages/core/src/BasicResources.ts#L88
Added line #L88 was not covered by tests
[warning] 90-90: packages/core/src/BasicResources.ts#L90
Added line #L90 was not covered by tests
[warning] 93-93: packages/core/src/BasicResources.ts#L93
Added line #L93 was not covered by tests
[warning] 96-104: packages/core/src/BasicResources.ts#L96-L104
Added lines #L96 - L104 were not covered by tests
[warning] 106-106: packages/core/src/BasicResources.ts#L106
Added line #L106 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)
packages/core/src/shader/state/BlendState.ts
[error] 180-180: 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 (8)
packages/core/src/shader/state/RenderState.ts (2)
38-39: Improved flexibility in render state managementThe addition of the
customRenderStatesparameter and its usage inblendState._applyandstencilState._applyenhances the flexibility of the render state management. This change addresses the customization concerns raised in previous comments and allows for more granular control over the rendering process.Also applies to: 46-46, 48-48
38-39: Verify impact on masks and shadowsWhile the changes improve flexibility, it's crucial to verify their impact on the overall rendering process, especially concerning masks and shadows as mentioned in the PR objectives.
Please run the following script to check for any potential conflicts or issues:
Also applies to: 46-46, 48-48
✅ Verification successful
Verified: Changes impact masks and shadows as intended
The modifications to
RenderStateare indeed relevant to masks and shadows, and they are likely to have a positive impact on the rendering process. Here's why:
- The addition of
shaderDataandcustomRenderStatesparameters provides more flexibility in managing render states, which is crucial for both mask and shadow rendering.- The codebase shows extensive usage of mask-related and shadow-related functionality that interacts with render states.
- These changes allow for more dynamic and context-specific render state modifications, which can be particularly useful for complex masking and shadow techniques.
The impact of these changes could include:
- More efficient mask rendering by allowing custom render states for specific mask interactions.
- Improved shadow quality or performance by enabling fine-tuned render state control during shadow passes.
- Enhanced flexibility in implementing advanced rendering techniques that require specific stencil or blend state configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for mask and shadow related code that might be affected by these changes # Test 1: Search for mask-related code echo "Searching for mask-related code:" rg --type typescript -i '\bmask\b' -C 5 # Test 2: Search for shadow-related code echo "Searching for shadow-related code:" rg --type typescript -i '\bshadow\b' -C 5 # Test 3: Search for stencil-related code echo "Searching for stencil-related code:" rg --type typescript -i '\bstencil\b' -C 5 # Test 4: Search for render state usage echo "Searching for render state usage:" rg --type typescript 'RenderState' -C 5Length of output: 622
Script:
#!/bin/bash # Description: Search for mask and shadow related code that might be affected by these changes # Test 1: Search for mask-related code echo "Searching for mask-related code:" rg --type ts -i '\bmask\b' -C 5 # Test 2: Search for shadow-related code echo "Searching for shadow-related code:" rg --type ts -i '\bshadow\b' -C 5 # Test 3: Search for stencil-related code echo "Searching for stencil-related code:" rg --type ts -i '\bstencil\b' -C 5 # Test 4: Search for render state usage echo "Searching for render state usage:" rg --type ts 'RenderState' -C 5Length of output: 247327
packages/core/src/BasicResources.ts (4)
Line range hint
1-39: LGTM: New imports and static properties for mask rendering.The new imports and static properties have been correctly added to support the mask rendering functionality. The properties are appropriately typed and initialized.
Line range hint
121-173: LGTM: New default materials and constructor updates.The new properties for default materials have been correctly added and initialized in the constructor. The use of private methods for material creation promotes code reusability and maintainability.
Line range hint
1-265: Overall assessment: Well-implemented mask rendering functionality.The changes to
BasicResources.tsintroduce robust mask rendering functionality and efficient material creation methods. The implementation is generally well-structured, with appropriate use of memoization and internal visibility modifiers.Key points:
- New static methods for managing mask render states are efficient and flexible.
- New default materials are correctly initialized and configured.
- Private methods for material creation promote code reusability.
Suggestions for improvement:
- Add error handling for unexpected enum values in static methods.
- Consider extracting common material setup logic to reduce duplication.
- Increase test coverage for the new methods to ensure long-term reliability.
These changes appear to be a valuable addition to the codebase, enhancing its capabilities for 2D and mask rendering.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-45: packages/core/src/BasicResources.ts#L45
Added line #L45 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: packages/core/src/BasicResources.ts#L52
Added line #L52 was not covered by tests
[warning] 55-55: packages/core/src/BasicResources.ts#L55
Added line #L55 was not covered by tests
[warning] 57-57: packages/core/src/BasicResources.ts#L57
Added line #L57 was not covered by tests
[warning] 59-59: packages/core/src/BasicResources.ts#L59
Added line #L59 was not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 65-69: packages/core/src/BasicResources.ts#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 81-81: packages/core/src/BasicResources.ts#L81
Added line #L81 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-86: packages/core/src/BasicResources.ts#L86
Added line #L86 was not covered by tests
[warning] 88-88: packages/core/src/BasicResources.ts#L88
Added line #L88 was not covered by tests
[warning] 90-90: packages/core/src/BasicResources.ts#L90
Added line #L90 was not covered by tests
[warning] 93-93: packages/core/src/BasicResources.ts#L93
Added line #L93 was not covered by tests
[warning] 96-104: packages/core/src/BasicResources.ts#L96-L104
Added lines #L96 - L104 were not covered by tests
[warning] 106-106: packages/core/src/BasicResources.ts#L106
Added line #L106 was not covered by tests
41-107: Consider adding unit tests for new methods.The static analysis tool indicates that many of the new lines in the
_getMaskInteractionRenderStatesand_getMaskTypeRenderStatesmethods are not covered by tests. While full coverage isn't always necessary, adding unit tests for these methods would help ensure their correctness and prevent potential regressions in the future.Consider adding tests that:
- Verify the correct render states are returned for different mask interactions and types.
- Check that memoization is working as expected (i.e., the same object is returned for repeated calls with the same parameters).
- Test edge cases and potential error conditions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-45: packages/core/src/BasicResources.ts#L45
Added line #L45 was not covered by tests
[warning] 50-50: packages/core/src/BasicResources.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: packages/core/src/BasicResources.ts#L52
Added line #L52 was not covered by tests
[warning] 55-55: packages/core/src/BasicResources.ts#L55
Added line #L55 was not covered by tests
[warning] 57-57: packages/core/src/BasicResources.ts#L57
Added line #L57 was not covered by tests
[warning] 59-59: packages/core/src/BasicResources.ts#L59
Added line #L59 was not covered by tests
[warning] 62-62: packages/core/src/BasicResources.ts#L62
Added line #L62 was not covered by tests
[warning] 65-69: packages/core/src/BasicResources.ts#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-71: packages/core/src/BasicResources.ts#L71
Added line #L71 was not covered by tests
[warning] 76-76: packages/core/src/BasicResources.ts#L76
Added line #L76 was not covered by tests
[warning] 81-81: packages/core/src/BasicResources.ts#L81
Added line #L81 was not covered by tests
[warning] 83-83: packages/core/src/BasicResources.ts#L83
Added line #L83 was not covered by tests
[warning] 86-86: packages/core/src/BasicResources.ts#L86
Added line #L86 was not covered by tests
[warning] 88-88: packages/core/src/BasicResources.ts#L88
Added line #L88 was not covered by tests
[warning] 90-90: packages/core/src/BasicResources.ts#L90
Added line #L90 was not covered by tests
[warning] 93-93: packages/core/src/BasicResources.ts#L93
Added line #L93 was not covered by tests
[warning] 96-104: packages/core/src/BasicResources.ts#L96-L104
Added lines #L96 - L104 were not covered by tests
[warning] 106-106: packages/core/src/BasicResources.ts#L106
Added line #L106 was not covered by testspackages/core/src/shader/state/BlendState.ts (2)
151-156: Update to_applymethod enhances flexibilityThe addition of the optional
customStatesparameter to the_applymethod allows for custom state configurations during the blend state application process, enhancing the flexibility and control over rendering states.
159-163: Update to_platformApplymethod allows dynamic state overridesIncluding the optional
customStatesparameter in the_platformApplymethod enables dynamic overrides of specific render state elements, providing greater adaptability in state management.
| sort(compareFunc: Function): void { | ||
| Utils._quickSort(this.elements, 0, this.elements.length, compareFunc); | ||
| } |
There was a problem hiding this comment.
Improve type safety in the sort method
The sort method uses Function as the type for compareFunc, which is not type-safe and can lead to potential bugs. Consider using a more specific function type to improve type safety and make the expected function signature explicit.
Replace the Function type with a specific function type:
sort(compareFunc: (a: RenderElement, b: RenderElement) => number): void {
Utils._quickSort(this.elements, 0, this.elements.length, compareFunc);
}This change will provide better type safety and make the expected function signature clear to developers using this method.
🧰 Tools
🪛 Biome
[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)
| sortBatch(compareFunc: Function, batcherManager: BatcherManager): void { | ||
| this._sort(compareFunc); | ||
| this._batch(batcherManager); | ||
| this.sort(compareFunc); | ||
| this.batch(batcherManager); | ||
| } |
There was a problem hiding this comment.
Improve type safety in the sortBatch method
While the refactoring of sortBatch to use the new sort and batch methods improves code modularity, the compareFunc parameter is still typed as Function, which is not type-safe.
Update the method signature to use a more specific function type:
sortBatch(compareFunc: (a: RenderElement, b: RenderElement) => number, batcherManager: BatcherManager): void {
this.sort(compareFunc);
this.batch(batcherManager);
}This change will improve type safety and consistency with the sort method.
🧰 Tools
🪛 Biome
[error] 34-34: 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)
| const colorWriteMaskState = customStates[RenderStateElementKey.BlendStateColorWriteMask0]; | ||
| colorWriteMaskState !== undefined && (colorWriteMask = <number>colorWriteMaskState); |
There was a problem hiding this comment.
Avoid assignments within expressions for better readability
At line 180, the assignment is performed within an expression using the && operator. This practice can reduce code readability and may lead to confusion, as it obscures the side effect of the assignment.
Recommend refactoring the code to separate the condition from the assignment for clarity:
const colorWriteMaskState = customStates[RenderStateElementKey.BlendStateColorWriteMask0];
- colorWriteMaskState !== undefined && (colorWriteMask = <number>colorWriteMaskState);
+ if (colorWriteMaskState !== undefined) {
+ colorWriteMask = <number>colorWriteMaskState;
+ }📝 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.
| const colorWriteMaskState = customStates[RenderStateElementKey.BlendStateColorWriteMask0]; | |
| colorWriteMaskState !== undefined && (colorWriteMask = <number>colorWriteMaskState); | |
| const colorWriteMaskState = customStates[RenderStateElementKey.BlendStateColorWriteMask0]; | |
| if (colorWriteMaskState !== undefined) { | |
| colorWriteMask = <number>colorWriteMaskState; | |
| } |
🧰 Tools
🪛 Biome
[error] 180-180: 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/core/src/RenderPipeline/MaskManager.ts (3 hunks)
🧰 Additional context used
🪛 Biome
packages/core/src/RenderPipeline/MaskManager.ts
[error] 35-35: 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)
packages/core/src/RenderPipeline/MaskManager.ts (1)
34-35: Fix the assignment within the expressionThe assignment
replaced._maskIndex = mask._maskIndexshould not be part of the logical expression. It makes the code harder to read and can lead to confusion.🧰 Tools
🪛 Biome
[error] 35-35: 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)
| 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); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability by using an explicit if statement
Using logical operators like && for control flow can reduce code readability. Consider refactoring to use an explicit if statement.
Apply this change:
- mask.influenceLayers & preMaskLayer && decrementMaskQueue.pushRenderElement(mask._renderElement);
+ if (mask.influenceLayers & preMaskLayer) {
+ decrementMaskQueue.pushRenderElement(mask._renderElement);
+ }📝 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.
| mask.influenceLayers & preMaskLayer && decrementMaskQueue.pushRenderElement(mask._renderElement); | |
| if (mask.influenceLayers & preMaskLayer) { | |
| decrementMaskQueue.pushRenderElement(mask._renderElement); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/core/src/RenderPipeline/RenderQueue.ts (3)
34-44: Approve refactoring of sort and batch operations, but improve type safetyThe separation of
sortandbatchoperations into distinct methods improves code modularity and readability. However, the use ofFunctiontype forcompareFuncin bothsortBatchandsortmethods is not type-safe.To improve type safety, consider updating the method signatures as follows:
sortBatch(compareFunc: (a: RenderElement, b: RenderElement) => number, batcherManager: BatcherManager): void { this.sort(compareFunc); this.batch(batcherManager); } sort(compareFunc: (a: RenderElement, b: RenderElement) => number): void { Utils._quickSort(this.elements, 0, this.elements.length, compareFunc); }This change will provide better type safety and make the expected function signature explicit.
🧰 Tools
🪛 Biome
[error] 34-34: 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] 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)
82-95: Improved mask and stencil state handling in render methodThe new logic for handling mask interactions and types enhances the rendering process. The use of
maskManagerfor drawing masks and checking stencil access, along with the retrieval of custom render states based on mask interaction or type, should lead to more predictable rendering behavior.To improve readability, consider extracting the mask interaction logic into a separate method:
private handleMaskInteraction( maskInteraction: SpriteMaskInteraction, maskType: RenderQueueMaskType, context: RenderContext, pipelineStageTagValue: string, subElement: SubRenderElement, material: Material ): Record<number, number | boolean> | null { const maskInteractionNotNone = maskInteraction !== SpriteMaskInteraction.None; if (maskInteractionNotNone) { this.scene._maskManager.drawMask(context, pipelineStageTagValue, subElement.component._maskLayer); } else { this.scene._maskManager.checkStencilAccess(material) & StencilAccess.Writable && (this.scene._maskManager.notWriteStencil = false); } if (maskInteractionNotNone) { return BasicResources.getMaskInteractionRenderStates(maskInteraction); } else if (maskType !== RenderQueueMaskType.No) { return BasicResources.getMaskTypeRenderStates(maskType); } return null; }This extraction would make the
rendermethod cleaner and easier to understand.🧰 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)
87-87: Improve readability of conditional assignmentThe current line uses an assignment within a condition, which can be less readable:
maskManager.checkStencilAccess(material) & StencilAccess.Writable && (maskManager.notWriteStencil = false);Consider rewriting this in a more explicit manner:
if (maskManager.checkStencilAccess(material) & StencilAccess.Writable) { maskManager.notWriteStencil = false; }This change improves readability and reduces the potential for confusion or errors.
🧰 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/BasicResources.ts (3)
30-97: LGTM: Stencil state management implementation.The new static properties and methods for managing stencil states are well-implemented. They provide a centralized and efficient way to handle different mask interactions and types. The caching mechanism is a good optimization.
Consider adding JSDoc comments to these methods to improve documentation.
231-255: LGTM: Material creation methods.The new private methods
_create2DMaterialand_createSpriteMaskMaterialare well-implemented, encapsulating the logic for creating specific types of materials. This promotes code reuse and maintainability.Consider adding error handling for cases where the shader might not be found.
Line range hint
1-264: Overall assessment: Significant improvements to stencil state management and material handling.The changes in this file represent a substantial enhancement to the
BasicResourcesclass. The introduction of centralized stencil state management and default materials improves code organization, performance, and reusability. These modifications align well with the PR objectives of refactoring the mask functionality.To further improve the code:
- Consider adding comprehensive JSDoc comments to new methods and properties.
- Implement error handling in material creation methods.
- Add unit tests to verify the behavior of the new stencil state management methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/core/src/BasicResources.ts (6 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/Scene.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/RenderPipeline/BasicRenderPipeline.ts
- packages/core/src/Scene.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)
🔇 Additional comments (7)
packages/core/src/RenderPipeline/RenderQueue.ts (2)
2-2: Improved code organization with new imports and refactored mask handlingThe addition of
BasicResourcesandStencilAccessimports, along with the destructuring of_maskManagerfrom the scene object, indicates a positive refactoring of mask handling logic. This change likely improves code organization and separation of concerns.Also applies to: 4-4, 7-7, 60-60
98-98: Improved flexibility in render state applicationThe addition of
customStatesto the_applyStatesmethod call enhances the flexibility of render state control. This change is consistent with the earlier improvements in mask and stencil state handling, allowing for more granular control over the rendering process.Also applies to: 175-176
packages/core/src/BasicResources.ts (3)
1-3: LGTM: New imports and type alias.The new imports and the
RenderStateElementMaptype alias are well-structured and necessary for the added functionality. They improve code organization and type safety.Also applies to: 14-21, 264-264
111-113: LGTM: Default material properties.The addition of default material properties for sprites, text, and sprite masks is a good optimization. It provides easy access to commonly used materials, potentially improving performance and reducing code duplication.
161-163: LGTM: Constructor initialization of default materials.The initialization of default materials in the constructor is well-placed and consistent with the initialization of other properties. This ensures that all necessary resources are ready when an instance of
BasicResourcesis created.packages/core/src/RenderPipeline/MaskManager.ts (2)
88-115:checkStencilAccessmethod is comprehensiveThe
checkStencilAccessmethod effectively determines stencil read and write access based on the material's stencil state. The logic is thorough and correctly identifies both readable and writable conditions.
43-59:drawMaskmethod logic is well-structuredThe
drawMaskmethod properly initializes and renders the increment and decrement mask queues for the specified mask layer. The separation of concerns and clear flow enhance maintainability and readability.
| @@ -19,29 +25,109 @@ | |||
| return (MaskManager._maskDecrementRenderQueue ||= new RenderQueue(RenderQueueType.Transparent)); | |||
| } | |||
|
|
|||
| allSpriteMasks = new DisorderedArray<SpriteMask>(); | |||
| preMaskLayer = 0; | |||
| notWriteStencil = true; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming notWriteStencil to improve clarity
The property notWriteStencil uses a double negative, which can be confusing. Renaming it to something like writeStencilEnabled or shouldWriteStencil would enhance readability and make its purpose clearer.
| const masks = this._allSpriteMasks; | ||
| const maskElements = masks._elements; |
There was a problem hiding this comment.
Avoid accessing private properties directly
Accessing the _elements property of DisorderedArray directly may violate encapsulation and could lead to maintenance issues if the internal implementation changes. Consider using public methods or iterators provided by DisorderedArray to access its elements.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
RenderQueueclass.MaskManagerclass.