Update to Bevy `0.10`

[geieredgar]

Currently blocked by

• [x] https://github.com/StarArawn/bevy_ecs_tilemap/pull/390
• [x] https://github.com/dimforge/bevy_rapier/pull/324
• [x] https://github.com/dimforge/nalgebra/pull/1213
• [x] Fix system race conditions with bevy_rapier in platformer example

BEGIN_COMMIT_OVERRIDE feat!: upgrade to bevy 0.10 (#168)

BREAKING CHANGE: In addition to updating to bevy 0.10, users may need define order between LdtkSystemSet::ProcessApi and other 3rd party system sets, like rapier. END_COMMIT_OVERRIDE

[geieredgar]

I've addressed all of the requested changes. Unfortunately, I've encountered what I believe is some race condition between systems of bevy_ecs_ldtk and bevy_rapier in the platformer example. I've seen them before, but hoped this was an issue with the then WIP bevy_rapier update. I will investigate them later today.

[geieredgar]

Here are my investigation results about the race condition in the platformer example:

Without any explicit ordering between LdtkSystemSet::ProcessApi and PhysicsSet::SyncBackend, the platformer example quite often crashes at startup with the following error:

thread 'main' panicked at 'error[B0003]: Could not insert a bundle (of type `bevy_rapier2d::dynamics::rigid_body::RapierRigidBodyHandle`) for entity 3814v0 because it doesn't exist in this World.

The system that issues the command to insert the bundle is init_rigid_bodies which is part of the PhysicsSet::SyncBackend set.

Even if the example does not crash, the player is sometimes spawned at the wrong position, see

Ordering PhysicsSet::SyncBackend before LdtkSystemSet::ProcessApi didn't crash anymore, but the wrong spawn position did also happen occasionally (like about 1 of 30 times). Sometimes the player didn't spawn at all (maybe it was only invisible/occluded by the level), see

Therefore, I changed the platformer example to explicitly schedule PhysicsSet::SyncBackend after LdtkSystemSet::ProcessApi. With this I didn't encounter any crashes or wrong spawn positions, but this doesn't prove it cannot happen.

Side note

As a side note, I've run the platformer example in release mode and looked at the first few frames using renderdoc. The frames looked the same between all configurations but there was a difference to the current main branch: In the main branch, the first frame was completely gray, the second completely blue. With the updates, the first three frames looked like this:

Frame 1: Frame 2: Frame 3:

In main "Frame 2" was often the third frame, sometimes "Frame 1" was the third frame, followed by "Frame 2".

I believe that this is related to #140.

[Trouv]

I see, it makes sense that we'd have to be explicit in the ordering of our systems vs rapiers now. I'm guessing with this explicit scheduling that the issue is truly gone, but it's hard to be sure. It's a bit of a shame to require users to do that - but I'm also okay with it for now. I think we could keep 0.6 focused on faithfully recreating the old schedule, and then later make more serious changes to the schedule with this issue and #140 in mind.

[geieredgar]

Yeah, I believe this is something bevy_ecs's ambiguity checker should be able to catch, but I think it is currently disabled by default because of the many false positives it reports. Improvements there might change that, so the user is at least warned about the race conditions. But this still leaves the question open which systems should run first, so I think it is important to document that somewhere.

Another solution might be to add a bevy_rapier feature that adds the scheduling constraint automatically in LdtkPlugin::build.

[Trouv]

Another solution might be to add a bevy_rapier feature that adds the scheduling constraint automatically in LdtkPlugin::build.

Hmm, I think this could be cool if we had more integration with bevy_rapier as part of such a feature. For example, more LdtkEntity attribute macros for collisions, or something. My gut reaction to this is that users will see that feature listed and expect it to be more in depth than one scheduling constraint.

I think making users responsible for the scheduling constraint is fine for now but I definitely want to announce it in the release and have it documented somewhere like you've already done. I'm interested in reworking the scheduling a bit after this release anyway, so the requirement may disappear at that point