Deeper file validation about cgltf HOT 8 CLOSED

Yeah - I'm not sure where the boundary lies. My main goal for part 1 is to make sure that cgltf_parse is safe to call on an arbitrary input document - as in, you get a document without crashing :) This seems like a reasonable goal. Some minimal checking for the presence of specific required elements isn't too hard, but I don't know if that's where it should belong.

For part 2, it's not clear how far we can/should go. If a validator that catches most severe issues is possible to write in, say, 10% of the current codebase - then it seems reasonable to do it as part of the same header.

from cgltf.

FWIW some of the checks above have landed - in particular, pointer values always “make sense” and are present if the spec says so. Node graph is guaranteed to be acyclic if you start traversal from root.

I found that some rules about required/optional pointers are not very intuitive - e.g. reading data from accessors basically requires to dereference accessor->buffer_view->buffer, but buffer_view can be NULL.

from cgltf.

I've looked at the GLTF spec and gathered an approximate list of things we can validate without accessing the buffer data. The list is pretty intimidating, not super keen on implementing all of these plus some of these are not as valuable and/or prone to issues with possible future extensions (for example, "skin" says that mesh primitives must contain JOINTS_0/WEIGHTS_0, but an extension that wants to store >4 bone influences could decide either to add JOINTS_1/etc. or to store joint bindings in a completely custom way). I've crossed out a few that I'm not intending to implement because of this.

• buffer view fits in buffer if buffer is set
• all attribute accessors for a primitive should have the same count
  index accessor refs buffer view with type indices removed because of KhronosGroup/glTF#1440
• index accessor has component type u8/u16/u32 & type scalar
  vertex accessor refs buffer view with type vertices removed because of KhronosGroup/glTF#1440
  camera "type" field is consistent with data defn not very valuable + possible extension conflicts
• node can have matrix or t/r/s but not both
• load_buffers must check bin_size (should be >= buffer 0 size) & uri == NULL
  accessor.byteOffset and accessor.byteOffset + bufferView.byteOffset must be a multiple of component type size highly likely to be randomly violated by export tools when packing 2-byte and 4-byte data together
• accessor.byteOffset + stride * (accessor.count - 1) + element_size must be <= bufferView.length
• accessor sparse indices/values have to have sparse.count elts
• mesh weights == NULL or mesh weights match mesh target count
• node weights == NULL or node weights match mesh target count
• all primitives have the same morph target count
• all morph target accessors must have the same count as the accessors of the original primitive
• skin has 1+ joints
  when the node contains skin, all mesh.primitives must contain JOINTS_0 and WEIGHTS_0 attributes - possible extension conflicts
• animation samplers have matching counts on input/output accessors
• animation sampler input must be a scalar
• animation sampler output must match animation target path in terms of accessor type

from cgltf.

Thinking some more about this... Is this kind of extensive validation actually in the expected scope of this library?
If you'd like to see it, might it maybe make more sense to have it in a separate header file?

from cgltf.

Worth noting is that I've been running into some issues with jsmn when fuzzing, we'll see how practical even part 1 is...

from cgltf.

Some minimal checking for the presence of specific required elements isn't too hard, but I don't know if that's where it should belong.

I meant basic things like "every bufferView should have a buffer index", "every camera needs to have a type" and such instead of keeping them in some unspecified state, again potentially causing the user code to misbehave. The spec requires only very few of the properties to be present (see the tables here) but where it does, it makes sense to check for them (and fail if they're not present). I think :)

For the extended validation, for my own use case I don't think it's too critical to have it. It would make sense to have it in a separate header, yes.

from cgltf.

I encountered such a file. Plays well with three.js, but tinygltf fails to import it due to missing buffer_view. Didn't investigate further but AFAIK that's to support sparse buffers for morph targets (or... compression extensions such as Draco? not sure). The file is here: https://threejs.org/examples/models/gltf/LittlestTokyo.glb (~4 MB), corresponding three.js example here: https://threejs.org/examples/webgl_animation_keyframes.html

from cgltf.

Most of the above landed. I decided to limit the scope here to integrity checks that may affect application stability - will ignore checks like “does node have redundant set of transforms” because of that. I intend to implement the index value validation and close this.

from cgltf.