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

Fix #5388. Made Vector2 angle methods consistent #5428

Merged
merged 9 commits into from Sep 2, 2020

Conversation

obigu
Copy link
Contributor

@obigu obigu commented Oct 26, 2018

Fix #5388 and #5385.

This PR contains two different changes to Vector2 angle methods:

  1. Fixed angle(Vector2) and angleRad(Vector2) to return counter-clockwise angles as specified in their javadocs.
  2. Made angle(Vector2) consistent to angle() to return values in the [0, 360) range.

@intrigus
Copy link
Contributor

This heavily breaks backwards compatibility, so I don't think this will get merged.

@obigu
Copy link
Contributor Author

obigu commented Dec 22, 2018

Well, there are 2 different points on the PR.

Point 1. is a matter of fixing a wrong implementation which I think comes as top priority over backwards compatibility (otherwise the only alternative solution is to change the javadocs to reflect what it actually does, mark it as deprecated and create a new one that is consistent with the rest of methods which are counter-clockwise). Or do we just ignore it's wrong?

Regarding 2. it does break backwards compatibility and it may be argued that for not a good enough reason, but not "heavily". There will be some cases in which it does break something but it won't in the majority of cases as angles are usually dealt with correctly no matter the range.

If the PR is not being merged due to point 2. I'm happy to remove it and keep only 1.

@ShibaBandit
Copy link

I would recommend just clarifying JavaDocs to the actual behavior.

@obigu
Copy link
Contributor Author

obigu commented Dec 22, 2018

I would recommend just clarifying JavaDocs to the actual behavior.

I assume you've read #5388.

If we just change the javadocs (and do not deprecate them and create new consistent ones) we would be validating/embracing a bad API and giving up on improving it.

Both approaches (breaking backwards compatibility or deprecation) are valid and acceptable depending on the case, taking shortcuts isn't.

@crykn
Copy link
Member

crykn commented Aug 31, 2020

Hm, my two cents on this issue: I thinks, in general, it makes sense to fix to fix these inconsistencies. However, to make it more obvious that something changed with the angle methods, I vote for renaming them to angleDeg(), as suggested in one of the initial issues. This way, the overall naming is more consistent and only angleRad(Vector2) returns different values than before.

@obigu
Copy link
Contributor Author

obigu commented Aug 31, 2020

I think it's a good suggestion @crykn. By renaming them to angleDeg, not only we "force" the users to look for the new method in CHANGES but we also make the name more meaningful (I've found myself checking in docs if angle() returned deg or rad more than once). I'll update the PR.

@Darkyenus
Copy link
Contributor

How about keeping the old methods and deprecating them for a few versions (then delete)? The price is very low and it would be a good opportunity to update the javadocs of the old methods to mention that the replacements have a different behavior.

@yuripourre
Copy link
Contributor

@Darkyenus now that libgdx is going to have more frequent releases sounds a very good idea, indeed.

@obigu
Copy link
Contributor Author

obigu commented Sep 1, 2020

@crykn @Darkyenus I've added new methods with "deg" suffix, deprecated the old ones and added javadocs were required. To make it consistent I've done it for all degrees-based methods (not only angle() and angle(Vector2). The only backwards compatibility change affects angleRad(Vector2) (which should not be a very commonly used method) and CHANGES have been updated to mention it especifically.

Copy link
Member

@crykn crykn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think deprecating rotate90 (int dir) is needed, but I don't mind.

@obigu
Copy link
Contributor Author

obigu commented Sep 1, 2020

I don't think deprecating rotate90 (int dir) is needed, but I don't mind.

It's just an unnecessary method

@mgsx-dev mgsx-dev added the breaking change Label for breaking changes label Sep 1, 2020
@Darkyenus
Copy link
Contributor

I am also against deprecating rotate90. While it is a functional subset, it is computationally more efficient, and, more importantly, 100% precise.

@obigu
Copy link
Contributor Author

obigu commented Sep 1, 2020

I don't agree with your point, I don't think it makes sense to accept this extra computational cost and lack of precision for any other rotation except for 90º (if that's critical for an app it should implement higher precision logic on their end). But from the thumbs up votes I guess most of you are in favout of keeping it so I go with the majority.

My question before making the change is, if this method is useful for the API, shall we rename it to something that is degrees or radians independent for consistency (like rotateToPerpendicular() or something like that?

@Darkyenus
Copy link
Contributor

Why break existing code over something that works well? I would keep the name as it is, 90 in context of rotations is self explanatory.

@obigu
Copy link
Contributor Author

obigu commented Sep 2, 2020

Requested change made

@obigu obigu merged commit 1fc7cd4 into libgdx:master Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Label for breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Vector2.angle and Vector2.angleRad consistent
7 participants