godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.35k stars 21.06k forks source link

parenthood check during add_child_below_node() is incorrect #40264

Closed jamie-pate closed 4 years ago

jamie-pate commented 4 years ago

Godot version: 3.2.2

OS/device including version:

GeForce GTX 1070 with Max-Q Design/PCIe/SSE2 No LSB modules are available. Distributor ID: Pop Description: Pop!_OS 20.04 LTS Release: 20.04 Codename: focal

Issue description:

The check to see if p_node is our child is incorrect. is_a_parent_of() is actually an ancestry check (poorly named?) which checks if the node is a child, or ancestor of this node. Should check p_node->parent == this instead.

void Node::add_child_below_node(Node *p_node, Node *p_child, bool p_legible_unique_name) {

    ERR_FAIL_NULL(p_node);
    ERR_FAIL_NULL(p_child);

    add_child(p_child, p_legible_unique_name);

    if (is_a_parent_of(p_node)) { // <-- WRONG
        move_child(p_child, p_node->get_position_in_parent() + 1);
    } else {
        WARN_PRINTS("Cannot move under node " + p_node->get_name() + " as " + p_child->get_name() + " does not share a parent.");
    }
}

Steps to reproduce:

Open vscode and read the code ;)

Minimal reproduction project:

Not actually required? (just don't have time for a PR atm)

pouleyKetchoupp commented 4 years ago

I can confirm this is causing the first test to pass when it shouldn't in case p_node is not the direct parent, causing the move_child method to use an invalid index in some cases.

Minimal reproduction project: add-below-node.zip

In the editor, when checking Add node in the inspector for the root node, this error appears:

ERROR: Invalid new child position: 3.
   At: scene/main/node.cpp:330

When it should log this warning instead:

WARNING: add_child_below_node: Cannot move under node Child-05 as Child-new does not share a parent.
     At: scene/main/node.cpp:1195

It only needs to be fixed on 3.2, because add_child_below_node was replaced with add_sibling on master in #38695.

akien-mga commented 4 years ago

Fixed by #40300.