Remove vertex_pulling_transfrom from PipelineCompilationOptions.

[bradwerth]

This option was only evaluated for Metal backends, and now it's required there so the option is going away. It is still configurable for tests via the PipelineOptions struct, deserialized from .ron files.

This also fixes some type problems with the unpack functions in writer.rs. Metal << operator extends operand to int-sized, which then has to be cast back down to the real size before as_type bit conversion.

Connections
N/A

Description
This is fully transitioning the Metal backend to use vertex pulling for all cases.

Testing
All existing Metal tests now use the vertex pulling transform.

Checklist

• Run cargo fmt.
• Run cargo clippy. If applicable, add:
  • --target wasm32-unknown-unknown
  • --target wasm32-unknown-emscripten
• Run cargo xtask test to run tests.
• Add change to CHANGELOG.md. See simple instructions inside file.

[ErichDonGubler]

I'm leaning towards approving this, but there are a couple of things I think we should resolve:

• I have some questions about the Metal codegen changes; I didn't expect or understand them yet. This makes me uneasy; weren't we shipping something tested to be correct before? 🤔
• I also noticed one incorrect thing with our usage of Vec::with_capacity that should be pretty trivial to resolve. EDIT: Resolved on my end!

[cwfitzgerald]

For a change like this, possibly before this lands, definitely before we release next (1 mo ish), we need to reach out to our users for gpu side performance testing. Obviously enabling should be the default, but if this forces significant regressions, we should be exposing a escape hatch to the user.

I also feel like we should have runtime vertex format tests now that we're polyfilling them on a platform.

[bradwerth]

I'm leaning towards approving this, but there are a couple of things I think we should resolve:

* I have some questions about the Metal codegen changes; I didn't expect or understand them yet. This makes me uneasy; weren't we shipping something tested to be correct before? 🤔

We don't have complete test coverage of all the vertex formats. @cwfitzgerald notes in a commment on this PR that we should. I'll expand this PR to include a test update that exercises evertyhing, which will help justify the changes to the unpack functions.

[bradwerth]

Converting to draft while I expand the test coverage of all the unpack functions.

[teoxoy]

This is great!

[teoxoy]

It took me a while to double-check the decimal to byte representations, maybe we can use rust's v.to_le_bytes() methods to improve the DX. Not a hard blocker though.

[cwfitzgerald]

We can also use half and bytemuck, or other such things. These are tests, so new dependencies aren't a big deal.

[bradwerth]

Well, however the bytes are specified, the checksums are verifying that they compute to the correct values.

[teoxoy]

We should resolve #5773 (comment).

[bradwerth]

It took me a while to double-check the decimal to byte representations, maybe we can use rust's v.to_le_bytes() methods to improve the DX. Not a hard blocker though.

The syntax for this is just markedly worse. Since the numbers are a mix of 2-byte, 4-byte, and 6-byte, and 8-byte values, it's not easy to flatten them into a single u8 array/Vec since there's no way to flatten those differently-types iterators. We end up with syntax like this:

let input_block_0 = &mut Vec::<u8>::new();
input_block_0.extend_from_slice(&0xC0C0C0C0u32.to_le_bytes()); // Unorm8x4 (0.5, 0.5, 0.5, 0.5)
input_block_0.extend_from_slice(&0xC000C000u32.to_le_bytes()); // Unorm16x2 (0.5, 0.5)
input_block_0.extend_from_slice(&0x4000400040004000u64.to_le_bytes()); // Unorm16x4 (0.25, 0.25, 0.25, 0.25)
input_block_0.extend_from_slice(&0x7F7F7F7Fu32.to_le_bytes()); // Snorm8x4 (1, 1, 1, 1)
input_block_0.extend_from_slice(&0xC000C000u32.to_le_bytes()); // Snorm16x2 (-1, -1)
input_block_0.extend_from_slice(&0x7FFF7FFF7FFF7FFFu64.to_le_bytes()); // Snorm16x4 (1, 1, 1, 1)
input_block_0.extend_from_slice(&0xFFFFu32.to_le_bytes()[0..2]); // Unorm8x2 (1, 1)
input_block_0.extend_from_slice(&0xC0C0u32.to_le_bytes()[0..2]); // Snorm8x2 (-1, -1)

I don't think this is improving readability. It's also actually wrong -- I got at least one of those values encoded incorrectly and it's messing up the checksums. I strongly urge that we leave the byte specification as-is. Re-open if you disagree.

[bradwerth]

Didn't mean to close, sorry.

[bradwerth]

I don't think this is improving readability. It's also actually wrong -- I got at least one of those values encoded incorrectly and it's messing up the checksums. I strongly urge that we leave the byte specification as-is. Re-open if you disagree.

The error is trivial upon reflection: the C0s should be 80s. I still think the syntax is worse.

Comments were addressed