EXT_meshopt_compression: accessor reads use wrong byte length / stride for accessors sharing a bufferView#821
Open
camnewnham wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Affected: 6.19.0 (com.atteneder.gltfast / com.unity.cloud.gltfast)
Summary
Two defects in the
EXT_meshopt_compressionbranches ofGltfImportBase's accessor readers break glTFs where multiple accessors share a single meshopt-compressed bufferView — e.g. glTF-Transform exports that pack animation sampler outputs together. Both stem from the meshopt branches diverging from the existing non-meshopt semantics in the same file.Note: Bug 1 is only a crash with
ENABLE_UNITY_COLLECTIONS_CHECKS. It doesn't crash in a player build -- though in combination with Bug 2 it causes corrupt animations.Bug 1 — crash: wrong byte length in
GetAccessorData<T>The meshopt branch passes
count(an element count) as the byte length toGetSubArray:For a
VEC3/FLOATaccessor withcount = 3at a non-zero offset this slices 3 bytes,and
Reinterpret<float3>()throws:The length must be
count * sizeof(T), matching the assertion right above it (GltfImport.cs#L2164) and the non-meshopt path (GltfImport.cs#L2223).Fix: GltfImport.cs#L2166
Bug 2 — silent corruption: wrong stride in
GetStridedAccessorData<T>The meshopt branch passes the outer
bufferView.byteStride, which is undefined (-1) for non-vertex bufferViews such as animation sampler outputs. The decoded meshopt data is packed atEXT_meshopt_compression.byteStride. With stride0,UnsafeUtility.ReadArrayElementWithStridereturns element 0 for every index — e.g. all translation keyframes collapse to the first value (no crash, wrong result).GetBufferViewalready reads the extension stride (GltfImport.cs#L2127), and the non-meshoptGetStridedAccessorDatafalls back tosizeof(T)(GltfImport.cs#L2240).Fix: GltfImport.cs#L2185-L2189
Reproduction
A GLB using
EXT_meshopt_compression+KHR_mesh_quantizationwith an animated translation track (e.g. a glTF-Transform v4.3.0 export). Bug 1 fails the load; with Bug 1 fixed, Bug 2 zeroes the translation animation.Here's a sample file that demonstrates this condition (and loads correctly with this PR). Drag + drop for editor-time loading is sufficient.
meshopt_animation_buffer_repro.zip
AI Disclosure
AI was used in diagnosing this issue and drafting this PR. I have verified the results manually (and visually) and provided the files for reproduction above.