Add unit tests for Node2D helper methods

[2nafish117]

Add Unit tests for Node2D helper methods (look_at, get_angle_to, to_local, to_global, get_relative_transform_to_parent)
Link to #43440

[2nafish117]

When you pass in a sibling into Node2D::get_relative_transform_to_parent, it must return Transform2D() according to the code, but it doesn't seem to do so. Does anyone know why?

CHECK(test_node3->get_relative_transform_to_parent(test_sibling).is_equal_approx(Transform2D()));

[AThousandShips]

When you pass in a sibling into Node2D::get_relative_transform_to_parent, it must return Transform2D() according to the code, but it doesn't seem to do so. Does anyone know why?

That's not what happens with the code, it doesn't check for that, it just goes up the tree until it finds a non-2D node or no parent, it errors if the parent isn't a 2D node and then returns the empty transform, but it first combines this with the local transform:

godot/scene/2d/node_2d.cpp

Lines 392 to 406 in c4279fe

	Transform2D Node2D::get_relative_transform_to_parent(const Node *p_parent) const {
	ERR_READ_THREAD_GUARD_V(Transform2D());
	if (p_parent == this) {
	return Transform2D();
	}
	Node2D *parent_2d = Object::cast_to<Node2D>(get_parent());
	ERR_FAIL_NULL_V(parent_2d, Transform2D());
	if (p_parent == parent_2d) {
	return get_transform();
	} else {
	return parent_2d->get_relative_transform_to_parent(p_parent) * get_transform();
	}
	}

[2nafish117]

When you pass in a sibling into Node2D::get_relative_transform_to_parent, it must return Transform2D() according to the code, but it doesn't seem to do so. Does anyone know why?

That's not what happens with the code, it doesn't check for that, it just goes up the tree until it finds a non-2D node or no parent, it errors if the parent isn't a 2D node and then returns the empty transform, but it first combines this with the local transform:

godot/scene/2d/node_2d.cpp

Lines 392 to 406 in c4279fe

	Transform2D Node2D::get_relative_transform_to_parent(const Node *p_parent) const {
	ERR_READ_THREAD_GUARD_V(Transform2D());
	if (p_parent == this) {
	return Transform2D();
	}
	Node2D *parent_2d = Object::cast_to<Node2D>(get_parent());
	ERR_FAIL_NULL_V(parent_2d, Transform2D());
	if (p_parent == parent_2d) {
	return get_transform();
	} else {
	return parent_2d->get_relative_transform_to_parent(p_parent) * get_transform();
	}
	}

The macro ERR_FAIL_NULL_V_MSG returns from the function if the pointer is null, and it doesnt combine with the local transform.

Here's how i think it executes:

because I'm passing in a sibling, we climb up the tree until we hit the root and then the root at the top of the tree will return null for get_parent(), and that causes the macro to err and return Transform().

I would like to debug this with breakpoints, but my breakpoints doesn't seem to hit on visual studio for tests, is there something I need to change to make that work?

/**
 * Try using `ERR_FAIL_NULL_V_MSG`.
 * Only use this macro if there is no sensible error message.
 *
 * Ensures a pointer `m_param` is not null.
 * If it is null, the current function returns `m_retval`.
 */
#define ERR_FAIL_NULL_V(m_param, m_retval)                                                              \
	if (unlikely(m_param == nullptr)) {                                                                 \
		_err_print_error(FUNCTION_STR, __FILE__, __LINE__, "Parameter \"" _STR(m_param) "\" is null."); \
		return m_retval;                                                                                \
	} else                                                                                              \
		((void)0)

[AThousandShips]

No it calls for the steps until the parent is not a Node2D:

return parent_2d->get_relative_transform_to_parent(p_parent) * get_transform(); 

[2nafish117]

@AThousandShips Ah yes, you're right, I somehow seemed to have forgotten about recursion going back down the stack.
Ive fixed the test now.

[2nafish117]

Hello, can someone review this PR, it is ready to merge.

[Calinou]

Code looks good to me.

[AThousandShips]

There's been some changes to the unit test system so a rebase to make sure everything still works right would be good

[AThousandShips]

You used a merge commit to update your branch, please use rebase in the future instead, see here, you should remove the merge commit and rebase instead

[AThousandShips]

You need to reset your branch to a safe state, and then rebase, do git reset --hard f359115

[2nafish117]

Ah, thank you @AThousandShips, I think it looks good now.

[akien-mga]

Thanks!