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]: Add Indexer to Vectors #59924

Closed
Avid29 opened this issue Oct 4, 2021 · 9 comments · Fixed by #60517
Closed

[API Proposal]: Add Indexer to Vectors #59924

Avid29 opened this issue Oct 4, 2021 · 9 comments · Fixed by #60517
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics
Milestone

Comments

@Avid29
Copy link

Avid29 commented Oct 4, 2021

Background and motivation

Running the same operation in-terms of different vector components is a fairly common operation.

In order to make this easier, having indexers into the System.Numerics Vectors that returns the value at a position in the vector would make a ton of sense.

Example:v.X == v[0], v.Y == v[1], etc would

API Proposal

namespace System.Numerics
{
    public partial struct Vector2
    {
        public float this[int index] { get; set; }
    }

    public partial struct Vector3
    {
        public float this[int index] { get; set; }
    }
    
    public partial struct Vector4
    {
        public float this[int index] { get; set; }
    }

    public partial struct Quaternion
    {
        public float this[int index] { get; set; }
    }

    public partial struct Matrix3x2
    {
        public float this[int row, int column] { get; set; }
    }

    public partial struct Matrix4x4
    {
        public float this[int row, int column] { get; set; }
    }
}

API Usage

Vector3 v3 = Vector3.UnitY;

v3[0] == 0;
v3[1] == 1;
v3[2] == 0;

Risks

None I don't think

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

ghost commented Oct 4, 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

Running the same operation in-terms of different vector components is a fairly common operation.

In order to make this easier, having indexers into the System.Numerics Vectors that returns the value at a position in the vector would make a ton of sense.

Example:v.X == v[0], v.Y == v[1], etc would

API Proposal

namespace System.Collections.Generic
{
    public struct Vector3 : System.IEquatable<System.Numerics.Vector3>, System.IFormattable
    {
        /// 0 is X, 1 is Y, 2 is Z
        public float this[int i] { get; }
    }
}

API Usage

Vector3 v3 = Vector3.UnitY;

v3[0] == 0;
v3[1] == 1;
v3[2] == 0;

Risks

None I don't think

Author: Avid29
Assignees: -
Labels:

api-suggestion, area-System.Numerics, untriaged

Milestone: -

@Avid29
Copy link
Author

Avid29 commented Oct 4, 2021

Here's an example of when this would be useful

public struct AABB
{
    public AABB(Vector3 maximum, Vector3 minimum)
    {
        Maximum = maximum;
        Minimum = minimum;
    }

    public bool IsHit(Ray ray, float maxClip, float minClip)
    {
        // Currently have to copy all my Vector3s to float[]s
        float[] min = { Minimum.X, Minimum.Y, Minimum.Z };
        float[] max = { Maximum.X, Maximum.Y, Maximum.Z };
        float[] origin = { ray.Origin.X, ray.Origin.Y, ray.Origin.Z };
        float[] dir = { ray.Direction.X, ray.Direction.Y, ray.Direction.Z };
        
        for (int a = 0; a < 3; a++)
        {
            float invD = 1f / dir[a];
            float t0 = (min[a] - origin[a]) * invD;
            float t1 = (max[a] - origin[a]) * invD;

            if (invD < 0.0f)
            {
                float swap = t0;
                t0 = t1;
                t1 = swap;
            }

            minClip = t0 > minClip ? t0 : minClip;
            maxClip = t1 < maxClip ? t1 : maxClip;

            if (maxClip <= minClip) return false;
       }

       return true;
    }

    public Vector3 Maximum { get; }

    public Vector3 Minimum { get; }
}
    ```

@hypeartist
Copy link

hypeartist commented Oct 4, 2021

@Avid29 You can use something like this today:

public static class Extensions
{
	public static ref float ElementAt(this in Vector3 v, nint idx){
		return ref Unsafe.Add(ref Unsafe.As<Vector3, float>(ref Unsafe.AsRef(v)), idx);
	}
}

@Avid29
Copy link
Author

Avid29 commented Oct 4, 2021

@hypeartist I thought about it a little more, and now I do use that.

However, I still believe this is a reasonable addition to the API. It's a sensible interpretation of the indexer on a Vector and Unity's Vectors already have this behavior.

@tannergooding
Copy link
Member

tannergooding commented Oct 4, 2021

@Avid29, I'm fine with this suggestion. It can also be optimized for constant inputs and brings it more inline with the other vector types.

However, could you please update the proposal to follow the form of:

public partial struct Vector3
{
     public float this[int i] { get; set; }
}

and to also list the other System.Numerics.* types that should have such an indexer (Vector2, Vector4, & Quaternion at the very least; potentially Matrix3x2 and Matrix4x4 if they don't have it, etc).

Once that's done, I can mark this ready for review (and if you get to this before tomorrow, it can likely goto API review then, assuming we have nothing else on the docket).

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Oct 4, 2021
@tannergooding tannergooding added this to the 7.0.0 milestone Oct 4, 2021
@Avid29
Copy link
Author

Avid29 commented Oct 4, 2021

That all sounds good, I'll add that Matrix indexers are a duplicate to #21705

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs author feedback labels Oct 4, 2021
@tannergooding
Copy link
Member

I'll add that Matrix indexers are a duplicate to #21705

If possible, I'd recommend covering them here. It's likely better to simply have get + set due to other issues with ref returns (not just that they aren't possible on structs today, but also around perf and other considerations).

@tannergooding
Copy link
Member

That all sounds good

I've updated the original post covering the types I listed.

@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 needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Oct 5, 2021
@bartonjs
Copy link
Member

bartonjs commented Oct 5, 2021

Video

Looks good as proposed. We went ahead and added read-only indexers on the other Vector types

namespace System.Numerics
{
    public partial struct Vector2
    {
        public float this[int index] { get; set; }
    }

    public partial struct Vector3
    {
        public float this[int index] { get; set; }
    }
    
    public partial struct Vector4
    {
        public float this[int index] { get; set; }
    }

    public partial struct Quaternion
    {
        public float this[int index] { get; set; }
    }

    public partial struct Matrix3x2
    {
        public float this[int row, int column] { get; set; }
    }

    public partial struct Matrix4x4
    {
        public float this[int row, int column] { get; set; }
    }
}

namespace System.Runtime.Intrinsics
{
    public partial struct Vector64<T>
    {
        public T this[int index] { get; }
    }

    public partial struct Vector128<T>
    {
        public T this[int index] { get; }
    }

    public partial struct Vector256<T>
    {
        public T this[int index] { get; }
    }
}

@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 Oct 5, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 17, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 22, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Nov 22, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants