Use bitbybit to encode the MeshPipelineKey #10246
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
MeshPipelineKey
is a "packed struct". Usingbitflags!
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-versabondrewd
: An old crate generating "reader" and "writers" so that specific fields can be read individually, avoiding the cost of unpacking other fieldsbitbybit
: A relatively recent crate with not many users, based onarbitrary-int
, of the same author.Why chose
bitbybit
?packed_struct
andbondrewd
that takes a&[u8]
), which is important for performancebondrewd
. The struct generated by the macro is pretty much aMeshPipelineKey(u32)
, then each method does the bitshifting/masking necessary1.0
, and still maintained.Downsides of
bitbybit
: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 forMeshPipelineKey
by one based onbitflags
. I'm pretty happy with the result. -160 and only forMeshPipelineKey
is promising.Future work
Migration Guide
The
MeshPipelineKey
associatedconst
s used to compute specific fields have been removed, please replace their usage by accessors such asMeshPipelineKey::with_taa
andMeshPipelineKey.may_discard
. With this change, code usingMeshPipelineKey
is likely to be cleaner and easier to read.