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

[3.x] Use Bullet's convex hull computer in place of QuickHull when available. #47307

Closed

Conversation

mortarroad
Copy link
Contributor

Fixes #45946, maybe more.

@mortarroad mortarroad requested review from a team as code owners March 23, 2021 18:48
@mortarroad mortarroad force-pushed the 3.x-convex-hull-bullet branch 2 times, most recently from a9d05b9 to e7b01ae Compare March 23, 2021 19:08
@Calinou Calinou added this to the 3.4 milestone Mar 23, 2021
@Calinou Calinou removed request for a team March 23, 2021 20:22
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

This is a good idea and it seems to work well!

There are just two things I would like to address before approving:

  • This change adds a dependency to VHACD module in Godot Physics, so given that the old QuickHull code is very small, I'd like to keep it as an alternative in case the engine is compiled with VHACD disabled (see my review comment).
  • Another PR needs to be opened on master to keep the codebase in sync. The usual process is to open a PR on master first, and then the changes can be cherry-picked on 3.x when possible. In this case the file names have changed on master, so it makes sense to keep this separate 3.x PR.

core/math/geometry.cpp Show resolved Hide resolved
@mortarroad
Copy link
Contributor Author

mortarroad commented Mar 24, 2021

Superseded by #47332

Another PR needs to be opened on master to keep the codebase in sync. The usual process is to open a PR on master first, and then the changes can be cherry-picked on 3.x when possible. In this case the file names have changed on master, so it makes sense to keep this separate 3.x PR.

I tend to make patches against 3.x, since that's what I use and where I encounter bugs that I want to fix.
With master, I can't test them as easily.

@mortarroad mortarroad closed this Mar 24, 2021
@pouleyKetchoupp
Copy link
Contributor

This PR can stay open, since it would still be nice to have this fixed on 3.4 as well and the file names have changed.
Sorry for the confusion, in my previous comment I just meant that it would need an extra PR on master.

@lawnjelly
Copy link
Member

And just to note I'm using quick hull in several modules (and I presume others are too) and also in the rooms / portals PR. So keeping the old version available would be very useful (any replacement would have to be a drop in replacement, and always available).

@mortarroad
Copy link
Contributor Author

And just to note I'm using quick hull in several modules (and I presume others are too) and also in the rooms / portals PR. So keeping the old version available would be very useful (any replacement would have to be a drop in replacement, and always available).

That's the plan. QuickHull will remain available.
And it should be possible to simply replace any call to QuickHull::build with Geometry::build_convex_hull.
Take a look at the PR against master for reference.

@mortarroad mortarroad force-pushed the 3.x-convex-hull-bullet branch from e7b01ae to 7f0a8c6 Compare March 25, 2021 11:08
@mortarroad mortarroad changed the title Replace QuickHull with Bullet's convex hull computer [3.x] Replace QuickHull with Bullet's convex hull computer Mar 25, 2021
@mortarroad mortarroad changed the title [3.x] Replace QuickHull with Bullet's convex hull computer [3.x] Use Bullet's convex hull computer in place of QuickHull when available. Mar 25, 2021
@jitspoe
Copy link
Contributor

jitspoe commented Mar 28, 2021

Does this VHACD have the same issue that the Mesh::convex_decompose() did? If I recall correctly, that also used the VHACD algorithm, and when "convcolonly" for mesh imports was updated to use that instead of quickhull, it would result in incorrect collision (ex: a simple box would have the verts slightly off).

Edit: Actually, looking closer, it seems the VHACD algorithm uses btConvexHullComputer, and you included the VHACD version, so you could just skip the VHACD:: in front of that and use the bullet one directly.

@mortarroad
Copy link
Contributor Author

@jitspoe I have not done much testing, but I assume that this algorithm is precise.
As you mentioned, this does not actually use VHACD; it simply uses what's provided by Bullet, which happens to be included in that module.
The reason is to allow compilation with Bullet disabled, as mentioned here: #41522 (comment)

@pouleyKetchoupp
Copy link
Contributor

@jitspoe From what I've tested with basic primitives, the Bullet convex hull algorithm itself doesn't introduce the problem you mentioned (#29971).

@akien-mga
Copy link
Member

Superseded by #48533.

@mortarroad mortarroad deleted the 3.x-convex-hull-bullet branch May 22, 2021 22:43
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.

6 participants