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

MVKCmdDraw: Fix indirect index for triangle fan topology #2419

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

dboyan
Copy link
Contributor

@dboyan dboyan commented Jan 13, 2025

When we call vkCmdDraw with vertexCount=N and firstVertex=X, we only need an array with N element, X, X+1, ..., X+N-1, to generate the indirect indices. However, the current logic generates all the way from 0 to X+N-1 (which is X+N elements) while only reserving N spaces. This causes issue when the unreserved part is overwritten, for example, when issuing consecutive draw calls with triangle fan primitives.

Fixes #2418

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2025

CLA assistant check
All committers have signed the CLA.

When we call vkCmdDraw with vertexCount=N and firstVertex=X, we only
need an array with N element, X, X+1, ..., X+N-1, to generate the
indirect indices. However, the current logic generates all the way
from 0 to X+N-1 (which is X+N elements) while only reserving N spaces.
This causes issue when the unreserved part is overwritten, for example,
when issuing consecutive draw calls with triangle fan primitives.
Copy link
Collaborator

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, this looked wrong, but, studying it for a while, I'm more and more convinced that this is correct.

@cdavis5e cdavis5e merged commit 30ec0de into KhronosGroup:main Jan 13, 2025
6 checks passed
@cdavis5e
Copy link
Collaborator

Oh, and, uh, thank you for your contribution. :)

@cdavis5e
Copy link
Collaborator

Actually, thinking about this some more... it's occurred to me that a similar change might need to be made for MVKCmdDrawIndirect::encodeIndirectIndexed().

/me checks...

...

Yes, a similar change needs to be made for the shader function, cmdDrawIndirectPopulateIndexes(). I could make an analogous change to fix this, or do you want to do the honors?

cdavis5e added a commit to cdavis5e/MoltenVK that referenced this pull request Jan 13, 2025
Analogously to KhronosGroup#2419, this fixes generation to add `vertexStart` to the
_generated_ indices, and start from zero in the array.
@cdavis5e
Copy link
Collaborator

I could make an analogous change to fix this,

I went ahead and made #2420.

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.

Rendering artifact when using ANGLE's vulkan backend with MoltenVK
3 participants