WIP: fix: correct SSBO/UBO binding point resolution in material system#2647
WIP: fix: correct SSBO/UBO binding point resolution in material system#2647zzuegg wants to merge 1 commit intojMonkeyEngine:masterfrom
Conversation
The material system was using wrong binding points for shader storage and uniform buffer objects, causing data to be bound to incorrect slots. - GLRenderer.updateShaderBufferBlock() now queries the actual binding from the compiled shader (glGetActiveUniformBlocki for UBOs, glGetProgramResourceiv for SSBOs) and respects layout(binding=N). Falls back to blockIndex when no explicit binding is declared. - Remove premature renderer.setShaderStorageBufferObject/ setUniformBufferObject calls from Material.updateShaderMaterialParameter that used a sequential counter as binding point. - Cache binding on ShaderBufferBlock (per-shader) instead of BufferObject (per-buffer), since the same buffer can be bound at different points in different shaders. - Add glGetActiveUniformBlocki to GL3 and glGetProgramResourceiv + GL_BUFFER_BINDING to GL4 with implementations in both LWJGL backends. - BufferObject.initializeEmpty() now calls setUpdateNeeded() so the GPU buffer gets allocated on first use. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves critical issues with Shader Storage Buffer Object (SSBO) and Uniform Buffer Object (UBO) binding within the material system. It introduces a more robust mechanism for resolving GL binding points by dynamically querying compiled shaders, which ensures that buffer data is correctly associated with its intended shader slots. A significant benefit of this change is the enhanced flexibility for developers, who can now utilize SSBOs and UBOs through the material system without needing to explicitly declare Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the SSBO/UBO binding logic to query binding points from compiled shaders and provides a fallback for shaders without explicit bindings, which is a significant improvement. I've identified one potential bug regarding the handling of layout(binding=0) and a minor performance optimization opportunity. Overall, the changes are well-structured and address the intended problem effectively.
| if (bindingPoint == 0) { | ||
| bindingPoint = blockIndex; | ||
| } |
There was a problem hiding this comment.
This logic incorrectly handles layout(binding=0). When a uniform block has an explicit layout(binding=0), the GL query will return 0. This code then overwrites bindingPoint with blockIndex, which might be different from 0, thus ignoring the explicit binding from the shader. This is a bug for shaders that use layout(binding=0). The ambiguity of the return value 0 from the GL functions makes this tricky, but the current implementation doesn't fully respect layout(binding=N) as intended.
There was a problem hiding this comment.
i don't think there is a correct solution, if binding trough the material system is allowed, and the direct binding version too. then unfortuantely due to opengl returning 0 for "not defined" creates always a a point where collision can happen.
| IntBuffer params = BufferUtils.createIntBuffer(1); | ||
| gl4.glGetProgramResourceiv(program, GL4.GL_SHADER_STORAGE_BLOCK, blockIndex, props, null, params); | ||
| return params.get(0); |
There was a problem hiding this comment.
This method allocates a new IntBuffer for params on every call. To improve performance and reduce garbage, you can reuse the class member intBuf1 which is already available for this purpose.
| IntBuffer params = BufferUtils.createIntBuffer(1); | |
| gl4.glGetProgramResourceiv(program, GL4.GL_SHADER_STORAGE_BLOCK, blockIndex, props, null, params); | |
| return params.get(0); | |
| intBuf1.clear(); | |
| gl4.glGetProgramResourceiv(program, GL4.GL_SHADER_STORAGE_BLOCK, blockIndex, props, null, intBuf1); | |
| return intBuf1.get(0); |
SSBO and UBO buffers set via the Material system were bound to wrong GL binding points, causing shader data to end up in the wrong slots. This fix queries the actual binding
point from the compiled shader and assigns unique fallback bindings when none is declared, so buffers reliably match their GLSL layout(binding=N) declarations.
This also enables using SSBOs and UBOs through the material system — including via MatParamOverride — without requiring layout(binding=N) in the shader. The renderer
automatically assigns a unique binding point per block, so multiple buffers won't collide. Just set the buffer on the material and it works.