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 interpolation and systems schedule #474

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Vixenka
Copy link

@Vixenka Vixenka commented Jan 31, 2024

Interpolation not works properly due the problem described in #464.

We removed TimestepMode from crate, because we do not see reason for why this should even exists. Idea of FixedUpdate loop which Bevy engine implements look fine, and another game engines also use this for physics steps. Description about current master implementation:

  • TimestepMode::Fixed - looks nice, but do not interpolate results, who wants to use that? This can be easy misleading for new users of crate.
  • TimestepMode::Variable - it make physics heavily addicted by Update's delta time and is really far of deterministic, also change speed of physics depending on render time which makes difficult to catch up render by physics what occurs slow simulation on e.g. weaker computers.
  • TimestepMode::Interpolation - before does not work correctly, user system execution was skipped and lerp was poorly implemented.

Now, after our changes, every user defined system which impact physics like movement etc. should execute in engine's schedule FixedUpdate and before crate's system set PhysicsSet::SyncBackend. Also for things like slow motion effects change engine's fixed delta time by Bevy API.

Resolves #464, resolves #379, closes #463
And probably #465, #475

liquidev and others added 11 commits December 19, 2023 21:57
Instead of trying to detect changes in the Transform, rely on the user telling us when to teleport instead of interpolating positions.
For that there is a new TransformInterpolation::teleport function, which resets the interpolation data manually.
Run physics in FixedUpdate instead of own loop
@Vixenka Vixenka marked this pull request as ready for review January 31, 2024 17:41
@andriyDev
Copy link

andriyDev commented Feb 5, 2024

As discussed in #475, the velocity is not correctly reported for KinematicCharacterControllers (it always seems to print 0.0).

It also looks like this PR makes the KinematicCharacterController translation field use meters per second, rather than units (or pixels) per second. This is a change from the existing behavior.

Edit: I'll try to review this and see if I spot anything.

.before(TransformSystem::TransformPropagate),
);
app.configure_sets(
Update,

Choose a reason for hiding this comment

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

I believe this should be PostUpdate? TransformSystem::TransformPropagate only runs in PostUpdate, so this line does nothing I think. It also makes sense to try to update the positions of stuff as late as possible.

Copy link
Author

Choose a reason for hiding this comment

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

This runs normally in Update, but why you want run this as late as possible? In my opinion running this as early as possible after FixedUpdate loop will be the best idea, because this do not create additional time for applying a data.

Self::get_systems(PhysicsSet::StepSimulation).in_set(PhysicsSet::StepSimulation),
);
app.add_systems(
Update,

Choose a reason for hiding this comment

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

PostUpdate here as well.

@@ -708,8 +700,7 @@ pub fn step_simulation<Hooks>(
mut context: ResMut<RapierContext>,
config: Res<RapierConfiguration>,
hooks: StaticSystemParam<Hooks>,
time: Res<Time>,
mut sim_to_render_time: ResMut<SimulationToRenderTime>,
time: Res<Time<Fixed>>,

Choose a reason for hiding this comment

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

step_simulation is being added to FixedUpdate, so there's no reason to specify Time<Fixed>. Bevy will automatically used Fixed as Res<Time>.

Copy link
Author

Choose a reason for hiding this comment

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

It is true, I changed this line so often and it stayed in this way.

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to remove this unnecessary generic attribute, but I noticed that this function is public and can be execute manually then I think this should be explicit, what do you think?

Choose a reason for hiding this comment

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

It's unclear to me why this is public at all, but assuming that it must be, there's no reason to force users to use this in FixedUpdate. Presumably if people are using this function directly, they know what they are doing, in which case making it more flexible is probably a good idea.

if let Some(body) = body_handle.and_then(|h| context.bodies.get_mut(h.0)) {
// TODO: Use `set_next_kinematic_translation` for position based kinematic bodies? - Vixenka 29.01.2024
body.set_translation(
body.translation() + movement.translation * physics_scale,

Choose a reason for hiding this comment

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

I think this is the culprit. It scales the translation here.

Copy link
Author

Choose a reason for hiding this comment

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

This multiplication is from original line what is lower on line 1468.

Choose a reason for hiding this comment

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

Not quite. That line refers to the Bevy Transform component. However, this line refers to the Rapier body's transform. The Rapier body's transform will get copied to the Bevy Transform later on, where it will be scaled up by the physics scale. So the movement vector gets scaled up by the physics_scale twice rather than just once.

I'm not sure if that made sense haha. I tried removing this multiplication on your branch and it works correctly now - Setting my KCC's speed to 50.0 actually produces 50.0 in my derived_velocity system.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, nice!

@@ -1463,7 +1453,16 @@ pub fn update_character_controls(
}
}

if let Ok(mut transform) = transforms.get_mut(entity_to_move) {
// NOTE: Update the translation of the Rapier's rigid body without editing the Bevy's transform for

Choose a reason for hiding this comment

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

Is this section necessary? I'm betting this is why the velocity is not being updated. The rigidbody is being updated directly here and whenever the velocity is being computed, it detects no change (because this code directly modifies the body).

Copy link
Author

@Vixenka Vixenka Feb 6, 2024

Choose a reason for hiding this comment

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

Ok, good that you find it. But we must do that here, because otherwise move of character controller will be recorded as user's modification which will be break interpolation. I guess we should compute velocity here or is it should be always zero @sebcrozet?

Unfortunately author of Rapier is so often offline, and he did not reply for this issue since above month.

@phisn
Copy link

phisn commented Mar 29, 2024

The current implementation does not allow for a lot of flexibility of schedule design anymore. My game uses a custom schedule to have more control over the physics loop which is not possible here anymore. Maybe we could define a self.physics_schedule and self.write_back_schedule to differentiate between both use cases. This can not be easily fixed by putting my systems before or after the rapier system set since it does not allow for not executing or executing irregularly.

@phisn
Copy link

phisn commented Mar 29, 2024

I also just noticed, that when the transform is changed, that the whole interpolation is canceled. In my game im currently setting the rotation in each frame but do still like the translation to be interpolated. Maybe its possible to do the interpolation component wise?

@Vixenka
Copy link
Author

Vixenka commented Mar 30, 2024

@phisn about custom schedules we need to have the expected behavior from #464 to have working interpolation. Bevy's FixedUpdate schedule was created for this purpose, and suits for this ideal. I do not know, why you want to replace this from API, because way how it works after my changes are so common in another game engines, and games just exists with that solution. But maybe you want to have possibility for execute next physics step by hand? Then user-defined systems should also be executed what I do not remember if it possible in Bevy to make (I guess yes), and this is a things what we can make (if we can).

About transform changes, and preverting interpolation. This exists originally in code, and this is a feature what have sense, because make teleporting instant instead of flying all over the map. Real world physics do not have things like teleport, everything works with impulses and forces, not by teleporting, and teleporting is not a things what can be interpolated, because physics engine do not know what changed in what way. You can create impulse or force, what will be rotate your object with having interpolation, or also add rotation similar to position to character controller what Rapier do not implement yet.

@Vrixyz Vrixyz added D-Difficult Needs strong technical background, domain knowledge, or impacts are high, needs testing... P-High arbitrary important item A-Integration very bevy specific C-Bug Something isn't working labels May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Integration very bevy specific C-Bug Something isn't working D-Difficult Needs strong technical background, domain knowledge, or impacts are high, needs testing... P-High arbitrary important item
Projects
None yet
5 participants