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

Add sin/cos/tan helpers for sf::Angle #2028

Closed
wants to merge 1 commit into from

Conversation

ChrisThrasher
Copy link
Member

Description

I noticed there were many instances of using asRadians just to use standard trigonometry functions so I added some additional functions that make it more easy to use sf::Angle with trig functions. This is a nice bit of syntactic sugar but also avoids the pitfalls of users calling asRadians which eschews the type safety of sf::Angle. We'd prefer any use of sf::Angle be in a type safe manner.

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #2028 (e9c1b62) into master (fab2c93) will increase coverage by 0.05%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2028      +/-   ##
=========================================
+ Coverage    7.16%   7.22%   +0.05%     
=========================================
  Files         185     186       +1     
  Lines       15793   15800       +7     
  Branches     4163    4171       +8     
=========================================
+ Hits         1132    1141       +9     
+ Misses      14529   14527       -2     
  Partials      132     132              
Impacted Files Coverage Δ
src/SFML/Graphics/CircleShape.cpp 0.00% <0.00%> (ø)
src/SFML/Graphics/View.cpp 0.00% <0.00%> (ø)
include/SFML/System/Angle.hpp 100.00% <100.00%> (ø)
src/SFML/Graphics/Transform.cpp 100.00% <100.00%> (ø)
src/SFML/Graphics/Transformable.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fab2c93...e9c1b62. Read the comment docs.

@eXpl0it3r
Copy link
Member

eXpl0it3r commented Feb 24, 2022

I'm not convinced of this change. From my point of view this is more API bloat than anything else. It puts the whole usage of sf::Angle in question, when we internally write short hands, because we consider the API too long/verbose. Additionally, while sin, cos and tan are the most common functions, what about asin, acos, atan, atan2, sinh, cosh, tanh, asinh, acosh, atanh? Or what about some other functions that takes angle?
On top of that, I really don't want SFML to pretend to be a math library, which by implementing our own version of sin, cos and tan you start to blur the line.

@ChrisThrasher ChrisThrasher deleted the angle_helpers branch February 24, 2022 23:31
@Bromeon
Copy link
Member

Bromeon commented Feb 25, 2022

Once #1979 is merged, the need to explicitly use sin/cos will also be reduced. Maybe not so much for SFML itself, but definitely for the user.

In this regard, I see vector APIs as an abstraction layer above trigonometric functions.

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

Successfully merging this pull request may close these issues.

None yet

3 participants