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

Fix collision normal being referred to as surface normal in PhysicsDirectSpaceState doc #101698

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

DinkeyKing
Copy link
Contributor

In the PhysicsDirectSpaceState class references (2D and 3D) the get_rest_info method descriptions state that the returned dictionary's "normal" field contains the surface normal at the contact point, but it's actually the collision normal.

@DinkeyKing DinkeyKing requested a review from a team as a code owner January 17, 2025 16:01
@DinkeyKing DinkeyKing changed the title Fix collision normal being referred to as surface normal in PhysicsDirectSpaceState get_rest_info doc Fix collision normal being referred to as surface normal in PhysicsDirectSpaceState doc Jan 17, 2025
@AThousandShips
Copy link
Member

This wording needs to be clarified, as the "collision normal" is the normal of either object, if it's not this object it should be the other one I assume

@AThousandShips AThousandShips requested a review from a team January 17, 2025 17:37
@AThousandShips AThousandShips added this to the 4.4 milestone Jan 17, 2025
@AThousandShips
Copy link
Member

What would be the difference between the collision normal and the surface normal? From what I recall of physics the collision normal should be that of the surface contact?

@DinkeyKing
Copy link
Contributor Author

What would be the difference between the collision normal and the surface normal? From what I recall of physics the collision normal should be that of the surface contact?

The collision/contact normal, to my knowledge, is the best direction by which to separate two colliding shapes. From my experience (I'm not an expert.): The collision normal at the contact point of the object often lines up with the surface normal, but, for example, when colliding the shape with a corner < , the collision normal be neither of the surface normals that make up that corner, but somewhere between the two, pointing away from the corner.

@AThousandShips
Copy link
Member

Then this sounds good and I'll leave it to the physics team to give their opinion!

Copy link
Contributor

@mihe mihe left a comment

Choose a reason for hiding this comment

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

The change here is definitely more accurate than what's there currently, especially considering that this is already being called a collision normal in ShapeCast3D.get_collision_normal, which returns this exact normal.

The documentation for these result fields seems to be a bit ambiguous though, with it referring to the hit object as "the colliding object", "the intersecting object" and "the object" all at once.

I would probably rephrase the documentation for the normal to say that it's the collision normal of the query shape, since it describes the direction in which to move the query shape away from the collision and not the hit object. I would probably also specify which direction it's pointing in, for those who might not be familiar with this kind of physics speak.

Lastly, we might as well normalize "the colliding object", "the intersecting object" and "the object" as just "the intersecting object", since that might be a little bit less ambiguous, but would also line up better with most of the existing documentation. This would need to be done for get_rest_info, intersect_point, intersect_ray and intersect_shape.

Copy link
Contributor

@mihe mihe left a comment

Choose a reason for hiding this comment

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

LGTM. I'm seeing some other things that I'd like to change here, so I can do the above mentioned "normalization" in a separate PR.

@Repiteo Repiteo merged commit 1f2dec7 into godotengine:master Jan 20, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 20, 2025

Thanks! Congratulations on your first merged contribution! 🎉

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.

4 participants