Z-order of tile/layer affects collisions

[stevepryde]

I yanked 0.3.2 because of an issue that was caused by the z-order changes.

The colliders also use the transform of the tile, and that is nested below the transform of the layer, which is nested below the map itself.

We probably need a better approach for this. For now I will revert the z-offset change, with a comment. And leave this issue open for a more comprehensive fix. It's likely that we need to somehow make the z-offset of the rendering layer independent of the physics. I'm not sure how to achieve that yet.

The goals of this ticket are:

1. Make tilemap layers respect the z coordinate for rendering
2. Make the physics still work correctly. Part of this is understanding what rapier does with the z-coordinate even in 2D. This is confusing to me. It doesn't seem to be documented and may be worth a GH issue.

[adrien-bon]

I tried to investigate this issue by creating a dedicated example on the dev branch. See the new controller_rapier example and associated map file.

Unfortunately, I did not reproduce the issue: when I re-apply the z-order changes, everything works properly. My RigidBody::KinematicPositionBased is still affected by all the colliders from my map, even when they are on different layers. Maybe I am missing something ? Can you give more information about the specific use case you encountered ?

[stevepryde]

I think that character controller only works if it is the only thing moving. I encountered the issue in my "dizzy ducklings" game, where the tilemap is actually rotating. It also uses a ball collider which might be treated as a sphere in rapier, even in 2D. I'm not sure.

One easy way to reproduce it is to checkout dizzy ducklings: https://github.com/stevepryde/dizzy-ducklings and then just change the one line to uncomment the z-order change within bevy_ecs_tiled. You will see everything starting to glitch through walls slowly.

It's worth pointing out this horrible hack I used to make the physics work: https://github.com/stevepryde/dizzy-ducklings/blob/main/src/game/movement.rs#L202

Without this, the character would slowly glitch through walls even without the z-order issue.

I suspect a simple way to see this in your simple physics example is to make the tilemap rotate - and also use a ball collider:

Change LevelMarker to whatever component is at the root of the tilemap.

fn rotate_world(time: Res<Time>, mut query: Query<&mut Transform, With<LevelMarker>>) {
    for mut transform in query.iter_mut() {
        transform.rotate(Quat::from_rotation_z(f32::to_radians(
            5. * time.delta_seconds(),
        )));
    }
}

[adrien-bon]

In my example, without changing anything, if I rotate the tilemap using Q/E keys, colliders from the tiles do not trigger collisions on my RigidBody when it goes through them. Collisions are triggered when I actually move the RigidBody using arrow keys. I observe this behaviour even without the z-order change.

Do you expect collisions to be triggered when rotating the tilemap ? Regarding this, I believe we have the same behaviour with and without the z-order change.

I believe this is an issue with the way we use Rapier or Rapier itself. Maybe it's because we move the colliders / tiles parent (ie. the tilemap) and not the collider entity itself ? (Hence, only the GlobalTransform component is changed and not the Transform component)

I tried to add your read_character_controller_collisions system to work-around this but it does not change initial statement: collisions are only triggered once I move the RigidBody. However, it prevents the RigidBody to get stuck inside a collider.

[stevepryde]

I ran into this issue with my game jam game. Commenting that one line allows the game to work. Uncommenting it breaks the physics.

If you have a better way to manage the physics, feel free to change it. The reason for the colliders being nested is so that they can follow the tilemap parent. The transform propagates from parent to child so that the colliders will always be positioned correctly w.r.t. the parent tile and tilemap. I'm not sure how else you would do this. If rapier isn't respecting changes to the GlobalTransform then I'd consider that a bug tbh. The Transform is always local and might not be useful at all. The GlobalTransform is really the only one that can accurately tell you if two things are in proximity to one another.

Personally I would expect a physics lib to work the way Godot works - which means if you have colliders, kinematic or otherwise, they should never intersect. Period. But it seems that many physics libs aren't set up that way and maybe the behaviour I was after just isn't how rapier is intended to work.

I'll leave it up to your judgement as to how it should work. I don't really have a strong opinion on it.

[adrien-bon]

First time using Rapier and indeed it's a bit confusing :)

I did not mean to critizice the way it's done: just trying to understand. I think the colliders being nested on the tiles / tilemap is indeed the way to go. My point was maybe there is some kind of configuration to apply on the colliders for them to work properly. I tried to add RigidBody on the tiles and tweak some components but it dos not improve the situation.

Anyway, I think I found what I missed in my example to reproduce the issue. You apply a gravity which refresh the player position every frame. I did not do that. And indeed, I need the read_character_controller_collisions system to be able to avoid to get my RigidBody stuck in tiles collider.

Now that I added both the gravity and the system, I reproduce the issue with my simple example. I still do not reproduce the issue with your game. I though the issue was caused when there was multiple layers and I only see a single layer in yours. Missing something again ?

However, I think the issue could be caused by the way the read_character_controller_collisions system computes the distance between colliders and RigidBody. It uses Vec3, so implicetely z coordinates. With the z-order change, it could results in anormaly high distances.

I tried to change it this way, and it seems to behave better:

diff --git a/src/game/movement.rs b/src/game/movement.rs
index 71a8191..a9d177f 100644
--- a/src/game/movement.rs
+++ b/src/game/movement.rs
@@ -217,10 +217,11 @@ fn read_character_controller_collisions(
                     // Collect Duckling.
                     commands.trigger(DucklingCollected(collision.entity));
                 } else {
-                    let delta = global_transform.translation() - collider_tf.translation();
+                    let delta = Vec2::new(global_transform.translation().x, global_transform.translation().y) - Vec2::new(collider_tf.translation().x, collider_tf.translation().y);
                     let distance = delta.length();
                     if distance < 64.0 {
                         let direction = delta.normalize();
+                        let direction = Vec3::new(direction.x, direction.y, 0.);
                         let movement = direction * (64.0 - distance);
                         transform.translation += movement * time.delta_seconds();
                     }

Could you give it a try ?

Thanks!

[adrien-bon]

After more testings, I observe that if I add a RigidBody::KinematicPositionBased to my tiles (in src/physics/rapier/shapes.rs) + a RigidBody::Dynamic for my player pawn (instead of a RigidBody::KinematicPositionBased, which mean rework a bit the movement system) I have much better results: player does not get stuck inside tiles collider anymore.

[stevepryde]

First time using Rapier and indeed it's a bit confusing :)

I did not mean to critizice the way it's done: just trying to understand. I think the colliders being nested on the tiles / tilemap is indeed the way to go. My point was maybe there is some kind of configuration to apply on the colliders for them to work properly. I tried to add RigidBody on the tiles and tweak some components but it dos not improve the situation.

That's a good point. I didn't think of that, but you're right - if they are moving then they should really have a rigid body attached. I read the documentation many times but it wasn't clear on what was needed for collisions to work. And I think the fact that collisions "kind of" worked made me assume this was enough.

Now that I added both the gravity and the system, I reproduce the issue with my simple example. I still do not reproduce the issue with your game. I though the issue was caused when there was multiple layers and I only see a single layer in yours. Missing something again ?

Now I'm second guessing and maybe it was something else in that 0.3.2 release that broke it. I haven't tried with 0.3.3. I will try shortly.

However, I think the issue could be caused by the way the read_character_controller_collisions system computes the distance between colliders and RigidBody. It uses Vec3, so implicetely z coordinates. With the z-order change, it could results in anormaly high distances.

Aha, this could very well be the cause! I didn't think of this.

I tried to change it this way, and it seems to behave better:

diff --git a/src/game/movement.rs b/src/game/movement.rs
index 71a8191..a9d177f 100644
--- a/src/game/movement.rs
+++ b/src/game/movement.rs
@@ -217,10 +217,11 @@ fn read_character_controller_collisions(
                     // Collect Duckling.
                     commands.trigger(DucklingCollected(collision.entity));
                 } else {
-                    let delta = global_transform.translation() - collider_tf.translation();
+                    let delta = Vec2::new(global_transform.translation().x, global_transform.translation().y) - Vec2::new(collider_tf.translation().x, collider_tf.translation().y);
                     let distance = delta.length();
                     if distance < 64.0 {
                         let direction = delta.normalize();
+                        let direction = Vec3::new(direction.x, direction.y, 0.);
                         let movement = direction * (64.0 - distance);
                         transform.translation += movement * time.delta_seconds();
                     }

Could you give it a try ?

Will do. I'll let you know what I find.

[stevepryde]

In general tilemaps shouldn't move though. Is there a static body that should be applied instead? Or is the collider sufficient if the tilemap isn't moving. It might be good to have a "moving platform" example because that's a lot more practical than my rotating tilemap :smile:

[adrien-bon]

You are right, it won't move most of the time, but since we can do it... Let's try to make it work properly :)

From the Rapier documentation I believe our tiles and objects should either be RigidBodyType::Fixed or RigidBodyType::KinematicPositionBased.

I tested both and the later seems to perform way better when moving the tilemap, which is confirmed by Rapier documentation which says we should use RigidBody::KinematicPositionBased for moving platforms. I also tested how it behaves if we have overlapping tiles and it seems to be OK as well. I don't know what it implies in terms of performances though.

I think we should provide a default behaviour which works "most of the time" and let the user tweak it if needed. After our investigations, I believe the default should be to use a collider + RigidBodyType::KinematicPositionBased. Though, I'm open to suggestion as I don't fully grasp the differences between both RigidBody.

If the user does not want that, we should give him the opportunity to change it, for instance through custom properties callbacks / observers. I'll open a PR with this change / fix and keep this in my head when I'll do the custom properties PR.

In your game, I think that you can add to your player a RigidBodyType::Dynamic component and it should behave correctly with the tiles. It will require to update a bit the logic for moving the player but it should hopefully work better overall. I believe it is recommended by Rapier to use this RigidBody for player controlled objects

[stevepryde]

You are right, it won't move most of the time, but since we can do it... Let's try to make it work properly :)

From the Rapier documentation I believe our tiles and objects should either be RigidBodyType::Fixed or RigidBodyType::KinematicPositionBased.

Might be worth making this configurable?

I tested both and the later seems to perform way better when moving the tilemap, which is confirmed by Rapier documentation which says we should use RigidBody::KinematicPositionBased for moving platforms. I also tested how it behaves if we have overlapping tiles and it seems to be OK as well. I don't know what it implies in terms of performances though.

I think we should provide a default behaviour which works "most of the time" and let the user tweak it if needed. After our investigations, I believe the default should be to use a collider + RigidBodyType::KinematicPositionBased. Though, I'm open to suggestion as I don't fully grasp the differences between both RigidBody.

I agree with this approach.

If the user does not want that, we should give him the opportunity to change it, for instance through custom properties callbacks / observers. I'll open a PR with this change / fix and keep this in my head when I'll do the custom properties PR.

Agreed.

In your game, I think that you can add to your player a RigidBodyType::Dynamic component and it should behave correctly with the tiles. It will require to update a bit the logic for moving the player but it should hopefully work better overall. I believe it is recommended by Rapier to use this RigidBody for player controlled objects

It feels wrong to have both a dynamic and kinematic rigid body though. The player already uses the kinematic controller which I assume includes a kinematic body already.

[stevepryde]

However, I think the issue could be caused by the way the read_character_controller_collisions system computes the distance between colliders and RigidBody. It uses Vec3, so implicetely z coordinates. With the z-order change, it could results in anormaly high distances.

Aha, this could very well be the cause! I didn't think of this.

I tried to change it this way, and it seems to behave better:

diff --git a/src/game/movement.rs b/src/game/movement.rs
index 71a8191..a9d177f 100644
--- a/src/game/movement.rs
+++ b/src/game/movement.rs
@@ -217,10 +217,11 @@ fn read_character_controller_collisions(
                     // Collect Duckling.
                     commands.trigger(DucklingCollected(collision.entity));
                 } else {
-                    let delta = global_transform.translation() - collider_tf.translation();
+                    let delta = Vec2::new(global_transform.translation().x, global_transform.translation().y) - Vec2::new(collider_tf.translation().x, collider_tf.translation().y);
                     let distance = delta.length();
                     if distance < 64.0 {
                         let direction = delta.normalize();
+                        let direction = Vec3::new(direction.x, direction.y, 0.);
                         let movement = direction * (64.0 - distance);
                         transform.translation += movement * time.delta_seconds();
                     }

Could you give it a try ?

Will do. I'll let you know what I find.

This was indeed the cause :man_facepalming: so this issue can be closed (and the offset_z change reinstated).

Thanks so much for your help with this.

[adrien-bon]

You are right, it won't move most of the time, but since we can do it... Let's try to make it work properly :) From the Rapier documentation I believe our tiles and objects should either be RigidBodyType::Fixed or RigidBodyType::KinematicPositionBased.

Might be worth making this configurable?

Yeah, you are right. I'll try to think about something.

I tested both and the later seems to perform way better when moving the tilemap, which is confirmed by Rapier documentation which says we should use RigidBody::KinematicPositionBased for moving platforms. I also tested how it behaves if we have overlapping tiles and it seems to be OK as well. I don't know what it implies in terms of performances though. I think we should provide a default behaviour which works "most of the time" and let the user tweak it if needed. After our investigations, I believe the default should be to use a collider + RigidBodyType::KinematicPositionBased. Though, I'm open to suggestion as I don't fully grasp the differences between both RigidBody.

I agree with this approach.

If the user does not want that, we should give him the opportunity to change it, for instance through custom properties callbacks / observers. I'll open a PR with this change / fix and keep this in my head when I'll do the custom properties PR.

Agreed.

In your game, I think that you can add to your player a RigidBodyType::Dynamic component and it should behave correctly with the tiles. It will require to update a bit the logic for moving the player but it should hopefully work better overall. I believe it is recommended by Rapier to use this RigidBody for player controlled objects

It feels wrong to have both a dynamic and kinematic rigid body though. The player already uses the kinematic controller which I assume includes a kinematic body already.

Again, you are right :) I don't really know how Rapier is supposed to work and I don't understand why they use a RigidBodyType::KinematicPositionBased in the documentation about character controller and a RigidBodyType::Dynamic in the example... I'll try to make it works for both cases.

[adrien-bon]

Fixed by #9