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

Feature custom attributes #157

Closed

Conversation

bhtruong93
Copy link

@bhtruong93 bhtruong93 commented Mar 23, 2021

#154

This PR adds support for custom attributes using a dictionary. It also exposes GetAccessorParams so that users can access the data.

Note: Once DisposeVolatileData is called, the buffer is gone and a user won't be able to get accessor data. I've added an optional disposeData argument in the load functions for this use case.

A ParseAttributes test was added to check if all attributes were parsed.

@atteneder
Copy link
Owner

Hi @bhtruong93,

thanks for your contribution. It'll take me ~2 weeks to get time to take a deeper look.

I want to understand your decisions and probably question some of them. No need to rush, imho.

regards,
Andi

@atteneder
Copy link
Owner

@bhtruong93 : Could you please provide demo data to test against?

@bhtruong93
Copy link
Author

@atteneder no worries, thanks! Looking forward to collaborating.
I've just added a couple of quick tests to get this started, here's an asset that's also used by MRTK:
9d707d7#diff-7e5ce09c40fa490326a56f5a6969d44c8160c490bd2a2ee5baf75a7796235a12R12

The custom glb has POSITION and _CUSTOM_ATTR attributes

@atteneder
Copy link
Owner

Thanks for the effort and sorry for the late reply

I'm afraid I cannot merge this PR in present form due to performance concerns. It's not great that we have to do a second JSON parsing pass already. This PR would add yet another one using regular expressions (which are powerful/clean, but I suspect also quite slow). Most users don't need this addition but all would suffer the downside.

My long-term vision is to replace JsonUtility by something similar fast/small, but a bit more features, including:

  • Remove the 2-pass parsing hack
  • Flexibility to parse and preserve unknown properties (so that they can be passed on to extension plug-ins)

This new parsing solution would allow your use case as well.

None the less I'm glad you found a good solution for your problem. Your input will surely help in shaping the solution to this.

Thanks a lot!

@atteneder atteneder closed this May 5, 2021
@FreakTheMighty
Copy link

@atteneder your library has gone through a bunch of updates since this pull request. Has anything changed that makes you think we could find a performant way to offer this feature?

@atteneder
Copy link
Owner

@FreakTheMighty I've made a proof-of-concept that uses Newtonsoft JSON for parsing and allows additional meta data. You can have a look at it in the demo-metadata branch.

No timeline on a publishes solution still.

@ketourneau
Copy link

@atteneder Hi, we need also to access metadata. Do you have any update about this feature ? Your branch link is broken.

@atteneder
Copy link
Owner

Yeah, the Newtonsoft JSON feature has been released (in 6.0.0 I think). See this sample for details.

There's a catch though. It only allows you to access known properties. You cannot iterate arbitrary ones. I did a PoC recently that allows more sophisticated meta data access, but came to the conclusion that this would be an API breaking change that has to wait for the next major release.

hth

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.

4 participants