Changed body_test_motion to discard by angle rather than depth

[mihe]

Fixes #626.
Fixes #643.

This applies the change to PhysicsServer3D.body_test_motion that was suggested by @yosoyfreeman in #812, which effectively reverts the hack introduced by #559 and replaces its depth-based contact discard with an angle-based one instead.

Note that this also impacts PhysicsBody3D.move_and_collide as well as PhysicsBody3D.test_move and of course CharacterBody3D.move_and_slide.

This will possibly lead to a regression in ghost collisions for some scenarios, like the one seen in #529, but the downsides of #559 have proved to be too great to keep it around.

[yosoyfreeman]

Thank you a lot for your work Mihe!

[mihe]

It turned out that this did not in fact fix #626, since apply_floor_snap still runs when walking across a perfectly parallel floor, which I probably should have caught before pushing this. It's not really an argument for reverting this though, since it was arguably worse before, but it's still somewhat unfortunate.

However, I am slightly worried over the fact that this change results in zero slide collisions when a character is walking over a perfectly parallel floor like that. That's obviously to be expected given that this change discards any such planes, and I would probably argue that such a movement shouldn't result in any slide collisions either, but I've seen a couple of examples where people rely on these slide collisions to gather information about the surface they're walking on, which will now be more difficult.

It would have been nice if move_and_slide stored the wall/floor contacts explicitly, rather than force people to rely on the slide collisions (where they typically check the normals themselves), in which case apply_floor_snap could have stored its collision results there instead of them being discarded completely.

EDIT: You know what, when testing the previous hack (i.e. #559) I'm seeing largely the same behavior of the slide collisions there, so maybe this wasn't actually a regression after all. I could have sworn that worked similar to Godot Physics at some point, but maybe not.

[yosoyfreeman]

Hi Mihe!

It turned out that this did not in fact fix #626, since apply_floor_snap still runs when walking across a perfectly parallel floor, which I probably should have caught before pushing this. It's not really an argument for reverting this though, since it was arguably worse before, but it's still somewhat unfortunate.

It is unfortunate indeed, but to be expected. Move and slide is not capable of knowing is touching a floor unless you directly collide against one. This means that, even if it works perfectly, a call to apply_floor_snap will be done, as is the only way the system has to retrieve floor info. Its the same problem that appears while moving parallel to a wall, it will report nothing.

However, I am slightly worried over the fact that this change results in zero slide collisions when a character is walking over a perfectly parallel floor like that. That's obviously to be expected given that this change discards any such planes, and I would probably argue that such a movement shouldn't result in any slide collisions either, but I've seen a couple of examples where people rely on these slide collisions to gather information about the surface they're walking on, which will now be more difficult.

To fix this issue with walls, once i touched one i add a minimal "force" or velocity towards it. This is enough to make the character collide every frame with it without altering user input too much, so i can gather the data without skips.

Could this work in this scenarios? I know is a little of a hack, but seems less of a hack than what move and slide was actually doing. Maybe a small gravity when on floor would be enough for this cases.

It would have been nice if move_and_slide stored the wall/floor contacts explicitly, rather than force people to rely on the slide collisions (where they typically check the normals themselves), in which case apply_floor_snap could have stored its collision results there instead of them being discarded completely.

The problem is that move and slide does not have any way of knowing nothing that is not the first contact on the direction of the movement or apply floor snap. What is reported by is on wall or is on floor is basically what is reported in one of the slide collisions as the last floor or wall hit. They are just a quicker way of looking for it. I learning my way to c++, so i could start trying to implement some hacks... But they would be that, hacks. Because of course we would want to rewrite it as soon as we would have better sweeping, normal data etc.

[mihe]

It is unfortunate indeed, but to be expected. Move and slide is not capable of knowing is touching a floor unless you directly collide against one. This means that, even if it works perfectly, a call to apply_floor_snap will be done, as is the only way the system has to retrieve floor info. Its the same problem that appears while moving parallel to a wall, it will report nothing.

This is however not how Godot Physics behaves, which seemingly never runs apply_floor_snap when just moving along a level surface like that, which bothers me quite a bit. I've spent a lot of time comparing the two implementations trying to figure out why that is, but have not been able to track down the exact cause.

I've always assumed it has something to do with the type of minimum contact depths that Godot Physics uses, where it will deliberately undershoot the recovery step, but if I try to replicate the same thing on my end I get a prohibitive amount of ghost collisions, to the point where it's almost impossible to walk up an inclined surface, hence why I took the opposite approach in #559 where I undershoot the final collision check step instead.

Could this work in this scenarios? I know is a little of a hack, but seems less of a hack than what move and slide was actually doing. Maybe a small gravity when on floor would be enough for this cases.

Anything like this would presumably need to happen in move_and_slide itself. body_test_motion is just a small piece of it all, and has no idea about any sort of orientation of the thing being moved. In fact, since it powers move_and_collide as well it doesn't even necessarily have to involve a CharacterBody3D.

The problem is that move and slide does not have any way of knowing nothing that is not the first contact on the direction of the movement or apply floor snap. What is reported by is on wall or is on floor is basically what is reported in one of the slide collisions as the last floor or wall hit.

Yes, I know, but what I'm saying is that people currently rely on slide collisions in a way where they iterate through them all, check the normal of them, and then decide based on the angle of that normal whether that collision is a wall or a floor, and then use those accordingly. This is obviously a bit awkward/redundant since move_and_slide has already done the work (in _set_collision_direction) to decide which slide collisions map to what surface.

This is made worse by the fact that apply_floor_snap does not actually store its collision results as additional slide collisions, which is why we end up with zero slide collisions when walking along a level surface in Godot Jolt, despite is_on_floor being true.

But if move_and_slide actually stored its collisions as floor_collisions and wall_collisions (in addition to the slide collisions) or something along those lines, then there's presumably no real reason why apply_floor_snap couldn't just add its collision results to floor_collisions and people could rely on those instead of inferring it from the slide collisions.

That obviously does nothing to fix the issue of wall collisions in Godot Jolt, but it would be an improvement at least.

[yosoyfreeman]

Hi Mihe, i got some time free today and checked the code with what you told me in mind.

I agree that adding floor snap results as slide collision can be a solution. I could try to do that (Still learning c++ and godot codebase). However, i do think the reason they are not stored is because they are technically not slide collisions, in the sense that they do not trigger the sliding behavior or alter the velocity in any way. However, the responsibilities of checking the environment and reacting to it are already mixed. Since no breaking changes can be done by now, this would allow to at least provide more information to the user. It does not require additional parameters or API either, unlike adding a new floor, wall and ceiling categories. As i see it, calling a floor collision an slide collision is a minor hack compared to some of the other ones that are already there.

If you think this could improve things, i can look into it and try to do a PR. As i said, is probably the only one with some chance to get accepted right now and no breaking changes are required.

[mihe]

As i said, is probably the only one with some chance to get accepted right now and no breaking changes are required.

What do you mean by "get accepted right now"? As in Godot 4.3-beta1 or Godot 4 in general?

I would perhaps argue that adding more collisions to the list of slide collisions (that may or may not belong there) is an actual breaking change (albeit a small one) whereas adding new categories is not. I have a hard time imagining either one making it into Godot 4.3 though, given that we've entered the beta phase already, and Godot Physics doesn't seem to need the change.

If you're just talking about Godot 4 in general, I wouldn't shy away from adding to the API necessarily. It seems to me like adding those new lists/categories would be fairly simple to provide a reason for, seeing as how users work around it by checking the normal when using either physics engine.

I am however realizing now that even if these separate categories were to be added, we would have a hard time categorizing get_last_slide_collision(), since that particular collision (which is probably important to some people) may or may not be in any of these new lists. Adding some kind of is_floor() and is_wall() to KinematicCollision3D would have been a nice option instead, but unfortunately it's shared with move_and_collide, which has no notion of walls and floors, so that doesn't seem right.