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.

Sign up for GitHub

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

Jump to bottom

Replace QuickHull with Bullet's convex hull computer. #48916

Merged
merged 1 commit into from
May 22, 2021
Merged

Replace QuickHull with Bullet's convex hull computer. #48916

merged 1 commit into from
May 22, 2021

Conversation

mortarroad
Copy link
Contributor

@mortarroad mortarroad commented May 21, 2021 •
edited by pouleyKetchoupp
Loading

The same as #48533, but ported to master.

Bug squad edit: fixes #44090, fixes #45946

@mortarroad mortarroad requested review from a team as code owners May 21, 2021 08:04
@mortarroad
Copy link
Contributor Author

Note that I cannot test this, master does not run at all for me.

@akien-mga akien-mga added this to the 4.0 milestone May 21, 2021
akien-mga
akien-mga reviewed May 21, 2021
View reviewed changes
core/math/convex_hull.cpp Outdated Show resolved Hide resolved
core/templates/paged_allocator.h Show resolved Hide resolved
@mortarroad mortarroad force-pushed the master-convex-hull-ported branch 3 times, most recently from 47f219e to bcff746 Compare May 22, 2021 06:18
@pouleyKetchoupp
Copy link
Contributor

This PR just needs to be synced with the latest changes from #48533 and it will be good to go.

@mortarroad
Replace QuickHull with Bullet's convex hull computer.
d1bc88d 
The code is based on the current version of thirdparty/vhacd and modified to use Godot's types and code style.

Additional changes:
- extended PagedAllocator to allow leaked objects
- applied patch from bulletphysics/bullet3#3037
@akien-mga akien-mga merged commit de4c17f into godotengine:master May 22, 2021
@akien-mga
Copy link
Member

Thanks for the amazing work!

@mortarroad mortarroad deleted the master-convex-hull-ported branch May 22, 2021 23:06
aaronfranke
aaronfranke reviewed Jun 2, 2021
View reviewed changes
core/math/convex_hull.cpp
Comment on lines +72 to +75
// -- GODOT start --
// Assembly optimizations are not used at the moment.
//#define USE_X86_64_ASM
// -- GODOT end --
Copy link
Member

Choose a reason for hiding this comment

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

Why? Not objecting, just curious.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would take more time to test on different platforms, and set conditions to enable it only on supported compilers so it's easier to just leave it disabled for now. It's already much faster than QuickHull.

But if inlined assembly does make a measurable difference for performance and someone is up for doing the work, it would be nice to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt it makes any difference. If the compiler does not already generate optimal code, maybe it can be adjusted such that it does without using assembly.
If anything, I'd replace it with intrinsics instead.

@kleonc kleonc mentioned this pull request Aug 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pouleyKetchoupp pouleyKetchoupp pouleyKetchoupp approved these changes

@aaronfranke aaronfranke aaronfranke left review comments

@akien-mga akien-mga akien-mga left review comments

Assignees
No one assigned
Labels
bug topic:core topic:editor topic:physics topic:3d
Projects
None yet
Milestone
4.0
4 participants
@mortarroad @pouleyKetchoupp @akien-mga @aaronfranke