Fix renderQueueType error #2437
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request enhance the logic within 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2437 +/- ##
==========================================
+ Coverage 69.44% 70.77% +1.33%
==========================================
Files 524 524
Lines 27484 27487 +3
Branches 4112 4111 -1
==========================================
+ Hits 19085 19453 +368
+ Misses 6894 6479 -415
- Partials 1505 1555 +50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/RenderPipeline/RenderQueue.ts (2)
108-108: Consider adding type validation for renderState.While the simplified check is good, consider adding runtime validation to ensure
renderStateis never undefined at this point, as that could lead to a TypeError.+ if (!renderState) { + continue; + } if (renderState.renderQueueType !== renderQueueType) { continue; }
102-108: Add tests to prevent regression.The changes look good, but according to the PR description, no tests were added. Consider adding tests that verify:
- Correct precedence of render states
- Proper application of shader-specific render queue types
- Edge cases with undefined or null render states
Would you like me to help create test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/RenderPipeline/RenderQueue.ts(1 hunks)
🔇 Additional comments (2)
packages/core/src/RenderPipeline/RenderQueue.ts (2)
102-103: LGTM! Clear precedence for render state resolution.
The nullish coalescing operator provides a clean way to prioritize shader pass render state while falling back to render states array when needed.
104-106: Verify that this fixes the renderQueueType override issue.
This change ensures that shader-specific render state is properly applied before the render queue type check. Let's verify this resolves the reported issue where renderQueueType was being incorrectly overridden.
✅ Verification successful
Verified: The renderQueueType override fix is correctly implemented
The implementation in RenderState._applyRenderQueueByShaderData shows that the renderQueueType is properly handled:
- It first checks if renderQueueType exists in the shader pass's renderStateDataMap
- If present, it sets the renderQueueType from shader data with a fallback to Opaque
- This happens before the render queue type check in the render pipeline, ensuring correct queue type application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related tests or usage patterns
rg -A 5 "_renderStateDataMap"
# Look for any other places where renderQueueType might be modified
ast-grep --pattern 'renderQueueType = $_'
# Check for related bug reports or issues
rg -l "renderQueueType.*override"
Length of output: 4558
Script:
#!/bin/bash
# Check the implementation of _applyRenderQueueByShaderData to understand the fix
ast-grep --pattern '_applyRenderQueueByShaderData($_,$_) {
$$$
}'
# Look for RenderState class definition and renderQueueType property
ast-grep --pattern 'class RenderState {
$$$
}'
# Search for any tests related to render queue type
rg -l "renderQueueType.*test"
Length of output: 5238
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/shader/state/RenderState.ts (1)
64-67: Consider adding type safety for renderStateDataMap values.The implementation looks good and handles undefined cases correctly. However, there's room for improvement in type safety.
Consider adding a type guard or assertion to ensure
renderStateDataMap[RenderStateElementKey.RenderQueueType]is of typeShaderProperty:_getRenderQueueByShaderData( renderStateDataMap: Record<number, ShaderProperty>, shaderData: ShaderData ): RenderQueueType { const renderQueueType = renderStateDataMap[RenderStateElementKey.RenderQueueType]; - if (renderQueueType === undefined) { + if (!renderQueueType || !(renderQueueType instanceof ShaderProperty)) { return this.renderQueueType; } else { return shaderData.getFloat(renderQueueType) ?? RenderQueueType.Opaque; } }packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
272-275: Consider adding test coverage for render queue assignmentSince this fixes a critical rendering issue, it would be valuable to add test cases that verify:
- Correct queue type determination for different shader configurations
- Proper handling of render states
- Queue assignment for each type (Opaque, AlphaTest, Transparent)
Would you like me to help create test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(1 hunks)packages/core/src/RenderPipeline/RenderQueue.ts(1 hunks)packages/core/src/shader/state/RenderState.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/RenderPipeline/RenderQueue.ts
🔇 Additional comments (3)
packages/core/src/shader/state/RenderState.ts (2)
59-62: LGTM! Method signature change improves code design.
The change from void to returning RenderQueueType makes the code more functional and gives better control to callers for determining the final queue type.
59-67: Verify all callers are updated to handle the return value.
Let's ensure all callers are properly updated to handle the return value instead of expecting the previous void behavior.
✅ Verification successful
Let me analyze the call sites of _getRenderQueueByShaderData to verify how the return value is being used.
Based on the analysis of the call sites, I can now generate the final response:
The return value is properly handled by all callers
The method _getRenderQueueByShaderData is called in two places:
BasicRenderPipeline.ts: assigns the return value torenderQueueTypeRenderQueue.ts: assigns the return value topassQueueType
Both callers correctly handle the return value of type RenderQueueType, and there are no instances of the old method name in the codebase, indicating a complete migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to both old and new method names to ensure complete migration
echo "Searching for old method calls (_applyRenderQueueByShaderData):"
rg "_applyRenderQueueByShaderData" --type ts
echo -e "\nSearching for new method calls (_getRenderQueueByShaderData):"
rg "_getRenderQueueByShaderData" --type ts
echo -e "\nSearching for RenderState usage patterns:"
ast-grep --pattern 'new RenderState()'
Length of output: 1392
Script:
#!/bin/bash
# Check how the return value is used in BasicRenderPipeline
echo "BasicRenderPipeline usage:"
rg -B 2 -A 2 "_getRenderQueueByShaderData" packages/core/src/RenderPipeline/BasicRenderPipeline.ts
echo -e "\nRenderQueue usage:"
rg -B 2 -A 2 "_getRenderQueueByShaderData" packages/core/src/RenderPipeline/RenderQueue.ts
# Check the method implementation
echo -e "\nRenderState implementation:"
rg -B 5 -A 5 "_getRenderQueueByShaderData" packages/core/src/shader/state/RenderState.ts
Length of output: 1297
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (1)
272-275: LGTM! Fix for renderQueueType determination
The change from _applyRenderQueueByShaderData to _getRenderQueueByShaderData correctly addresses the issue where renderQueueType was being overridden. The new method properly determines the queue type based on shader data without modifying it.
Let's verify the impact of this change on render queue assignments:
✅ Verification successful
Based on the search results, I can now generate the final response:
Verified: renderQueueType determination change is safe and consistent
The change to use _getRenderQueueByShaderData is correct and aligns with existing usage patterns in the codebase:
- The method is already being used in
RenderQueue.tsfor queue type determination - No other instances of the old
_applyRenderQueueByShaderDatamethod exist, confirming complete migration - All other renderQueueType modifications happen through proper state management in
RenderStateand material classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other potential renderQueue type modifications
# Look for any other places that might be modifying renderQueueType
# Search for methods that might modify renderQueueType
rg -g '*.ts' '(set|apply|modify|override).*[Rr]enderQueue'
# Search for direct assignments to renderQueueType
rg -g '*.ts' 'renderQueueType\s*='
# Search for related RenderState usage
rg -g '*.ts' '_getRenderQueueByShaderData|_applyRenderQueueByShaderData'
Length of output: 3487
GuoLei1990
force-pushed
the
fix/renderqueue-order
branch
from
5c35eff
to
c9701ee
Compare
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug Fix.
What is the current behavior? (You can also link to an open issue here)
ShaderPass's
renderQueueTypewill be override byBasicRenderPipeline.pushRenderElementByTypeWhat is the new behavior (if this is a feature change)?
Should Correct true
renderQueueTypebefore render.Summary by CodeRabbit