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

Support for use within Bevy's FixedUpdate schedule? #263

Closed
Tracked by #433
Architector4 opened this issue Nov 28, 2023 · 3 comments · Fixed by #457
Closed
Tracked by #433

Support for use within Bevy's FixedUpdate schedule? #263

Architector4 opened this issue Nov 28, 2023 · 3 comments · Fixed by #457
Labels
A-Scheduling Relates to scheduling or system sets

Comments

@Architector4
Copy link

Architector4 commented Nov 28, 2023

I'm doing a bunch of logic before, within, and after the FixedUpdate schedule in Bevy, on the fixed timestep. I'd like to run XPBD physics within the FixedUpdate schedule, to ensure that I can record all entity transforms how they are before the physics step, and record them after they have been adjusted by the physics step after it, so that I can catch the difference and do my own logic with that.

Unfortunately, that doesn't seem to be doable. The Time<Physics> resource could be adjusted to use TimestepMode::FixedOnce right before the physics step, but there doesn't appear to be a way to ensure the physics engine is notified of removals of various components it tracks.

ComponentRemovals<T> only gives info about removals that happened in the same or previous app update, and so they are frequently missed in FixedUpdate if the fixed timestep is slow enough. As such, it'd be nice to be able to run XPBD's systems that use that data in a schedule I designate that I run every update, but it's not possible because it's hardcoded.

In particular, the detection only happens in PhysicsSchedule, which is used to step the entire simulation forward:

https://github.com/Jondolf/bevy_xpbd/blob/088facfe3761235f0b297bc1493927a12a6fe21b/src/plugins/prepare.rs#L96-L97

These systems themselves are private, and so they can't be run individually outside of the crate without stepping the simulation. The resource that handle_collider_storage_removals system uses, ColliderStorageMap, is also private, so it can't be notified of removals manually either.

I assume there may be other roadblocks that prevent my use case from being possible, but this is the one that I noticed.

@Jondolf Jondolf added the A-Scheduling Relates to scheduling or system sets label Nov 28, 2023
@Jondolf
Copy link
Owner

Jondolf commented Nov 29, 2023

So if I understood correctly, the main issue is that some systems currently use RemovedComponents<T>, but if the engine is configured to run in FixedUpdate with a slow enough timestep, it frequently misses component removals.

I think one way to fix this could be to have a RemovedColliders resource that is cleared at the end of the PhysicsSchedule. To avoid removals being missed, the removed colliders would be added to the list in Last. This way, the collider removals would be detected correctly and only cleared after they have been handled in PhysicsSchedule. Would that be enough to address your issue?

Note that I believe all the collider removal workarounds like ColliderStorageMap should become redundant once Bevy has component lifecycle hooks (bevyengine/bevy#10756), since the component data would be accessible when responding to the removals.

@Architector4
Copy link
Author

Architector4 commented Nov 30, 2023

So if I understood correctly, the main issue is that some systems currently use RemovedComponents<T>, but if the engine is configured to run in FixedUpdate with a slow enough timestep, it frequently misses component removals.

Yes, basically.

To avoid removals being missed, the removed colliders would be added to the list in Last.

In my particular case, some removals happen in a custom schedule I designate that runs every single frame right before FixedUpdate, and I'd like the physics engine run in FixedUpdate to be able to respond to them immediately, if the FixedUpdate schedule runs right after that custom schedule in the same frame. This is the case that is handled perfectly by the engine right now, as it trivially detects the remove.

That doesn't sound like it would be handled by using a list that is only updated in Last, but I guess this could work if the current and proposed removal detections are used together.

...But then that might introduce difficulties, as both the current detection will respond to removals immediately, and they would also be added to the proposed removed colliders list and handled again on the next frame, which might wreck havoc in case some game logic adds the collider back to an entity in the very next frame, which would then be added and erroneously removed.

I assume that would be fixed by first handling the removals from the list, which would also handle the duplicate removals as a no-op, and only then add colliders.

That's suboptimal, but it might be a good workaround until the component lifecycle hooks are added.

Edit: or maybe not, the above logic could break the more general case of when an entity has had a collider added and then immediately removed, in the same frame, both right before the physics engine is run. Or maybe if a collider was added, then removed, then added back, or removed then added then removed. I'll be completely honest, I'm unsure how bevy_xpbd handles this even now, but I felt like this is also something worth noting.

Sorry, this has turned into a bit of a mind dump lol

@janhohenheim
Copy link
Contributor

janhohenheim commented Jul 17, 2024

Probably blocked by Take note of bevyengine/bevy#7836

Jondolf added a commit that referenced this issue Aug 11, 2024
# Objective

Closes #263 (the removal detection issue should also already be fixed thanks to hooks and observers)

So far, Avian has run in `PostUpdate` by default, but with a custom fixed timestep solution. This has given us more control over scheduling and allowed working around some historical issues with Bevy's own `FixedUpdate`, but nowadays Bevy's fixed timestep schedules are much more usable.

Having two different fixed timesteps is confusing, annoying to maintain, and duplicates a lot of API surface area. The scheduling in Avian 0.1 also has other serious issues:

- Physics slows down at lower frame rates, for example when the window is not focused.
- Physics can be clearly indeterministic when running in `PostUpdate`, even with the built-in fixed timestep.

For a native experience, Avian should be using Bevy's own fixed timestep and scheduling.

## Solution

Physics now runs in `FixedPostUpdate` by default. `TimestepMode` has been removed, and `Time<Physics>` no longer has a custom timestep. Instead, it follows the clock of the schedule where physics is run in. In `FixedPostUpdate`, physics uses `Time<Fixed>`, but if physics is instead configured to run in a schedule like `PostUpdate`, it will use `Time<Virtual>`.

Previously, the physics timestep could be configured like this:

```rust
app.insert_resource(Time::new_with(Physics::fixed_hz(60.0)));
```

In schedules with a fixed timestep, you even needed to use `fixed_once_hz`, which was rather confusing and footgunny:

```rust
app.insert_resource(Time::new_with(Physics::fixed_once_hz(60.0)));
```

Now, if you are running physics in `FixedPostUpdate`, you should simply configure `Time<Fixed>` directly:

```rust
app.insert_resource(Time::<Fixed>::from_hz(60.0)));
```

`Time<Physics>` still exists to allow people to configure the simulation speed, pause and unpause the simulation independently of the schedule's default clock, and set up their own custom scheduling for physics.

Running physics with Bevy's fixed timestep has also fixed the other issues mentioned earlier: physics no longer runs slower at lower frame rates, and behavior seems a lot more deterministic (at least without the `parallel` feature). More testing is required to determine if we have full cross-platform determinism though.

## Why `FixedPostUpdate` instead of `FixedUpdate`?

`FixedUpdate` is very often used for gameplay logic and various kinds of simulations. It is also commonly used for applying physics logic, like character movement, explosions, moving platforms, effects that apply forces/impulses, custom gravity, and so on.

For a lot of these use cases, it is important to run logic before physics, and if physics was run in `FixedUpdate`, systems would need to be ordered explicitly, which would not be a good experience. And if you didn't do that, you could get determinism issues caused by system ordering ambiguities, along with frame delay issues.

And as for `FixedPreUpdate`: if we ran physics *before* gameplay logic in `FixedUpdate`, movement and anything else that affects physics could have an additional delay of one or more frames.

I believe that using `FixedPostUpdate` is the sensible default, and it is also in line with engines like Unity and Godot, where internal physics is run near the end of the fixed update/process step. People can also always configure the ordering in their own applications if needed.

## Caveats

Bevy's fixed timestep is 64 Hz by default, unlike our old default of 60 Hz. This can lead to noticeable jitter on 60 Hz displays, as physics is sometimes run twice within a single frame. This can be partially worked around by configuring `Time<Fixed>`, but I am also implementing transform interpolation and extrapolation, which should make it possible to fix the issue properly by smoothing out the visual result.

Another change to the old scheduling is that physics no longer runs during the first frame. This is because Bevy's `FixedUpdate` and other fixed timestep schedules don't seem to run until the second update.

---

## Migration Guide

Previously, physics was run in `PostUpdate` with a custom fixed timestep by default. The primary purpose of the fixed timestep is to make behavior consistent and frame rate independent.

This custom scheduling logic has been removed, and physics now runs in Bevy's `FixedFixedUpdate` by default. This further unifies physics with Bevy's own APIs and simplifies scheduling. However, it also means that physics now runs before `Update`, unlike before.

For most users, no changes should be necessary, and systems that were running in `Update` can remain there. If you want to run systems at the same fixed timestep as physics, consider using `FixedUpdate`.

The `Time<Physics>` clock now automatically follows the clock used by the schedule that physics is run in. In `FixedPostUpdate` and other schedules with a fixed timestep, `Time<Fixed>` is used, but if physics is instead configured to run in a schedule like `PostUpdate`, it will use `Time<Virtual>`.

Previously, the physics timestep could be configured like this:

```rust
app.insert_resource(Time::new_with(Physics::fixed_hz(60.0)));
```

Now, if you are running physics in `FixedPostUpdate`, you should simply configure `Time<Fixed>` directly:

```rust
app.insert_resource(Time::<Fixed>::from_hz(60.0)));
```

The following types and methods have also been removed as a part of this rework:

- `TimestepMode`
- `Physics::from_timestep`
- `Physics::fixed_hz`
- `Physics::fixed_once_hz`
- `Physics::variable`
- `Time::<Physics>::from_timestep`
- `Time::<Physics>::timestep_mode`
- `Time::<Physics>::timestep_mode_mut`
- `Time::<Physics>::set_timestep_mode`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scheduling Relates to scheduling or system sets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants