jkuhlmann / cgltf

:diamond_shape_with_a_dot_inside: Single-file glTF 2.0 loader and writer written in C99
MIT License
1.44k stars 136 forks source link

Suggestion: store indices instead of pointers in the data structures #202

Closed AntonShalgachev closed 1 year ago

AntonShalgachev commented 1 year ago

I've noticed that various data structures contain pointers to the other objects (e.g. cgltf_texture::image), not indices into the respective arrays (e.g. into cgltf_data::images in case of a texture). This decision forces me to compute the indices myself by computing the difference between the pointers (e.g. between cgltf_data::images and cgltf_texture::image) I've examined the source code and noticed that cgltf actually stores the index inside the pointer in cgltf_parse_json_xxx methods during parsing, and only then produces the pointers in cgltf_fixup_pointers

A bit of explanation why I need the indices: my GLTF loader creates some metadata for loaded GLTF objects. So for all images I would have an additional array of image metadata, with the assumption that ith cgltf image would have metadata stored in ith element of the metadata array. When parsing the texture, I need to access this image metadata instead of the cgltf image, which is why I need an index at this stage

Is there a reason for the current solution? Would it be in line with the project to replace pointers with the indices (or to add indices alongside pointers)?

zeux commented 1 year ago

Is computing the indices a problem? gltfpack uses indices extensively in various parts of the pipeline and it wasn't a big problem, even though the code is sometimes a little repetitive. If we only have indices, this breaks compatibility but also makes cgltf more cumbersome to use as you need to convert from indices to pointers. If we have both indices and pointers everywhere, then we'd need to store the equivalent data redundantly, which, in addition to being a bit wasteful, probably breaks cases where a file is loaded, mutated, and saved via cgltf_write (as existing code would need to be changed to update both indices and pointers...).

jamesdolan commented 1 year ago

Needing to recompute indices (something I suspect most users do) adds a bit of trickiness since the user needs to carefully determine which pointers can get turned into indices and which ones don't since the API doesn't make this a clear contract. Even though implementation is trivial for users to do it themselves, adding some helper functions to the API would at least provide such a contract and make it easier to functionally verify usage. e.g. cgltf_get_material_index(const cgltf_data*, const cgltf_material*).

zeux commented 1 year ago

A set of helper functions might be the best solution to this. It keeps the parsing core as is and it doesn't bloat the uses of pointers + doesn't create redundancy in the representation, but for every individual pointer it's obvious as to how to recover the index.

AntonShalgachev commented 1 year ago

That solution works for me as well. Thanks for looking into it!