properly bind fragment uniform buffer in multi-pass rendering #337
Conversation
Previously, the fragment uniform buffer (`fragmentParamBuffer_`) was not correctly bound for non-OpenGL backends, which could result in incorrect rendering or unexpected colors in multi-pass render sessions. - Ensures `fragmentParamBuffer_` is bound in `render()` for Metal and D3D12. - Maintains correct uniform binding for OpenGL backends. - Verified multi-pass rendering output remains consistent across backends. Ref: TQMultiRenderPassSession.cpp
corporateshark
left a comment
corporateshark
left a comment
There was a problem hiding this comment.
Please avoid reformatting any existing code. It makes the review and merge process much more involved.
Added a check to ensure fragmentParamBuffer_ is valid before binding in the render pass. This prevents potential crashes while keeping all rendering logic and backend support intact.
corporateshark
left a comment
corporateshark
left a comment
There was a problem hiding this comment.
Please address the above-mentioned comments.
Add null check for fragmentParamBuffer when binding in render() to avoid potential issues on non-OpenGL backends.
SajanGhimire1
left a comment
SajanGhimire1
left a comment
There was a problem hiding this comment.
Addressed the requested changes: added explicit check for fragment uniform buffer pointer without reformatting existing code.
| struct VertexPosUv { | ||
| iglu::simdtypes::float3 position; // SIMD 128b aligned | ||
| iglu::simdtypes::float2 uv; // SIMD 128b aligned | ||
| iglu::simdtypes::float2 uv; // SIMD 128b aligned |
There was a problem hiding this comment.
No need to change this line.
| {{1.0f, 1.0f, 0.0}, {1.0, 1.0}}, | ||
| {{-1.0f, -1.0, 0.0}, {0.0, 0.0}}, | ||
| {{1.0f, -1.0f, 0.0}, {1.0, 0.0}}, | ||
| {{1.0f, -1.0, 0.0}, {1.0, 0.0}}, |
There was a problem hiding this comment.
Please avoid changing this.
|
|
||
| if (backend != igl::BackendType::OpenGL) { | ||
| commands->bindBuffer(0, fragmentParamBuffer.get()); | ||
| if (fragmentParamBuffer) { |
There was a problem hiding this comment.
Since calling bindBuffer() with a nullptr buffer is allowed by the API contract, what exact problem is this PR trying to solve?
Removed unnecessary closing brace for conditional block.
|
@corporateshark has imported this pull request. If you are a Meta employee, you can view this in D90706470. |
|
@corporateshark merged this pull request in bd388a9. |
3 participants
Fix syntax error in uniform buffer binding
Fixed misplaced brace that prevented uniform buffers from being bound
for Metal/D3D12 backends in multi-pass rendering.
Root Cause: The original code had incorrect brace placement: