bepu / bepuphysics2

Pure C# 3D real time physics simulation library, now with a higher version number.
Apache License 2.0
2.4k stars 274 forks source link

Use System.Numerics.Quaternion instead of BepuUtilities.Quaternion #85

Closed Aminator closed 4 years ago

Aminator commented 4 years ago

I'm trying to integrate BepuPhysics into my game engine, but I found it very frustrating needing to constantly convert between System.Numerics.Quaternion and BepuUtilities.Quaternion since I always use the former for transforms in my engine and various other places. It doesn't even offer a cast.

Now I understand that BepuUtilities.Quaternion offers more functionality, but since that is only provided through static methods, it would be better to put those that are missing from System.Numerics.Quaternion in a separate extension class with a different name that takes System.Numerics.Quaternion as inputs/outputs. This would also remove the need to declare the right Quaternion in a using declaration.

(Same goes for BepuUtilities.Matrix and BepuUtilities.Plane.)

vpenades commented 4 years ago

An alternative solution would be to include implicit operator conversions between both types in bepuphysics quaternion

public static implicit operator System.Numerics.Quaternion (BepuUtilities.Quaternion src) { ... }
public static implicit operator BepuUtilities.Quaternion (System.Numerics.Quaternion src) { ... }

That way, both types can be seamlessly exchangeable.

Aminator commented 4 years ago

Yes, that works, but it's not ideal. That still leaves the issue of name clashes and passing by reference doesn't work well here. System.Numerics.Quaternion offers almost all of the functionality, so it's better to be able to share the same type across .NET libraries and extending it with extra methods in an extension class.

vpenades commented 4 years ago

Yes, I agree the library must use system.numerics types whenever possible, and at the very least, all the public APIs should use common types for better interoperability.

So maybe bepuphysics could make bepuutilities.quaternion internal, instead of public until it can be fully removed.

RossNordby commented 4 years ago

It's on the chopping block. Just a question of when I get around to verifying codegen for the System.Numerics.Quaternion (the reason the BepuUtilities version exists is mostly the JIT emitting a bunch of extra copies for the numerics version). There will still need to be some kind of BepuUtilities helper to cover some of the special cases, but that doesn't need to cause name collisions.

RossNordby commented 4 years ago

Notably, they're both 16 byte structs with the same layout, so if you really needed to minimize interop overhead, you could use Unsafe.As.

Aminator commented 4 years ago

Ok, that sounds good, hopefully you will get around to it soon. I will use Unsafe.As for now.

RossNordby commented 4 years ago

Unfortunately, it looks like the JIT still can't quite handle the pass-by-value on non-intrinsic numerics types efficiently. I can't directly use them within the engine.

That leaves some mildly gross options. I suspect a QuaternionEx that does everything that the current BepuUtilities.Quaternion does (but with System.Numerics.Quaternion parameters) would work (unless the JIT treats them as partially intrinsic, which could cause problems- but I doubt that). I'll look into that.

I'm tempted to leave Matrix as-is since it does not directly conflict with the System.Numerics.Matrix4x4 type, it's harder to do the QuaternionEx style trick, and the numerics type has some really nasty stack shuffling issues.

BepuUtilities.Plane, at least, is an easy choice to remove. It's not used in the engine at all and does very little.

RossNordby commented 4 years ago

QuaternionEx, operating on System.Numerics.Quaternion, replaces Quaternion in 50f86577e666593156e9ec0adfdd0ba2e25140fd.