stride icon indicating copy to clipboard operation
stride copied to clipboard

Deprecate stride math for numerics alternatives

Open Doprez opened this issue 1 year ago • 5 comments

PR Details

Doing some benchmarks and discussing with @Eideren and @ericwj there are some major speed benefits using System.Numerics instead of Stride math.

This PR is meant to deprecate the math functions that are available with the System.Numerics equivalents.

Related Issue

https://github.com/stride3d/stride/pull/2238 https://github.com/stride3d/stride/issues/2576

Types of changes

  • [ ] Docs change / refactoring / dependency upgrade
  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [ ] My change requires a change to the documentation.
  • [ ] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.
  • [ ] I have built and run the editor to try this change out.

IMPORTANT

https://github.com/stride3d/stride/pull/2238 must be merged first for the implicit conversions

Doprez avatar Dec 25 '24 19:12 Doprez

first hurdle: Invert seems to give NaN values where Stride did not. It seems like the

Assert.Equal() Failure: Values differ
Expected: [M11:0 M12:0 M13:0 M14:0] [M21:0 M22:0 M23:0 M24:0] [M31:0 M32:0 M33:0 M34:0] [M41:0 M42:0 M43:0 M44:0]
Actual:   [M11:NaN M12:NaN M13:NaN M14:NaN] [M21:NaN M22:NaN M23:NaN M24:NaN] [M31:NaN M32:NaN M33:NaN M34:NaN] [M41:NaN M42:NaN M43:NaN M44:NaN]

same with Orthonomolize

Assert.Equal() Failure: Values differ
Expected: [M11:0.18257418 M12:0.36514837 M13:0.5477226 M14:0.73029673] [M21:0.8164966 M22:0.40824836 M23:-1.4600097E-07 M24:-0.40824822] [M31:0 M32:0 M33:9.536743E-07 M34:0] [M41:-0.4714045 M42:-0.4714045 M43:0.70710677 M44:-0.23570225]
Actual:   [M11:0.18257418 M12:0.36514837 M13:0.5477225 M14:0.73029673] [M21:0.8164966 M22:0.40824836 M23:1.4600097E-07 M24:-0.40824822] [M31:NaN M32:NaN M33:NaN M34:NaN] [M41:NaN M42:NaN M43:NaN M44:NaN]

So it seems like 0 values in System.Numerics arent handled in these methods?

Doprez avatar Dec 25 '24 19:12 Doprez

Beginning benchmarks mostly for fun for now since Invert is broken: image

Doprez avatar Dec 25 '24 19:12 Doprez

My misplaced comment from the now merged PR #2238, slightly clarified.

I wouldn't add any surface area to Stride's math in terms of parameters it accepts. It accepts Stride types and returns Stride types now. Whatever how it is implemented is a private detail and that can use System.Numerics types. In addition these conversion methods what #2238 is about make it a lot easier to mix-and-match, so let people use them. The exception is Matrix, since the conversion involves a transpose and hence is not free.

Don't deprecate math that is perfectly fine and which can be optimized by changing private implementation. Deprecate performance smells such as by-ref and in-place modification (i.e. void Transpose() and void Add(ref ro, ref ro, out), Subtract(in, in) etc).

  • This will eventually reduce the surface area and align what remains with System.Numerics for as far as the features exist. Whether you use the one or the other, hence when/if they are deprecated, is moot.
  • The discussion about removing Stride's math in favor of some third party library should be about what has no equivalent in System.Numerics and the occasional feature that with differences in behavior, such as your first comment above.

One goal is to get Stride's math DRY. It is so freaking silly to have a handful of implementations for one operation! So if any Stride math is touched, for the purpose of deprecations or otherwise, have them all delegate to the most performant implementation. This will be the by-value one, which would work for readonly struct. And let the JIT handle the ref in ref ro dilemmas - it should often be able to inline and hide the smell when these methods are used in larger blocks in user code.

ericwj avatar Dec 28 '24 19:12 ericwj

The tests TestMathUtil_Invert et TestMathUtil_Orthonomolize are failing.

Kryptos-FR avatar Feb 22 '25 18:02 Kryptos-FR

The issue (identified by the failing tests) is that our version of those types added checks that are relevant to a game engine but not in the general case.

Namely, our implementation of Stride.Core.MathematicsVector4.Normalize() checks whether the vector is almost zero, and does nothing in that case:

public void Normalize()
{
    float length = Length();
    if (length > MathUtil.ZeroTolerance)
    {
        float inverse = 1.0f / length;
        X *= inverse;
        Y *= inverse;
        Z *= inverse;
        W *= inverse;
    }
}

While System.Numerics.Vector4.Normalize() just applies the numeric formula:

public static Vector4 Normalize(Vector4 vector) => vector / vector.Length();

which makes sense since this can be SIMD'd.

We might need to fix all usage of Stride.Core.MathematicsVector4.Normalize() to first check for zero vectors, or add a helper class and redirect code to that class even when Numerics will be adopted.

Note: there might be other APIs with the same issue.

Kryptos-FR avatar Feb 23 '25 13:02 Kryptos-FR