Add 1-component ({s,u}{int,norm}{8,16}, float16) and unorm8x4-bgra vertex formats
#6632
Conversation
There was a problem hiding this comment.
Ah wait, hold on, metal is a lot more complicated unfortunately, as we don't use normal vertex formats, we manually transform to using vertex pulling.
First step is that I would add tests for all these new formats in https://github.com/gfx-rs/wgpu/blob/e23146aa3e8f5de0820cec108a9c1aa967454a3b/tests/tests/vertex_formats/mod.rs. This can probably join one of the other sets of tests.
Second step is to add support for these formats in naga -
wgpu/naga/src/back/msl/writer.rs
Line 4002 in 314e196
I would appreciate the webgpu update as a separate PR, as this has a bit of work left still.
nolanderc
force-pushed
the
vertex-format-1-component
branch
2 times, most recently
from
4f77000 to
faac144
Compare
nolanderc
force-pushed
the
vertex-format-1-component
branch
5 times, most recently
from
7fdb5dc to
d788e62
Compare
|
This should now be good to go @cwfitzgerald @ErichDonGubler |
| Sint32x4 = 29, | ||
| Sint32x4 = 38, | ||
| /// Three unsigned 10-bit integers and one 2-bit integer, packed into a 32-bit integer (u32). [0, 1024] converted to float [0, 1] `vec4<f32>` in shaders. | ||
| #[cfg_attr( | ||
| any(feature = "serialize", feature = "deserialize"), | ||
| serde(rename = "unorm10-10-10-2") | ||
| )] | ||
| Unorm10_10_10_2 = 34, | ||
| #[cfg_attr(feature = "serde", serde(rename = "unorm10-10-10-2"))] | ||
| Unorm10_10_10_2 = 43, | ||
| /// Four unsigned 8-bit integers, packed into a 32-bit integer (u32). [0, 255] converted to float [0, 1] `vec4<f32>` in shaders. | ||
| #[cfg_attr(feature = "serde", serde(rename = "unorm8x4-bgra"))] | ||
| Unorm8x4Bgra = 44, |
There was a problem hiding this comment.
question: @gfx-rs/naga: This code has a 5-slot gap for texture indices here. Is there a reason that this gap should continue to exist, given these changes?
There was a problem hiding this comment.
I noticed the gap in the previous code corresponded to the missing Float64 formats. I suppose having the enum discriminant values be a 1:1 mapping between those in wpgu_types and msl could be yield more efficient conversions between the two, or similar, so I just left it as is.
| @@ -4005,6 +4005,13 @@ template <typename A> | |||
| ) -> Result<(String, u32, u32), Error> { | |||
There was a problem hiding this comment.
question: @gfx-rs/naga: Why do we not re-use the output of wgpu_core::VertexFormat::size here? 🤔 I feel like it should be the same in all cases, but I admittedly haven't analyzed for divergences.
There was a problem hiding this comment.
LGTM. I have questions that I've noted above, but they're not blocking. I will try testing this in CTS on Firefox first before merging, since I have a patch stack ready-but-WIP on the Firefox side. If somebody else needs this, I'm happy to merge earlier.
@cwfitzgerald, did you still want to review this?
ErichDonGubler
force-pushed
the
vertex-format-1-component
branch
from
664d2a0 to
5b1e5d7
Compare
ErichDonGubler
dismissed
cwfitzgerald’s stale review
Feedback supposedly handled?
|
I'll take a cursory look. |
ErichDonGubler
force-pushed
the
vertex-format-1-component
branch
from
5b1e5d7 to
b1aac0e
Compare
|
I don't think it makes sense to leave this to get stale after being reviewed, so I'm going to merge it. @cwfitzgerald, if you find something you'd like addressed, I'm happy to review a follow-up PR! |
|
Nah this fell off my radar, merging was the right choice. |
Connections
Closes #6614
Description
When adding the translations from
wgpu::VertexFormatto the various backends, I did my best to choose the appropriate formats, but I'm not too familiar with DX12 or Metal, so please, take some extra time to verify those.Testing
N/A
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.