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
Store angles internally as radians #2978
Conversation
Personally, I can't see a downside to this change. Think it makes sense, and is a nice, small improvement atop the already great work done by adding |
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'm not sure whether I prefer this new implementation. While this change may theoretically lead to fewer internal conversions, it also means using this API in terms of degrees yields results that are less exact than before.
include/SFML/System/Angle.hpp
Outdated
/// Internally, sf::Angle stores the angle value as radians. | ||
/// | ||
/// For the best performance and to minimize run-time conversions, | ||
/// it is recommended to work with radians whenever possible. | ||
/// |
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 don't necessarily recommend this. I want users to do whatever they find most ergonomic. Besides, it breaks encapsulation to start making recommendations based on implementation details.
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.
and also, in many cases there is no cost as the compiler will convert constants at compile time.
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 think it's good to have this but will leave for a future PR to minimize friction in this one
This may very well be true, but given the usecases of SFML within games is quite prevalent, the use of radians is preferred in this scenario in my view. Also tying back to the fact the standard library maths functions also prefer radians, SFML internally using radians seems sensible. The fact it will be slightly less exact when you do If we look to raylib it requires users pass an angle in degrees, but internally always convert that to radians for use in their own maths libraries. GLM will always require radians for rotations. I think the fact we have this nice abstraction means users can still work within the realm of degrees, but internally I think it's sensible for SFML to utilise radians. |
Other than the iOS sensor impl I'm not really seeing any less conversions, so does this actually change anything? As far as I can tell the only motivation is that some people may think the implementation is unintuitive, but the same could easily be said for using radians. It seems subjective without any technical reason to change |
I have linked a few now removed conversions here: #2976 |
Every call to |
And every call to |
It's really not because most of the programming/gamedev world has already "agreed" on using radians. |
I don't really see what relation that has, as SFML doesn't use radians or degrees in it's API now (would have been relevant with 2.x API where degrees was used). This is just about which is used internally Worth noting that everybody upgrading to SFML 3 will be passing degrees in too due to the existing API |
I believe one of the main benefits is, that for certain internal calculations (anything using As for the API usage perspective, I don't think there's a point in making guesses in who and how many are using which API. The point of |
SFML can be and is often used in conjuction with other libraries such as Vulkan, glm, or even the Standard Library that all expect angles in radians. |
Pull Request Test Coverage Report for Build 9068001424Details
💛 - Coveralls |
fcebbce
to
30cdd64
Compare
Removed the controversial changes |
Did a quick accounting of where we need angles as radians versus where we need angles as degrees to help understand which format gets more use, at least within SFML itself. Is it true we use As radians:
As degrees:
As it stands now users are getting free construction from degrees and then repeated conversions to radians under the hood since as evident by the source code. Upon changing to a radians-based implementation, construction from degrees is possibly the only place where a conversion between degrees and radians (in either direction) would be necessary. We have to expect users will continue to prefer writing The only place degrees would be superior is if a user has lots of code that performs arithmetic of angles in terms of degrees. This is code that theoretically exists but I have no evidence to believe that users are in fact routinely writing code like I'll also mention as a historical fact that In conclusion, my hesitation about a radians-based implementation are probably outweighed by the benefits of performing fewer casts between radians and degrees. I think I'm leaning in favor of this PR, certainly enough so that I don't want to block it. If the team is in favor then let's go for it. |
It is worth noting that the conversion can happen at compile time when using hardcoded contant angles or constant expressions because Angle uses constexpr everywhere. |
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.
The core of the PR is good.
My one nit is your use of .f
suffixes. We don't need to do that. sf::degrees(360)
is perfectly safe and well defined. No need to add .f
in so many places. I find it no easier to read and it has no technical benefits.
30cdd64
to
692032c
Compare
@eXpl0it3r Any final thoughts? |
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.
just some thoughts
@@ -55,104 +53,104 @@ constexpr Angle::Angle() = default; | |||
//////////////////////////////////////////////////////////// | |||
constexpr float Angle::asDegrees() const | |||
{ | |||
return m_degrees; | |||
return m_radians * (180.f / priv::pi); |
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.
Note that these constructs already carry a baked in rounding error.
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.
Same with calls to asDegrees()
, I guess it could have a double overload, I'm not sure if we have to care that much? Just pointing it out
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.
Are you saying that we could get marginally better accuracy if we used a single literal to represent 180 / pi rather than using this particular expression?
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.
Yes. Keyword here is marginally.
I tried to demonstrate that with the godbolt link, a different value is present when I calculated the constant with extra precision and then converted it to float.
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.
57.295776
- float (Compiler Explorer)
57.2957802
- double (Compiler Explorer)
57.295779513082323402053960025447
- actual (Windows calculator)
0.000003513082323402053960025447
- difference float
0.000000686917676597946039974553
- difference double
I guess in theory you could also do the full math in doubles and only covert at the end, but I feel like if you care about such a precision, you may want to implement the mathy bits yourself.
The change makes sense to me, but could also be a separate PR.
Personally, I like types to be explicit and not rely on implicit conversion, especially since you then have to memorize in which scenario it's not considered a narrowing conversion.
I too think it's a sensible change, given there's no real reason to stick to degrees when the interface is abstracted and allows us to do fewer conversions internally. |
You don’t have to memorize anything. The compiler won’t allow narrowing conversions to happen since we have all the conversion warnings turned on. |
Discuss in #2976