Godot: Node.add_child_below_node mismatch with Node.add_child

[dodgyville]

Godot version:
3.x

Issue description:
void add_child ( Node node, bool legible_unique_name=false )
void add_child_below_node ( Node node, Node child_node, bool legible_unique_name=false )

The variable _node_ in _add_child_ is the node we are adding, the variable _node_ in _add_child_below_node_ is the node we using to add _child_node_ below. Different behaviour. I think the docs should be clearer or even better, the variable names unified.

[aaronfranke]

The second variable should probably be changed to parent_node and have its functionality changed accordingly. This would break compatibility though. Also, the name could be simplified to add_child_below since what else but a node would we be adding another node below?

[KoBeWi]

The parameter name change wouldn't break anything.
As for method name change, this could go here: #16863

[aaronfranke]

Is there any reason for add_child_below_node() to exist? Instead of add_child_below_node(a, b) can't you just do a.add_child(b)?

[KoBeWi]

Instead of add_child_below_node(a, b) can't you just do a.add_child(b)?

a is a sibling of the new node, not parent.

[dreamsComeTrue]

It was updated lately (master) by @Calinou and now is less confusing :smiley:

        <method name="add_child">
            <return type="void">
            </return>
            <argument index="0" name="node" type="Node">
            </argument>
            <argument index="1" name="legible_unique_name" type="bool" default="false">
            </argument>
            <description>
                Adds a child node.........
            </description>
        </method>
        <method name="add_child_below_node">
            <return type="void">
            </return>
            <argument index="0" name="preceding_node" type="Node">
            </argument>
            <argument index="1" name="node" type="Node">
            </argument>
            <argument index="2" name="legible_unique_name" type="bool" default="false">
            </argument>
            <description>
                Adds a child node.........
            </description>
        </method>

[aaronfranke]

I do still think it would make more sense for the order to be swapped, so that the node being added is the first argument in both functions.

Perhaps we should also rename this to something like add_child_as_sibling? It's not immediately clear what "below" means (as shown in my comment from last year which I crossed out).

[dreamsComeTrue]

I do still think it would make more sense for the order to be swapped, so that the node being added is the first argument in both functions.

Well, to maintain consistency - that's good proposal, but as the API is public - it may break something. Maybe Hugo would comment on that, as he originally did that update/rename.

[Calinou]

I think it would be better to have the arguments swapped as well. This way, the first argument of add_child_below_node() is the same as add_child() (which is arguably less confusing).

This is a breaking change, but we can do it for 4.0.

[KoBeWi]

Or it could be renamed to add_sibling and the parent argument could be removed entirely (it's not needed anyways) 🤔

[dreamsComeTrue]

Or it could be renamed to add_sibling and the parent argument could be removed entirely (it's not needed anyways) thinking

Well, actually there is no 'parent' argument in here - second argument is needed to calculate offset in childrens' list (as we need somehow to pass WHERE should we put the node in question - which is 'int' value at the end.)

[KoBeWi]

Ah, right .-.

[aaronfranke]

What about preceding.add_sibling(new_node), and add_sibling() just calls get_parent() internally? It's not as if the sibling node could have a different parent anyway.

[dreamsComeTrue]

OK, you convinced me :) I've replaced 'add_child_below_node' with 'add_sibling' - now it brings less confusion compared to how it was before, and clearly indicates who will be the parent of node being added (as it's implicitly taken from current node).

[remram44]

add_sibling() is not great because it makes it seem that the node will be added as a sibling of the node it's called on. E.g. seeing the function name, I expect node_a.add_sibling(node_b) to do basically node_a.get_parent().add_child(node_b). I might even pass that method in the list of methods while I'm looking for a way to insert a child.

At the risk of bike shedding, I would like to propose add_child_after(new_child, after_node), or even insert_child_after() (to match the name insert used to add an element in an Array at a different place than the end).

I don't really have an opinion on the order of arguments. Array.insert() takes (position, value) but here it's not a position but another Node.

On the subject of the docs, it is unclear how to insert a child in the first position (e.g. below/after none).

[aaronfranke]

@remram44 That's exactly the idea.

    child1
    child2
    child3

Then child2.add_sibling(new_child) would do this:

    child1
    child2
    new_child
    child3

[remram44]

I missed the fact that the behavior is changing too. Then it is extremely well named! :smile:

Sorry for the noise!

[jluini]

I think add_child_below_node was more clear than add_sibling (because the _below_ word), but the method isn't necessary at all.

I was confused by the fact that neither add_child nor add_child_below_node allows adding a node as first child (in a non-empty parent). I now know that move_child must be used after add_child to add a node as first child (please correct me if this is wrong).

I propose:

1. Keep add_child_below_node for compatibility, changing the parameter names if that helps (add_sibling is not needed and doesn't clarify anything).

2. Add a method like add_child_at_index with the obvious behaviour.

3. If the method I propose is not added, change the description of add_child to point out that move_child can be used afterwards. Or add an equivalent add_child_above_node.

[andy-noisyduck]

I appreciate this has already been merged, but I didn't notice at the time, so adding my late 2 cents.

The implemented behaviour of add_sibling feels super funky to me. Calling a method on a node which results in a change of state on the parent instead (i.e. where the new child is added)? That feels pretty strange. Is there a precedent for this anywhere else in Godot?

This solution also doesn't fix the issue that there is still no way to add a child at index 0 (without adding and then moving). That seems like a pretty reasonable thing to want to do.