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

Check intersect_ray normal is not a zero vector, to ensure the ray is colliding with and not just enclosed in the shape. #41277

Closed

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Aug 15, 2020

While investigating #41167, I noticed that RectangleShape2D returns the Object when calling get_collider() for a RayCast2D that is completely enclosed by the shape. However, CapsuleShape2D, CircleShape2D and ConvexPolygonShape2D all return null. The other shapes don't create enclosures.

In 3D Bullet physics, all of the Shapes (BoxShape3D, CapsuleShape3D, CylinderShape3D, SphereShape3D and ConvexPolygonShape3D) return null when calling get_collider() if the RayShape3D is completely enclosed. However, in Godot 3D physics, BoxShape3D and CapsuleShape3D also return the Object when calling get_collider() and a zero length normal Vector3 when calling get_collision_normal(). The SphereShape3D and the ConvexPolygonShape3D don't. Godot 3D physics doesn't support the CylinderShape3D.

This patch makes Rect2D::intersects_segment() and AABB::intersects_segment() return false when the segment doesn't intersect an edge or a face respectively, and hence doesn't set the normal to a zero length vector either.
This patch checks that intersects_segment() sets the normal to a normal vector, to ensure the ray is colliding with and not just enclosed in the shape, before deciding there is a collision.

This makes all shapes consistently return null when calling get_collider() if the Raycast is completely enclosed in the shape.

@madmiraal madmiraal added enhancement topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Aug 15, 2020
@madmiraal madmiraal added this to the 4.0 milestone Aug 15, 2020
@madmiraal madmiraal changed the title Return false from Rect2::instersects_segment and AABB::intersects_seg… Return false from Rect2::instersects_segment() and AABB::intersects_segment() when segment is enclosed. Aug 15, 2020
@akien-mga
Copy link
Member

Is this safe to cherry-pick for 3.2? It might be seen as a compat breaking change, no?

@madmiraal
Copy link
Contributor Author

It might be seen as a compat breaking change

Personally, I think the consistency is more important.

However, the bigger concern I now have is what impact it has on other uses of intersects_segment(), because it's not just used by RayCast. Other uses (which may not depend on the normal) may expect it to return true if it's enclosed.

I think I'll change this so that it's the RayCast that checks if there's a normal before deciding whether or not there is a collision.

colliding with and not just enclosed in the shape.
@madmiraal madmiraal force-pushed the fix-rect2-intersect-segment branch from 23c66eb to 0df54cb Compare August 16, 2020 06:32
@madmiraal
Copy link
Contributor Author

Updated to get _update_raycast_state() to check for zero length normals instead returning false from intersects_segment().

@madmiraal madmiraal changed the title Return false from Rect2::instersects_segment() and AABB::intersects_segment() when segment is enclosed. Check intersect_ray normal is not a zero vector, to ensure the ray is colliding with and not just enclosed in the shape. Aug 16, 2020
@madmiraal madmiraal removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 16, 2020
@madmiraal
Copy link
Contributor Author

Removed the cherrypick:3.2 label and created a 3.2 version of this PR (#41304) because this PR is no longer cherry-pickable.

@Xrayez
Copy link
Contributor

Xrayez commented Aug 16, 2020

Would this sort of fix #35845?

@madmiraal
Copy link
Contributor Author

Would this sort of fix #35845?

Looking at #35845, #38343, #38873 and #39859, it appears as if the expectation is that RayCast::get_collider() does return the Shape that is enclosing it (even if the normal is incalculable). In other words, it's only RectangleShape2D that is currently behaving correctly.

Going to close this PR for now, and see if I can create an alternative solution that gets all the Shapes to behave like RectangleShape2D.

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.

3 participants