GLTFLoader: Fix empty groups when multiple scenes reference same nodes

[YusakuNo1]

Fixed #27993.

Description

When multiple glTF scenes reference the same root node, the loader now clones nodes that already have a parent to prevent them from being removed from previous scenes. Uses SkeletonUtils.clone() to properly preserve skeletal animation connections.

[Mugen87]

@donmccurdy Looking good to you?

[Mugen87]

For testing: https://rawcdn.githack.com/YusakuNo1/three.js/839aec01b9d5dbfbef34febde76f29592144741f/examples/webgl_loader_gltf.html

[takahirox]

A few comments.

First, I think we can treat this as a bug and it should be fixed. (Agree with #27993 (comment))

And since this issue is probably rare, I think this fix is fine.

However, considering the original intent, I feel that creating a clone may not be ideal. For example, if scene[0] points to the root of the entire scene graph and scene[1] points to the root of a subtree within it. I think users would be assuming that scene[1] is contained within scene[0], but this would end up creating a separate node tree.

So ideally, scene and scenes should not create Group(s) that represents them, but should be references that point to a node within the node tree. Since a scene itself in glTF has no transform, this probably wouldn’t cause major issues. (Though this approach might make handling scene extensions or extras awkward in some cases.)

But changing it to make them references would be a breaking change, and from a compatibility standpoint, I don’t think we should introduce such a breaking change just to handle a rare edge case. I just wanted to leave a comment about what the ideal design would be. Not a blocker.

// Something like this
gltf.scene: Object3D[];
gltf.scenes: Object3D[][];

[donmccurdy]

@takahirox I believe that problem is prevented by the glTF spec with this language:

Scenes are defined in a scenes array. All nodes listed in scene.nodes array MUST be root nodes, i.e., they MUST NOT be listed in a node.children array of any node. The same root node MAY appear in multiple scenes.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#scenes

Could be more challenges (does this affect a KHR_animation_pointer extension?) but in any case I agree this PR should improve the situation. Thanks @YusakuNo1!

[takahirox]

Ah, I see, I had overlooked that part of the spec. In that case, it seems even less likely that this issue will actually occur. For now, it sounds good to introduce this simple workaround and revisit it later if needed. Thanks!

[YusakuNo1]

A few comments.

First, I think we can treat this as a bug and it should be fixed. (Agree with #27993 (comment))

And since this issue is probably rare, I think this fix is fine.

However, considering the original intent, I feel that creating a clone may not be ideal. For example, if scene[0] points to the root of the entire scene graph and scene[1] points to the root of a subtree within it. I think users would be assuming that scene[1] is contained within scene[0], but this would end up creating a separate node tree.

So ideally, scene and scenes should not create Group(s) that represents them, but should be references that point to a node within the node tree. Since a scene itself in glTF has no transform, this probably wouldn’t cause major issues. (Though this approach might make handling scene extensions or extras awkward in some cases.)

But changing it to make them references would be a breaking change, and from a compatibility standpoint, I don’t think we should introduce such a breaking change just to handle a rare edge case. I just wanted to leave a comment about what the ideal design would be. Not a blocker.

Thanks @takahirox , it's a good point. Let me try something different...

[takahirox]

Ah, sorry. My wording may have been unclear. I think the current PR code is fine as-is. The subtree example I gave is invalid in glTF, so we didn’t need to consider it. Even so, the reference-based approach has the advantage of avoiding clones, but it would be a breaking change, and multiple scenes are a rare use case anyway, so I don’t think it’s desirable to introduce a breaking change just for such a rare case. So cloning seems much more reasonable.

My comment was only meant to share an approach that would be a breaking change but might be a more ideal option. I don’t think we should choose that option now, but I wanted to share it as one possible direction for the future.

Anyway, thanks for improving GLTFLoader!