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

Add support for VK_PRIMITIVE_TOPOLOGY_TRIANGLE_FAN. #1962

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

billhollings
Copy link
Contributor

To reduce complexity and repetitive copy-pasted spaghetti code, the design approach here was to implement triangle fan conversion on MVKCmdDrawIndexedIndirect, as the most general of the draw commands, and then populate and invoke a synthetic MVKCmdDrawIndexedIndirect command from the other draw commands.

  • Rename pipeline factory shader cmdDrawIndexedIndirectMultiviewConvertBuffers() to cmdDrawIndexedIndirectConvertBuffers(), and in addition to original support for modifying indirect content to support multiview, add support for converting triangle fan indirect content and indexes to triangle list.

  • Modify MVKCmdDrawIndexedIndirect to track need to convert triangle fans to triangle list, and invoke kernel function when needed.

  • Modify MVKCmdDraw, MVKCmdDrawIndexed, and MVKCmdDrawIndirect to populate and invoke a synthetic MVKCmdDrawIndexedIndirect command to convert triangle fans to triangle lists.

  • Add pipeline factory shader cmdDrawIndirectPopulateIndexes() to convert non-indexed indirect content to indexed indirect content.

  • MVKCmdDrawIndexedIndirect add support for zero divisor vertex buffers potentially coming from MVKCmdDraw and MVKCmdDrawIndexed.

  • Rename pipeline factory shader cmdDrawIndexedIndirectConvertBuffers() to cmdDrawIndexedIndirectTessConvertBuffers() so it will be invoked from MVKCommandEncodingPool::getCmdDrawIndirectTessConvertBuffersMTLComputePipelineState() (unrelated).

Fixes #1419.
Fixes #1799.

To reduce complexity and repetitive copy-pasted spaghetti code,
the design approach here was to implement triangle fan conversion on
MVKCmdDrawIndexedIndirect, as the most general of the draw commands,
and then populate and invoke a synthetic MVKCmdDrawIndexedIndirect
command from the other draw commands.

- Rename pipeline factory shader cmdDrawIndexedIndirectMultiviewConvertBuffers()
  to cmdDrawIndexedIndirectConvertBuffers, and in addition to original support
  for modifying indirect content to support multiview, add support for
  converting triangle fan indirect content and indexes to triangle list.
- Modify MVKCmdDrawIndexedIndirect to track need to convert triangle fans
  to triangle list, and invoke kernel function when needed.
- Modify MVKCmdDraw, MVKCmdDrawIndexed, and MVKCmdDrawIndirect to populate
  and invoke a synthetic MVKCmdDrawIndexedIndirect command to convert triangle
  fans to triangle lists.
- Add pipeline factory shader cmdDrawIndirectPopulateIndexes() to convert
  non-indexed indirect content to indexed indirect content.
- MVKCmdDrawIndexedIndirect add support for zero divisor vertex buffers
  potentially coming from MVKCmdDraw and MVKCmdDrawIndexed.

- Rename pipeline factory shader cmdDrawIndexedIndirectConvertBuffers()
  to cmdDrawIndexedIndirectTessConvertBuffers() so it will be invoked from
  MVKCommandEncodingPool::getCmdDrawIndirectTessConvertBuffersMTLComputePipelineState()
  (unrelated).
@billhollings
Copy link
Contributor Author

@cdavis5e As described above in the PR description , the implementation here delegates from MVKCmdDraw, MVKCmdDrawIndexed, and MVKCmdDrawIndirect, to a synthetic MVKCmdDrawIndexedIndirect instance, whenever the topology is triangle fans. I did this to avoid having to duplicate the conversion code across all of the draw commands.

It got me to contemplating whether a similar approach might be useful for the entire tessellation draw flow. Basically, implement any non-native-Metal workflow(s) once in MVKCmdDrawIndexedIndirect, and delegate from the other draw commands, leaving those draw commands to just handle behaviour that Metal supports natively.

Thoughts about the practicality of such an approach? You know the tess implementation much better than I do.

@IsaacMarovitz
Copy link

Screenshot 2023-07-01 at 12 43 45 Can confirm this fixes the UI in No Man's Sky!

@billhollings billhollings merged commit 41dbd9d into KhronosGroup:main Jul 1, 2023
6 checks passed
@billhollings billhollings deleted the tri-fans branch July 1, 2023 15:15
This pull request was closed.
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.

VkPrimitiveTopology value 5 is not supported Enhancement assessment: VK_PRIMITIVE_TOPOLOGY_TRIANGLE_FAN
2 participants