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

ColladaLoader instance nodes imported with names like 'instance_0' #13365

Closed
3 of 13 tasks
shelbyspeegle opened this issue Feb 19, 2018 · 6 comments
Closed
3 of 13 tasks
Milestone

Comments

@shelbyspeegle
Copy link
Contributor

shelbyspeegle commented Feb 19, 2018

Description of the problem

After the replacement of ColladaLoader with ColladaLoader2 (now named just ColladaLoader), it looks like the imported information has changed a little bit.

heres a (condensed) snippet of my dae.

<library_nodes>
  <node id="ID3" name="Element_entry-door-handle">
    <node id="ID47" name="instance_3">
      </matrix>
      <instance_node url="#ID48" />
    </node>
  </node>
  <node id="ID48" name="Anchor">
    </instance_geometry>
  </node>
</library_nodes>

the behavior that i used to see was that the imported dae object would look like this:

scene.children[0] = [
  {
    name: 'Element_entry-door-handle',
    children: [
      {
        name: 'Anchor'
      }
    ]
  }
]

now it is looking like this:

scene.children[0] = [
  {
    name: 'instance_0',
    children: [
      {
        name: 'instance_3'
      }
    ]
  }
]

is this the expected output? on one hand i see at as incorrect because of how ColladaLoader used to work, but on the other hand i see it as correct because it matches how the dae is represented.

i am able to achieve what i need by doing a search through library.nodes and marrying the 'instance' up with its original definition and getting that name.

would you accept a PR if i ensure that instance nodes are coming through with their definitions name rather than 'instance_0', 'instance_1' etc?

i'll work on putting together a jsfiddle and updating this issue once complete.

Three.js version
  • Dev
  • r90
  • r88
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 19, 2018

i'll work on putting together a jsfiddle and updating this issue once complete.

👍

@shelbyspeegle
Copy link
Contributor Author

alright i am super close, i just need a way of referencing the previous version of ColladaLoader.js in the demo to show the old functionality.

i have logging in the console to show the part of the scene that has the instance_0 name. here is the demo: http://jsfiddle.net/zhpv6pn6/7/.

you can find my dae at https://raw.githubusercontent.com/shelbyspeegle/threejs-13365/master/source-model.dae.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 21, 2018

Can you please check what happens if you change the following line of code to:

if ( object.name === '' ) object.name = ( type === 'JOINT' ) ? data.sid : data.name;

Does this change improve the names of your scene graph elements?

@shelbyspeegle
Copy link
Contributor Author

yes! thanks for the line number too! passing all my tests now without special lookup against library nodes 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 21, 2018

Do you wanna make a PR with the change? I think we can put this in ColladaLoader.

@shelbyspeegle
Copy link
Contributor Author

sure!

@mrdoob mrdoob added this to the r91 milestone Feb 21, 2018
WJsjtu pushed a commit to WJsjtu/three.js that referenced this issue Feb 26, 2018
…ciated library nodes name, vs 'instance_#'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants