-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
WIP: fix: correct SSBO/UBO binding point resolution in material system #2647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
b1cbcc7
0a76df2
775d769
d706616
efbb280
3f5c986
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1456,7 +1456,6 @@ protected void updateShaderBufferBlock(final Shader shader, final ShaderBufferBl | |||||||||||||
|
|
||||||||||||||
| final BufferObject bufferObject = bufferBlock.getBufferObject(); | ||||||||||||||
| final BufferType bufferType = bufferBlock.getType(); | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| if (bufferObject.isUpdateNeeded()) { | ||||||||||||||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||||||||||||||
|
|
@@ -1473,35 +1472,52 @@ protected void updateShaderBufferBlock(final Shader shader, final ShaderBufferBl | |||||||||||||
|
|
||||||||||||||
| final int shaderId = shader.getId(); | ||||||||||||||
|
|
||||||||||||||
| int bindingPoint = bufferObject.getBinding(); | ||||||||||||||
| // Resolve the block index (location) from the compiled shader | ||||||||||||||
| int blockIndex = bufferBlock.getLocation(); | ||||||||||||||
| if (blockIndex < 0) { | ||||||||||||||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||||||||||||||
| blockIndex = gl4.glGetProgramResourceIndex(shaderId, GL4.GL_SHADER_STORAGE_BLOCK, bufferBlock.getName()); | ||||||||||||||
| } else { | ||||||||||||||
| blockIndex = gl3.glGetUniformBlockIndex(shaderId, bufferBlock.getName()); | ||||||||||||||
| } | ||||||||||||||
| bufferBlock.setLocation(blockIndex); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Block not found in the shader — skip silently | ||||||||||||||
| if (blockIndex < 0 || blockIndex == NativeObject.INVALID_ID) { | ||||||||||||||
| bufferBlock.clearUpdateNeeded(); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Resolve the binding point for this block. First try to read the | ||||||||||||||
| // binding declared in the shader (layout(binding=N)). If the query | ||||||||||||||
| // returns 0 the shader may not have an explicit binding, so fall | ||||||||||||||
| // back to blockIndex to avoid multiple blocks colliding on point 0. | ||||||||||||||
| int bindingPoint = bufferBlock.getBinding(); | ||||||||||||||
| if (bindingPoint < 0) { | ||||||||||||||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||||||||||||||
| bindingPoint = queryShaderStorageBlockBinding(shaderId, blockIndex); | ||||||||||||||
| } else { | ||||||||||||||
| bindingPoint = gl3.glGetActiveUniformBlocki(shaderId, blockIndex, GL3.GL_UNIFORM_BLOCK_BINDING); | ||||||||||||||
| } | ||||||||||||||
| if (bindingPoint == 0) { | ||||||||||||||
| bindingPoint = blockIndex; | ||||||||||||||
| } | ||||||||||||||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||||||||||||||
| gl4.glShaderStorageBlockBinding(shaderId, blockIndex, bindingPoint); | ||||||||||||||
| } else { | ||||||||||||||
| gl3.glUniformBlockBinding(shaderId, blockIndex, bindingPoint); | ||||||||||||||
| } | ||||||||||||||
| bufferBlock.setBinding(bindingPoint); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| switch (bufferType) { | ||||||||||||||
| case UniformBufferObject: { | ||||||||||||||
| setUniformBufferObject(bindingPoint, bufferObject); // rebind buffer if needed | ||||||||||||||
| if (bufferBlock.isUpdateNeeded()) { | ||||||||||||||
| int blockIndex = bufferBlock.getLocation(); | ||||||||||||||
| if (blockIndex < 0) { | ||||||||||||||
| blockIndex = gl3.glGetUniformBlockIndex(shaderId, bufferBlock.getName()); | ||||||||||||||
| bufferBlock.setLocation(blockIndex); | ||||||||||||||
| } | ||||||||||||||
| if (bufferBlock.getLocation() != NativeObject.INVALID_ID) { | ||||||||||||||
| gl3.glUniformBlockBinding(shaderId, bufferBlock.getLocation(), bindingPoint); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| setUniformBufferObject(bindingPoint, bufferObject); | ||||||||||||||
| break; | ||||||||||||||
| } | ||||||||||||||
| case ShaderStorageBufferObject: { | ||||||||||||||
| setShaderStorageBufferObject(bindingPoint, bufferObject); // rebind buffer if needed | ||||||||||||||
| if (bufferBlock.isUpdateNeeded() ) { | ||||||||||||||
| int blockIndex = bufferBlock.getLocation(); | ||||||||||||||
| if (blockIndex < 0) { | ||||||||||||||
| blockIndex = gl4.glGetProgramResourceIndex(shaderId, GL4.GL_SHADER_STORAGE_BLOCK, bufferBlock.getName()); | ||||||||||||||
| bufferBlock.setLocation(blockIndex); | ||||||||||||||
| } | ||||||||||||||
| if (bufferBlock.getLocation() != NativeObject.INVALID_ID) { | ||||||||||||||
| gl4.glShaderStorageBlockBinding(shaderId, bufferBlock.getLocation(), bindingPoint); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| setShaderStorageBufferObject(bindingPoint, bufferObject); | ||||||||||||||
| break; | ||||||||||||||
| } | ||||||||||||||
| default: { | ||||||||||||||
|
|
@@ -1512,6 +1528,22 @@ protected void updateShaderBufferBlock(final Shader shader, final ShaderBufferBl | |||||||||||||
| bufferBlock.clearUpdateNeeded(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Queries the binding point of a shader storage block using | ||||||||||||||
| * glGetProgramResourceiv with GL_BUFFER_BINDING. | ||||||||||||||
| * | ||||||||||||||
| * @param program the shader program id. | ||||||||||||||
| * @param blockIndex the block index within the program. | ||||||||||||||
| * @return the binding point assigned to the block. | ||||||||||||||
| */ | ||||||||||||||
| private int queryShaderStorageBlockBinding(int program, int blockIndex) { | ||||||||||||||
| IntBuffer props = BufferUtils.createIntBuffer(1); | ||||||||||||||
| props.put(GL4.GL_BUFFER_BINDING).flip(); | ||||||||||||||
| IntBuffer params = BufferUtils.createIntBuffer(1); | ||||||||||||||
| gl4.glGetProgramResourceiv(program, GL4.GL_SHADER_STORAGE_BLOCK, blockIndex, props, null, params); | ||||||||||||||
| return params.get(0); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method allocates a new
Suggested change
|
||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+1592
to
+1598
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To improve performance and reduce garbage collection, you can avoid re-allocating For example, you could add these fields to private static final IntBuffer PROPS_BUFFER_BINDING = (IntBuffer) BufferUtils.createIntBuffer(1).put(GL4.GL_BUFFER_BINDING).flip();
private final IntBuffer resourceParams = BufferUtils.createIntBuffer(1);Then this method would become: private int queryShaderStorageBlockBinding(int program, int blockIndex) {
resourceParams.clear();
gl4.glGetProgramResourceiv(program, GL4.GL_SHADER_STORAGE_BLOCK, blockIndex, PROPS_BUFFER_BINDING, null, resourceParams);
return resourceParams.get(0);
} |
||||||||||||||
|
|
||||||||||||||
| protected void updateShaderUniforms(Shader shader) { | ||||||||||||||
| ListMap<String, Uniform> uniforms = shader.getUniformMap(); | ||||||||||||||
| for (int i = 0; i < uniforms.size(); i++) { | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic incorrectly handles
layout(binding=0). When a uniform block has an explicitlayout(binding=0), the GL query will return 0. This code then overwritesbindingPointwithblockIndex, which might be different from 0, thus ignoring the explicit binding from the shader. This is a bug for shaders that uselayout(binding=0). The ambiguity of the return value 0 from the GL functions makes this tricky, but the current implementation doesn't fully respectlayout(binding=N)as intended.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.