Skip to content

[WIP] [2.0] A maths library for Silk.NET #190

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

Closed
wants to merge 19 commits into from
Closed

[WIP] [2.0] A maths library for Silk.NET #190

wants to merge 19 commits into from

Conversation

Perksey
Copy link
Member

@Perksey Perksey commented Jun 15, 2020

HELP WANTED!

Help us implement the maths library (intrinsics not required, can be done at a later date)

  • Half - sourced from corefxlab under the MIT license
  • Box2 - @HurricanKai
  • Box3 - @HurricanKai
  • Vector2 - @HurricanKai
  • Vector3 - @HurricanKai
  • Vector4 - @HurricanKai
  • Matrix2X2 - unclaimed
  • Matrix2X3 - unclaimed
  • Matrix2X4 - unclaimed
  • Matrix3X2 - unclaimed
  • Matrix3X3 - unclaimed
  • Matrix3X4 - unclaimed
  • Matrix4X2 - unclaimed
  • Matrix4X3 - unclaimed
  • Matrix4X4 - @HurricanKai

Nice-to-haves

  • Brush up & expose Scalar
  • Plane - a plane in 3D space.
  • Curve3 - a quadratic bezier curve
  • Curve4 - a cubic bezier curve

Information about generic implementations

    /* Note: The following patterns are used throughout the code here and are described here
    *
    * PATTERN:
    *    if (typeof(T) == typeof(int)) { ... }
    *    else if (typeof(T) == typeof(float)) { ... }
    * EXPLANATION:
    *    At runtime, each instantiation of Vector<T> will be type-specific, and each of these typeof blocks will be eliminated,
    *    as typeof(T) is a (JIT) compile-time constant for each instantiation. This design was chosen to eliminate any overhead from
    *    delegates and other patterns.
    */
public static Vector2<T> Add(Vector2<T> left, Vector2<T> right)
{
    if (typeof(T) == typeof(float))
    {
        var leftF = (Vector2<float>)(object)left;
        var rightF = (Vector2<float>)(object)right;
        return (Vector2<T>)(object)new Vector2<float>(leftF.X + rightF.X, leftF.Y + rightF.Y);
    }
    else if (typeof(T) == typeof(double))
    {
        // etc
    }
}

Summary of the PR

Adds a maths library that patches in the holes left by System.Numerics.

What version does this PR target?

2.0

Related issues, Discord discussions, or proposals

Links go here.

Further Comments

@Perksey Perksey added help wanted Extra attention is needed and removed size/XXL labels Jun 15, 2020
@Perksey Perksey added this to the 2.0 milestone Jun 15, 2020
@john-h-k
Copy link

aren't some of these types a bit redundant given SysNumerics (and mathsharp)

1 similar comment
@john-h-k

This comment has been minimized.

@HurricanKai
Copy link
Member

should implementations forward to .net (5) implementations as much as possible or re-implement everything?

@HurricanKai
Copy link
Member

also, will this be available to other projects separately?
If so, I'd be happy to implement Vector2/3/4 Half/Int/Long/float/double
should be mostly copy-paste

@Perksey
Copy link
Member Author

Perksey commented Jun 15, 2020

@HurricanKai We can forward the float implementations to System.Numerics using Unsafe where applicable they'll likely be faster. Given that this is primarily targeting .NET Standard 2.0 (apart from hardware acceleration which will be done via multi-targeting), there's not much else we can do forwarding-wise.

@HurricanKai (2) Yep, it won't even depend on Silk.NET.Core. It will be entirely standalone.

@john-h-k There's no way I'll get the APIs I want in System.Numerics within this decade due to API review, so I have to do it here. As for MathSharp, well I mean MathSharp's just an intrinsics library right (albeit providing some minimal storage types)

@HurricanKai

This comment has been minimized.

@Perksey

This comment has been minimized.

@HurricanKai

This comment has been minimized.

@Perksey

This comment has been minimized.

@HurricanKai
Copy link
Member

@Perksey In line with what dotnet/runtime does, and also because I'd think performance wise it makes more sense, I'd suggest things like One/Zero/UnitX etc. be (static) properties

@Perksey
Copy link
Member Author

Perksey commented Jun 15, 2020

Yeah that'd make more sense, fine by me 👍

@HurricanKai
Copy link
Member

What is private static readonly string ListSeparator; for?
Seems like that should just be NumberFormatInfo.GetInstance(formatProvider).NumberGroupSeparator;

@Perksey
Copy link
Member Author

Perksey commented Jun 15, 2020

Yeah that's what it should be, didn't take into account NumberFormatInfo when originally speccing the API

@HurricanKai
Copy link
Member

HurricanKai commented Jun 15, 2020

What's LengthFast?
There seem to be a lot of Fast methods? why would anyone not choose those?

@Perksey
Copy link
Member Author

Perksey commented Jun 15, 2020

Ah you can ignore that, Length was gonna be Math.Sqrt((X * X) + (Y * Y)) whereas LengthFast was gonna be 1.0 / InverseSqrtFast((X * X) + (Y * Y) + (Z * Z) + (W * W)) where InverseSqrtFast is:

var xhalf = 0.5f * x;
var i = *(int*)&x; // Read bits as integer.
i = 0x5f375a86 - (i >> 1); // Make an initial guess for Newton-Raphson approximation
x = *(float*)&i; // Convert bits back to float
x = x * (1.5f - (xhalf * x * x)); // Perform left single Newton-Raphson step.
return x;

But in this day and age LengthFast is probably slower so you should just remove LengthFast, as well as most other *Fast methods as the implementation you come up with is likely as fast as it can be, and adding "fast" overloads will probably just end up confusing users.

Sorry about all this, I had pulled up on the other screen the API spec for another maths library and wrote the overloads without thinking.

@john-h-k
Copy link

where InverseSqrtFast is:

or just use Sse.ReciprocalSqrt 😄 which is an approximation and super super fast (way faster than 1 / qsrt)

@Perksey
Copy link
Member Author

Perksey commented Jun 15, 2020 via email

@HurricanKai
Copy link
Member

what is PerpDot for? very ambiguous, and if I understand what it means, is easily doable using PerpendicularLeft + Dot

@Perksey
Copy link
Member Author

Perksey commented Jun 15, 2020

The perpendicular dot product is (left.X * right.Y) - (left.Y * right.X);

@HurricanKai
Copy link
Member

What is BaryCentric supposed to do, if I understand wikipedia correctly, I can't implement it without assuming in what kind of coordinate system X & Y are. For all I know X & Y could already be Barycentric coordinates...

@Perksey
Copy link
Member Author

Perksey commented Jun 15, 2020

Ah it interpolates 3 Vectors using Barycentric coordinates (so it returns a when u=0v=0, b when u=1,v=0, c when u=0,v=1, and a linear combination of a,b,c otherwise).

We should probably move these questions to Discord and the GitHub thread is getting pretty full

* Add Scalar<T>.Two

* Add Scalar<T>.Smaller

* Add Scalar<T> SmallerEquals & LargerEquals

* Implement Box2<T>

* Make Min/Max public

* Missed Equals

* Box3<T>

* Make Box2/3 readonly

* Add With methods

* Rename past tense Methods
* Make Vecttor2<T> readonly

* Make Vector3<T> readonly

* Make Vector4<T> readonly

* Rename past tense Methods
@HurricanKai
Copy link
Member

Will begin with Matrix4x4 :)

@Perksey
Copy link
Member Author

Perksey commented Jun 18, 2020

Awesome :D

* Nullability

* CLS compliance

* Rename CLSCompliant.cs to AssemblyInfo.cs

* <Nullable>enable</Nullable>
@HurricanKai
Copy link
Member

HurricanKai commented Jun 18, 2020

All Scalar methods with 9+ block need to be split up into multiple < 9 block functions.
See dotnet/runtime#38106

@Perksey
Copy link
Member Author

Perksey commented Jun 18, 2020

Damn that is really really unfortunate, but necessary if it poses a nuisance. I'm fine with that being done, albeit a little sad.

@HurricanKai
Copy link
Member

For reference, One, Tau etc. don't seem to be affected.
Methods like Multiply seem to be affected:
Normal Multiply:

G_M60209_IG01:
       4883EC28             sub      rsp, 40
       C5F877               vzeroupper 

G_M60209_IG02:
       C5FA100519000000     vmovss   xmm0, dword ptr [reloc @RWD00]
       C5FA100D15000000     vmovss   xmm1, dword ptr [reloc @RWD04]
       E894F7FFFF           call     Silk.NET.Maths.Scalar`1[Single][System.Single]:Multiply(float,float):float
       90                   nop      
G_M60209_IG03:
       4883C428             add      rsp, 40
       C3                   ret      
RWD00  dd    3FC90FDBh
RWD04  dd    40000000h

Split Multiply:

G_M60209_IG01:
       C5F877               vzeroupper 
G_M60209_IG02:
       C5FA100505000000     vmovss   xmm0, dword ptr [reloc @RWD00]
G_M60209_IG03:
       C3                   ret      
RWD00  dd    40490FDBh

that's a rather big difference for a multiply

@Perksey
Copy link
Member Author

Perksey commented Jun 18, 2020

Interesting, that is a massive improvement.

@HurricanKai
Copy link
Member

currently exposing more of Math/MathF and doing all the optimizations in Scalar<T> but it seems we at least end up with a call for Math/MathF :/

* Add Basic Scalar Tests & Fix Scalar mistakes

* netcoreapp5.0 -> net5.0
@@ -248,57 +248,57 @@ public static T SquareRoot(T value)
{
if (typeof(T) == typeof(byte))
{
return (T) (object) (MathF.Sqrt((byte) (object) value));
return (T) (object) (byte) (MathF.Sqrt((byte) (object) value));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't lie about integral square root. Either this should use an integral square root path, or it should be not supported. Users will get very unexpected results doing this

@@ -1071,12 +1071,12 @@ public static T Acos(T value)

if (typeof(T) == typeof(ulong))
{
return (T) (object) (Math.Acos((ulong) (object) value));
return (T) (object) (ulong) (Math.Acos((ulong) (object) value));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC above. You shouldn't "fake" floating point ops on integral types

This was referenced Jun 30, 2020
@Perksey Perksey removed the help wanted Extra attention is needed label Jul 19, 2020
@Perksey Perksey requested a review from HurricanKai as a code owner July 20, 2020 14:00
Co-authored-by: Dylan Perks <dmp9biz@gmail.com>
@Perksey
Copy link
Member Author

Perksey commented Jul 20, 2020

@HurricanKai It's probably best we close this PR and reopen it anew seeing as you've taken over maths development - the PR is currently still in my name.

@HurricanKai
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants