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

COLLADA2GLTF does not enforce top-level skeleton node must be top-level glTF node #648

Closed
pjcozzi opened this issue Jul 9, 2016 · 9 comments

Comments

@pjcozzi
Copy link
Member

pjcozzi commented Jul 9, 2016

See RiggedSimple.gltf and #624 (comment).

@lasalvavida
Copy link
Contributor

@lexaknyazev Is there a compelling reason to force a top level skeleton node to be a top level node?

If not, it may be better to remove what seems like an arbitrary restriction to me.

@lexaknyazev
Copy link
Member

Here's example layout:

  • node_4
    • Armature
    • Bone
      • Bone_001
    • Bone2
      • Bone2_001
    • Cylinder

Skinned node (Cylinder) references Bone as its skeleton:

"Cylinder": {
  "meshes": [
    "Cylinder-mesh"
  ],
  "skeletons": [
    "Bone"
  ],
  "skin": "Armature_Cylinder-skin"
},

Since we've decided to forbid mixing skeleton nodes with non-skeleton nodes in one tree, we need to move Armature out of node4:

  • node_4
    • Cylinder
  • Armature
  • Bone
    • Bone_001
  • Bone2
    • Bone2_001

@pjcozzi said, that jointName has skeleton-tree scope. So how many skeleton-tree scopes are here:

  • Two: (Bone + its children) and (Bone2 + its children), or
  • One: (Armature + its children)?

With actual skeletons being top-level, we could also avoid extra parent transformations: Armature matrix could be pre-applied to Bone and Bone2.

@lexaknyazev
Copy link
Member

@pjcozzi Could you confirm this restriction or should it be reconsidered?

@lasalvavida
Copy link
Contributor

If this is something we want to do, I can pretty easily throw something into gltf-pipeline in the short term to correct the sampleModels. However, a collada2gltf fix is going to be a lot more involved.

@pjcozzi
Copy link
Member Author

pjcozzi commented Aug 10, 2016

I am open to reconsidering this if it makes exporters/converters complicated. I don't like the idea of putting workarounds in gltf-pipeline ("output an invalid glTF then let gltf-pipeline fix it" is not the foundation we want to build on).

@lexaknyazev what does this improve for loaders? Is it significant? Perhaps the tradeoff isn't worth keeping it.

@lexaknyazev
Copy link
Member

See my comment above. It's about defining local scope for jointName.

@pjcozzi
Copy link
Member Author

pjcozzi commented Aug 22, 2016

Ah, OK.

@lasalvavida what is hard about updating COLLADA2GLTF?

One non-ideal solution could be to:

@lasalvavida
Copy link
Contributor

@lasalvavida what is hard about updating COLLADA2GLTF?

I'll see what I can do with COLLADA2GLTF. It just isn't super consistent with its memory management, so adding post-processing stages usually involves digging through SEGFAULTS.

@lexaknyazev
Copy link
Member

Continuation:
KhronosGroup/COLLADA2GLTF#16

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