Vector3.Reflect inconsistent with GLSL

[hallo1126]

Tested versions

v4.2.1.stable.mono.official [b09f793f5]

System information

Godot v4.2.1.stable.mono - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 Ti (NVIDIA; 31.0.15.3640) - AMD Ryzen 7 5800X3D 8-Core Processor (16 Threads)

Issue description

The C# implementation of Vector3 contains the following code, which produces the inverse of the expected result:

https://github.com/godotengine/godot/blob/aef11a14274f6f9e74ad91ead1d7c07ea1dd7f5f/modules/mono/glue/GodotSharp/GodotSharp/Core/Vector3.cs#L534

public readonly Vector3 Reflect(Vector3 normal) {
            return 2f * Dot(normal) * normal - this;
}

Expected implementation for parity with GLSL reflect(vec3, vec3) and other industry implementations:

public readonly Vector3 Reflect(Vector3 normal) {
            return this - 2f * Dot(normal) * normal;
}

Note: There is already a correct implementation with Vector3.Bounce, however this naming inconsistency is confusing and can easily cause errors:

https://github.com/godotengine/godot/blob/aef11a14274f6f9e74ad91ead1d7c07ea1dd7f5f/modules/mono/glue/GodotSharp/GodotSharp/Core/Vector3.cs#L148

Steps to reproduce

Call Vector3.Reflect(Vector3 normal)

Minimal reproduction project (MRP)

Self-explanatory

[AThousandShips]

This can't be changed, that would break compatibility, but the documentation could be clarified with references

[hallo1126]

That is unfortunate but understandable. Yeah, documention plus in-IDE with a tooltip like:

"Note: Not to be confused with Vector3.Bounce, which has parity to GLSL"

should be fine

[clayjohn]

This is a duplicate of https://github.com/godotengine/godot/issues/11678 and would be resolved by https://github.com/godotengine/godot/pull/68392

The whole reflect/bounce issue is a longstanding issue with Godot and unfortunately, we didn't remember to fix it when moving to Godot 4.0. bounce() is the operation that most people would call reflect() and reflect() is a different type of reflection. It is super confusing

[hallo1126]

This is a duplicate of #11678 and would be resolved by #68392

The whole reflect/bounce issue is a longstanding issue with Godot and unfortunately, we didn't remember to fix it when moving to Godot 4.0. bounce() is the operation that most people would call reflect() and reflect() is a different type of reflection. It is super confusing

Does this mean we could expect a fix with Godot 5.0? I really hope so.

If yes, I would suggest adding one of these issues to the milestone and closing the others. Thank you very much.

[clayjohn]

This is a duplicate of #11678 and would be resolved by #68392 The whole reflect/bounce issue is a longstanding issue with Godot and unfortunately, we didn't remember to fix it when moving to Godot 4.0. bounce() is the operation that most people would call reflect() and reflect() is a different type of reflection. It is super confusing

Does this mean we could expect a fix with Godot 5.0? I really hope so.

If yes, I would suggest adding one of these issues to the milestone and closing the others. Thank you very much.

I hope we remember for 5.0 as well. But it will be many years from now and we will have built up a long list of renames

For now, I have made https://github.com/godotengine/godot/pull/89404 which supersedes https://github.com/godotengine/godot/pull/68392 and renames the parameter for good measure.