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

[API Proposal]: Convert between degrees and radians #86402

Closed
bricelam opened this issue May 17, 2023 · 13 comments · Fixed by #88866
Closed

[API Proposal]: Convert between degrees and radians #86402

bricelam opened this issue May 17, 2023 · 13 comments · Fixed by #88866
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics help wanted [up-for-grabs] Good issue for external contributors

Comments

@bricelam
Copy link
Contributor

bricelam commented May 17, 2023

Background and motivation

Most applications performing 2D and 3D spatial calculations need to convert between radians and degrees. For example, Unity includes the constants Mathf.Deg2Rad and Rad2Deg; Microsoft XNA/MonoGame includes the functions MathHelper.ToDegrees and ToRadians; and NetTopologySuite inclues the methods Radians.ToDegrees and Degrees.ToRadians.

API Proposal

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

And, if Math and MathF can still evolve despite the introduction of generic math, it would be nice to also include sugar for these there.

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();
    }
}

API Usage

var angleInRadians = Math.PI;

// Convert from radians to degrees. Returns 180.0
var angleInDegrees= double.ToDegrees(angleInRadians);

// Convert from degrees to radians. Returns (an aproximation of) π
angleInRadians= double.ToRadians(angleInDegrees);

Alternative Designs

An alternative design was proposed in #38566:

  namespace System {
      public static class MathF {
+        public const float RadiansToDegrees = 57.295779513f;
+        public const float DegreesToRadians = 0.0174532925f;
    }

However, this was rejected in favor of methods:

Functions will allow a more precise implementation to be provided or for other optimizations to occur, without restricting things.

There was one counterpoint that should still be considered tough:

A constant could also be used for vectors

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

Risks

  • Adding members to an interface is always risky
  • There may be some churn and confusion as developers migrate from various APIs in existing libraries, but this is short-term
@bricelam bricelam added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 17, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 17, 2023
@ghost
Copy link

ghost commented May 17, 2023

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Most applications performing 2D and 3D spatial calculations need to convert between radians and degrees. For example, Unity includes the constants Mathf.Deg2Rad and Rad2Deg; Microsoft XNA/MonoGame includes the functions MathHelper.ToDegrees and ToRadians; and NetTopologySuite inclues the methods Radians.ToDegrees and Degrees.ToRadians.

API Proposal

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

And, if Math and MathF can still evolve despite the introduction of generic math, it would be nice to also include sugar for these there.

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();
    }
}

API Usage

var angleInRadians = Math.PI;

// Convert from radians to degrees. Returns 180.0
var angleInDegrees= angleInRadians.ToDegrees();

// Convert from degrees to radians. Returns (an aproximation of) π
angleInRadians= angleInDegrees.ToRadians();

Alternative Designs

An alternative design was proposed in #38566:

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

However, this was rejected in favor of methods:

Functions will allow a more precise implementation to be provided or for other optimizations to occur, without restricting things.

Risks

  • Adding members to an interface is always risky
  • There may be some churn and confusion as developers migrate from various APIs in existing libraries, but this is short-term
Author: bricelam
Assignees: -
Labels:

api-suggestion, area-System.Numerics

Milestone: -

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels May 17, 2023
@tannergooding
Copy link
Member

There was one counterpoint that should still be considered tough: A constant could also be used for vectors

If this is an important scenario, equivalent vector methods should be exposed instead.

@hez2010
Copy link
Contributor

hez2010 commented May 18, 2023

Instead of putting them under System.Math, I would like .NET to provide graphics primitives and spatial calculation helpers in System.Numerics.Spatial as a separate new assembly.

@tannergooding
Copy link
Member

Conversion between degrees and radians are fairly core and primitive operations, there is no reason for them to be in their own less discoverable namespace/location nor for them to be disconnected from the generic math interfaces.

If you have a desire for additional functionality, you'll need to open a separate issue/discussion covering what exactly you're looking for in more detail. -- "graphics primitives" and "spatial calculation helpers" is itself fairly vague and could cover many topics. We already have a number of graphics primitives under System.Numerics and a plethora of APIs/building blocks that can be used for gaming, spatially aware contexts, etc.

For a discussion, covering the gist of what's desired with a few examples in text is fine. For an actual issue proposing new API surface, it needs to follow the API proposal template and include the full surface area of what's being requested and details about the design, usage, other considerations, etc.

@bartonjs
Copy link
Member

bartonjs commented May 18, 2023

Video

  • We changed ToRadians to DegreesToRadians and ToDegrees to RadiansToDegrees so they read better (double.RadiansToDegrees(double.Pi) vs double.ToDegrees(double.Pi))
  • The method will be specifically implemented (and thus visible) on Single, Double, Half, and NFloat.
  • The proposal also mentioned adding these to Math and MathF, but that is redundant with the methods being exposed on Double and Single already.
  namespace System.Numerics {
      public interface ITrigonometricFunctions<TSelf> {
+         public static virtual TSelf RadiansToDegrees(TSelf radians) { imprecise, but working, body }
+         public static virtual TSelf DegreesToRadians(TSelf degrees) { imprecise, but working, body }
      }
  }

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 18, 2023
@Tarun047
Copy link
Contributor

Hi I'd like to take this up, if it's ready for grabs 🙂

@tannergooding tannergooding added the help wanted [up-for-grabs] Good issue for external contributors label May 31, 2023
@tannergooding
Copy link
Member

Assigned out, let me know if you need any assistance 😄

@Tarun047
Copy link
Contributor

Tarun047 commented Jun 4, 2023

@tannergooding
I think we will have to make it abstract and have the implementations in Single, Double, Half, and NFloat.
Because the type TSelf has no assurance on the type having support for basic arithmetic operators like * or /.
This causes build errors like the one shown below:
Screenshot 2023-06-04 at 3 35 36 PM

When I tried to cast it as double and perform the operation.
It's causing the following build error
Screenshot 2023-06-04 at 3 33 40 PM

@tannergooding
Copy link
Member

It does, because ITrigonometricFunctions inherits from IFloatingPointConstants which inherits from INumberBase which itself implements IMultiplyOperators and IDivisionOperators.

The implementation you've defined is invalid, is the problem. The implementation should be something more like:

public static virtual TSelf RadiansToDegrees(TSelf radians)
{
    return (radians * TSelf.CreateChecked(180)) / TSelf.Pi;
}

public static virtual TSelf DegreesToRadians(TSelf degrees)
{
    return (degrees * TSelf.Pi) / TSelf.CreateChecked(180);
}

When writing generic math based implementations, things need to be in terms of TSelf.

@danmoseley
Copy link
Member

Will the Jit be able to lower this into a constant, and if so does it matter whether there are parens to do the division first?
TSelf.Pi) / TSelf.CreateChecked(180)

@tannergooding
Copy link
Member

This is explicitly not lowerable to a constant and we don't want it to be because it may produce a different result for some inputs. That being said, for a constant input, the JIT would be able to optimize it down to a constant output.

The parens are there for clarity and to help emphasize the correct order of operations, even if it matches what the compiler will actually do. Where division is done in an expression is a place that many people frequently get wrong so having them helps clarify it. It also makes it clear that PI / 180 or 180 / PI will not (and should not) be folded.

@Joe4evr
Copy link
Contributor

Joe4evr commented Jun 5, 2023

@danmoseley As Tanner said in the API review:

"180 divided by Pi is not going to be exactly 180 divided by Pi"

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2023
@tannergooding
Copy link
Member

I've put up #88866 since there's been no response in over a month and we only have a couple more days before we lose the opportunity to land these in .NET 8

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants