Godot-proposals: Set owner by default when calling add_child

[Error7Studios]

Describe the project you are working on:
JRPG/CCG (N/A)

Describe the problem or limitation you are having in your project:
It is very easy to forget to call set_owner() after calling add_child() to create nodes in the SceneTree with code.

Describe how this feature / enhancement will help you overcome this problem or limitation:
I think add_child() should automatically set the owner of the added/instanced node as the node that called add_child().
For instance, a savestate could be easily created, as all instanced nodes would be saved into the PackedScene when saving.
If the user didn't want this behavior, and instead wanted instanced nodes to be left out of the save file, they could set the owner as null when calling add_child().
add_child(instance, null)

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:
(see proposed default code below)

Describe implementation detail for your proposal (in code), if possible:
void add_child(instance: Node, owner: Node = self, legible_unique_name: bool = false)

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

Is there a reason why this should be core and not an add-on in the asset library?:
I think most users would expect that calling add_child() would be all that they would have to do to build the SceneTree with code.

By not having add_child() automatically call set_owner(), loaded game files would be missing all of the nodes that were created in code, because the user didn't know that they had to call set_owner() manually.

[volzhs]

in most cases of developing a game, you don't need set_owner.
it's a special case of creating a tool inside the editor.
the most use case of using owner is to see if the node is inside of a specific scene instance with a specific script.

if node.ower is preload("res://some_script.gd"):
    # do somthing with this node

making owner default value to self would make developing game harder.
most users should not forget about add_child(node, null)

it should be documentation issue about creating a tool for editor inside, not changing default owner value to self.

[volzhs]

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

it is actually Yes.
just adding one line of instance.set_owner(owner_node)

[golddotasksquestions]

@volzhs The problem is the inconsistency with adding nodes via editor, which sets the owner automatically. I had no idea why owner sometimes worked and sometimes did not for months, because I add nodes via the Editor most of the time but not always; until @Error7Studios informed me about the difference. It's easy to miss and if you don't know about this inconsistency a source of confusion and endless bug hunting.
Even if the documentation is improved, the issue is still the inconsistency.

I never used owner the way you described btw, but rather to get a node in a relationship independent way. Therefore I would also welcome if by default owner would be not just the parent, but the top most node in the scene tree, just like it is when you add nodes via editor.

[volzhs]

i still document should clearly explain what the owner is.
the owner is not just parent but the scene which has it in scene file.

[volzhs]

that only cares about creating a editor plugin.
if the owner points to the scene root, what about runtime?

A.scn is created by editor tool. fine.
you have a instance of B.scn which has a instance of A.scn.
now, A.owner == B.scn if you do not specified owner by add_child(load("A.scn").instance())
what about you have a instance of B.scn which has a instance of A.scn?
now we got A.owner == C.scn.

how you distinguish between them?
what if they have same name with same hierarchy?

but rather to get a node in a relationship independent way

@golddotasksquestions would you explain how you did it?

[golddotasksquestions]

owner.find_node("node_name")
or
owner.get_node("node_path")
if I know the path won't change.

[volzhs]

AFAIK, find_node is pretty expansive when scene has pretty compliated hierarchy.
what if multiple instances? it couldn't keep same node_path in runtime.
what if same name with same hierarchy but not has the script?

don't you think it's a another documentation issue as you described in your way?

[golddotasksquestions]

find_node is pretty expansive when scene has pretty complicated hierarchy.

Yes, I know what you mean. Never experienced a performance dip, so far I've only used it in rather shallow trees though. For very deep and complex hierarchy I would use get_tree().get_nodes_in_group("group_name"), this has the slight disadvantage to have to have the node added to a unique group first.

what if multiple instances?

same get_tree().get_nodes_in_group("group_name"), same slight disadvantage

what if same name with same hierarchy but not has the script?

I don't understand the question.

don't you think it's a another documentation issue as you described in your way?

No, for me the issue is the inconsistency between doing the same action in code vs in editor. I believe these should work the same way. Either have in both cases no owner assigned or do have the same owner assigned automatically.

[Xrayez]

The question is whether this breaks any compatibility and many assumptions which the engine currently has over how the owner is assigned. If it does, then it's 4.0 material, but even if it does make sense to make this default, unfortunately, I think there's too many changes required to make this convenient for others.

I do recall myself trying to create nodes via tool plugin/script the first time stumbling upon similar nuances. It was 2.1 times where documentation was really lacking though, in 3.0+ it's improved significantly.

When I look at the Node docs, I find this bit:

To keep track of the scene hierarchy (especially when instancing scenes into other scenes), an "owner" can be set for the node with the owner property. This keeps track of who instanced what. This is mostly useful when writing editors and tools, though.

This does not specify in what cases this is mostly useful, though.

[Error7Studios]

in most cases of developing a game, you don't need set_owner

I just created a test project that generates scenes through code. If I didn't use set_owner, the entire thing would break or nodes would go missing when loading a save file.

it's a special case of creating a tool inside the editor.

It's not just used for tools, as I just explained. My use case had nothing to do with tools.

it is actually Yes.
just adding one line of instance.set_owner(owner_node)

No, it is not "one line." It is "one line" _every single time_ you use add_child() if your project involves dynamically creating, saving, and loading scenes, and if you forget to call set_owner() even _one_ time, your entire project can collapse. That's like saying writing closing brackets < }, ], ) > every time you write an open bracket < {, [, ( > is just _one_ thing you have to do. No, it is every single time.

At the very least, the editor should push a warning when you call add_child() but don't call set_owner() afterwards. If the user wants to dismiss or disable that warning, no problem.

The other option would be to add a new method, add_child_persistent(), that requires you to pass in an owner node. Either of those options shouldn't break any compatibility.

This does not specify in what cases this is mostly useful, though.

Dynamically created scenes are at least one example.

[Xrayez]

Dynamically created scenes are at least one example.

Just saying that this phrase should be in the documentation at least, with the code example for people to follow. 🙂

[willnationsdev]

If you have an owner, then you are serialized and you are not encapsulated, i.e. you are exposed as a visible node in the Editor. As such, if you made add_child() make an owner exist, you would be saying that, by default, nodes are serialized (no longer opt-in) and are not encapsulated (which is super bizarre as the default setting in an OOP API).

Furthermore, most of the time that you are creating nodes, it will be to build trees of nodes, not just flat lists of nodes. Therefore, having every one default to the adding instance means that if you built a tree with more than 2 depth, you'd end up with each descendant node believing its parent is its owner, by default. That makes even less sense than having every node have no owner by default, where none of them are serializable or exposed.

I do think it would be useful to make owner an optional parameter of the add_child() method call, that way it saves you having to make a separate line of code, but I would still have it default to null since that just makes more sense from an Object-Oriented perspective.

[PrestonKnopp]

You don't have to remember to manually set owner on every child. You can automate it. For example:

func save(root: Node, path: String) -> int:
    _recursively_set_owner(root, root)
    var pck = PackedScene.new()
    assert(pck.pack(root) == OK)
    return ResourceSaver.save(pck, path)

func _recursively_set_owner(root: Node, owner: Node) -> void:
    for child in root.get_children():
        child.owner = owner
        _recursively_set_owner(child, owner)

Hope this helps.

[Jummit]

Better would be to use propagate_call:
propagate_call("set_owner", [owner])

[Shadowblitz16]

or when you add the child...

add_child(child, [set_owner])

[Shadowblitz16]

you know I just ran into this problem again I got to say I would actually prefer that add_child by default set the owner.
like when would you need to have the owner be different then the parent's owner?

it just makes sense that add_child would also set the owner to the parents owner.
unity basically does this exact thing and it makes things so much easier both to understand and to use.

EDIT 1
I just realized having a private node would be one case in which you don't set owner, however why not just have a optional private option for add_child() instead?

[willnationsdev]

when would you need to have the owner be different then the parent's owner?

1. Private nodes which you have already commented on.
   
   
   
   • Note that the entire Godot Editor and most, if not all, of the nodes in the engine source code are built using private nodes.
   
   
2. Node trees that are created ad-hoc and then later added to the tree
   
   
   
   • Engine/Editor source code again does this frequently.
   
   
   • People who use any sort of procedural generation algorithm are very likely to do this.
   
   

however why not just have a optional private option for add_child() instead?

This was my suggestion here.

[Shadowblitz16]

@willnationsdev I would have to disagree with you about defaulting to hidden.
in more cases users want to see their nodes being added to the scene tree and its more beginner friendly too.
I think that something like.. add_child(node, hidden=false) would be good.

I'm not sure what you mean by number 2 but I'm sure it could be worked around by just storing the node and adding it later?

[willnationsdev]

Edit: forgot to change true to false in my third response's first paragraph. Whoops, lol.
Edit 2: 3rd response, 3rd implementation detail of add_my_child needs some work.

@willnationsdev I would have to disagree with you about defaulting to hidden.

Architecturally, it is a better design decision. add_child, as a method, would behave in complete antithesis to best practices if it were modified in the way you describe.

in more cases users want to see their nodes being added to the scene tree and its more beginner friendly too.

This is not always true. The common use case in scripting, perhaps? But anyone who wishes to design a node that properly encapsulates its internal state and/or features will intend to create hidden dependencies by default.

I think that something like.. add_child(node, hidden=false) would be good.

I actually think that having a bool would be more problematic. If the bool is true, then you leave owner null. Okay, but what if it is false? You infer it from the calling node? What if I want a different node to be the owner? E.g. it could be the calling node or it could be the calling node's owner. The method is better designed if someone can simply pass on their preferred owner optionally.

However, if you make it an optional owner parameter, that would make it impossible to have any inference of the owner. The explicitness is good, as a design decision, but it gets in the way of this proposal's goal. If you want to have another way of doing it that is more user-friendly for the scripting context specifically, then I'd recommend having a dedicated method just for that purpose. Not only would that make it easier to document the differences between the two, and keep the complexity of add_child lower, but it would also keep devs from needing to modify the entirety of the engine core to accommodate the change.

What if we had an add_my_child(node) method? The intent would be that you could construct any node tree you wanted in script code, either piece by piece or as a whole tree, and it would make all the attached nodes visible no matter what.

In practice, I think it would need to do the following:

1. If the adding node does not have an owner, then the added node would set the adding node as its owner.
2. If the adding node does have an owner, then the added node also sets that owner as its owner. That way, whether you create a new node tree or add to an existing one, you are still keeping the original root node of the tree as the owner for the newly added node.
3. Any of the added nodes' descendants which see the added node as their owner would need to have their owner reassigned to the adding node. It would need to not proceed beyond that point though for any given branch since you might encounter a subscene and you wouldn't want to break those scenes' nodes' view of their local scene root as being their owner. This would actually break adding a scene as a child, which I was trying to avoid. Would have to think over more deeply how to handle that scenario cause you'd want a node tree to be made visible (i.e. see the root as their new owner), but you wouldn't want to disrupt scenes that do the same: so maybe have it check the filename property to see if it's empty, and only reassign the descendants' owners in that case?

I'm not sure what you mean by number 2 but I'm sure it could be worked around by just storing the node and adding it later?

You can build a node tree piece by piece where you make a node, add it, make a node, add it, etc. Or, you can make a whole tree of nodes outside of the SceneTree and then later add the entire tree of nodes as a child at once.

In the former case, having a function that only updates the added node with the adding node's owner would work great.

In the latter case, it would only make the root node work since the separate node tree wouldn't have an owner to begin with and so none of the descendant nodes in the node tree would get the owner from that tree's root. Therefore, the nodes would still be hidden when the whole tree is attached to the SceneTree.

The customized method I mentioned above though would handle both of those scenarios smoothly I believe.

[Shadowblitz16]

@willnationsdev I guess there could be a add_child_owner(child, owner) method which would be more clear on what it does.
however I think that add_child(child, hidden=false) by the name of the function would by default add the child and make it visible in the scene tree.

[ZackingIt]

@willnationsdev What would you recommend we do in 3.2.2 in cases where we need to call_deferred the add_child, and also immediately set an owner? In these cases the add_child may not go through before setting the owner, throwing an error.

[willnationsdev]

@ZackingIt I'm not sure I follow. Couldn't you just make a helper function that does both in the order you need, and then use call_deferred on the helper function?

[ZackingIt]

Great point -- I did try that once before and it failed for an unrelated
reason, I guess I should just write a generic helper function per your
advice.

On Tue, Sep 15, 2020 at 12:15 AM Will Nations [e-mail]
wrote:

@ZackingIt https://github.com/ZackingIt I'm not sure I follow. Couldn't
you just make a helper function that does both in the order you need, and
then use call_deferred on the helper function?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot-proposals/issues/390#issuecomment-692452544,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AF6QHGVDSCBNIPVZAMF3EB3SF3S6PANCNFSM4KIQ4X6Q
.