Fix the double __root__ issue for glb #7257
Conversation
Bloadrick
changed the title
| var scene = this._gltf.scenes[indexScene]; | ||
|
|
||
| // Root node must be unique | ||
| if (scene.nodes.length != 1) { |
There was a problem hiding this comment.
| if (scene.nodes.length != 1) { | |
| if (scene.nodes.length !== 1) { |
| if (scene.nodes.length != 1) { | ||
| return null; | ||
| } | ||
| else if(this._gltf.nodes != null) { |
There was a problem hiding this comment.
| else if(this._gltf.nodes != null) { | |
| else if(this._gltf.nodes !== null) { |
| var root = this._gltf.nodes[indexRoot]; | ||
|
|
||
| // Check rotation and scale | ||
| if(root.rotation != undefined && root.scale != undefined){ |
There was a problem hiding this comment.
ok...stopping here but please fix all triple equals ;)
| } | ||
|
|
||
| // Check name | ||
| if (root.name != "__root__") { |
There was a problem hiding this comment.
not sure we want to check this one. As long as we have the correct transforms we are fine
| if (root.name != "__root__") { | ||
| return null; | ||
| } | ||
| }else{ |
|
|
||
| private _createRootNode(nodes : INode[]): INode { | ||
|
|
||
| var ExistingRootNode = this._checkForRootNode(); |
|
|
||
| var ExistingRootNode = this._checkForRootNode(); | ||
| var RootAlreadyImplemented = false; | ||
| var INodeToReturn; |
|
This change is not in the right place. The double root should be removed on export and not on import. |
|
Ok I was under the same impression Closing this PR then |
|
Hi, I think we are both right here after thinking about it again. So 3ds max and maya are right handed, they export in Babylon which is left handed, no root here, we are changing the geometry and the transform. Current state: After that if we put a GLTF in the sandbox we need to add a root to put it in left handed coordinates. What to do: When we export in GLB from the sandbox we need to delete that root to keep it right handed. If you are ok with that I can add the delete of the root node on export and keep this PR with the changes asked implemented. |
|
I agree we should not add a root node if not necessary. But I struggle to convince myself that we will have the cae. When will this happen if we fix the serializer? |
|
If a GLB has already been exported, in my test files some of them have up to 4 or 5 root nodes. |
|
Ok that makes to me |
|
I don't think the loader should do anything different. The loader must do something in order to convert the data from right-handed (glTF) to left-handed (Babylon assuming useRightHandedSystem is false). I don't think the loader should muck with the hierarchy of the glTF itself. If the existing glTF already has a bunch of unnecessary root nodes, then it should be up to the author of the glTF to fix the problem. The main fix is to make sure the exporter does not perpetuate the issue. |
|
Yeah you are right. We always have to switch to left...so yeah we only need the serializer part :D |
Fix for the the double root issue: #6349