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

Better damping implementation for Bullet rigid bodies #37314

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented Mar 26, 2020

Apply linear and angular damping in RigidBodyBullet to replace the default implementation in Bullet, in order to make it easier to tweak and consistent with Godot Physics.

Edit: Implementation changed to use the old method for damping within bullet instead of re-implementing it on godot side.

Fixes #19182
Fixes #30991

@pouleyKetchoupp
Copy link
Contributor Author

I've just pushed an update from the previous discussion:

  • No more custom implementation for applying damping on velocity
  • Changes in Bullet to use the old method for damping (also merged to the Bullet repo as fix #ifdef for old damping method in btRigidBody bulletphysics/bullet3#2748)
  • Combined values for gravity and damping from area overrides are added instead of averaged to follow the documentation and make it work the same way as in godot physics
  • Support for default damping values from the physics world, applied as an area override with lowest priority (default is 0.1 from project settings)

@AndreaCatania
Copy link
Contributor

@akien-mga Is it ok import the Bullet master version? We need a change made there.

Copy link
Contributor

@AndreaCatania AndreaCatania 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. The only thing that I would like to change is the change made directly on the bullet lib. In general I prefer to use the new lib version.

Other than that It's fine for me, thanks.

@akien-mga
Copy link
Member

Is it ok import the Bullet master version? We need a change made there.

For the Godot master branch that should be fine yeah.

@akien-mga
Copy link
Member

akien-mga commented Apr 26, 2020

For the Godot master branch that should be fine yeah.

Actually, if the only bullet change that we need is bulletphysics/bullet3#2748, then I would prefer simply backporting that PR as done here and including the patch in thirdparty/bullet/patches. I prefer to use stable version + simple patch instead of a random mid-development commit.

If there have been more changes since 2.89 that we would benefit from/need, then it's OK to sync with bullet master.

@pouleyKetchoupp
Copy link
Contributor Author

Thanks! What is the purpose of the patches folder if the fix is already integrated into master on bullet side?

@akien-mga
Copy link
Member

akien-mga commented Apr 26, 2020

Thanks! What is the purpose of the patches folder if the fix is already integrated into master on bullet side?

To document what changes we made to the upstream version that we document in thirdparty/README.md, so that they can be re-applied if need be when syncing with newer version. Anecdotally, it can also enable downstream Linux packagers to include the patch in their bullet package if they want to use it for Godot (that probably only applies to me ;)).

That being said, that could lead to situations where people build against system bullet 2.89 without the patch and thus end up with an incompatible Godot build. So it might indeed be better to sync with upstream master, and bump the min bullet version in platform/linuxbsd/detect.py to 2.90.

@pouleyKetchoupp
Copy link
Contributor Author

Makes sense. So if it's ok with both of you, I'll make a separate PR with the latest master from bullet. I've seen there are especially lots of changes in soft bodies. I'm not very familiar with it, so it might be better for @AndreaCatania to review and test. And we can decide then if we go with the full update or just patch with the damping fix.

@pouleyKetchoupp
Copy link
Contributor Author

On second thoughts, does it mean that we'll have to keep in sync with bullet master until 2.90 release to avoid the last issue you mentioned @akien-mga? It seems easier in this case to just add the patch and document it in the third party readme.

@AndreaCatania
Copy link
Contributor

In any way you do, make sure that:

bump the min bullet version in platform/linuxbsd/detect.py to 2.90

@pouleyKetchoupp
Copy link
Contributor Author

Ok, and I guess I can add the exact commit hash I'm syncing to in the third party readme file.

Apply old method for linear & angular damping in Bullet, in order to
make it easier to tweak and consistent with Godot Physics.
@pouleyKetchoupp
Copy link
Contributor Author

I've just rebased on master after the bullet update so it should be ready to merge.

Copy link
Contributor

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

It Looks nice

@akien-mga akien-mga merged commit 088cf37 into godotengine:master Apr 28, 2020
@akien-mga
Copy link
Member

Thanks!

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