Fix Zero-Vector Normalize

[rhksdn2314]

Hi, I found out that Vector2.Normalize return (Nan, Nan) with input Vector.Zero at ContactSolver.cs
Which means, if you spawn two circle object at same position, it works wrong.
With two circles, we have to choose which way these two objects move.
So i decided to use Vector(0, 1) when they have same initial position.

Thanks

[codingben]

@rhksdn2314 Thank you! let's ask also review from others. @HughPH @thomasvt @NArnott Would you like to review? 😄

[thomasvt]

Sorry, I vote against this change.

tl;dr: application problem, not an engine problem

Checking for this is a permanent burden for the engine's hot code, while it never occurs spontaneously. It only occurs when deliberately provoked by the application, so it should be the application that prevents this during scene building. In fact, the engine reacting weird, makes some sense, because placing two circles on top of each other makes no physical sense. The original Box2D doesn't check for this either, although it reacts differently because they don't revert to NaN.

regards

[rhksdn2314]

I partily agree with @thomasvt, but i think it's not that unique or only deliberately provoked situation. I found this because i tried to do that and It's likey to happen when making games. Also as you said, original Box2D works properly(at least, not unacceptable) when this situation happen, contrary to netstandard version close application or give critical impact.
/+ same situation but with two Box shape Colliders occur no error.

So I think engine should prevents this or at least validate it, and my code is one of these preventions.

Thanks,

[HughPH]

No. I added a custom Matrix rotation function to remove bounds checking present in Microsoft's code because, to @thomasvt 's point, it's very much Hot Path stuff. The behaviour is standard to System.Numerics.Vector2. OpenTK.Mathematics.Vector2.Normalize behaves the same way.

If this occurs when the circles have different collision/mask flags, and should never collide, that's a different bug.

If you want a special Box2D that allows you to put colliding circles on top of each other, you should keep your specialisations in your fork. You could also detect when you have two circles at the exact same point, and move one of them by Settings.FLT_EPSILON.

I use customised Box2D versions by including the code in my solution and then making changes that suit what I'm doing. That's the intended use of the original Box2D as well.

[rhksdn2314]

Oh, I got it! :)
I just thought that it can be matter if somthing behaves differently between Box2D original and porting version.

Thank you for comments!

[codingben]

@rhksdn2314 I'm glad you got the answers, thanks to the participants. Can we close this PR?

[rhksdn2314]

Yes, thank you!

[HughPH]

It's worth noting that Box2DSharp and the original Box2D have this check in the Normalize function, which is custom code in both cases. Unity has slightly different code, but it still checks if the distance is less than epsilon. In both C# cases, as with this project, Aggressive Inlining is used.

For C#, which utilizes SIMD extensions, placing an if statement in the middle of a set of calculations could have an interesting impact on the generated instructions (though TBH I haven't looked into this)

[HughPH]

Without the if statement, the Normalize function is 47 instructions. Adding the if statement adds 14 instructions, bringing it up to 61. This is not an exact depiction of the impact, since different instructions take different numbers of clock cycles, but it's a fair gauge generally.