Store angles internally as radians

[vittorioromeo]

Discuss in #2976

[Bambo-Borris]

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 sf::Angle to the API.

[ChrisThrasher]

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.

[ChrisThrasher]

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.

[L0laapk3]

and also, in many cases there is no cost as the compiler will convert constants at compile time.

[vittorioromeo]

I think it's good to have this but will leave for a future PR to minimize friction in this one

[Bambo-Borris]

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.

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 sf::degrees(90.f) is really going to be a question of what scale does it take for a user to notice this inexact angle? I think by the time it becomes noticeably inexact, there are probably other visual problems a user has that far exceed the angle problem.

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.

[JonnyPtn]

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

[L0laapk3]

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

[vittorioromeo]

Other than the iOS sensor impl I'm not really seeing any less conversions, so does this actually change anything?

Every call to sf::Angle::asRadians used to incur a conversion cost prior to this PR.

[JonnyPtn]

And every call to asDegrees will in cur a conversion cost with this PR too right? So it's an arbitrary decision really

[vittorioromeo]

And every call to asDegrees will in cur a conversion cost with this PR too right? So it's an arbitrary decision really

It's really not because most of the programming/gamedev world has already "agreed" on using radians.

[JonnyPtn]

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

[eXpl0it3r]

I believe one of the main benefits is, that for certain internal calculations (anything using <cmath> functions), we wouldn't need to convert from degrees to radians. Meaning, every asRadians() that is there currently, won't require a conversion now - this doesn't necessarily show up in the PR, as no change is needed.

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 sf::Angles is people are free to choose, thus there will always be one or the other that requires a conversion.

[vittorioromeo]

using <cmath> functions), we wouldn't need to convert from degrees to radians. Meaning, every asRadians() that is there currently, won't require a conversion now - this doesn't necessarily show up in the PR, as no change is needed.

As for the API usage perspective, I don't think there's a point in making

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.

[coveralls]

Pull Request Test Coverage Report for Build 9068001424

Details

• 24 of 24 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.003%) to 51.903%

---

Totals
Change from base Build 9057848497:	0.003%
Covered Lines:	10478
Relevant Lines:	19006

---

💛 - Coveralls

[vittorioromeo]

Removed the controversial changes

[ChrisThrasher]

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 .asRadians() to a similar degree as .asDegrees()? Let's read the code.

As radians:

• Trigonometry in 4 example programs (see Add sin/cos/tan helpers for sf::Angle #2028 for a better API for common trig with sf::Angle)
• Use with miniaudio which expects radians
• Italic text calculations in sf::Text
• 2D matrix math in sf::Transform
• 2D matrix math in sf::Transformable
• 2D matrix math in sf::View
• 2D matrix math in sf::Vector2<T>

As degrees:

• Reporting gyroscope rates on iOS as degrees per second (See iOS reports gyroscope rates as deg/s while Android reports radians/s #3001 for why this may actually be a bug)

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 sf::degrees(45) rather than sf::radians(0.78539816339) since humans have a much easier time reasoning about angles in terms of degrees. Not necessarily saying anything new but I wanted to provide some more evidence for this fact. There is seemingly no place in the code (outside of that one iOS snippet) where a degree-based implementation reduces the number of degree<->radian conversions.

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 sf::degrees(360) % sf::degrees(15) + sf::degrees(45) or anything like that so I can't lean too heavily on previous arguments I made to this end.

I'll also mention as a historical fact that sf::Angle is currently implemented in terms of degrees because SFML 2 represented all angles as degrees. We did not ever discuss changing the underlying representation of angles in that PR. It was not something that ever crossed our mind if I recall correctly.

---

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.

[kimci86]

We have to expect users will continue to prefer writing sf::degrees(45) rather than sf::radians(0.78539816339) since humans have a much easier time reasoning about angles in terms of degrees.

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.

[ChrisThrasher]

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.

[ChrisThrasher]

@eXpl0it3r Any final thoughts?

[L0laapk3]

just some thoughts

[L0laapk3]

Note that these constructs already carry a baked in rounding error.

[L0laapk3]

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

[ChrisThrasher]

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?

[L0laapk3]

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.

[eXpl0it3r]

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.

[eXpl0it3r]

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.

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.

@eXpl0it3r Any final thoughts?

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.

[ChrisThrasher]

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.

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.

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.