Add spliceNode function to reparent an entire scene or subtree

[rotu]

This is inspired by https://github.com/donmccurdy/glTF-Transform/issues/1530#issuecomment-2414064338. In particular, instead of a transformScene function, this creates a new Node to which you can apply transforms.

[rotu]

I'm not sure about the API. In particular, the const newNode = new Node(parent.getGraph()). I'm not sure if it's kosher for a function to create a Node object outside of document.createNode(). The alternative is to require the caller to pass in the new parent, e.g.

const newParent = document.createNode().setRotation([1,0,0,0]);
spliceNode(oldParent, newParent);

but then it's not clear what to do if newParent already has a parent. Or worse, if newParent is already a child of oldParent.

Also not totally happy with the name of this function.

[donmccurdy]

It's OK for the function to create a node, I think, as long as the naming is consistent with that expectation. Calling the Node constructor (or other Property subclass constructors) directly will cause issues though, I would copy the pattern here:

https://github.com/donmccurdy/glTF-Transform/blob/1693ffdb8245c1765ab019d566f58dffb6a56093/packages/functions/src/list-texture-slots.ts#L13-L16

I'd like to make that more obvious at some point, but in the meantime this avoids requiring users to pass in a document to functions unnecessarily.

That said, having the user pass in both nodes would sound OK to me too.

[rotu]

It's OK for the function to create a node, I think, as long as the naming is consistent with that expectation. Calling the Node constructor (or other Property subclass constructors) directly will cause issues though, I would copy the pattern here:

https://github.com/donmccurdy/glTF-Transform/blob/1693ffdb8245c1765ab019d566f58dffb6a56093/packages/functions/src/list-texture-slots.ts#L13-L16

I'd like to make that more obvious at some point, but in the meantime this avoids requiring users to pass in a document to functions unnecessarily.

Here are some passing notes about how I was confused. I'm sure more hacking on this will clear things up:

• I didn't use fromGraph because I inferred from the name (incorrectly, but in analog with Array.from(...)) that it creates a new Document. It also has the problem that it can return null and it's not clear what to do if the node is merely part of a document fragment.
• getGraph is fuzzy about whether the returned object is necessarily the same for all GraphNodes, or if it's just the reachable subgraph (subtree for Node, since the node hierarchy is acyclic)
• I'm super uncertain about the distinction between a Graph and a Document and what's needed in order to maintain class invariants. I called the constructor that way because that's what the createNode function seems to do.

[donmccurdy]

Sorry for the delayed followup on this! My feeling is that reparenting X under Y is conceptually a "generic" tree operation that shouldn't have special cases for skins. But without those special cases (1) it doesn't really solve #1530, and (2) it's simple enough to not need an abstraction. So I think I'm inclined to close this PR, sorry, though I'd still be open to a more 'opinionated' function like transformScene from the earlier discussion.

A Document and Graph have a fixed 1:1 relationship, and all resources created within a Document are members of the same Graph. This is usually a "hidden" implementation detail, but I've let it escape the abstraction a little in order to avoid requiring document inputs to every function. tl;dr, this is quite safe and cannot return null:

const document = Document.fromGraph(texture.getGraph())!; 

It is something I'd like to clean up eventually though. Perhaps just a getDocument() accessor on each Property, or:

• https://github.com/donmccurdy/glTF-Transform/issues/1594

[rotu]

@donmccurdy No worries. Thank you for considering.