[3.x] Port Bullet's convex hull computer to replace QuickHull

[mortarroad]

Supersedes #47307 and #47332.
Fixes #45946.

The code is based on the current version of thirdparty/vhacd and modified to use Godot's types.

Again, against 3.x first, since that's what I work with.
I can port it to master, once it is ready to be merged.

I still have some questions:

• What to do with the assertions? I think they may be too heavy to replace them with error macros, since that causes them to be left in for release builds, but I can't find how assertions are done in Godot.
• Is LocalVector the proper type to use here?
• Perhaps the custom classes PoolArray and Pool should be replaced with Godot's types (perhaps PagedAllocator) or even no pooling at all.

[pouleyKetchoupp]

Thanks for your work! I haven't tested yet, but I've left some comments about coding style.

Concerning your specific questions:

What to do with the assertions? I think they may be too heavy to replace them with error macros, since that causes them to be left in for release builds, but I can't find how assertions are done in Godot.

What you can do is to wrap error calls with defines to avoid doing them in release, so they will show up only in the editor (it's configured in release_debug) and in game debug/release_debug.

Something like that:

#ifdef DEBUG_ENABLED
#define CHULL_ASSERT(m_cond) if (unlikely(!m_cond)) ERR_PRINT("Assertion \"" _STR(m_cond) "\" failed.")
#else
#define CHULL_ASSERT(m_cond)
#endif

Is LocalVector the proper type to use here?

Yes, it's the proper type to avoid copy-on-write.

Perhaps the custom classes PoolArray and Pool should be replaced with Godot's types (perhaps PagedAllocator) or even no pooling at all.

Yes, it looks like it would work well in this case! It should be fine to backport paged_allocator.h along with spin_lock.h (which you don't need in your case, but better keep things synchronized with master).

Since files are organized in a slightly different way on 3.x, paged_allocator.h should probably go to the core folder directly.

[pouleyKetchoupp]

@akien-mga I'm not sure what should be added to COPYRIGHT.txt, since it's based on Bullet, but b3ConvexHullComputer only contains copyright from the original author. Help please :)

Here's the Bullet source file:
https://github.com/godotengine/godot/blob/master/thirdparty/bullet/Bullet3Geometry/b3ConvexHullComputer.cpp

[mortarroad]

Considering PagedAllocator:
That won't work, since the implementation does not free all objects. I.e. the Pool takes care of freeing them in the end.

[mortarroad]

I think this should be ready for review again now.

[pouleyKetchoupp]

Concerning PagedAllocator:
Unless I'm missing something specific, it has a method reset() to deallocate everything, and it's called in the destructor so it should be fine.

Concerning my comments:
It's just more things about code style. I didn't comment all of them, but I've seen more local variables in different methods of ConvexHullInternal that don't use snake case.

[pouleyKetchoupp]

I've just made some tests and it seems to work well!
This PR also fixes #44090.

I've made a quick test using PagedAllocator and it doesn't seem as straightforward, it's causing crashes for me, so it's something that can be revisited later (unless you'd like to try and make it work).

[mortarroad]

That's very confusing.
Apparently, the addresses returned by the pool are expected to be contiguous somehow.
PagedAllocator does not do that (in fact, it returns the addresses in the opposite order).
I'm not sure where this property is used, or whether it is maybe a bug in the convex hull implementation (because it is still allocated in blocks, so it is not contiguous everywhere).
It also crashes, if all new_object() calls are replaced with a simple memnew, and the memory is never freed. (Leaking should not cause a crash.)

[mortarroad]

This right here assumes contiguous memory.
Should be safe to replace with original_vertices[p_start+1]

[mortarroad]

PagedAllocator seems to have the same performance as Pool.
Both take around 75% of the time of memnew.

[mortarroad]

I also added an allow_leak flag to PagedAllocator, which does not error if not everything has been freed. This is not a problem if the type has no destructor, since all the memory is freed anyway.

[pouleyKetchoupp]

Good job on finding the issue with PagedAllocator and fixing it!

Could you make it a separate function reset_no_check() (or an argument in reset() with a default value) to skip the error, instead of an extra template parameter? It would make things a bit more flexible for other use cases.

This way the error can stay in the destructor, and it would be up to the convex hull process to call this reset function at the end. It can still throw an error if the condition !_has_trivial_destructor(T) is true.

[pouleyKetchoupp]

Looks great! We just need a few last things to finalize this PR and finally merge it:

1. There's one recent patch from Bullet that would be interesting to apply to this implementation to avoid potential issues:
   CWE-190 integer overflow in btConvexHullComputer bulletphysics/bullet3#3037
   I've checked the git history and everything else seems to be mostly coding style and compilation fixes, so it should be ok with just this change.

2. Please add this extra bit to COPYRIGHT.txt:

diff --git a/COPYRIGHT.txt b/COPYRIGHT.txt
index f2e2a86c05..4a5baf756f 100644
--- a/COPYRIGHT.txt
+++ b/COPYRIGHT.txt
@@ -55,6 +55,14 @@ Comment: Godot Engine logo
 Copyright: 2017, Andrea Calabró
 License: CC-BY-4.0

+Files: ./core/math/convex_hull.cpp
+ ./core/math/convex_hull.h
+Comment: Bullet Continuous Collision Detection and Physics Library
+Copyright: 2011, Ole Kniemeyer, MAXON, www.maxon.net
+ 2007-2021, Juan Linietsky, Ariel Manzur.
+ 2014-2021, Godot Engine contributors.
+License: Expat and Zlib
+
 Files: ./modules/fbx/fbx_parser/
 Comment: Open Asset Import Library (FBX parser)
 Copyright: 2006-2020, assimp team

3. Then you can squash all your commits into one (interactive rebase).

4. We need a separate PR for master to keep the two branches in sync. I can do it myself if needed, but porting the code should be straightforward.

[pouleyKetchoupp]

Also I realize I didn't add core team for review before, so adding just in case.

[mortarroad]

Squashed and rebased onto 3.x

[akien-mga]

Looks good to me overall.

[pouleyKetchoupp]

Looks great!

[akien-mga]

Thanks!

[lawnjelly]

I'm not sure if I've made a mistake in some tests I'm running, but the meshes generated by this seem to have windings in the opposite direction to the old Quickhull, i.e. facing towards the centre of the hull, instead of pointing outward (for backface culling). If so it would be easy enough to change. Can anyone confirm this?

[pouleyKetchoupp]

@lawnjelly Yes, I can confirm that generated faces have the same plane normals, but indices are inverted compared to the previous version. Good catch, thanks!