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

When a KinematicBody is stuck, set the unsafe proportion to the minimum instead of zero. #38529

Closed
wants to merge 1 commit into from

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented May 7, 2020

When a KinematicBody is stuck, setting the unsafe proportion to the minimum instead of zero, ensures that collision information is extracted even if the penetration causing the body to be stuck is less than the test_motion_min_contact_depth. More importantly, this ensures that a collision is reported and doesn't allow the body to tunnel. Furthermore, it also ensures that the collision information is extracted from the CollisionShape in the direction of motion not the CollisionShape that happens to be closest or first.

Note: The minimum is 1/2^8 because there are (an arbitrary) eight steps in the binary search.

Fixes #37798.

PS See my comment in #37798 for a more in depth analysis.

@@ -989,7 +989,7 @@ bool Space2DSW::test_body_motion(Body2DSW *p_body, const Transform2D &p_from, co
if (stuck) {

safe = 0;
unsafe = 0;
unsafe = 1.0 / (1 << 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should probably be a comment above this line to explain why this is done exactly. (Feel free to use GH-38529 as a reference to this pull request.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

// Set unsafe to the minimum after eight steps = 1/2^8.

Copy link
Member

@Calinou Calinou May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think code comments like these should be more about the why than the how. If I read that comment, I have no idea why it's being done this way 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about why the unsafe is being set to the minimum, why the minimum is 1/2^8 or why 1/(1<<8) = 1/2^8?

Copy link
Member

@Calinou Calinou May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this?

// When a KinematicBody is stuck, setting the unsafe proportion to the
// minimum instead of zero ensures that collision information is extracted
// even if the penetration causing the body to be stuck is less than the
// `test_motion_min_contact_depth`. See GH-38529 for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm. I think that is excessive. Code shouldn't document why a change was made. It makes it look like setting the value to 1/2^8 is unusual, which it isn't. It is simply the value that unsafe would have been set to if this loop had been allowed to run:

for (int k = 0; k < 8; k++) { //steps should be customizable..

@@ -900,7 +900,7 @@ bool Space3DSW::test_body_motion(Body3DSW *p_body, const Transform &p_from, cons
if (stuck) {

safe = 0;
unsafe = 0;
unsafe = 1.0 / (1 << 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

instead of zero. This ensures that collision information is extracted,
in the direction of motion and ensures that a collision is reported.
@raphaklaus
Copy link

Hi guys, just want to know if this one is going to be merged.

@capnm
Copy link
Contributor

capnm commented Sep 29, 2020

Just a few pointers for good git commit messages, that's a good place for an extended 'why'
that also get directly inline displayed in 'smart' editors.
E.g.: https://www.theserverside.com/video/Follow-these-git-commit-message-guidelines

  • Limit the subject line to 50 characters.
  • Put a blank line between the subject line and the body.
  • Wrap the body at 72 characters.
    (The PR comment is I think a good body candidate.)

@Calinou Calinou added this to the 4.0 milestone Jan 5, 2021
@YuriSizov YuriSizov requested a review from a team August 24, 2021 23:11
@akien-mga
Copy link
Member

Superseded by #52953.

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

Successfully merging this pull request may close these issues.

KinematicBody2D passing through tile when there is one cell between parallel tiles
5 participants