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

Update property/documentation of shape margins #75079

Merged
merged 2 commits into from
Mar 20, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Mar 18, 2023

Seeing as how the margin property of Shape3D managed to make its way into 4.0-stable, despite not being used by Godot Physics, I figured we might as well make some changes to better support other physics engines.

I rephrased the documentation to reflect that it's not only Bullet Physics that uses this, since it's also a thing in other GJK-based physics engines such as Havok, Jolt, Rapier, etc.

I also opened up the bounds of the property by changing the lower bound from 0.001 to 0 and adding the or_greater hint to allow for values greater than its upper bound. Jolt Physics (for example) has no issues using a margin of 0, apart from the performance drawbacks, and having 10 be a hard upper bound seemed somewhat arbitrary.

@mihe mihe requested a review from a team as a code owner March 18, 2023 20:12
@YuriSizov YuriSizov merged commit 7ca4ad8 into godotengine:master Mar 20, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov
Copy link
Contributor

Ah, my mistake. I should've asked you to squash your two commits into one before merging. It's not a huge deal here, as both of those commits are self-sufficient, but please mind that in your future contributions.

@mihe
Copy link
Contributor Author

mihe commented Mar 20, 2023

My bad! I thought that rule only applied to "fix-up" commits. I'll make sure to squash them all in the future.

@mihe mihe deleted the shape-margin branch March 20, 2023 16:28
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.2.

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.

4 participants