Make intersect_ray() return true, if ray orginates inside of a shape.

[madmiraal]

Fixes #35845
Fixes #38343
Fixes #38873
Fixes #39859
Fixes #44003
Supersedes #41277
Addresses comment in #41167.

The issues above suggest that, if the ray originates inside of a shape, intersect_ray() is expected to return true. This change performs this check and assigns the first shape that it detects that the ray is within as the colliding Object. It also sets the colliding point to the ray origin. Since the ray is not colliding with an edge or face, there is no normal; so the zero length vector is assigned. This makes all CollisionShapes consistent with the current results when a ray originates inside a Rectangle2D or BoxShape3D (in Godot physics).

This change has been applied to both Godot 2D and Godot 3D physics. It has not been applied to Bullet physics, which uses the opposite approach: returns no collision if the ray originates inside a shape (has an incalculable normal); as proposed in #41277. However, unlike in Godot physics, in Bullet physics, there does not appear to be an easy way to determine if a point (the ray origin) is contained inside a CollisionShape to make this consistent in Bullet physics too.

[ghost]

This will be good, so when it intersects at the start point it will always be a zero normal, and it can be consistently detected.

It would be nice to amend the documentation about that part. I find it's not intuitive to expect a field called normal to return a 0 length. I would probably pickup on the possibility of this behavior if there were a boolean field about this kind of intersection in the result dictionary.

I was once wondering about this situation. Though it requires more working around, I lean towards preferring the behavior in this PR so there is information about the starting point intersection occurring. That aside, I was wondering what your thoughts about these two ideas would be. (Both would probably need an additional field to the dictionary indicating that the start point intersected.)

A) The ray's starting point is extended backwards until it is not intersecting, and use that as the start point.

B) Find the nearest outside position, and use that as the point of collision.

[madmiraal]

The fact that there are at least two options proves that there isn't a correct solution. Personally, I think the correct solution is the Bullet physics approach i.e. not return a collision if the ray originates inside of a shape. However, with this PR, in Godot physics, this can be achieved by simply also checking if the normal is a zero vector:

if is_colliding() and get_collision_normal() != Vector2():
	#Not colliding and not originating inside of a collider
	...

Using a zero vector enables this.

[madmiraal]

Updated to include updates to the relevant class documentation.

[ghost]

I think the correct solution is the Bullet physics approach i.e. not return a collision if the ray originates inside of a shape.

Without having to a do point intersection test before the ray cast, how does one know this situation has occurred?

In more than one situation I've had a raycast coming from something moving and was using only raycasts (not safely from inside a collider), and could slip into geometry. I needed to catch this situation, because otherwise the algorithm I was using would get trapped, as it relied on some type of outward normal to repel away. If there was no collision result at all, these things would've carried on ghosting through walls.

The point of confusion around this issue in these posts (including mine), is from unexpected results. Until this PR the documentation is not making any mention of this failing condition.

This condition is such an edge case that it doesn't really appear right away. Related to my post, it wasn't until several weeks after finishing the initial work did an object find itself caught in a sharp corner. It just seemed really mysterious where the problem may be. It was also dreadfully difficult to reproduce.

I typically comb over my algorithms first, assuming I missing something.

It took some additional time to create the experiments to tease out the behavior from the engine and work around it.

I don't think general users or beginners are going to ever get this far with it. Given how many issues linked so far, this is a serious hole that you're patching up. 👍

As a post thought to the slipperiness of this: It may be drastic, but better still to pass back a null normal instead of an empty vector, and perhaps even a null position. This way trying to directly handle these fields as a vector will cause a crash and point immediately to the source.

var result = intersect_ray(...)
position = result.position # null

var result = intersect_ray(...)
position = result.normal * speed # null * speed

var result = intersect_ray(...)

# check result
if(not result.empty() and result.normal):

People asking for help on null normals or positions, or looking at the documentation should now get a swifter resolution.

[madmiraal]

Rebased following #41954.

[AndreaCatania]

Thanks!

[akien-mga]

I'm a bit concerned with this being inconsistent in 3D with GodotPhysics vs Bullet. At least it's well documented, but it would be best if they behaved similarly.

Otherwise this seems good as it fixes many valid issues. There's always the concern that some users might be relying on this behavior as a feature, but I guess they'll be able to get it back using collision layers?

[madmiraal]

Updated with the suggested changes to the documentation.

I'm a bit concerned with this being inconsistent in 3D with GodotPhysics vs Bullet. At least it's well documented, but it would be best if they behaved similarly.

Unfortunately, in Bullet physics, there does not appear to be an easy way to determine if a point (the ray origin) is contained inside a CollisionShape. To make them consistent I originally suggested #41277, but, as the issues this PR closes indicate, this is not the expected behaviour.

[pouleyKetchoupp]

This code at the beginning of BulletPhysicsDirectSpaceState::intersect_ray will do the trick for Bullet:

ShapeResult result;
if (0 < intersect_point(p_from, &result, 1, p_exclude, p_collision_mask, p_collide_with_bodies, p_collide_with_areas)) {
	r_result.position = p_from;
	r_result.normal = Vector3();
	r_result.shape = result.shape;
	r_result.rid = result.rid;
	r_result.collider_id = result.collider_id;
	r_result.collider = 0 == r_result.collider_id ? NULL : ObjectDB::get_instance(r_result.collider_id);
	return true;
}

The changes in GodotPhysics look fine. I think it makes sense to have this behavior by default, and harmonize all the physics backends.

Lots of users will expect the behavior to be the other way around though since it's how it's been working. A very common use case is raycasts starting from inside a character controller, and if we don't communicate properly about this change of behavior, we might get a ton of extra issues.

@akien-mga: Do we have a specific place to list changes we need to communicate for different releases? Or is it based on breaks-compat tags? We need to make sure there will be a warning for users who port their projects to 4.0, as well as in the 3.2.4 release notes if this change is cherry-picked.

[Calinou]

Do we have a specific place to list changes we need to communicate for different releases? Or is it based on breaks-compat tags? We need to make sure there will be a warning for users who port their projects to 4.0, as well as in the 3.2.4 release notes if this change is cherry-picked.

The release post as well as the changelog are typically used to list compatibility breakage. However, we typically only update the changelog whenever a new release is made.

[pouleyKetchoupp]

@madmiraal Could you add the Bullet fix from #41347 (comment) and confirm it works? It would be nice to include this PR in 3.2.4 once all the physics backends are consistent.

[madmiraal]

I don't think the suggestion is a good one. Bullet's implementation of intersect_point() is a hack to get the Bullet physics interface to work. It basically creates a small sphere and then checks for collisions. As I stated in the OP:

... in Bullet physics, there does not appear to be an easy way to determine if a point (the ray origin) is contained inside a CollisionShape ...

I think the difference should be documented, and, if the user wants this behaviour, they can perform the intersect_point() test themselves.

[pouleyKetchoupp]

@madmiraal The technical difference between a very small sphere and a point is less important than making the physics backends consistent.

Given that Bullet is currently the default engine, you must include this fix for the PR to be merged into 3.2.

[reduz]

I really think this should be optional, there are too many cases where you want either one or the other behavior.

[akien-mga]

Superseded by #54857.