jkuhlmann / cgltf

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

support KHR_xmp_json_ld read in cgltf.h #237

Open PsycoTodd opened 9 months ago

PsycoTodd commented 9 months ago

Hello,

This is a change that try to support KHR_xmp_json_ld extensions.

Basically, the extension allow to provide json_ld format things as array of packets at top level, and refer by index in asset, scene, node, mesh, material, image, animation.

The current read implementation is for expose the data at each component as a new array of xmp_json_ld, since it is possible to refer multiple packets by one, for example, image/node, etc.

Will see the feedback and try to also implement the cgltf_write.h functions.

Test pass by locally create model with the field and load inside filament.

One small change:

I notice that we create extensions for each component, and allocate array with total extension size, but only feel the array when we encounter unprocessed extensions. Or other recognizable extensions are processed in other if branch. This seems to make a extension array potentially larger than it needs. Since it only hold unprocessed extensions.

I made a small change to get the number of valid/supported extensions first, then allocate the extensions array accordingly.

zeux commented 9 months ago

On the data structure, I feel like using cgltf_xmp_json_packet** xmp_packets; size_t xmp_packets_count; instead of a struct with a packet index would fit the rest of the API better. We can add a cgltf_xmp_packet_index similar to all other _index functions to go back from a pointer to an index. You already see this pattern eg with child nodes for cgltf_node.

On unprocessed extensions, I would strongly recommend moving this out of this PR. This is a problem worth attacking but it should not be part of a PR that adds a new extension, and it should be done in a way that is minimaly invasive on existing code - eg repeating extension names will result in subtle bugs in the future, so we need to find a different way to do this, for example lazily reallocating unprocessed extensions as we encounter them.

On actual XMP parsing code, I think this needs to be less repetitive. Ideally every object that needs XMP (thank you for restricting these to the top level important objects! that's a good call) should just need a couple extra lines to process these. It seems like this is missing some refactoring on the parser side.

I'm hoping that with suggested tweaks the PR can be dramatically smaller which would make it easier to review and test.

PsycoTodd commented 9 months ago

On actual XMP parsing code, I think this needs to be less repetitive. Ideally every object that needs XMP (thank you for restricting these to the top level important objects! that's a good call) should just need a couple extra lines to process these. It seems like this is missing some refactoring on the parser side.

I think in the same way let me think a little. But I just notice that different compoenent may have different extension parsing logic. Like materials has a lot of extensions to parse to its own structure, while others may not. What is the method in C we recommend to have this behavior difference? To me seems like for node, mesh, image, etc. the way to parse the extensions list, the process is similar, but the detail would differ due to extension would be different.

This seems make the code has to repeat somehow on the structure level.

PsycoTodd commented 8 months ago

Updated with the comments.

PsycoTodd commented 6 months ago

Hello, please take a look with the new cr. thanks.