Skip to content
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

MVKCmdDrawIndirect: Fix indirect index for triangle fans. #2420

Closed
wants to merge 1 commit into from

Conversation

cdavis5e
Copy link
Collaborator

Analogously to #2419, this fixes generation to add vertexStart to the generated indices, and start from zero in the array.

Analogously to KhronosGroup#2419, this fixes generation to add `vertexStart` to the
_generated_ indices, and start from zero in the array.
@dboyan
Copy link
Contributor

dboyan commented Jan 14, 2025

I'm not 100% sure yet if this is correct. The case with drawCount>1 might be interesting, but currently I haven't tested it. I tested drawCount=1 case with my own minimal program (written in gles and hooked to angle's vulkan backend because I'm not a vulkan expert), via glDrawArraysIndirect. It rendered correctly with or without the change (I guess separate calls don't suffer from buffer overwriting problem here). But I couldn't verify the more generic case as I didn't have the extensionGL_EXT_multi_draw_indirect with angle. I'll see if I can obtain a vulkan version of test program.

@dboyan
Copy link
Contributor

dboyan commented Jan 15, 2025

I was able to get a working test program and now I can confirm the change isn't in the right direction. Actually the original code makes sense somewhat.

Let's consider the situation where we have 16 vertices, and we call vkCmdDrawIndirect with drawCount=2, using triangle fan primitive topology. The indirect buffer contains

Item 0: { vertexCount=4, firstVertex=0 }
Item 1: { vertexCount=4, firstVertex=8 }

Notice that the compute kernel cmdDrawIndirectPopulateIndexes handles both draws with 2 threads, and the resulting index are populated into a single buffer idxBuff. With the original logic, idxBuff will contain

[0, 1, 2, 3, X, X, X, X, 8, 9, 10, 11, X, X ...]    (X means uninitialized element)

This is the intended behavior because indices from multiple draws are put "in place" and will not interfere with each other. They will be further processed to generate indices for triangles according to the destBuff output of the current stage. It seems a waste of space but it's logically correct.

On the other hand, after the change, all the indices from different draw all start from the beginning of idxBuff, and that creates trouble. My #2419 was correct because vkCmdDraw can only issue a single draw, so one doesn't need to worry about the issue above.

In fact, even with the original code, the rendering is not 100% correct. The triangle fan translation seems a little off. I'll open a issue when I get time.

@cdavis5e
Copy link
Collaborator Author

Yeah, I realized a bit after the fact that it was already adding the base before my change. Thanks for confirming.

@cdavis5e cdavis5e closed this Jan 15, 2025
@cdavis5e cdavis5e deleted the fix-trifan-indirect branch January 15, 2025 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants