Fix character max/min slope #701

Vrixyz · 2024-07-24T09:13:56Z

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

Initially, I was investigating on character controller example: added 2 slopes to test max/min slopes bevy_rapier#532.

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.

⚠️ 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.
- to be noted it would be more maintainable and robust with a reflect feature for rapier, this argument wasn't brought on Add optional bevy feature for reflection #467.

Screenshot controller exposed

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

Testbed update to bevy 0.14

a more focused PR on this is Update testbeds to bevy 0.14 #723

Vrixyz · 2024-07-25T15:22:16Z

examples3d/character_controller3.rs

-    .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);


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

examples3d/character_controller3.rs

Vrixyz · 2024-07-25T15:24:29Z

src/control/character_controller.rs

+        // 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;


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

src/control/character_controller.rs

Vrixyz · 2024-07-25T15:25:31Z

src/control/character_controller.rs


-        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


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.

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.

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

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.

src/control/character_controller.rs

Vrixyz · 2024-07-25T15:27:59Z

src/control/character_controller.rs

+
+    use crate::{control::KinematicCharacterController, prelude::*};
+
+    #[cfg(all(feature = "dim3", feature = "f32"))]


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

Vrixyz · 2024-07-25T15:29:14Z

src_testbed/ui.rs

+            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();


this was copied to easily know which parameters I had to "inspect", I didn't go all the way.

Vrixyz · 2024-07-25T15:29:49Z

src/control/character_controller.rs

+                    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,


part 1 of the fix

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.

Vrixyz · 2024-07-25T15:31:52Z

src/control/character_controller.rs

+        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;


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.

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.

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

…er into testbed_character_controller

Vrixyz · 2024-07-26T08:49:39Z

src/control/character_controller.rs

-            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),
+        );


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

Looks reasonable.

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.

…tial implementation

Vrixyz · 2024-07-30T10:30:22Z

src/control/character_controller.rs

@@ -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,


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.

Vrixyz · 2024-07-30T14:14:32Z

src/control/character_controller.rs

-        // 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.


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.

Vrixyz · 2024-07-30T14:17:14Z

src/control/character_controller.rs

+        /*
+         * Create a slope we can’t climb.
+         */
+        let impossible_slope_angle = 0.6;


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

…ller

update to bevy 0.14 + add some control for testbed character controller

d2c9c35

Vrixyz marked this pull request as draft July 24, 2024 09:14

Vrixyz mentioned this pull request Jul 24, 2024

character controller example: added 2 slopes to test max/min slopes dimforge/bevy_rapier#532

Draft

Vrixyz added 2 commits July 25, 2024 11:49

adapt example + add simpler test to reproduce max slope bug

5a03a09

fix character controller max slope + add tests + better testbed setup

63233f6

Vrixyz commented Jul 25, 2024

View reviewed changes

examples3d/character_controller3.rs Show resolved Hide resolved

Vrixyz commented Jul 25, 2024

View reviewed changes

src/control/character_controller.rs Outdated Show resolved Hide resolved

Vrixyz commented Jul 25, 2024

View reviewed changes

src/control/character_controller.rs Outdated Show resolved Hide resolved

Vrixyz commented Jul 25, 2024

View reviewed changes

Update src/control/character_controller.rs

222c2ec

Vrixyz commented Jul 25, 2024

View reviewed changes

Vrixyz added 2 commits July 26, 2024 10:45

fix some clippy warnings + better fix for grounded status

64c810e

Merge branch 'testbed_character_controller' of github.com:Vrixyz/rapi…

175316b

…er into testbed_character_controller

Vrixyz commented Jul 26, 2024

View reviewed changes

Vrixyz added 3 commits July 29, 2024 14:58

reinstate nudge factor

2a04835

more margin for impossible slope example + as much possible reuse ini…

e4c923e

…tial implementation

lower nudge factor to avoid messing too much with ground detection

c1fe32b

Vrixyz commented Jul 30, 2024

View reviewed changes

Vrixyz added 2 commits July 30, 2024 15:10

slightly better comments

9862850

revert ground change + better ci test

a9c5b68

Vrixyz commented Jul 30, 2024

View reviewed changes

Vrixyz added 2 commits July 30, 2024 16:22

remove unused comments + spaces polish

7204849

polish testbec character ui

bccd234

Vrixyz marked this pull request as ready for review July 30, 2024 14:26

fmt

8cdb21a

Vrixyz requested a review from sebcrozet August 2, 2024 14:16

Vrixyz added 4 commits August 5, 2024 23:09

Merge remote-tracking branch 'upstream' into testbed_character_contro…

c6b359d

…ller

fix clippy

0bd887b

fix test f64 warnings

553d164

fix other warnings

160fbb3

sebcrozet approved these changes Sep 6, 2024

View reviewed changes

Vrixyz changed the title ~~Testbed bevy update + other improvements~~ Fix character max/mn slope Sep 8, 2024

Vrixyz changed the title ~~Fix character max/mn slope~~ Fix character max/min slope Sep 8, 2024

Vrixyz mentioned this pull request Sep 8, 2024

KinematicCharacterController methods (enableAutoStep, maxSlopeClimbAngle, minSlopeClimbAngle) not working in rapier2d-compat dimforge/rapier.js#291

Open

Merge remote-tracking branch 'upstream' into testbed_character_contro…

c6420f6

…ller

AlaricBaraou mentioned this pull request Sep 22, 2024

[WIP ]Kinematic character controller pmndrs/react-three-rapier#702

Open

Vrixyz added 2 commits September 23, 2024 09:50

Merge branch 'master' into testbed_character_controller

677ed84

update changelog

98b58e6

Vrixyz merged commit 76357e3 into dimforge:master Sep 23, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix character max/min slope #701

Fix character max/min slope #701

Vrixyz commented Jul 24, 2024 •

edited

Loading

Vrixyz Jul 25, 2024

Vrixyz Jul 25, 2024

Vrixyz Jul 25, 2024 •

edited

Loading

sebcrozet Jul 26, 2024

Vrixyz Jul 26, 2024

Vrixyz Jul 29, 2024

Vrixyz Jul 25, 2024

Vrixyz Jul 25, 2024

Vrixyz Jul 25, 2024

Vrixyz Jul 26, 2024

Vrixyz Jul 25, 2024

sebcrozet Jul 26, 2024

Vrixyz Jul 26, 2024

Vrixyz Jul 26, 2024

sebcrozet Jul 26, 2024

Vrixyz Jul 30, 2024

Vrixyz Jul 30, 2024

Vrixyz Jul 30, 2024 •

edited

Loading

Vrixyz Jul 30, 2024


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

		#[cfg(all(feature = "dim3", feature = "f32"))]

Fix character max/min slope #701

Fix character max/min slope #701

Conversation

Vrixyz commented Jul 24, 2024 • edited Loading

character controller max_slope bug

Testbed character controller parameters

Testbed update to bevy 0.14

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vrixyz Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vrixyz Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vrixyz commented Jul 24, 2024 •

edited

Loading

character controller `max_slope` bug

Vrixyz Jul 25, 2024 •

edited

Loading

Vrixyz Jul 30, 2024 •

edited

Loading