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

Modify contact_max_allowed_penetration precision to 3 significant digits #75665

Merged

Conversation

Malcolmnixon
Copy link
Contributor

@Malcolmnixon Malcolmnixon commented Apr 4, 2023

This pull request addresses #75634 by modifying the numeric precision of "physics/3d/solver/contact_max_allowed_penetration" to three significant digits, so values down to "0.001" can be specified. This is necessary in VR situations where the scene contains objects at the scale of human hands.

image

Bugsquad edit:

@capnm
Copy link
Contributor

capnm commented Apr 5, 2023

Is there a reason why a value 0.0 is also valid ?
physics/2d/solver/contact_max_allowed_penetration - has, I guess, the similar issue(s) ??

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Seems fine to me. A minimum value of 1 mm seems more reasonable to me than a minimum of 1 cm

Copy link
Contributor

@fabriceci fabriceci left a comment

Choose a reason for hiding this comment

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

Did you look if it doesn't fix other issues (jitter on small objects)?

@rburing
Copy link
Member

rburing commented Apr 5, 2023

Would you mind adding a Co-authored-by line in the commit message?

Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

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

Looks good. For 2D no change should be necessary as the value is in pixels.

P.S. My e-mail address is public, and can be found e.g. on my website or by using git log on a local Godot repo.

@Calinou
Copy link
Member

Calinou commented Apr 5, 2023

@rburing Is a value of 0.0 legal? The property hint currently allows it.

@rburing
Copy link
Member

rburing commented Apr 5, 2023

@Calinou Setting it to 0.0 means no penetration is allowed, so the engine will try to depenetrate (rigid and soft) bodies pretty much constantly, which is almost certain to cause constant jitter.

So we could set the minimum to 0.001 to be a bit more user-friendly.

Maybe

  • physics/3d/solver/contact_recycle_radius and
  • physics/3d/solver/contact_max_separation

also need a similar treatment of increasing the precision.

@Calinou
Copy link
Member

Calinou commented Apr 5, 2023

In this case, remember to also change the minimum value for the 2D contact_max_allowed_penetration 🙂

@Malcolmnixon
Copy link
Contributor Author

If we're going to include additional changes to ranges and limits in this PR, I would like to get confirmation on the changes:

  1. 3D units are physical distances, and so 0.001 (1mm or 3 significant digits) is appropriate:
    a. physics/3d/solver/contact_recycle_radius modified from 0,0.1,0.01,or_greater to 0,0.1,0.001,or_greater
    b. physics/3d/solver/contact_max_separation modified from 0,0.1,0.01,or_greater to 0,0.1,0.001,or_greater
    c. physics/3d/solver/contact_max_allowed_penetration modified from 0,0.1,0.01,or_greater to 0.001,0.1,0.001,or_greater
  2. 2D units are pixels, and so 0.01 (2 significant digits) is appropriate:
    a. physics/2d/solver/contact_recycle_radius keeps its range of 0,10,0.01,or_greater
    b. physics/2d/solver/contact_max_separation keeps its range of 0,10,0.01,or_greater
    c. physics/2d/solver/contact_max_allowed_penetration modified from 0,10,0.01,or_greater to 0.01,10,0.01,or_greater

Is everyone fine with the significant digits choices based on 2D vs. 3D, and that only contact_max_allowed_penetration needs to have zero excluded from the range?

@Calinou
Copy link
Member

Calinou commented Apr 5, 2023

Is everyone fine with the significant digits choices based on 2D vs. 3D, and that only contact_max_allowed_penetration needs to have zero excluded from the range?

Sounds good on my end, but I can't confirm whether contact_recycle_radius and contact_max_separation should be able to use 0.0.

…to three significant digits, so values down to "0.001" can be specified.

Updated additional 2D and 3D physics parameters based on team recommendations

Co-Authored-By: Ricardo Buring <[email protected]>
@Malcolmnixon
Copy link
Contributor Author

The PR has been updated with the appropriate recommendations.

Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

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

Looks like a fine improvement as is.

@YuriSizov YuriSizov changed the title Modify contact_max_allowed_penetration" precision to 3 significant digits Modify contact_max_allowed_penetration precision to 3 significant digits Apr 7, 2023
@YuriSizov YuriSizov merged commit 47af40c into godotengine:master Apr 7, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.3.

@akien-mga akien-mga changed the title Modify contact_max_allowed_penetration precision to 3 significant digits Modify contact_max_allowed_penetration precision to 3 significant digits May 12, 2023
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.

[GodotPhysics] RigidBody3D Objects Overlapping
7 participants