Fix scene/camera shaderData in blitter #2558
Conversation
WalkthroughThe changes introduce a local Changes
Sequence Diagram(s)sequenceDiagram
participant BP as Blitter
participant C as Context
participant SP as Shader Program
BP->>C: Retrieve camera
C-->>BP: Returns camera
BP->>SP: Upload sceneUniformBlock with camera data
BP->>SP: Upload cameraUniformBlock with camera data
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/RenderPipeline/Blitter.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (2)
packages/core/src/RenderPipeline/Blitter.ts (2)
51-51: LGTM! Good optimization.Storing the frequently accessed camera object in a local variable improves code readability and potentially performance by reducing property access.
78-79: LGTM! Successfully implements shader data access.The added shader data uploads correctly implement the PR objective of enabling access to scene and camera shader data in the blitter. The implementation follows a logical order and maintains consistency with the existing pattern.
Note: This change enables access to shader data elements like
camera_DepthTextureas mentioned in the PR objectives.
| ShaderMacroCollection.unionCollection( | ||
| context.camera._globalShaderMacro, | ||
| camera._globalShaderMacro, | ||
| blitMaterial.shaderData._macroCollection, | ||
| compileMacros | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider using a public accessor for the shader macro.
The direct access to _globalShaderMacro (with underscore prefix) suggests it's a private property. Consider using a public accessor method if available to make the code more maintainable and resilient to internal implementation changes.
Let's check if a public accessor exists:
🏁 Script executed:
#!/bin/bash
# Search for potential public accessors for globalShaderMacro
rg -A 5 "globalShaderMacro" --type tsLength of output: 5226
Action Required: Introduce a Public Accessor for Shader Macro
Our investigation confirms that no public accessor exists for the shader macro—the codebase consistently accesses the internal _globalShaderMacro property (e.g., in Camera, Renderer, Scene, etc.). Direct use of this internal property could make the code susceptible to breaking changes if its implementation evolves.
- Recommendation: Consider adding a public getter (e.g.,
getGlobalShaderMacro()) in the relevant class (such as Camera or Renderer) to encapsulate the shader macro collection. - Next Step: Update the call in
packages/core/src/RenderPipeline/Blitter.tsto use this new accessor once implemented, or document the rationale for direct access if it’s intended for internal usage.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2558 +/- ##
==========================================
+ Coverage 68.90% 68.92% +0.01%
==========================================
Files 961 961
Lines 100266 100269 +3
Branches 8679 8682 +3
==========================================
+ Hits 69092 69108 +16
+ Misses 30914 30901 -13
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Users should be allowed to use the shader data of scene and camera in blitter shader, such as
camera_DepthTexture.Summary by CodeRabbit