-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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 rotate_toward
and angle_difference
methods.
#80225
Conversation
I'd suggest adding tests to ensure correct behaviour of these new functions |
3496167
to
b60adfe
Compare
46df1af
to
dc30bcd
Compare
Haven't tried setting up tests myself yet, would it be okay if someone else could do it when possible? |
81c4940
to
c76f0a0
Compare
4ba70b5
to
134f1a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should have unit tests for these new methods. It looks like there are already unit tests for lerp_angle
, and angle_difference
is simpler, so fewer unit tests are needed for angle_difference
than lerp_angle
, but there should still be some. rotate_toward
should have a similar number of unit tests as lerp_angle
.
134f1a9
to
798d92d
Compare
Tried it out (probably not the best). |
798d92d
to
9bc7a36
Compare
9bc7a36
to
9cdfbad
Compare
rotate_toward
and angle_difference
methods.
9ca94f0
to
8ec90b3
Compare
03679a6
to
66e012b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the code/behavior (but I've suggested the current implementation so yeah... 🙃).
I'm not sure/convinced about the rotate_toward
naming, personally I liked move_toward_angle
more. But if rotate_toward
is what people like more then I'm fine with it. Not having "angle" anywhere could possibly be bad for discoverability though so maybe e.g. rotate_toward_angle
should be considered as an alternative. 🤔 Either way I'm fine with any of these.
AFAICT the methods being added are desired/used quite often (and they seem easy to implement incorrectly). Proposal (godotengine/godot-proposals#2074) has positive feedback, also personally in the last ~2 monts I've redirected users ~5 times to take a look at this PR for how to implement such functions (which they were directly/indirectly asking/looking for).
cc @akien-mga as not sure who to poke here. 🙂 If overall we're fine with adding such functions then I think resolving this for 4.2 makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akien-mga Can you do a production team review and merge?
@kleonc and @aaronfranke maintain some of the math codes
d100e5a
to
3a39de4
Compare
Sorry, still have trouble with using git so i took a while to squash commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of having this method, I reviewed the code, test cases, and docs. It looks good to me 👍
Main thing that motivated me to rename this to |
Both As for |
Thanks! And congrats for your first merged Godot contribution 🎉 |
Finally I don't have to write my own clunky stuff to rotate my NPCs to target... 🎉 |
Thanks, and welcome @ettiSurreal ! |
Closes: godotengine/godot-proposals#2074
Supersedes: #69081
Adds:
angle_difference
, gets the difference between two angles in radians.rotate_toward
, an angular analog ofmove_toward
. Movesfrom
toto
by an increment specified bydelta
, similarly to lerp_angle wrapping around after TAU and picking the shortest path. Passing in a negative value moves toward the exact opposite angle.Called
move_toward_angle
in the old PR.Additionally
lerp_angle
is internally modified to utilizeangle_difference
in order to avoid code repetition.Implemented for GDScript and C#.
Current implementation based on:
#69081 (comment)