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

Primitive strip cut values #1510

Open
kvark opened this issue Sep 20, 2017 · 3 comments
Open

Primitive strip cut values #1510

kvark opened this issue Sep 20, 2017 · 3 comments

Comments

@kvark
Copy link
Member

kvark commented Sep 20, 2017

Metal has strip cut value of 0xFF..FF (depending on the index format) at all times enabled.
D3D12 allows one to choose between no value, 0xFFFF, and 0xFFFFFFFF.

It's not entirely clear how to please every backend here. Forcing the strip cut ON for everyone would require us to know the index buffer format at PSO creation for D3D12...

My proposal is to extend the Primitive type to be:

enum StripCutValue {
    Disabled,
    Ffff,
    Ffffffff,
}
enum Primitive {
   PointList,
   LineList,
   LineStrip(StripCutValue),
   TriangleList,
   TriangleStrip(StripCutValue),
   ...
}

So basically, D3D12 style with a bit of type sugar. Metal PSO creation would then return an error if the value is Disabled. An error would also be returned from using an index buffer on Metal that has a different bit width than the strip cut value.

This is mostly penalizing Metal, at the cost of not requiring extra info on PSO creation, while still supporting the strip cuts. Any ideas/corrections are welcome!

@msiglreith
Copy link
Contributor

Looks nice! Especially coupling it with the Primitive type, allowing us remove This is mostly the field here and we should also got rid of this one.

@royaltm
Copy link
Contributor

royaltm commented Aug 12, 2019

Currently primitive_restart property of the input_assembler is ignored in vulkan backend https://docs.rs/gfx-backend-vulkan/0.3.0/src/gfx_backend_vulkan/device.rs.html#522
in create_graphics_pipeline implementation.
I hope it's not a BUG, but a TODO?

@kvark
Copy link
Member Author

kvark commented Aug 12, 2019

@royaltm the issue is quite old, discussion predates us just settling with Vulkan API.
That code is a bug. Care to fix it with a PR?

bors bot added a commit that referenced this issue Aug 12, 2019
2952: Vulkan: fix setting up primitive restart enable flag while creating graphics pipeline r=kvark a=royaltm

Related to #1510
PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:
- [ ] `rustfmt` run on changed code

Hello!
The fix is obvious, the: `primitiveRestartEnable` property of the [VkPipelineInputAssemblyStateCreateInfo](https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VkPipelineInputAssemblyStateCreateInfo) should be `VK_TRUE` for `primitive_restart` == `pso::PrimitiveRestart::U16` or `pso::PrimitiveRestart::U32`. It doesn't matter which `IndexType` you later bind the index buffer with.


Co-authored-by: Rafal Michalski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants