-
-
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 5 commits
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) { | ||
|
|
@@ -1469,39 +1468,20 @@ protected void updateShaderBufferBlock(final Shader shader, final ShaderBufferBl | |
| int usage = resolveUsageHint(bufferObject.getAccessHint(), bufferObject.getNatureHint()); | ||
| if (usage == -1) return; // cpu only | ||
|
|
||
| bindProgram(shader); | ||
|
|
||
| final int shaderId = shader.getId(); | ||
|
|
||
| int bindingPoint = bufferObject.getBinding(); | ||
| int bindingPoint = bufferBlock.getBinding(); | ||
| if (bindingPoint < 0) { | ||
| // Binding not yet resolved — skip until resolveBufferBlockBindings runs | ||
| bufferBlock.clearUpdateNeeded(); | ||
| return; | ||
| } | ||
|
|
||
| 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 +1492,111 @@ protected void updateShaderBufferBlock(final Shader shader, final ShaderBufferBl | |
| bufferBlock.clearUpdateNeeded(); | ||
| } | ||
|
|
||
| /** | ||
| * Resolves binding points for all buffer blocks in a shader. Runs once | ||
| * per shader program. Queries the binding from the compiled shader, | ||
| * detects collisions, and reassigns duplicates to unique binding points. | ||
| * | ||
| * @param shader the shader whose buffer blocks to resolve. | ||
| */ | ||
| private void resolveBufferBlockBindings(final Shader shader) { | ||
| final ListMap<String, ShaderBufferBlock> bufferBlocks = shader.getBufferBlockMap(); | ||
| final int shaderId = shader.getId(); | ||
|
|
||
| bindProgram(shader); | ||
|
|
||
| // Pass 1: resolve block indices and query bindings from the compiled shader | ||
| for (int i = 0; i < bufferBlocks.size(); i++) { | ||
| ShaderBufferBlock block = bufferBlocks.getValue(i); | ||
| if (block.getBinding() >= 0) continue; // already resolved | ||
|
|
||
| BufferType bufferType = block.getType(); | ||
| if (bufferType == null) continue; // not yet configured | ||
|
|
||
| // Resolve block index (location) | ||
| int blockIndex = block.getLocation(); | ||
| if (blockIndex < 0) { | ||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||
| blockIndex = gl4.glGetProgramResourceIndex(shaderId, GL4.GL_SHADER_STORAGE_BLOCK, block.getName()); | ||
| } else { | ||
| blockIndex = gl3.glGetUniformBlockIndex(shaderId, block.getName()); | ||
| } | ||
| block.setLocation(blockIndex); | ||
| } | ||
|
|
||
| if (blockIndex < 0 || blockIndex == NativeObject.INVALID_ID) { | ||
| continue; | ||
| } | ||
|
|
||
| // Query the binding declared in the shader | ||
| int binding; | ||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||
| binding = queryShaderStorageBlockBinding(shaderId, blockIndex); | ||
| } else { | ||
| binding = gl3.glGetActiveUniformBlocki(shaderId, blockIndex, GL3.GL_UNIFORM_BLOCK_BINDING); | ||
| } | ||
| block.setBinding(binding); | ||
| } | ||
|
|
||
| // Pass 2: detect and resolve collisions. | ||
| // UBOs and SSBOs use separate GL binding namespaces, so track them independently. | ||
| Set<Integer> usedUboBindings = new HashSet<>(); | ||
| Set<Integer> usedSsboBindings = new HashSet<>(); | ||
| int nextFreeUbo = 0; | ||
| int nextFreeSsbo = 0; | ||
|
|
||
| for (int i = 0; i < bufferBlocks.size(); i++) { | ||
| ShaderBufferBlock block = bufferBlocks.getValue(i); | ||
| int binding = block.getBinding(); | ||
| if (binding < 0) continue; | ||
|
|
||
| BufferType bufferType = block.getType(); | ||
| Set<Integer> usedBindings; | ||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||
| usedBindings = usedSsboBindings; | ||
| } else { | ||
| usedBindings = usedUboBindings; | ||
| } | ||
|
|
||
| if (!usedBindings.add(binding)) { | ||
| // Collision within the same namespace — find a free binding point | ||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||
| while (usedBindings.contains(nextFreeSsbo)) nextFreeSsbo++; | ||
| binding = nextFreeSsbo; | ||
| } else { | ||
| while (usedBindings.contains(nextFreeUbo)) nextFreeUbo++; | ||
| binding = nextFreeUbo; | ||
| } | ||
| usedBindings.add(binding); | ||
| block.setBinding(binding); | ||
| } | ||
|
|
||
| // Set the binding on the shader program | ||
| int blockIndex = block.getLocation(); | ||
| if (bufferType == BufferType.ShaderStorageBufferObject) { | ||
| gl4.glShaderStorageBlockBinding(shaderId, blockIndex, binding); | ||
| } else { | ||
| gl3.glUniformBlockBinding(shaderId, blockIndex, binding); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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); | ||
| } | ||
|
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++) { | ||
|
|
@@ -1529,6 +1614,11 @@ protected void updateShaderUniforms(Shader shader) { | |
| */ | ||
| protected void updateShaderBufferBlocks(final Shader shader) { | ||
| final ListMap<String, ShaderBufferBlock> bufferBlocks = shader.getBufferBlockMap(); | ||
| // Resolve binding points once per shader, detecting and fixing collisions | ||
| if (bufferBlocks.size() > 0 && bufferBlocks.getValue(0).getBinding() < 0) { | ||
| resolveBufferBlockBindings(shader); | ||
| } | ||
|
|
||
| for (int i = 0; i < bufferBlocks.size(); i++) { | ||
| updateShaderBufferBlock(shader, bufferBlocks.getValue(i)); | ||
| } | ||
|
|
@@ -1559,6 +1649,45 @@ public int convertShaderType(ShaderType type) { | |
| } | ||
| } | ||
|
|
||
| private static final Pattern BUFFER_BLOCK_PATTERN = Pattern.compile( | ||
| "layout\\s*\\([^)]*\\)\\s*(buffer|uniform)\\s+\\w+"); | ||
|
|
||
| private static final Pattern BINDING_ZERO_PATTERN = Pattern.compile( | ||
| "layout\\s*\\([^)]*binding\\s*=\\s*0[^)]*\\)\\s*(buffer|uniform)\\s+\\w+"); | ||
|
|
||
| /** | ||
| * Checks that layout(binding=0) is not used on a non-first buffer block, | ||
| * since the GL query cannot distinguish explicit binding=0 from the | ||
| * default, making collision detection unreliable for that case. | ||
| * | ||
| * @param source the GLSL source code. | ||
| * @param sourceName the name of the shader source for error messages. | ||
| */ | ||
| private void validateBufferBlockBindings(String source, String sourceName) { | ||
| Matcher allBlocks = BUFFER_BLOCK_PATTERN.matcher(source); | ||
| Matcher binding0Blocks = BINDING_ZERO_PATTERN.matcher(source); | ||
|
|
||
| // Find positions of all buffer/uniform block declarations | ||
| List<Integer> allPositions = new ArrayList<>(); | ||
| while (allBlocks.find()) { | ||
| allPositions.add(allBlocks.start()); | ||
| } | ||
|
|
||
| if (allPositions.size() < 2) return; // single block, no ambiguity possible | ||
|
|
||
| int firstBlockPos = allPositions.get(0); | ||
|
|
||
| while (binding0Blocks.find()) { | ||
| if (binding0Blocks.start() != firstBlockPos) { | ||
| throw new RendererException( | ||
| "Shader '" + sourceName + "' uses layout(binding=0) on a non-first " | ||
| + "buffer block. This is ambiguous because the GL query cannot " | ||
| + "distinguish explicit binding=0 from the default. Use a non-zero " | ||
| + "binding or declare this block first in the shader."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public void updateShaderSourceData(ShaderSource source) { | ||
| int id = source.getId(); | ||
| if (id == -1) { | ||
|
|
@@ -1623,7 +1752,11 @@ public void updateShaderSourceData(ShaderSource source) { | |
| stringBuf.append("#define ").append(source.getType().name().toUpperCase()).append("_SHADER 1\n"); | ||
|
|
||
| stringBuf.append(source.getDefines()); | ||
| stringBuf.append(source.getSource()); | ||
|
|
||
| String sourceCode = source.getSource(); | ||
| validateBufferBlockBindings(sourceCode, source.getName()); | ||
|
|
||
| stringBuf.append(sourceCode); | ||
|
|
||
|
|
||
| intBuf1.clear(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,6 +158,7 @@ public void initializeEmpty(int length) { | |
| BufferUtils.destroyDirectBuffer(data); | ||
| } | ||
| this.data = BufferUtils.createByteBuffer(length); | ||
| setUpdateNeeded(); | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -167,11 +168,12 @@ public void initializeEmpty(int length) { | |
| * @param data ByteBuffer containing the data to pass | ||
| */ | ||
| public void setData(ByteBuffer data) { | ||
| if (data != null) { | ||
| BufferUtils.destroyDirectBuffer(data); | ||
| if (this.data != null) { | ||
| BufferUtils.destroyDirectBuffer(this.data); | ||
| } | ||
| this.data = BufferUtils.createByteBuffer(data.limit() - data.position()); | ||
| this.data.put(data); | ||
| setUpdateNeeded(); | ||
| } | ||
|
Comment on lines
170
to
177
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 will throw a Also, using public void setData(ByteBuffer data) {
if (this.data != null) {
BufferUtils.destroyDirectBuffer(this.data);
}
if (data != null) {
this.data = BufferUtils.createByteBuffer(data.remaining());
this.data.put(data);
} else {
this.data = null;
}
setUpdateNeeded();
} |
||
|
|
||
|
|
||
|
|
||
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 method allocates a new
IntBufferforparamson every call. To improve performance and reduce garbage, you can reuse the class memberintBuf1which is already available for this purpose.