Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify the difference between Vector2/3's reflect() and bounce() #68392

Closed

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Nov 8, 2022

master version of #68393.

This closes #11678.

mhilbrunner
mhilbrunner previously approved these changes Nov 9, 2022
@mhilbrunner
Copy link
Member

I wonder if it makes sense to have examples.

Besides that, this definitely makes sense as a fix for now, although I can see the arguments for renaming these if most other engines indeed use the term differently, even if we're getting tired of renames at this point.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this definitely needs examples, I had to write short examples myself to confirm what the descriptions mean. I think the descriptions are accurate, but read in isolation I don't think they will be enough.

var direction = Vector2(1, -1)
print(direction.reflect(Vector2(0, 1))) # prints -1, -1
print(direction.bounce(Vector2(0, 1))) # prints 1, 1

We also really need to change the param "n" for reflect, because it is not a normal (which is usually what "n" means).

@@ -311,6 +312,7 @@
<param index="0" name="n" type="Vector2" />
<description>
Returns the vector reflected (i.e. mirrored, or symmetric) over a line defined by the given direction vector [param n].
[b]Note:[/b] [method reflect] performs [i]mathematical[/i] reflection, as opposed to [method bounce] which performs [i]physical[/i] reflection. Double-check which method you actually need to use, as other engines may use these terms differently.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performs [i]mathematical[/i] reflection

I'm not sure that this is properly called "mathematical" reflection. From what I can see, "n" is always the normal vector (i.e. pointing perpendicular from the surface). While in this case "n" is not the normal, but the plane itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more insight, the wikipedia page for "reflection (mathematics)" gives a definition for reflection the same as what we use for reflection, but calls the parameter "line" instead of "normal". (https://en.wikipedia.org/wiki/Reflection_(mathematics))

Bounce on the other hand, is a mathematical reflection over a normal. It is also equivalent to -vec.reflect()

Instead of calling them "mathematical reflection" and "physical reflection" I would highlight that you are reflecting over a normal vector perpendicular to the plane, or a vector describing the plane itself. So you are either "bouncing off a surface with normal n" or you are "reflecting across the surface n"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are either "bouncing off a surface with normal n"

Isn't that what bounce() does?

@akien-mga
Copy link
Member

Would be nice to finalize this to clear to PRs from the backlog ;)

CC @Mickeon

@Mickeon
Copy link
Contributor

Mickeon commented Dec 12, 2022

I agree on what @clayjohn said above.

It definitely needs examples, over the extra note. That extra note talking about mathematical and physical is pretty meaningless for most users, unless they're savvy enough to know what the term even means.

Of course, because the two terms are so intertwined, a note or remark at the end of the sentence linking the two methods would still be necessary (e.g. "To bounce the vector off a surface, use bounce, instead."), still adding that other engines may use these terms differently, since it's a pretty noticeable difference.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 10, 2023
@razcore-rad
Copy link
Contributor

razcore-rad commented Apr 13, 2023

I think because the examples are usually done through use of character motion, it implies a physical context and in physics, reflect means what bounce() does.

I guess that's why in software we usually use mirror when we talk about math reflection. I think this would solve the confusion and bounce() can remain as is. I'd just rename reflect() to mirror().

edit: I also had to check what the difference between the two methods are cause the docs weren't clear for me.

@mhilbrunner
Copy link
Member

@Calinou Gentle reminder poke :)

@AThousandShips
Copy link
Member

AThousandShips commented Jul 15, 2023

I think the relevant difference is this (succinctly put):

  • reflect returns the vector as if flipped through the normal, i.e. returns a vector with the same direction to the normal but on the opposite side
  • bounce returns the vector as if flipped through the surface with that normal, i.e. returns a vector with the same direction to the surface but on the opposite side

Visual representation would be very useful here

@celyk
Copy link

celyk commented Feb 14, 2024

In what sense does Vector3.reflect() reflect over a plane? The fixed points of the transformation don't make a plane, but a line perpendicular to the plane.

I will argue that the function was misnamed from the start, and this documentation issue would disappear if it simply fell in line with convention.

Typically, 3D reflection works just like a mirror, or like flipping 1 coordinate of a vector. The fixed points make up the plane which is reflected about/over. As said by the Wikipedia article (paragraph 2):

unqualified use of the term "reflection" means reflection in a hyperplane.

Hence, the generic "reflect" function should be about a plane defined by it's normal.

Extra: reflect() is currently equivalent to a 180 degree rotation. So I'm really saying that bounce(normal) should be named reflect(normal), and reflect(axis) should be named rotate180(axis), or whatever.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 14, 2024

I personally do appreciate the input but renames are not something that can be done now, as it would break compatibility. It's worth keeping this in mind next time it can be done (open a proposal if you feel strongly about this). But for now it is very worth to clarify the confusion between these two as much as possible with good documentation.

@celyk
Copy link

celyk commented Feb 14, 2024

Sorry, I'm not super aware of when things break compatibility. Hopefully it's still insightful. I don't feel strongly, but changing plane to line/axis would minimize confusion in describing Vector3.reflect

@mhilbrunner
Copy link
Member

Closing as superseded by #89404.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vector2.reflect() result is unexpected
8 participants