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 character max/min slope #701

Merged
merged 21 commits into from
Sep 23, 2024

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Jul 24, 2024

This PR has a bit too many different changes, but bare with me:

This PR approaches:

character controller max_slope bug

  • While testing rapier, I thought rapier did not have this bug, but looking more carefully, the setup for the slope we can't climb is slightly incorrect and we should jump on it to check that we're blocked.
screenshot testbed

We probably want the bigger slope to be reachable without jumping.

image

⚠️ So the slope bug seems to originate from rapier, and not bevy / javascript integration.

Testbed character controller parameters

  • character controller has some parameters, which are note exposed in the testbed, making it difficult to quickly test a behaviour.
Screenshot controller exposed

WIP 🚧, it lacks some options and a title group, same controls could be added to the vehicle controller.

image

Testbed update to bevy 0.14

Comment on lines -102 to +99
.translation(vector![ground_size + slope_size, -ground_height + 0.4, 0.0] * scale)
.rotation(Vector::z() * slope_angle);
let collider = ColliderBuilder::cuboid(slope_size, ground_height, slope_size)
.translation(vector![0.1 + slope_size, -ground_height + 0.4, 0.0])
.rotation(Vector::z() * slope_angle);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

scale to add back ; not sure it's still usable though 🤔

// An object is trying to climb if its vertical (according to self.up) input motion points upward.
let climbing_intent = self.up.dot(&_vertical_input) > 0.0;
// An object is climbing if the tangential movement induced by its vertical movement points upward.
let climbing = self.up.dot(&decomp.vertical_tangent) > 0.001;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe a comment to justify that would be useful, and use the explicit epsilon ?


let allowed_movement = if hit.is_wall && climbing && !climbing_intent {
let allowed_movement = if hit.is_wall && (climbing && !climbing_intent) {
// Can’t climb the slope, remove the vertical tangent motion induced by the forward motion.
decomp.horizontal_tangent + decomp.normal_part
Copy link
Contributor Author

@Vrixyz Vrixyz Jul 25, 2024

Choose a reason for hiding this comment

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

Part 3 of the fix: when blocking climb, we don't want to apply the nudge factor, right ?

That's why I moved the nudge within the blocks below.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, applying the nudge factor even if we block the climb helps with preserving the margin between the controller and the floor. Otherwise that margin might end up getting smaller and smaller as you slide against a wall or floor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we still need to avoid climbing as a side effect 🤔 I think.

Not sure on the correct math to do that 😅 ; mayyybe we should cross that horizontal tangeant with self.up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after testing, removing the nudge factor doesn't seem to fix the "can climb forbidden slopes", so I'm reverting that change. To be clear, the current behaviour is still better than what's on master.


use crate::{control::KinematicCharacterController, prelude::*};

#[cfg(all(feature = "dim3", feature = "f32"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gated it behind features because it's already a big test, I fear adding support for multiple dimensions and precision would make it even less approachable

Comment on lines 273 to 294
pub offset: CharacterLength,

/// Should the character automatically step over small obstacles? (disabled by default)
///
/// Note that autostepping is currently a very computationally expensive feature, so it
/// is disabled by default.
pub autostep: Option<CharacterAutostep>,
/// Should the character be automatically snapped to the ground if the distance between
/// the ground and its feed are smaller than the specified threshold?
pub snap_to_ground: Option<CharacterLength>,
/// Increase this number if your character appears to get stuck when sliding against surfaces.
///
/// This is a small distance applied to the movement toward the contact normals of shapes hit
/// by the character controller. This helps shape-casting not getting stuck in an always-penetrating
/// state during the sliding calculation.
///
/// This value should remain fairly small since it can introduce artificial "bumps" when sliding
/// along a flat surface.
pub normal_nudge_factor: Real,
*/
}
ui.separator();
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 was copied to easily know which parameters I had to "inspect", I didn't go all the way.

Comment on lines 274 to 280
stop_at_penetration:
// We want to know the collision we're currently in,
// otherwise we risk ignoring useful collision information (walls / ground...)
iter_index == 0
// If we stop at penetration while not allowing sliding alongside shapes,
// it will be impossible to get away of a collision.
&& self.slide,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

part 1 of the fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this fixes ground detection but only by side effect: the condition with // No interference along the path. contains a break, and grounded is computed just after. I think we actually want to computed the grounded status after the loop.

Comment on lines 558 to 564
let slipping_intent = self.up.dot(&_vertical_input) < 0.0;

// An object is slipping if the tangential movement induced by its vertical movement points downward.
let slipping = self.up.dot(&decomp.vertical_tangent) < 0.0;

// An object is trying to climb if its indirect vertical motion points upward.
let climbing_intent = self.up.dot(&input_decomp.vertical_tangent) > 0.0;
let climbing = self.up.dot(&decomp.vertical_tangent) > 0.0;
// An object is trying to climb if its vertical (according to self.up) input motion points upward.
let climbing_intent = self.up.dot(&_vertical_input) > 0.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

part 2 of the fix: using input to infer "user intent", rather than movement tangent.

This leads to unused horiz_input_decomp and input_decomp.

Copy link
Member

Choose a reason for hiding this comment

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

Mmh, I think I tried that at some point and this had a side-effect of having the user climb some slopes it shouldn’t. It would be worth moving around in the character-controller scene.

This also makes me think that perhaps we should make a test scene where we have many character-controller (in isolation) that try to walk on various surface configurations. That would help spot regressions more easily without having to move the character manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a scene with multiple character controller is an interesting idea ; to be noted I added an automated test for character controller.

Comment on lines 324 to 341
result.grounded = self.detect_grounded_status_and_apply_friction(
dt,
bodies,
colliders,
queries,
character_shape,
&(Translation::from(result.translation) * character_pos),
&dims,
filter,
Some(&mut kinematic_friction_translation),
Some(&mut translation_remaining),
);

if !self.slide {
break;
}
}

result.grounded = self.detect_grounded_status_and_apply_friction(
dt,
bodies,
colliders,
queries,
character_shape,
&(Translation::from(result.translation) * character_pos),
&dims,
filter,
Some(&mut kinematic_friction_translation),
Some(&mut translation_remaining),
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

part 1 of the fix (the break from line 322 (in block // No interference along the path.) would bypass the ground detection, resulting in not grounded. That's happening because we're not checking for current colliding shapes. when doing the cast.

reworked from https://github.com/dimforge/rapier/pull/701/files#r1691673551

Copy link
Member

Choose a reason for hiding this comment

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

Looks reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after more testing with technique from #705 ; this doesn't seem to improve significantly the behaviour, so I'll leave it out of this PR and focus on the slope. This PR builds a useful test to plus such verification into it though.

@@ -163,12 +163,13 @@ impl Default for KinematicCharacterController {
max_slope_climb_angle: Real::frac_pi_4(),
min_slope_slide_angle: Real::frac_pi_4(),
snap_to_ground: Some(CharacterLength::Relative(0.2)),
normal_nudge_factor: 1.0e-4,
normal_nudge_factor: 1.0e-5,
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 was messing up the grounded status on the test scene ; if that's a bad default, more explaination and/or a test case for this default should be added.

Comment on lines -554 to +555
// An object is trying to climb if its indirect vertical motion points upward.
let climbing_intent = self.up.dot(&input_decomp.vertical_tangent) > 0.0;
// An object is trying to climb if its vertical input motion points upward.
let climbing_intent = self.up.dot(&_vertical_input) > 0.0;
// An object is climbing if the tangential movement induced by its vertical movement points upward.
Copy link
Contributor Author

@Vrixyz Vrixyz Jul 30, 2024

Choose a reason for hiding this comment

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

in my understanding, "climbing intent" refers to an explicit movement going upward of the slope, that's why I'm testing only the input, and not the decomposition according to the hit.

the naming is confusing because slipping_intent doesn't refer exactly to the same thing, but I'm not sure how to rename.

/*
* Create a slope we can’t climb.
*/
let impossible_slope_angle = 0.6;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

on master, this impossible angle can be climb in this test. That's what https://github.com/dimforge/rapier/pull/701/files#r1697044257 fixes.

@Vrixyz Vrixyz marked this pull request as ready for review July 30, 2024 14:26
@Vrixyz Vrixyz requested a review from sebcrozet August 2, 2024 14:16
@Vrixyz Vrixyz changed the title Testbed bevy update + other improvements Fix character max/mn slope Sep 8, 2024
@Vrixyz Vrixyz changed the title Fix character max/mn slope Fix character max/min slope Sep 8, 2024
@Vrixyz Vrixyz merged commit 76357e3 into dimforge:master Sep 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants