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 Vector APIs for cross product for 2D/4D #41671

Open
wants to merge 3 commits into
base: master
from

Conversation

@micampbell
Copy link

commented Oct 9, 2019

Here is the pull request with only the addition of the two new methods as discussed.

Fixes #35434

@tannergooding

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Thanks @micampbell.

Could you close the other pull request and update the title of this one to reflect the change being made (just using the same title as the PR should be fine).

/// <param name="value1">The first vector.</param>
/// <param name="value2">The second vector.</param>
/// <returns>The value of the z-coordinate from the cross product.</returns>
[JitIntrinsic]

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 9, 2019

Member

This would need to be [Intrinsic]. It may be better to not mark it as such until after we can make it actually intrinsic in the JIT though.

If #36379 goes through API review and is approved, we can add the support in managed code instead of adding new intrinsic support for this to the JIT, which may be more maintainable.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

It would be good to add tests for the new methods.

@stephentoub

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

This is adding new API. Is there an api-approved issue this is fixing? If yes, please update the PR/commit description with that, as well as update the PR/commit title to be more descriptive. If no, our process is that PRs aren't opened for new APIs until the API shape is approved.

vector1.Z * vector2.X - vector1.X * vector2.Z,
vector1.X * vector2.Y - vector1.Y * vector2.X,
vector1.W * vector2.W);
}

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 9, 2019

Member

In addition to tests, these would also need to be added to the relevant .cs file under \ref. Otherwise, they won't be exposed.

@Gnbrkm41

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2019

Related: #35434

@micampbell micampbell changed the title fix to previous pull request added cross product for 2D and 4D following discussion of Issue #35434 (https://github.com/dotnet/corefx/issues/35434)) Oct 9, 2019
@micampbell micampbell changed the title added cross product for 2D and 4D following discussion of Issue #35434 (https://github.com/dotnet/corefx/issues/35434)) added cross product for 2D and 4D following discussion of Issue #35434 (https://github.com/dotnet/corefx/issues/35434) Oct 9, 2019
@aaronfranke

This comment has been minimized.

Copy link

commented Oct 10, 2019

Why are these static methods and not instance methods? IMO it's better to have a.Cross(b) instead of Vector2.Cross(a, b), the former is shorter and don't involve repeating the type name.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

Static methods were what were reviewed and approved in #35434. We haven't typically exposed "operations" as instance methods on other .NET types. They are typically static methods that you explicitly call.

@micampbell

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

Why are these static methods and not instance methods?

I was thinking the same. Long ago, I made a similar project called StarMath (https://github.com/DesignEngrLab/StarMath) that has all static functions. In there, I added the this-extension throughout, so that one can write either a.Cross(b) or Vector2.Cross(a, b). The function is defined as

        public static double[] Cross(this IList<double> A, IList<double> B)
        {...}

(note the this before the first argument)

@micampbell

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

I'm sorry, but I can't get the test project to compile. I'm looking all over for the issue but not finding help. (I get an error on )

at any rate, here are some test functions. You can put the first in Vector2Tests.cs and the other in Vector4Tests.cs

        // A test for Cross (Vector2f, Vector2f)
        [Fact]
        public void Vector2CrossTest()
        {
            Vector3 a3 = new Vector3(1.0f, 2.0f, 0.0f);
            Vector3 b3 = new Vector3(3.0f, 4.0f, 0.0f);


            Vector2 a2 = new Vector2(1.0f, 2.0f);
            Vector2 b2 = new Vector2(3.0f, 4.0f);



            float expectedZ = Vector3.Cross(a3, b3).Z;
            float actual;

            actual = Vector2.Cross(a2, b2);
            Assert.True(MathHelper.Equal(expectedZ, actual), "Vector2f.Cross did not return the expected value.");
        }
        // A test for Cross (Vector2f, Vector2f)
        [Fact]
        public void Vector4CrossTest()
        {
            Vector3 a3 = new Vector3(1.0f, 2.0f, 3.0f);
            Vector3 b3 = new Vector3(4.0f, 5.0f, 6.0f);


            Vector4 a4 = new Vector4(4.0f, 8.0f, 12.0f, 4.0f);
            Vector4 b4 = new Vector4(20.0f, 25.0f, 30f, 5.0f);



            var expected = Vector3.Cross(a3, b3);
            var actualVect4= Vector4.Cross(a4, b4);
            var actual = new Vector3(actualVect4.X / actualVect4.W, actualVect4.Y / actualVect4.W, actualVect4.Z / actualVect4.W);

            Assert.True(MathHelper.Equal(expected, actual), "Vector4f.Cross did not return the expected value.");
        }
@micampbell micampbell closed this Oct 10, 2019
@micampbell micampbell reopened this Oct 10, 2019
@Gnbrkm41

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2019

is JitIntrinsicAttribute a thing in .NET Core? I can't seem to find that attribute on source.dot.net, seems it is present in .NET Framework though.

as @tannergooding mentioned in https://github.com/dotnet/corefx/pull/41671/files#r333005118, I think it's better not to bother marking it as intrinsics though.

@stephentoub

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

but I can't get the test project to compile

That's because you haven't updated the ref file, per my comment.

@stephentoub

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

@tannergooding, what api-approved issue is this for?

@Gnbrkm41

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2019

@stephentoub, it's for #35434.

@stephentoub

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

Great, thanks.

@stephentoub stephentoub changed the title added cross product for 2D and 4D following discussion of Issue #35434 (https://github.com/dotnet/corefx/issues/35434) Add Vector APIs for cross product for 2D/4D Oct 10, 2019
@Gnbrkm41

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2019

So, to enable this change, you will have to:

  • Edit the source codes in src/System.Numerics.Vectors/src which will be the implementation
    • Already done, but you probably will have to get rid of JitIntrinsics attribute
  • Edit the ref file under src/System.Numerics.Vectors to add the newly added methods (e.g. public static System.Numerics.Vector4 Cross(System.Numerics.Vector4 vector1, System.Numerics.Vector4 vector2) { throw null; }), in order to make them visible to the public surface
  • Edit the test codes in src/System.Numerics.Vectors/tests to add the corresponding tests for those methods
@micampbell

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

That's because you haven't updated the ref file, per my comment.

actually, no. My error is "NETSDK1013 The TargetFramework value '' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly. System.Numerics.Vectors (src\System.Numerics.Vectors) C:\Program Files\dotnet\sdk\3.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets 93"

@stephentoub

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

Have you built the repo from the root with build.cmd?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.