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

Use bitbybit to encode the MeshPipelineKey #10246

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Oct 24, 2023

Objective

MeshPipelineKey is a "packed struct". Using bitflags! doesn't scale for a packed struct, as the key grows in size, so does the complexity of setting its fields and reading them.

Solution

There are a few crates on crates.io defining macros to make handling packed struct much easier:

  • packed_struct: A venerable crate used in many different libraries, allow reading structs from packed &[u8] and vis-versa
  • bondrewd: An old crate generating "reader" and "writers" so that specific fields can be read individually, avoiding the cost of unpacking other fields
  • bitbybit: A relatively recent crate with not many users, based on arbitrary-int, of the same author.

Why chose bitbybit?

  • It supports arbitrary backing storage for our packed structs (unlike packed_struct and bondrewd that takes a &[u8]), which is important for performance
  • It supports reading subset of fields, similarly to bondrewd. The struct generated by the macro is pretty much a MeshPipelineKey(u32), then each method does the bitshifting/masking necessary
  • It is post 1.0, and still maintained.

Downsides of bitbybit:

  • There are some cryptic errors hidden in there if you are not careful about how you convert from an arbitrary-int to a rust native type (eg: u3 -> u8)

Other crates simply do not fit the bill, so I took the one that enabled me to do what I want.

We then replace the bitflags! definition for MeshPipelineKey by one based on bitflags. I'm pretty happy with the result. -160 and only for MeshPipelineKey is promising.

Future work

  1. This still doesn't fix my major gripe with the rendering code, which is the decoupling between key, shader defs and conditions for shader defs. (But reducing the amount of code reduces how much thinking one has to do to inspect code)
  2. There are other pipeline keys that could do with this improvements, but I'm refusing to update them before this PR gets merged. It's going to hit a lot of merge conflicts and I don't have the time to resolve all of them. I also need the greenlight from the community before moving forward with more changes.

Migration Guide

The MeshPipelineKey associated consts used to compute specific fields have been removed, please replace their usage by accessors such as MeshPipelineKey::with_taa and MeshPipelineKey.may_discard. With this change, code using MeshPipelineKey is likely to be cleaner and easier to read.

@nicopap nicopap added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Oct 24, 2023
@nicopap nicopap force-pushed the use-bitbybit branch 2 times, most recently from dc82090 to d40e79c Compare October 24, 2023 14:27
@nicopap
Copy link
Contributor Author

nicopap commented Oct 24, 2023

I need to think a bit more how to handle MSAA flags. It's very strange that the Msaa enum is only up to 8 samples, while the docs advertises "up to 128"

@nicopap nicopap added this to the 0.13 milestone Oct 25, 2023
@nicopap nicopap force-pushed the use-bitbybit branch 2 times, most recently from 2bd7457 to 8dc10d6 Compare October 25, 2023 08:01
@nicopap nicopap marked this pull request as ready for review October 25, 2023 08:34
@nicopap nicopap force-pushed the use-bitbybit branch 2 times, most recently from ff1470f to ea4ab77 Compare October 27, 2023 11:50
This removes all the maths around bit shifting and masking, it also
makes reading key flags easier, and setting key flags easier.
@lain-dono
Copy link
Contributor

@nicopap Nvidia GTX 1080 supports x16 and x32 msaa samples. Just to give you an example.

WebGPU and wgpu.rs doesn't explicitly limit you.

You need 2 bits to support x1, x2, x4 and x8. Another bit gives you x16, x32, x64, x128. see #5826 (comment)

@alice-i-cecile alice-i-cecile removed this from the 0.13 milestone Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants