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 uses libraryName in place of instanceName (regression) #15658

Closed
4 of 12 tasks
thedayofcondor opened this issue Jan 28, 2019 · 7 comments · Fixed by #19325
Closed
4 of 12 tasks

ColladaLoader uses libraryName in place of instanceName (regression) #15658

thedayofcondor opened this issue Jan 28, 2019 · 7 comments · Fixed by #19325

Comments

@thedayofcondor
Copy link

Description of the problem

Since #13432 (r91) the ColladaLoader discards the node name attributes in instances of library objects and replaces it with the name of the referred library_node.

I believe this is against the intended DAE behaviour - where the node "name" attribute is not respected.

As an example:

[...]

<library_visual_scenes>
  <visual_scene id="ID2">
    <node id="ID3" name="Cube_1">
      <instance_node url="#ID4" />
    </node>
    <node id="ID13" name="Cube_2">
      <instance_node url="#ID4" />
    </node>
    <node id="ID14" name="Cube_3">
      <instance_node url="#ID4" />
    </node>
  </visual_scene>
</library_visual_scenes>

[...]

<library_nodes>
  <node id="ID4" name="Cube">
    [...]
  </node>
</library_nodes>

[...]

See cubes.dae.txt

results in a scene with 3 objects all called "Cube", in place of "Cube_1", "Cube_2" and "Cube_3" (see code below)

var loader = new THREE.ColladaLoader();
 loader.load('/models/cubes.dae', function (collada) {
  collada.scene.traverse(function (node) {
    console.log("Name:",node.name);
  });
});
Three.js version
  • Dev
  • r100
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@mrdoob
Copy link
Owner

mrdoob commented Jan 29, 2019

@shelbyspeegle what do you think?

@shelbyspeegle
Copy link
Contributor

shelbyspeegle commented Feb 4, 2019

hey! @thedayofcondor its kind of funny, if i recall, the reason i made #13432 was because of a regression from previous behavior 😂. so to be honest, i don't really know which way is correct and i am not emotionally tied to the way things are implemented.

i wasn't sure what the intended behavior should be so i checked out the collada 1.4 spec (https://www.khronos.org/files/collada_spec_1_4.pdf) and i couldn't find anything specifically mentioning the behavior / specification of what the name property on nodes should be.

i was using a workaround to get the definition node from the instance node. i don't really have a problem if the node.name changes. i'm totally down to go with the flow / spec if you are familiar with it 👍

@shelbyspeegle
Copy link
Contributor

on a side note - it looks like you're using sketchup 18. when this was an issue for me i was working on 15. i plan to upgrade to sketchup 18 soon, i'm curious if my loading would break if i exported from sketchup 18 🤔.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 4, 2019

I suspect the "correct" behavior of the loader depends in some sense on the exporting tool you are using. Is it possible to export glTF files from SketchUp? Otherwise you guys might want to consider to convert your Collada files to glTF.

@thedayofcondor
Copy link
Author

@Mugen87 and @shelbyspeegle yes, I believe you are correct. It seems Sketchup "natively" has two names for each object, the "definition" name (which must be unique in the scene) and the "instance" name (so you can distinguish every copy of an object). My issue is that, with the new loader, if I have 5 cubes I cannot tell which cube is which - but I understand that different 3D packages can just assign a meaningless name to the instance.

In my suggestion is that every object should simply have 2 (or 3) names- "libraryName", "instanceName" and (possibly) "name" (either a copy of one the above or a combination of the two). However, I am not familiar with the Collada format - is every object an instance of a library object?

And no - unfortunately Sketchup export is very limited (no glTF), also the combination "sketchup dae export - threejs dae import" is the only one that seemed to preserve all the schene properties I need (in term of objects hierarchy, materials etc)

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 5, 2019

It would be interesting to see how this tool preserves the mentioned name properties when converting from Collada to glTF:

https://github.com/KhronosGroup/COLLADA2GLTF

You can use this viewer in order to verify the result of the conversion. The glTF file itself is JSON so you can check in the "meshes" and "nodes" section the respective names.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 5, 2019

However, I am not familiar with the Collada format - is every object an instance of a library object?

AFAIK, no. For instance a node in a visual scene can be directly defined without the usage of instance_node. The elf model from the following demo is a good example for this.

https://threejs.org/examples/webgl_loader_collada

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

Successfully merging a pull request may close this issue.

4 participants