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]: Quaternion.Zero is missing #57253

Closed
michael-hawker opened this issue Aug 12, 2021 · 11 comments · Fixed by #58406
Closed

[API Proposal]: Quaternion.Zero is missing #57253

michael-hawker opened this issue Aug 12, 2021 · 11 comments · Fixed by #58406
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
Milestone

Comments

@michael-hawker
Copy link

Background and motivation

Vector2, Vector3, and Vector4 when created with new() are all zeroed out and have corresponding .Zero static properties. Quaternion on the otherhand only has the Identity static property and doesn't have the corresponding Zero one to represent its base state when created with new().

API Proposal

namespace System.Numerics
{
     public class Quaternion
     {
        /// <summary>Gets a newly initialized quaternion.</summary>
        /// <value>A quaternion whose values are <c>(0, 0, 0, 0)</c>.</value>
        public static Quaternion Zero
        {
            get => new();
        }
     }
}

API Usage

// New Quaternion
var q = Quaternion.Zero;
q.X = 0.5;

// Checking
if (q == Quaternion.Zero)
{
  // Was uninitialized
}

Risks

No breaking changes, adds value to matching base struct types in the System.Numerics package for base initialization state.

@michael-hawker michael-hawker added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 12, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Numerics untriaged New issue has not been triaged by the area owner labels Aug 12, 2021
@ghost
Copy link

ghost commented Aug 12, 2021

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

Vector2, Vector3, and Vector4 when created with new() are all zeroed out and have corresponding .Zero static properties. Quaternion on the otherhand only has the Identity static property and doesn't have the corresponding Zero one to represent its base state when created with new().

API Proposal

namespace System.Numerics
{
     public class Quaternion
     {
        /// <summary>Gets a newly initialized quaternion.</summary>
        /// <value>A quaternion whose values are <c>(0, 0, 0, 0)</c>.</value>
        public static Quaternion Zero
        {
            get => new();
        }
     }
}

API Usage

// New Quaternion
var q = Quaternion.Zero;
q.X = 0.5;

// Checking
if (q == Quaternion.Zero)
{
  // Was uninitialized
}

Risks

No breaking changes, adds value to matching base struct types in the System.Numerics package for base initialization state.

Author: michael-hawker
Assignees: -
Labels:

api-suggestion, area-System.Numerics, untriaged

Milestone: -

@GrabYourPitchforks
Copy link
Member

Quaternion also has an IsIdentity property. If this API is approved, would IsZero come along for the ride for symmetry?

@michael-hawker
Copy link
Author

Quaternion also has an IsIdentity property. If this API is approved, would IsZero come along for the ride for symmetry?

That's a great question, would it make sense to have IsZero and IsOne type function on Vector2, Vector3, and Vector4 too for symmetry with the Quaternion function too?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 13, 2021
@hrrrrustic
Copy link
Contributor

hrrrrustic commented Aug 13, 2021

@GrabYourPitchforks @tannergooding Is there any reason why Quaternion fields are mutable?

public struct Quaternion : IEquatable<Quaternion>
{
private const float SlerpEpsilon = 1e-6f;
/// <summary>The X value of the vector component of the quaternion.</summary>
public float X;
/// <summary>The Y value of the vector component of the quaternion.</summary>
public float Y;
/// <summary>The Z value of the vector component of the quaternion.</summary>
public float Z;
/// <summary>The rotation component of the quaternion.</summary>

There are no public usages with mutations, so it's easy to make whole struct readonly, just need to rewrite code like that

Quaternion ans;
ans.X = value1.X + value2.X;
ans.Y = value1.Y + value2.Y;
ans.Z = value1.Z + value2.Z;
ans.W = value1.W + value2.W;

@GrabYourPitchforks
Copy link
Member

@hrrrrustic I suspect it's mostly that there wasn't a whole lot of consistency when these types were first introduced all those years ago. Maybe partly that the original designers believed that updating individual values would be a common operation and that apps didn't want to incur the cost of updating the entire struct? (Keep in mind - this was originally designed for a much older JIT / platform.)

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 13, 2021
@hrrrrustic
Copy link
Contributor

Well, I'd like to propose to make the whole struct readonly at least for consistency with other numbers/numerics-like types
But I'll wait for @tannergooding answer, maybe he has some strong arguments against that

@svick
Copy link
Contributor

svick commented Aug 13, 2021

@hrrrrustic That would be a breaking change, so I don't see that happening.

@tannergooding
Copy link
Member

Yep, its a breaking change since the fields are public and so we cannot make the fix.

Ideally these would be readonly, but since they can't be all the methods should (and I believe already have been updated to be) readonly instead.

@tannergooding
Copy link
Member

I think exposing Zero makes sense here. I'm not convinced IsZero is worthwhile to expose, developers can simply use value == Quaternion.Zero, which will be more efficient.

@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 Aug 13, 2021
@tannergooding tannergooding added this to the 7.0.0 milestone Aug 16, 2021
@bartonjs
Copy link
Member

bartonjs commented Aug 17, 2021

Video

Looks good as proposed.

namespace System.Numerics
{
     partial struct Quaternion
     {
        /// <summary>Gets a newly initialized quaternion.</summary>
        /// <value>A quaternion whose values are <c>(0, 0, 0, 0)</c>.</value>
        public static Quaternion Zero
        {
            get => new();
        }
     }
}

@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 Aug 17, 2021
@tannergooding tannergooding added the help wanted [up-for-grabs] Good issue for external contributors label Aug 17, 2021
@tannergooding
Copy link
Member

Marked this as up-for-grabs, but note that properly implementing this may require JIT side work to ensure the codegen is optimal like for the Vector4 scenario.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 17, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2021
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
6 participants