Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Unit tests for Node2D helper methods #91654

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

2nafish117
Copy link
Contributor

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 2nafish117 marked this pull request as ready for review May 7, 2024 12:01
@2nafish117 2nafish117 requested a review from a team as a code owner May 7, 2024 12:01
@AThousandShips AThousandShips added this to the 4.x milestone May 7, 2024
@2nafish117 2nafish117 marked this pull request as draft May 9, 2024 07:07
@2nafish117
Copy link
Contributor Author

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()));

@2nafish117 2nafish117 marked this pull request as ready for review May 9, 2024 10:41
@AThousandShips
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants