search

gltf-rs / gltf

A crate for loading glTF 2.0
Apache License 2.0
534 stars 124 forks source link

bug: Reader::read_positions does not honour `accessor.componentType` #416

Closed kanerogers closed 7 months ago

kanerogers commented 8 months ago

Hi there!

Background

When attempting to import a node whose position accessor has type 5123 (UNSIGNED_SHORT), the following debug assertion is tripped on line 348 of src/accessor/util.rs:

debug_assert_eq!(mem::size_of::<T>(), accessor.size())

Since T = ReadPositions::Item is [f32; 3] and accessor.size() is 6, this resolves to

debug_assert_eq!(mem::size_of::<[f32; 3]>(), accessor.size()) // left == right failed: left: 12, right: 6

However, this is incorrect. It is perfectly valid (though admittedly uncommon) for a position accessor to be of any type other than UNSIGNED_INT as per section 5.1.3 of the spec. I have confirmed the file can be validated and opened with Gestaltor.

I have attached below a zip file containing a glb that reproduces the above issue. A similar file can be obtained by running gltfpack over any glTF file without including the -noq (disable quantisation) option.

This issue was first raised in #311 but was incorrectly self-closed as user error.

Solution

The solution is to handle read_positions in a similar fashion to read_colors; finding the correct data type for the accessor and delegating to a specific reader.

enderman.zip

alteous commented 7 months ago

Thanks for reporting, I'll look into it.

alteous commented 7 months ago

This is the same issue as https://github.com/gltf-rs/gltf/issues/400.

It is perfectly valid (though admittedly uncommon) for a position accessor to be of any type other than UNSIGNED_INT as per section 5.1.3 of the spec.

This is not true. There is a list of valid accessor and component types for each semantic value. For POSITION, this is limited to VEC3 of float in the base specification.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#meshes-overview

This list is expanded by the KHR_mesh_quantization extension, but this is not supported currently.

kanerogers commented 7 months ago

Aha, right you are - I had not read that part of the spec previously, and hadn't taken note of the presence of the mesh quantization extension!

Thanks so much for the concise and informative response.