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 Rayshape not working properly in move_and_slide and move_and_slide_with_snap #45005

Closed

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented Jan 8, 2021

These changes improve Rayshape behavior for Godot Physics 2D and 3D when using move_and_slide with and without snapping.

Kinematic margin is now applied to ray shapes when handling snapping collision tests and separation raycasts to help getting consistent results in slopes and flat surfaces.

Recovery is calculated without the margin and a depth of 0 is still considered a collision to stabilize results when on flat surface.

Recovery is split based on the amount of shapes to fix cases where multiple rayshapes would cause the body to bounce.

Fixes #34098 (Godot Physics only, still happens in Bullet)
Fixes #34663 (Godot Physics only, still happens in Bullet)
Fixes #27689

3.2 version of this PR: #45028

@pouleyKetchoupp pouleyKetchoupp added bug topic:physics cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jan 8, 2021
@AndreaCatania
Copy link
Contributor

I wonder if you should handle this on the move and snap, instead. If you apply the margin on the move and snap raycast call, and then you remove the margin just before apply the height on the character, you can fix it for any engine integrates the PhysicsServer.

@akien-mga akien-mga added this to the 4.0 milestone Jan 8, 2021
@pouleyKetchoupp
Copy link
Contributor Author

@AndreaCatania The snap code doesn't know about the ray at all, which is a shape of the kinematic body, handled in the server. In Bullet, it would have to be fixed in GodotRayWorldAlgorithm and btRayShape but I'm not sure what the proper way is. Move and slide has more issues in Bullet too, so it might require other fixes for the ray shape to work properly.

These changes improve Rayshape behavior for Godot Physics 2D and 3D
when using move_and_slide with and without snapping.

Kinematic margin is now applied to ray shapes when handling snapping
collision tests and separation raycasts to help getting consistent
results in slopes and flat surfaces.

Recovery is calculated without the margin and a depth of 0 is still
considered a collision to stabilize results when on flat surface.

Recovery is split based on the amount of shapes to fix cases where
multiple rayshapes would cause the body to bounce.

Fixes godotengine#34098
Fixes godotengine#34663
@pouleyKetchoupp
Copy link
Contributor Author

I've pushed extra changes in space_2d_sw and space_3d_sw in order to fix more stabilization problems involving Rayshape with and without snapping.

All these changes have to be done together, because the previous changes were causing things to be worse in some cases without snap (by taking kinematic margin into account for ray separation tests).

@pouleyKetchoupp pouleyKetchoupp changed the title Fix Rayshape not snapping properly in move_and_slide_with_snap Fix Rayshape not working properly in move_and_slide and move_and_slide_with_snap Jan 8, 2021
if (depth > result.collision_depth) {
result.collision_depth = depth;
result.collision_point = b;
result.collision_normal = (b - a).normalized();
result.collision_normal = ray_normal;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always using raycast normal as a result, to make sure it only does separation, as described in this article:
https://godotengine.org/article/godot-31-will-get-many-improvements-kinematicbody

body_transform.elements[2] += recover_motion;
body_aabb.position += recover_motion;

recover_attempts--;
} while (recover_attempts);
}

//optimize results (remove non colliding)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is not needed anymore, since we're considering hits at depth 0 as a collision now, in order to stabilize floor tests.

As a side note, this block was not working correctly, as it would skip one element after each swap.
Proper implementation would have been:

for (int i = 0; i < rays_found; i++) {
	if (r_results[i].collision_depth == 0) {
		rays_found--;
		SWAP(r_results[i], r_results[rays_found]);
		i--; // because we need to check the element we just moved
	}
}

@akien-mga
Copy link
Member

Needs a rebase.

@pouleyKetchoupp
Copy link
Contributor Author

Closing since ray shapes have been removed on master.

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