Skip to content

Conversation

@CookStar
Copy link
Contributor

Added new overload.

#   Mathlib
from mathlib import QAngle
from mathlib import Vector

angle = QAngle()
forward = Vector()
angle.get_angle_vectors(forward)

@jordanbriere
Copy link
Contributor

Thanks! However, QAngle.get_angle_vectors(forward) was supposed to work already but it doesn't because I made a mistake when I originally exported it.

Basically, the Python keywords should be defaulted to None, not NULL. Being currently defined to NULL, Python interprets them as 0 meaning that the above call is currently interpreted as (QAngle, Vector, int, int) which obviously throw an error. Boost knows that None should be interpreted as NULL, so the correct fix would be to replace the original export:

		.def("get_angle_vectors",
			GET_FUNCTION(void, AngleVectors, const QAngle &, Vector *, Vector *, Vector *),
			"Euler QAngle -> Basis Vectors.  Each vector is optional",
			(arg("forward")=NULL, arg("right")=NULL, arg("up")=NULL)
		)

With:

		.def("get_angle_vectors",
			&GET_FUNCTION(void, AngleVectors, const QAngle &, Vector *, Vector *, Vector *),
			"Euler QAngle -> Basis Vectors.  Each vector is optional",
			(arg("forward")=object(), arg("right")=object(), arg("up")=object())
		)

So that the above call is properly interpreted as (QAngle, Vector, NULL, NULL). If you want to go ahead and PR that I will merge it, otherwise I will do that change most likely tonight.

Also, your overload export is ambiguous because QAngle.get_vector_angles(Vector) could match both because of the keywords. Which means the first method to be looked up, would be resolved regardless. For example, foo(1) is a valid call for foo(bar) but also foo(bar, baz=0) so the resolution would be ambiguous and the call undefined. Doesn't really matter here because both would have the same behaviour, but can cause confusing results into scenarios where the overload are not behaving the same but share signatures.

@CookStar
Copy link
Contributor Author

That seems to be the case.
I will close this pull request.

As for the overload, I am aware of that.
If the keywords are unified and the results are the same, I don't think there is a problem.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants