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

Add non-normative note about the current usage of mesh.extras.targetNames #1631

Merged
merged 3 commits into from
Jun 23, 2019

Conversation

Ybalrid
Copy link
Contributor

@Ybalrid Ybalrid commented Jun 18, 2019

Morph target names are not required to perform morphing. A real time application that consume glTF assets doesn't need morph target names in the general case.

Rightly so, glTF doesn't impose assets to have target names, but it also doesn't provide a way to optionally add them.

It seems that, since the release of glTF 2.0 in mid-2017, a number of implementation (three.js, the official Blender exporter, UnityGLTF, a and at least one Maya exporter (and probably some others software, I haven't made a list, these are the few I'm aware of)).

This has been discussed in issue #1036.

The goal is to help implementations that wants to do so have a way to do so in an interoperably way. Currently this has been done by adding a targetNames extras properties to the object.

Maybe a more proper way would be to have an extension doing so, however, this practice is already quite widespread, so I would argue that at least documenting it here and integrate a better (and standardized) way of doing so in glTF 2.x/3.0 is the better thing to do.

I am not opposed to the idea of writing an extension to support morph target names, but that would add 2 not really standard way of doing it instead of the one already out in the wild.

…ames

Morph target names are not required to perform morphing. A real time application that consume glTF assets doesn't need morph target names in the general case. 

Rightly so, glTF doesn't impose assets to have target names, but it also doesn't provide a way to optionally add them.

It seems that, since the release of glTF 2.0 in mid-2017, a number of implementation (three.js, the official Blender exporter, UnityGLTF, a and at least one Maya exporter (and probably some others software, I haven't made a list, these are the few I'm aware of)).

This has been discussed in issue KhronosGroup#1036. 

The goal is to help implementations that wants to do so have a way to do so in an interoperably way. Currently this has been done by adding a `targetNames` extras properties to the object.

Maybe a more proper way would be to have an extension doing so, however, this practice is already quite widespread, so I would argue that at least documenting it here and integrate a better (and standardized) way of doing so in glTF 2.x/3.0 is the better thing to do.

I am not opposed to the idea of writing an extension to support morph target names, but that would add 2 *not really standard* way of doing it instead of the one already out in the wild.
@Ybalrid Ybalrid mentioned this pull request Jun 18, 2019
@donmccurdy
Copy link
Contributor

Thanks! I'm in favor of adding the note, but would shorten it and omit the example:

Implementation note: A significant number of authoring and client implementations associate
names with morph targets. While the glTF 2.0 specification does not provide a way to specify names, most tools use a list of strings, mesh.extras.targetNames, for this purpose. The targetNames list and all primitive targets must have the same length.

@Ybalrid
Copy link
Contributor Author

Ybalrid commented Jun 19, 2019

Hey @donmccurdy I agree with your proposition, it is way more concise and does tell basically the same information.

So, I have removed the example, and I have updated the note to what you proposed. I have replaced "list" by "array" (as these are JSON indexed "array").

@Ybalrid
Copy link
Contributor Author

Ybalrid commented Jun 21, 2019

I think it's good to be merged now?

@emackey
Copy link
Member

emackey commented Jun 22, 2019

@lexaknyazev or @bghgary Any thoughts before this gets merged?

@bghgary
Copy link
Contributor

bghgary commented Jun 22, 2019

Ok to me.

@donmccurdy donmccurdy merged commit 596605d into KhronosGroup:master Jun 23, 2019
@donmccurdy
Copy link
Contributor

Thanks for proposing the addition @Ybalrid!

@Ybalrid
Copy link
Contributor Author

Ybalrid commented Jun 27, 2019

@donmccurdy Thanks for accepting it 😉

@Ybalrid Ybalrid deleted the patch-1 branch June 27, 2019 03:00
hybridherbst added a commit to prefrontalcortex/UnityGLTF that referenced this pull request Jan 6, 2024
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.

5 participants