Add set_parent(parent_node, keep_transform) method

[Flavelius]

Describe the project you are working on

Not relevant

Describe the problem or limitation you are having in your project

The default add_child(..) method of node fails if the child node already has a parent. Recently the reparent(..) method was added to simplify the case where unparenting and keeping the node's worldspace transformation was necessary. This was a step in the right direction but now has the opposite problem of failing when the node to reparent has no current parent.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

https://github.com/godotengine/godot/pull/36301#issuecomment-1378743935 this comment describes it very well: set_parent(parent_node, keep_transform) can ultimately act as a unified parenting method that handles both cases without needing to error out.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

It will work like both methods combined with what user code currently needs to do on top. If node currently has a parent, act like reparent(), of not, act like add_child, basically: check if node already has a parent but instead of erroring out (like current reparent) unparent it and then add it as child to the new node (and not error out like add_child as the parenting case is thus handled)

If this enhancement will not be used often, can it be worked around with a few lines of script?

It can be worked around in code, but as this is very frequently used functionality that currently requires boilerplate code and now has 2 methods to cover basically the same case, it's a prime candidate for improving at the engine level.

Is there a reason why this should be core and not an add-on in the asset library?

For performance reasons and discoverability, as it is frequently used functionality that will even ease a new user's experience.

[Zireael07]

I see elsewhere that the general idea is to unify add_child and reparent.

+100 in that case

[YuriSizov]

I think adding this after we've added reparent in its current form may make API very confusing for a very small benefit. It'd be nice to do this before adding reparent, IMO, with maybe some conditional check to validate that the node is in a correct state. But now, having both set_parent (or whatever similar name we pick) and reparent doesn't look like a good idea for API comprehension.

[Flavelius]

Personally, I'm in favor of removing add_child and reparent for a unified set_parent, but I can also see that this would be a huge user facing api change which is not to be taken lightly. But even just undoing the very recent addition of reparent (after all it's still not in release) for set_parent will likely make anyone prefer using that instead of add_child in the long run

[YuriSizov]

You can't remove add_child, it's a method of the parent itself, which has to exist for parenting to work even if you use set_parent on a child node. It doesn't have to be public, of course, but it also doesn't hurt anyone.

[TyPhyter]

Personally, this solution makes way more sense to me. The add_child/reparent duality forcing you to switch from connecting parent->child to child->parent depending on the previous state of the child is a little inconsistent and confusing. A universal and reliable method sounds more ideal.

I'd also add that I do think there is still a case for the existence of reparent, as it might be useful to have a method for parenting that fails if not previously parented without using calls to get_parent() and branching logic. As such, the differences between the appropriate cases to use add_child/reparent and the proposed set_parent are such that adding set_parent doesn't harm API comprehension as much as speculated IMO.

Another suggestion (that completely ignores a lack of conventional precedence for it in the API), It would be good if reparent could become try_reparent, making the differences in the reliability of these methods more apparent, although I understand the breaking change may not be worth the marginal benefit.

[KoBeWi]

I didn't notice this proposal and opened a similar one: #6845 It also includes an implementation and my idea had one difference - you can do set_parent(null) to unparent a node.

[frostymm]

As a new user this annoyed me way more than I would like to admit. At the very least the api doesn't need to be changed. Just have addChild or Reparent function without erroring out. If I reparent something that doesn't have a parent, just set the parent XD Why does it matter if it already had one? It doesn't. Same if I add a child, I'm stealing it from whatever parent had it before, that parent is now dead to us.

[RobProductions]

I think it makes sense from the user side to have both set_parent and reparent, as one doesn't care about an existing parent and the other does, and the naming of these two already convey that. This would definitely help simplify a very common operation for people