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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion servers/physics_2d/space_2d_sw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,8 @@ bool Space2DSW::test_body_motion(Body2DSW *p_body, const Transform2D &p_from, co
if (stuck) {

safe = 0;
unsafe = 0;
// Set unsafe to the minimum after eight steps = 1/2^8.
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 •
edited
Loading

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 •
edited
Loading

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:

godot/servers/physics_2d/space_2d_sw.cpp

Line 948 in e253451

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

best_shape = body_shape_idx; //sadly it's the best
break;
}
Expand Down
3 changes: 2 additions & 1 deletion servers/physics_3d/space_3d_sw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,8 @@ bool Space3DSW::test_body_motion(Body3DSW *p_body, const Transform &p_from, cons
if (stuck) {

safe = 0;
unsafe = 0;
// Set unsafe to the minimum after eight steps = 1/2^8.
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.

best_shape = j; //sadly it's the best
break;
}
Expand Down