Mono C# Quat works differently from GD to C#.

[ricardoalcantara]

Godot version:

Godot Engine v3.2.3.stable.mono.official

OS/device including version:

Windows, GLES3

Issue description:

I have migrated the TPS Demo to C#, besides the fact that RotationQuat is internal and I opened a issue already for that #42239 . I tried to copy the method from godot, which is a simple one, but it crashes with the Exception:

E 0:00:12.743   Godot.Quat Godot.Quat.Slerp(Godot.Quat , Single ): System.InvalidOperationException: Quat is not normalized
  <C++ Error>   Unhandled exception
  <C++ Source>  /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Quat.cs:236 @ Godot.Quat Godot.Quat.Slerp(Godot.Quat , Single )()
  <Stack Trace> Quat.cs:236 @ Godot.Quat Godot.Quat.Slerp(Godot.Quat , Single )()
                PlayerEntity.cs:232 @ void GodotThirdPersonShooterDemoWithCSharp.Player.PlayerEntity._PhysicsProcess(Single )()

There is also another issue, when the level loads the player is not on screen:

If I normalize the Quat()

It runs but the Slerp doesn't look right
The aimed rotation does't look right, I checked many times and I couldn't find any difference or mistake.

Steps to reproduce:
Just run the game from the mono_csharp branch
https://github.com/ricardoalcantara/godot-tps-demo-with-csharp/

Minimal reproduction project:

[aaronfranke]

I think the problem is that the C# code for Basis.Quat() is completely different from and not at all similar to core's Basis::get_quat() code. The C# code was added by @tagcup in #11899. I don't know what the purpose was with using a different implementation for C#, but I don't think it makes sense to have two different implementations when we can instead have consistency. I haven't actually tried making any changes yet, but I have a feeling that the bug would be gone if we simply ported this C++ code to C#. @ricardoalcantara You're welcome to try if you'd like, would (probably) be a good first PR.

[ricardoalcantara]

@aaronfranke I think the same about the different implementation, before I create this issue I took a look at those implementations and I noticed the difference. I will go back and try to make that port!

[ricardoalcantara]

@aaronfranke After a lot of research, digging and energy drinks, I found out that the Access Modifiers what the only issue, but why the other issues?
The first main issue of my parse is that in GDScript when calling Transform() it generate an Identity Transform(1, 0, 0, 0 ,1, 0, 0, 0, 1, 0, 0, 0) but in C# new Transform() generate a all zeroed Transform(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0).

I fixed that by changing to Transform.Identity

Later because in C# I have to always get the Object, change the value and set it again, I made a mistake that was very hard to catch by eyes.

Wrong

var rotation = _cameraBase.Rotation;
rotation.x = _cameraXRot;
_cameraRot.Rotation = rotation;

Right

var rotation = _cameraRot.Rotation;
rotation.x = _cameraXRot;
_cameraRot.Rotation = rotation;

So, in the end it wasn't that GOOD first PR but having everything working is fine too.

[aaronfranke]

The first main issue of my parse [...]

I fixed that bug in the second commit of #34668 but it hasn't been merged yet :/

I'm still concerned that the core and C# code for Basis.Quat() is completely different, it should probably be converged eventually, but if both work then I guess it can wait until we have test cases to ensure we don't break anything when changing it (and also if they indeed behave the same it would be good to figure out which one is faster). Not sure if it would be better to keep this open to track that, or I guess we can just close this.

[ricardoalcantara]

I fixed that bug in the second commit of #34668 but it hasn't been merged yet :/

No problem, I have learned a lot!

About the difference, I tried to parse the C++ version to CSharp but since it's working and I am not very experienced on those complexes math equations I choose to leave it untouched.

[ricardoalcantara]

@aaronfranke I may have misunderstood what bug you fixed, at first I thought you have fixed the internal to public, but after your reply and getting a bit further in your PR I am thinking that you meant the new Transform() (and other things) that got fixed.