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

Math & MathF: Conversion constants between degrees and radians #38566

Closed
Ramobo opened this issue Jun 29, 2020 · 16 comments
Closed

Math & MathF: Conversion constants between degrees and radians #38566

Ramobo opened this issue Jun 29, 2020 · 16 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Numerics
Milestone

Comments

@Ramobo
Copy link

Ramobo commented Jun 29, 2020

Background and Motivation

My use-case is working with quaternions, since the inputs for Quaternion.CreateFromYawPitchRoll(...) are in radians.

Proposed API

namespace System
{
    public static class Math
    {
+        public const double RadiansToDegrees = 57.295779513;
+        public const double DegreesToRadians = 0.0174532925;
    }

    public static class MathF
    {
+        public const double RadiansToDegrees = 57.295779513f;
+        public const double DegreesToRadians = 0.0174532925f;
    }
}

Don't quote me on the values, and add more precision to the double variant. float should have 9 decimal places, and double should have 29.

Usage Examples

Vector3 input = new Vector3(10f, 20f, 30f) * MathF.DegreesToRadians;

Quaternion quaternion = Quaternion.CreateFromYawPitchRoll(input.Y, input.X, input.Z);

Alternative Designs

  • Use methods instead. Not ideal, since then you need more code to convert types with multiple numerical components:
    • Constant: vector * MathF.DegreesToRadians;
    • Method: public Vector3 DegreesToRadians(this Vector3 vector) => new Vector2(MathF.DegreesToRadians(vector.X), MathF.DegreesToRadians(vector.Y));

Risks

None.

@Ramobo Ramobo added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 29, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Numerics untriaged New issue has not been triaged by the area owner labels Jun 29, 2020
@ghost
Copy link

ghost commented Jun 29, 2020

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

@tannergooding tannergooding added this to the Future milestone Jun 29, 2020
@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Jun 29, 2020
@Clockwork-Muse
Copy link
Contributor

... what's the standard with respect to doing something like calculating the value, instead of a literal constant. Eg:

namespace System
{
    public static class Math
    {
+        public const double RadiansToDegrees = 180.0 / PI;
+        public const double DegreesToRadians = 1.0 / RadiansToDegrees;
    }
}

@tannergooding
Copy link
Member

I think these should be functions rather than constants. Functions will allow a more precise implementation to be provided or for other optimizations to occur, without restricting things.

@Ramobo
Copy link
Author

Ramobo commented Jun 29, 2020

Can we have both? What about the vector situation?

@tannergooding
Copy link
Member

When dealing with floating-point, defining a constant using the precisely computed value is generally better than using a constant expression.

180 / π expressed as a literal is     57.2957795130823208767981548141051703324054724665643215491602438612028471483215526324409689958511109441862233816328...
As the nearest expressible double is: 57.29577951308232286464772187173366546630859375
As the nearest expressible float:     57.295780181884765625

However, computing the same using an expression gives:

Double Literal:    57.29577951308232286464772187173366546630859375
180.0 / Math.PI:   57.29577951308232286464772187173366546630859375
Float Literal:     57.295780181884765625
180.0f / MathF.PI: 57.2957763671875

So, while double is the same, float is not.

@devel0
Copy link

devel0 commented Aug 8, 2020

+1 for the use of constant with expression value ( ie. public const double RadiansToDegrees = 180.0 / PI; )
that automatically convert to the right constant at compile-time avoiding expression re-evaluation in the runtime.
source code results more clear to read and precision keep at the maximum allowed for the used type.

image

@hawkerm
Copy link

hawkerm commented Dec 28, 2022

Surprised this is still missing from the built-in Math library in C#. I'm converting some Python code over and it's a pain. They just have nice built-in functions since forever to do this: https://docs.python.org/3/library/math.html#angular-conversion

image

Would be nice to just have constants or easy to use ToDegrees and ToRadians function helpers (or both). I do find functions help call out more easily what the equation does as it's right in the name of the functions.

@tannergooding
Copy link
Member

Constants are a non-starter for the reasons I gave above.

If someone wants to open a new proposal, in the appropriate format, covering functions then I'd be willing to take it forward.

Please keep in mind that new functions are being exposed on the primitive types directly and may need to update the appropriate generic math interfaces with such methods.

@bricelam
Copy link
Contributor

To list some prior art in this area...

Unity has these constants:

  • Mathf.Rad2Deg
  • Mathf.Deg2Rad

Microsoft XNA/MonoGame has these methods:

  • MathHelper.ToDegrees(x)
  • MathHelper.ToRadians(x)

NetToplogySuite has these methods:

  • Radians.ToDegrees(x)
  • Degrees.ToRadians(x)

@tannergooding
Copy link
Member

tannergooding commented May 12, 2023

Functions remain the only path I'd be willing to drive forward for the reasons given above. This remains a case where if someone opens a proposal covering the functions I'd be happy to mark it api-ready-for-review.

Such functions should likely be DIMs hanging off of IFloatingPointIeee754<T>

@bricelam
Copy link
Contributor

bricelam commented May 17, 2023

Feels like these belong on ITrigonometricFunctions:

  namespace System.Numerics {
      public interface ITrigonometricFunctions<TSelf> {
+         public static abstract TSelf ToDegrees(TSelf radians);
+         public static abstract TSelf ToRadians(TSelf degrees);
      }
  }

Would we also add sugar for them on Math and MathF?

namespace System
{
    public static class Math
    {
+        public static double ToDegrees(double radians) => radians.ToDegrees();
+        public static double ToRadians(double degrees) => degrees.ToDegrees();
    }

    public static class MathF
    {
+        public static float ToDegrees(float radians) => radians.ToDegrees();
+        public static float ToRadians(float degrees) => degrees.ToDegrees();
    }
}

@tannergooding
Copy link
Member

Feels like these belong on ITrigonometricFunctions:

That sounds like a good place to me given their general usage. Could you open a new issue with the proposed surface and link back to this issue?

Would we also add sugar for them on Math and MathF?

This will probably be discussed in API review since this will be the first really "new" API since we introduced the generic math interfaces. My expectation is "no" and that their existence directly as float.ToDegrees and double.ToRadians will be sufficient and that will be how we approach the surface area moving forward.

@bricelam
Copy link
Contributor

Could you open a new issue

I can, but... How would it be different than this one? Isn't this one just iterating on the design of a feature for converting between radians and degrees? Seems kinda silly to open a new issue every time the proposed design of a feature changes. Wouldn't you lose all the context and feedback from upvotes and comments? Prioritization would be biased toward features initially proposed with acceptable designs with no regard for functionality. It's madness!!

lol, but yeah, I'll open a new issue.

@tannergooding
Copy link
Member

How would it be different than this one?

API review expects the top post to contain the proposal and summarize discussion points when/where appropriate. Updating this issue would require editing the original post to contain the new info, which is generally undesirable.

Seems kinda silly to open a new issue every time the proposed design of a feature changes

In this case, the premise behind the idea is the same, but the actual proposed surface is significantly different. Mentioning other proposals by referencing them helps ensure the relevant context isn't lost, but also avoids polluting the main proposal.

Consider that this issue was more a discussion (just created before GitHub discussions existed), rather than a proposal, and that the new thing is the actual proposal.

@bricelam
Copy link
Contributor

bricelam commented May 17, 2023

Filed #86402. Everyone, please review (and upvote) to make sure I adequately covered your scenarios and incorporated your feedback.

@tannergooding
Copy link
Member

Closing this in favor of #86402

@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Numerics
Projects
None yet
Development

No branches or pull requests

7 participants