Refactor mesh compression methods#24572
Conversation
beicause
force-pushed
the
refactor-mesh-compression-methods
branch
from
291e23e to
3376c3f
Compare
kfc35
added
A-Rendering
greeble-dev
left a comment
greeble-dev
left a comment
There was a problem hiding this comment.
Looks good. Just a few minor comments and nitpicks.
Maybe worth noting that this adds some features as well as refactoring - joint weights can now be quantized to UNORM8 (which is fairly common), and users can apply quantization to their custom attributes.
I'm going to add M-Migration-Guide as there are API changes to Mesh::compressed_mesh and GltfPlugin. But I don't know if this PR needs to provide a full guide. There will probably be follow up API changes like #24535, so maybe better to just put in a placeholder guide for now?
| let octahedral_wrap = (1.0 - n.yx().abs()) | ||
| * Vec2::select( | ||
| n.xy().cmpgt(vec2(0.0, 0.0)), | ||
| n.xy().cmpge(vec2(0.0, 0.0)), |
There was a problem hiding this comment.
What's the motivation for this change?
There was a problem hiding this comment.
With this change, the error in the test is reduced from 10e-4 to 10e-7, though I haven't understood the reason yet
| .compress_attributes | ||
| .contains(MeshAttributeCompressionFlags::COMPRESS_POSITION) | ||
| { | ||
| let _ = self.compress_positions(); |
There was a problem hiding this comment.
What's the reasoning behind suppressing all errors? I can see how MissingAttribute might be fine to suppress in this case, but the others should be propagated?
There was a problem hiding this comment.
I think earlier failures should not prevent later attempts
There was a problem hiding this comment.
I'm not a fan of that approach. Yes, it allows a second attempt, but how does the user know to try again if the previous attempt silently failed? Maybe other reviewers will disagree, but I'd propagate the errors (except for MissingAttribute).
There was a problem hiding this comment.
This can also return a Vec/SmallVec of MeshVertexCompressionError, but I'm not sure if it's worth it:
- If you want to know which compressions succeeded, you can check the
attribute_compressionflags - If you want to handle each error, you can use the individual compression methods instead of this one
| pub quantize_attributes: MeshAttributeQuantization, | ||
| /// Compress some attributes to smaller representation. Shaders need change to decode them correctly. | ||
| /// See [`MeshAttributeCompressionFlags`] for the details. | ||
| pub compress_attributes: MeshAttributeCompressionFlags, |
There was a problem hiding this comment.
Just to get ahead of things, what changes do you expect to make to compress_attributes so that it can support the TBN compression of #24535? Mainly worried about whether it can stay as a bitfield when the TBN compression is mutually exclusive with some of the other flags. Can also leave that discussion to the other PR if you prefer.
There was a problem hiding this comment.
I think compress_attributes is more like a list of compression methods to apply. if angle encoding isn't applicable the individual compression should be applied. So these flags are not really mutually exclusive yet, because we can apply them in the appropriate order.
I expect to keep the TBN compression as a bitfield, if there isn't a better solution.
Migration guide isn't needed. The API isn't in 0.19 release |
Yep, my bad. |
4 participants
Objective
#24535 (comment) Refactor part of the mesh compression code to improve maintainability.
Solution
create_compressed_attribute_valuesinto separateMesh::compress_*methods, each independently callable and returning an error if compression failsquantize_float32_attribute, instead of being limited to unorm8/float16 for color and unorm16 for joint weightcompressed_meshfunction toMeshCompressionArgsMesh::compress_*Testing
The unit tests and
many_foxesandmany_morph_targetsstill work