Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support KHR_xmp_json_ld read in cgltf.h #237

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

PsycoTodd
Copy link

@PsycoTodd PsycoTodd commented Nov 2, 2023

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
Copy link
Contributor

zeux commented Nov 3, 2023

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
Copy link
Author

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
Copy link
Author

Updated with the comments.

  • Removed the extension count functions.
  • add only code to handle xmp cases.
  • Use pointer reference rather than the index to trace the json string.
  • Each component can only have one KHR_xmp_json_ld index, so we do not use array anymore.

cgltf.h Outdated Show resolved Hide resolved
cgltf.h Outdated Show resolved Hide resolved
cgltf.h Show resolved Hide resolved
cgltf.h Outdated Show resolved Hide resolved
cgltf.h Show resolved Hide resolved
cgltf.h Outdated Show resolved Hide resolved
cgltf.h Outdated Show resolved Hide resolved
cgltf.h Outdated Show resolved Hide resolved
@PsycoTodd
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants