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 one-way collision for moving objects #36280

Closed
wants to merge 3 commits into from

Conversation

bemyak
Copy link
Contributor

@bemyak bemyak commented Feb 16, 2020

There are several bugs with one-way collisions:
Fixes #25732
Fixes #27593
Fixes #28794
Fixes #35776
Fixes #34242
Fixes #34215
Fixes #32138
Fixes #27689
Fixes #27652
Fixes #7920
Fixes #9332
Bugsquad edit: Fixes #40006
(probably few more, too lazy to find all of them)

Removing this check seems to fix them or at least occur less frequently. I can't come up with a reason why that check was there before.

There are still problems when a body is being pushed through one-way collision, I'm trying to figure out how to fix it.

UPDATE: Some (lots of, actually) debugging lead me to several issues in servers/physics_2d/space_2d_sw.cpp:

  1. We're setting invalid_by_dir at the beginning of do...while loop, no reason to set it again and again
  2. When we are calculating cbk.valid_depth we need take the motion of current body into account as well as platform's motion
  3. When applying recover_motion we used to multiply it by magic 0.4. Thus the entire do...while loop could end only when run out of recover_attempts.

UPDATE2: Looks like I fixed all one-way collision bugs except mine 😄
So, the reason to "weaken" this condition in servers/physics_2d/physics_2d_server_sw.cpp is how TileMaps work. If your body is moving along TileMap line, there will be a moment when it'll step on a new tile. In that case it's movement will not match the condition and it will fall through that new tile.

UPDATE3: The last commit also fixes #28895
UPDATE4: I uploaded 3.2 builds with this patch included here so it'll be easier to test it: https://pepya.tk/nextcloud/index.php/s/3Bqbfe7EMsrE9px

Before merging this, I highly recommend merging #38471 first, it fixes most of the bugs in a proper and clean way.

@bemyak bemyak requested a review from akien-mga February 18, 2020 15:07
@bemyak bemyak force-pushed the master branch 8 times, most recently from a3e6794 to 26cd124 Compare February 22, 2020 18:19
@akien-mga akien-mga added this to the 4.0 milestone Feb 23, 2020
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 24, 2020
@bemyak bemyak force-pushed the master branch 3 times, most recently from 2b83b5f to 4cde6ed Compare February 24, 2020 14:09
@bemyak bemyak requested review from reduz and removed request for akien-mga February 24, 2020 14:13
@fabriceci
Copy link
Contributor

@bemyak for #35550, the expected behaviour is: the body should not move (and follow the platform). Is one of the setups works with this patch?

@bemyak
Copy link
Contributor Author

bemyak commented Mar 9, 2020

@fabriceci,
It looks like this now:
Peek 2020-03-09 16-31
(sorry for low fps, my setup is not quiet good for screen recording :) )

@fabriceci
Copy link
Contributor

fabriceci commented Mar 9, 2020

@bemyak Unfortunately that does not fix the issue: the green block should follow the platform (basic moving platform), I expected only the first or the last row to work properly (setup 1 & 4). The first not working (the block not follow the platform), the last, I suppose like in the current version, follow but you would not be able to jump when the platform move up, and the horizontal speed when moving will be cumulated.

Anyway thanks for your works!

@bemyak
Copy link
Contributor Author

bemyak commented Mar 9, 2020

@fabriceci , I'm not an expert, but I don't think it is supported configuration.
In 1st and 4th setup, the platform's KinematicBody2D complains (shows ⚠️ sign next to it) about not having shape attached, such as CollisionShape2D or CollisionPligon2D.

Setup 2 and 3 looks good, since collision area is part of the TIleSet.

Usually, Kinematic Bodies are used to gradually control how body moves, despite physics. If you want to implement proper "jump on moving platform", you'll need to get platform's velocity and apply it when calculating velocity for Kinematic Body.

If you want to have proper physics, maybe you consider switching to RigidBody2D instead of KinematicBody2d. Here is a great video that explains the difference and how to use them both.

As for the vertical movement, it works as expected, since it is a part of "bodies should not stuck in each other" logic. Which was a bit broken, and this PR aims to fix it :)
So, at least this point from your original issue seems to be fixed:

  • vertical movement : the body "enter" in the platform

@fabriceci
Copy link
Contributor

you'll need to get platform's velocity and apply it when calculating velocity for Kinematic Body.

That's not the behavior with standard shapes, but anyway it's out of purpose. (but thanks :) )

As for the vertical movement, it works as expected, since it is a part of "bodies should not stuck in each other" logic. Which was a bit broken, and this PR aims to fix it :)

Great news and maybe even better, maybe you are correcting another of my issues : #34242

@bemyak
Copy link
Contributor Author

bemyak commented Mar 10, 2020

Yes, it fixes that one also. Added it to the description and removed #35550 from it.

@kutetapolu
Copy link

Will this be accepted for 4.0? Pretty please, @reduz @akien-mga

@ZingBlue
Copy link

This would fix so many issues with my game. Some of which make it hard to justify shipping the game (it's physics based, so it really needs these bugs fixed).

@ZingBlue
Copy link

Is there anything I can do to help increase the likelyhood this will be merged? I'm a Python programmer but I could probably catch on to C++ quickly.

@AttackButton
Copy link
Contributor

AttackButton commented Jul 1, 2020

UPDATE4: I uploaded 3.2 builds with this patch included here so it'll be easier to test it: https://pepya.tk/nextcloud/index.php/s/3Bqbfe7EMsrE9px

Wow! It solved one of my issues #40006. Thank you man. I will wait for the merge to test it more.

I tested the other issue, but it didn't fix it. I even tested the minimal project of this issue #34345 of securas that is identical to mine and it also didn't work.

@ZingBlue
Copy link

ZingBlue commented Jul 3, 2020

I tested the build you included, I used my game to test it. The two issues I originally had #27593 and #34215 were 100% fixed. My game played 10x better than before. I encountered no new issues. Thank you for fixing these issues.

@AttackButton
Copy link
Contributor

AttackButton commented Jul 8, 2020

UPDATE4: I uploaded 3.2 builds with this patch included here so it'll be easier to test it: https://pepya.tk/nextcloud/index.php/s/3Bqbfe7EMsrE9px

Wow! It solved one of my issues #40006. Thank you man. I will wait for the merge to test it more.

I tested the other issue, but it didn't fix it. I even tested the minimal project of this issue #34345 of securas that is identical to mine and it also didn't work.

Ok. this fixed it very well, makes the CollisionShape2D act like a solid instead of a gelatin xD. However, I notice that the KinematicBody2D became a little easier to be pushed when it shouldn't be, something that didn't happen before. Maybe it's a new bug related to #34345?

@ShraddhaDevaiya
Copy link

hello @akien-mga and @reduz , can you please review this PR? so that they can make necessary changes if needed.

@bemyak
Copy link
Contributor Author

bemyak commented Jul 23, 2020

I noticed the conflict and tried to resolved it. It turned out that currently master branch (without my commits) looks like this:
Peek 2020-07-23 22-21
(this is a test project from here)

I guess I'll close this PR and open a new one for 3.2 branch only, I don't think I'll be able to fix the current state.

@bemyak
Copy link
Contributor Author

bemyak commented Jul 23, 2020

Okay, I created #40645 to make it easier to maintain both 3.2 and 4.0 versions.

I also updated builds available in the first post to the latest 3.2. Enjoy =)

@epoblaguev
Copy link

No solution to contribute, but maybe one more datapoint for #32138.

Example: https://imgur.com/a/otCrzpx

It looks like the issue occurs when the ball (RigidBody2d) collides with tiles, but not with static bodies.

Continuous collision settings don't seem to have an effect on the issue.

@jtaart
Copy link

jtaart commented Jul 28, 2020

No solution to contribute, but maybe one more datapoint for #32138.

Example: https://imgur.com/a/otCrzpx

It looks like the issue occurs when the ball (RigidBody2d) collides with tiles, but not with static bodies.

Continuous collision settings don't seem to have an effect on the issue.

Are you sure? Turning off continuous cd fixes it for me.

gif

@bemyak
Copy link
Contributor Author

bemyak commented Oct 2, 2020

Closing this in favor of #38471

@bemyak bemyak closed this Oct 2, 2020
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Oct 2, 2020
@Calinou Calinou removed this from the 4.0 milestone Oct 2, 2020
@madmiraal
Copy link
Contributor

#38471 is made against the 3.2 branch not the master branch.

@bemyak
Copy link
Contributor Author

bemyak commented Oct 3, 2020

@madmiraal , this doesn't actually work on the master branch, see my comment above: #36280 (comment)
And since the issue affects 3.2, I think it should first be merged into 3.2 and then cherry-picked into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment