Clarify bounce and reflect docs and update param names

[clayjohn]

Supersedes: #68392
Fixes: #11678

A large part of the confusion comes from the parameter naming. reflect says it takes a normal vector, but it actually takes a vector that is parallel to the line/plane which is an uncommon way of specifying a plane.

I am fairly certain that changing parameter names doesn't break compat, but I would appreciate confirmation from @dsnopek and @raulsntos just in case.

[Calinou]

Should be good to merge after applying suggestions.

To further clarify the difference between bouncing and reflecting, I'll look into creating comparison diagrams we can commit to the godot-docs repository and link here in a future PR.

[raulsntos]

I am fairly certain that changing parameter names doesn't break compat

In C# it breaks source compat¹ because a user could be calling the method with named parameters like this:

new Vector2(100, 100).Reflect(normal: new Vector2(0, -1));

But also, the Variant types are implemented manually in C#, so this PR doesn't affect them.

I don't think we need to be strict on source compat though, it's not as disruptive as binary compat, so if the change is justified I think it's fine.

Footnotes

1. In case you're not familiar with .NET compatibility terms: https://learn.microsoft.com/en-us/dotnet/core/compatibility/categories ↩

[akien-mga]

Thanks!

[aaronfranke]

This doesn't make sense to me. You cannot uniquely define a plane using only a single parallel direction.

The C++ source code still calls the parameter normal:

Vector3 Vector3::reflect(const Vector3 &p_normal) const {

Same thing in 2D. I think this PR should be reverted, because it is not consistent with the C++ code, and for 3D it does not make any sense to define a plane by a parallel direction.

[mhilbrunner]

@clayjohn ^

[hallo1126]

As I mentioned in #89250, this PR should not be reverted. Instead, if the C++ code has the same inconsistency with all other standards like GLSL, it should be corrected as well instead of clinging to an old confusing and honestly simply needlessly nonstandard implementation. I propose that bounce(), on the next major change, absolutely should be switched with reflect(), and until then the documentation should be updated to explain the difference to avoid confusion, both in C# as well as in C++

[clayjohn]

@aaronfranke Whether it makes sense to define a plane given by a direction or not is kind of besides the point. That's how our reflect() method works. Calling the parallel direction a "normal" doesn't make it into a normal vector. Nor does it make this function API make sense. The least we can do for users is accurately describe what happens when they call this function.

You cannot uniquely define a plane using only a single parallel direction.

Why would we need to uniquely define a plane? Any plane aligned with this direction will result in the same reflected vector.

Same thing in 2D. I think this PR should be reverted, because it is not consistent with the C++ code, and for 3D it does not make any sense to define a plane by a parallel direction.

The description is consistent with the C++ code, maybe not with the parameter name, but again. The issue here comes from the parameter names being outright wrong and misleading. I am not going to support incorrect documentation just to be internally consistent. If you really care about the C++ parameter names, then please feel free to make a PR that updates the parameter names in the C++ code to something more accurate. Making the documentation inaccurate is not an option.

[aaronfranke]

@clayjohn There are an infinite amount of planes that pass through this parallel direction X axis, how does it determine which plane to use?

This PR does not fix the problem. The problem is that the bounce/reflect names are swapped from what users expect. It also introduces a new problem: not making any sense.

Any plane aligned with this direction will result in the same reflected vector.

That doesn't make sense to me. Reflecting or bouncing over the planes in the GIF will produce different results.

The description is consistent with the C++ code, maybe not with the parameter name, but again.

Yes, that's what I mean. The parameter names are not consistent.

I am not going to support incorrect documentation just to be internally consistent.

But now it's neither correct nor consistent.

[clayjohn]

@clayjohn There are an infinite amount of planes that pass through this parallel direction X axis, how does it determine which plane to use?

You are right. I was oversimplifying above and can see that now.

When visualizing the direction as a vector across a given plane (with a specific normal) it makes sense. But indeed, for any direction there are an infinite number of possible planes, so you would need the specific normal to know which plane is being used.

For 2D, it works because a line can be defined by a given direction, but that doesn't extend to 3D.

I'll make a PR with a better description!