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

GLTFLoader: Allow multiple associations #21737

Merged
merged 2 commits into from
Sep 21, 2021

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Apr 28, 2021

Related issue: #19359 (comment)

Description

This PR allows GLTFLoader to save multiple associations by changing from

{
  types: 'nodes',
  index: nodeIndex
}

to

{
  nodes: nodeIndex,
  meshes: meshIndex,
  primitives: primitiveIndex
}

and newly saves mesh and primitive indices.

Regarding adding mesh and primitive indices, from @donmccurdy comment #19359 (comment)

I'm OK with adding that if we have clear use cases to help us design it usefully.

My use case is I want to get mesh and primitive definitions for KHR_materials_variants extension from Three.js Mesh object because the extension definition is under primitive

https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_materials_variants/README.md#example

and different variants can be selected after the loader parses glTF.

And a Three.js Mesh can be associated to both a node definition and mesh definition so I would like to suggest to allow multiple associations.

This change requires users who already use parser.associations to update their code because of associations data structure change. Is it ok? @cdata

BTW who should run npm run build-examples? Me and I should add examples/js/loaders/GLTFLoader.js change in this PR?

@Mugen87
Copy link
Collaborator

Mugen87 commented May 2, 2021

BTW who should run npm run build-examples?

I think it's okay to not include examples/js files in PRs. They can be generated from time to time directly on dev and right before a release.

@mrdoob mrdoob requested a review from donmccurdy May 3, 2021 12:29
@mrdoob
Copy link
Owner

mrdoob commented May 3, 2021

/cc @elalish

@mrdoob mrdoob added this to the r129 milestone May 3, 2021
@donmccurdy
Copy link
Collaborator

Sounds fine to me. We may want to have unit tests on this behavior.

The changed API may affect @drcmda as well.

@takahirox
Copy link
Collaborator Author

We may want to have unit tests on this behavior.

Sounds good to me. I added a unit test.

@takahirox
Copy link
Collaborator Author

@donmccurdy @cdata @elalish @drcmda

Do you think it's good to go with this change? I hope we can get it into r129.

@drcmda
Copy link
Contributor

drcmda commented May 18, 2021

i see nothing that could be a blocker. 👍

Copy link
Contributor

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be hard to update our code to match this, I think. And I too want to support variant editing, so I imagine I'd run into the same problem before long. I suppose it's natural that we might start associating with more parts of the glTF as we go along.

type: 'textures',
index: textureIndex
} );
parser.associations.set( texture, { textures: textureIndex } );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I might change textures to texture and make the rest singular too, since we're only giving a single index.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the reason why they are plural is to easily access json data because they are plural in glTF json.

// The existing associations
const def = json[association.type][association.index];

// The new associations
for (const key in associations) {
  const def = json[key][associations[key]];
}

@mrdoob mrdoob modified the milestones: r129, r130 May 27, 2021
@mrdoob mrdoob modified the milestones: r130, r131 Jun 30, 2021
@mrdoob mrdoob modified the milestones: r131, r132 Jul 28, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looking at this PR as I'm pondering the same issue. I'm actually doing a deeper integration of code you graciously added to Model-Viewer. I have not tried to tackle the issue in three.js code but my initial thought would be to just associate the Mesh with a glTF primitive instead of with a glTF node. Your suggestion is a nice work-around but it I'm not sure it helps in the case of a skinned mesh where there are multiple primitives per mesh (see BrainStem sample for example). It seems to me the because a three.js Mesh is associated with a glTF node (instead of a Primitive) the issue will keep arising as the format gets more complicated and tries to allow for more interaction, with Variants just being the first most obvious problem of the moment. I think the issue boils down to the fact that in glTF a Node represents transform and can hold Mesh which can have multiple primitives and in three.js a Mesh has all three but is limited to one primitive.

@mrdoob mrdoob modified the milestones: r132, r133 Aug 26, 2021
ghost pushed a commit to SrirachaSource/three.js that referenced this pull request Sep 16, 2021
Allows for multiple glTF associations per Three.js Object.
Provides glTF self look up indices on a Three.js Object in order to facilitate easy identification of the objects glTF counterparts.
Related mrdoob#21737

When Three.js loads a glTF file it combines the glTF node, mesh, and primitive components into single objects in some cases, this changes provides a map of those combinations in order to preserve an objects relationship to the original glTF file.
@mrdoob mrdoob merged commit aca1c4f into mrdoob:dev Sep 21, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 21, 2021

Thanks!

@takahirox takahirox deleted the GLTFLoaderMultipleAssociations branch September 21, 2021 19:28
ghost pushed a commit to google/model-viewer that referenced this pull request Sep 24, 2021
weswigham pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Sep 25, 2021
elalish pushed a commit to google/model-viewer that referenced this pull request Sep 30, 2021
* Updates apply the latest changes coming down from Three.js in regards to multiple-associations (mrdoob/three.js#22544)

* Updates to multiple-associations changes in Three.js mrdoob/three.js#21737

* Adds tests to exercise updated correlations/associations

* remove test.only

* remove unneeded awaits

* Added default material e2e test to primitive

* remove test.only

* Update Three to r133, update types/three to 0.132.1

* updates package-lock

* updates to test
weswigham added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Oct 1, 2021
weswigham added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Oct 1, 2021
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Oct 21, 2021
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Oct 21, 2021
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.

6 participants