dimforge / bevy_rapier

Official Rapier plugin for the Bevy game engine.
https://rapier.rs
Apache License 2.0
1.28k stars 260 forks source link

KinematicCharacterController Velocity is dependent on framerate. #475

Open andriyDev opened 10 months ago

andriyDev commented 10 months ago

The Velocity component is updated by bevy_rapier even for KinematicCharacterController/Rigidbody::KinematicPositionBased. Unfortunately, the reported velocity is tied to the framerate! Higher delta times results in a higher velocity.

This example (using Bevy 0.12 and bevy_rapier2d 0.24) shows what happens if we artificially decrease the framerate (by enabling the laggy system:

use std::time::Duration;

use bevy::{prelude::*, render::camera::ScalingMode};
use bevy_rapier2d::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_plugins(RapierPhysicsPlugin::<NoUserData>::pixels_per_meter(350.0))
        .add_plugins(RapierDebugRenderPlugin::default())
        .add_systems(Startup, spawn_collider)
        .add_systems(Update, set_move)
        .add_systems(PostUpdate, print_velocity.after(PhysicsSet::Writeback))
        // .add_systems(Update, laggy) // Uncomment for lag!
        .run();
}

fn spawn_collider(mut commands: Commands) {
    commands
        .spawn(SpatialBundle::default())
        .insert(Collider::cuboid(100.0, 100.0))
        .insert(RigidBody::KinematicPositionBased)
        .insert(Velocity::default())
        .insert(KinematicCharacterController::default());

    commands.spawn(Camera2dBundle {
        projection: OrthographicProjection {
            near: -1000.0,
            far: 1000.0,
            scaling_mode: ScalingMode::FixedVertical(700.0),
            ..Default::default()
        },
        ..Default::default()
    });
}

fn laggy() {
    std::thread::sleep(Duration::from_millis(100));
}

fn set_move(time: Res<Time>, mut controllers: Query<&mut KinematicCharacterController>) {
    for mut controller in controllers.iter_mut() {
        controller.translation = Some(Vec2::NEG_X * 50.0 * time.delta_seconds());
    }
}

fn print_velocity(velocities: Query<&Velocity>) {
    dbg!(velocities.single());
}

My results: Without the laggy system, my velocity is ~50. With the laggy system, my velocity is >300.

My concrete use case for the Velocity component on a KinematicCharacterController is a generic camera. I want my camera to be able to follow any entity and I also want the camera to "look ahead" of where the entity is going. I do this by adding the entity's velocity (multiplied by some "prediction time") to the entity's position and lerping the camera there. Ideally I want this system to work for physics objects or kinematic characters or whatever else. Unfortunately this breaks with kinematic characters because of this bug.

andriyDev commented 10 months ago

This is also tied in to another issue. The velocity seems to be derived in the simulation step (or otherwise part of the same step rate). This means that multiplying my translation vector by time.delta_seconds() is subtly wrong. The simulation will always have a slightly shorter total delta time (when using fixed timesteps). This means a character's velocity can be inconsistent, since the delta time is slightly different. This can lead to jitter (especially in my camera system haha).

Vixenka commented 10 months ago

@andriyDev it seems to the problem which I had and (yesterday) repaired. Look to my PR #474 and you can try apply my changes:

Change downstream in Cargo.toml:

bevy_rapier2d = { git = "https://github.com/liquidev/bevy_rapier", rev = "bf89857fbca823bd8fdbe18b471c074f6a889dae" }

Change system execution in main.rs:

        .add_systems(FixedUpdate, set_move.before(PhysicsSet::SyncBackend))
        .add_systems(FixedUpdate, print_velocity.after(PhysicsSet::Writeback))

Now, after my changes every things which you make with physics must be done in FixedUpdate and before SyncBackend set.

You can also remove time.delta_seconds() because with this the slow motion effect will not work due to the change in constant update rate (but you can archive it by physical scale).

If you will try to do that please write if this works or not. It works for me :)

andriyDev commented 10 months ago

Sometimes one hits enter too early... I am goofy.

@Vixenka I tried running your PR and it looks like it has some issues. I decided not to remove the delta_seconds since I still want my movement to be independent of the FixedUpdate rate. Regardless, the velocity does not update correctly. It seems to always be set to 0.0. In fact, it looks like the actual applied velocity is seriously scaled up. The example doesn't show it well, but in my actual game, I use the same 500 speed, and it zooms off super fast. Very occasionally on some frames it prints something like ~130,000 which is way faster a velocity than I am using.

Vixenka commented 10 months ago

@andriyDev can you send your code?

andriyDev commented 10 months ago

@Vixenka The example above works to show it. If you change the speed of the controller from 50 to 1, you can see it is moving much faster than expected (before it shoots off).

As for the "occasionally printing the speed", I can't seem to recreate it on my project :/ Perhaps it has something to do with system ordering? idk.

Edit: actually now that I think about it, scaling down by 50 seems to suggest this is a delta time issue, since I believe the default FixedUpdate rate is 64? So perhaps rather than multiplying the delta time, the delta time is being divided somewhere in bevy_rapier code?

Vixenka commented 9 months ago

@andriyDev I runned this and it looks valid:

use std::time::Duration;

use bevy::{prelude::*, render::camera::ScalingMode};
use bevy_rapier2d::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_plugins(RapierPhysicsPlugin::<NoUserData>::pixels_per_meter(350.0))
        .add_plugins(RapierDebugRenderPlugin::default())
        .add_systems(Startup, spawn_collider)
        .add_systems(FixedUpdate, set_move.before(PhysicsSet::SyncBackend))
        .add_systems(
            FixedUpdate,
            print_velocity.after(PhysicsSet::StepSimulation),
        )
        //.add_systems(Update, laggy) // Uncomment for lag!
        .run();
}

fn spawn_collider(mut commands: Commands) {
    commands
        .spawn(SpatialBundle::default())
        .insert(Collider::cuboid(100.0, 100.0))
        .insert(RigidBody::KinematicPositionBased)
        .insert(Velocity::default())
        .insert(KinematicCharacterController::default());

    commands.spawn(Camera2dBundle {
        projection: OrthographicProjection {
            near: -1000.0,
            far: 1000.0,
            scaling_mode: ScalingMode::FixedVertical(700.0),
            ..Default::default()
        },
        ..Default::default()
    });
}

fn laggy() {
    std::thread::sleep(Duration::from_millis(100));
}

fn set_move(time: Res<Time>, mut controllers: Query<&mut KinematicCharacterController>) {
    for mut controller in controllers.iter_mut() {
        controller.translation = Some(Vec2::NEG_X * 0.5 * time.delta_seconds());
    }
}

fn print_velocity(velocities: Query<&Velocity>) {
    dbg!(velocities.single().linvel.x);
}

But I do not understand why you use position based kinematic character controller instead of velocity based, but for current, also from my PR bevy_rapier's version this looks like do not change nothing. And after my PR print_velocity always printing zero, but things on screen looks valid.

Answering for a question, my PR did not using delta time anywhere for simultion, only for interpolation to the screen.

andriyDev commented 9 months ago
controller.translation = Some(Vec2::NEG_X * 0.5 * time.delta_seconds());

@Vixenka This line means "move in the negative X direction at 0.5 units per second multiplied by the seconds for this frame." That means our velocity should be 0.5 units per second. The vertical height is set to a fixed 700 so lets estimate the horizontal width is 1000 units. Since the origin point is in the center, that means the distance to the edge is 500 units. So to travel 500 units with a speed of 0.5 units per second, we expect a time of ~1000 seconds. On my machine, this happens in about 5 seconds. So clearly something is going wrong.

But I do not understand why you use position based kinematic character controller instead of velocity based

To be honest, this is my gap in knowledge. There don't seem to be many examples that show the difference between these two. KinematicCharacterController has explicit translation fields (including in its KinematicCharacterControllerOutput), but nothing for velocity. It wasn't clear to me how this would work. Regardless, that seems to be irrelevant. This should still all work with position-based characters?

andriyDev commented 9 months ago

I've added a system that computes the velocity by measuring the change in position:

use std::time::Duration;

use bevy::{prelude::*, render::camera::ScalingMode};
use bevy_rapier2d::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_plugins(RapierPhysicsPlugin::<NoUserData>::pixels_per_meter(350.0))
        .add_plugins(RapierDebugRenderPlugin::default())
        .add_systems(Startup, spawn_collider)
        .add_systems(FixedUpdate, set_move.before(PhysicsSet::SyncBackend))
        .add_systems(FixedUpdate, print_velocity.after(PhysicsSet::Writeback))
        .add_systems(FixedUpdate, derive_velocity.after(print_velocity))
        // .add_systems(Update, laggy) // Uncomment for lag!
        .run();
}

fn spawn_collider(mut commands: Commands) {
    commands
        .spawn(SpatialBundle::default())
        .insert(Collider::cuboid(100.0, 100.0))
        .insert(RigidBody::KinematicPositionBased)
        .insert(Velocity::default())
        .insert(KinematicCharacterController::default());

    commands.spawn(Camera2dBundle {
        projection: OrthographicProjection {
            near: -1000.0,
            far: 1000.0,
            scaling_mode: ScalingMode::FixedVertical(700.0),
            ..Default::default()
        },
        ..Default::default()
    });
}

fn laggy() {
    std::thread::sleep(Duration::from_millis(100));
}

fn set_move(time: Res<Time>, mut controllers: Query<&mut KinematicCharacterController>) {
    for mut controller in controllers.iter_mut() {
        controller.translation = Some(Vec2::NEG_X * 0.5 * time.delta_seconds());
    }
}

fn print_velocity(velocities: Query<&Velocity, Changed<Velocity>>) {
    let Ok(velocity) = velocities.get_single() else {
        return;
    };
    dbg!(&velocity);
}

fn derive_velocity(
    time: Res<Time>,
    controllers: Query<&Transform, With<KinematicCharacterController>>,
    mut last_position: Local<Vec2>,
) {
    let transform = controllers.single();
    let derived_velocity = (transform.translation.xy() - *last_position) / time.delta_seconds();
    dbg!(&derived_velocity);
    *last_position = transform.translation.xy();
}

Nothing else has changed except for the new system.

For my it prints -175 constantly. This is way faster than 0.5 units per second!

I just realized why - my pixels_per_meter is 350. So 0.5 * 350 = 175. Is the translation meant to be in meters per second??? This is a big departure from the previous behaviour.

Vixenka commented 9 months ago

On my machine, this happens in about 5 seconds. So clearly something is going wrong.

@andriyDev II thought about it earlier and I think it is just in meters per seconds, not units per seconds. And when width is around 1000 units, and meter have 350 units which is configured in main function. Then 1000 u/m / 350 u/m =~ 2.85714 m and 2.85714 m / 0.5 m/s = 5,71428 s Does this look sensible?

This should still all work with position-based characters

I do not notice differences between that, on code which I modified, also I did not investigate that.

This is a big departure from the previous behaviour.

Before my PR it was in units per seconds? If so, I am little surprised :/

andriyDev commented 9 months ago

@Vixenka Yes that makes more sense, specifically in regards to the speed. This still means the velocity is not being reported correctly (it still prints a velocity of 0.0).

Before my PR it was in units per seconds?

Yes, I have been using units per second with no fuss. This change should be reverted and made as a separate PR. Then we need a healthy serving of doc changes to make it clear it is translation in meters. But for your existing PR, I don't think this change should be included.

Vixenka commented 9 months ago

Yea, I noticed earlier velocity which is zero, but I do not know why it changed in my PR. Probably this was a part where I changed position applier for the character controller.

And about units per second I also do not know what changed that, it is so weird. But probably this is the same part of code as upper.

Vrixyz commented 6 months ago

I'd recommend to store your expected velocity and anticipate based on that, as you sleep some time before moving and use delta_time it's expected that the velocity has quite a jump on 1 frame.

I understand how it can be surprising, but I'm worried you're trying to reproduce your issue incorrectly.

Out of curiosity have you tried adding set_move to FixedUdate ?