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

Add csg boolean operators using elalish/manifold. #91748

Closed
wants to merge 5 commits into from

Conversation

fire
Copy link
Member

@fire fire commented May 9, 2024

Fixes: godotengine/godot-proposals#9711

  • Need to discuss with @elalish how to trim the dependent libraries
  • Need to discuss with @elalish about removing exceptions
  • Add copyright notices.
  • Pending work to remove Thrust
  • Pending work to remove exceptions

image

Screen_Recording_2024-05-07_at_4.42.45_PM.mp4

@fire fire requested review from a team as code owners May 9, 2024 07:44
modules/csg/csg_shape.cpp Outdated Show resolved Hide resolved
@Chaosus Chaosus added this to the 4.x milestone May 9, 2024
@fire fire force-pushed the vsk-csg-4.3 branch 6 times, most recently from 947cacb to a1461a5 Compare May 9, 2024 13:13
modules/csg/csg_shape.cpp Outdated Show resolved Hide resolved
"src/manifold/src/subdivision.cpp",
"src/manifold/src/csg_tree.cpp",
"src/cross_section/src/cross_section.cpp",
"src/third_party/quickhull/QuickHull.cpp",
Copy link

Choose a reason for hiding this comment

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

This is good practice for me, since I haven't seen a large project integrate our library this way before. I would like to make it easier if possible, and more modular. I take it you don't want to integrate us as a submodule?

If you're not interested in 2D tools or convex hulls, it should be possible to drop those dependencies, but we could probably structure our library better for doing this. @pca006132 do you know any way to allow e.g. building Manifold without quickhull, so that the Manifold::ConvexHull methods are just stubs?

Choose a reason for hiding this comment

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

Some kind of #if will work, change them to throwing exceptions when user don't want to compile quickhull.

Copy link
Member Author

@fire fire May 9, 2024

Choose a reason for hiding this comment

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

As a general rule, Godot Engine vendors its entire dependencies because it makes development much easier for contributors and for CI/CD. I am interested in the other libraries, but it would be best if I can find ways to shrink the binary size.

Although, if your quickhull is good, @lawnjelly has been in that area and could evaluate the quality if it's useful.

Copy link

Choose a reason for hiding this comment

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

Would love to hear your evaluation - in fact we have a GSoC contributor who just started and is looking into improving our hulling - either using a different dependency or writing a new one. opencax/GSoC#89

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest my best experience is with https://github.com/SarahWeiii/CoACD for convex hulling.

@elalish
Copy link

elalish commented May 9, 2024

Manifold should already be set up to work just fine without exceptions if you don't compile with them - is it giving you any trouble?

@pca006132
Copy link

pca006132 commented May 9, 2024

Manifold should already be set up to work just fine without exceptions if you don't compile with them - is it giving you any trouble?

Probably not, I forgot to only throw exceptions for vector out of range when user enable compiling with exceptions. And tbb uses exceptions iirc.

The first case is easy to fix, the second case is probably not.

@fire fire force-pushed the vsk-csg-4.3 branch 4 times, most recently from 0b7d9e5 to 80f72e6 Compare May 10, 2024 17:40
modules/csg/csg_shape.cpp Outdated Show resolved Hide resolved
@fire fire force-pushed the vsk-csg-4.3 branch 11 times, most recently from a0173a0 to 142bf4e Compare June 16, 2024 12:47
fire and others added 2 commits June 16, 2024 05:56
Support a material, add an icon, add simple documentation.

Co-Authored-By: 31 <[email protected]>
@fire fire force-pushed the vsk-csg-4.3 branch 3 times, most recently from 9230b29 to 68abed4 Compare June 16, 2024 13:20
Manifold supports tracking materials associated with a run of triangles.
This commit changes Godot's Manifold pack and unpack logic to use that
functionality rather than storing the material id in each vertex.
@fire fire requested a review from a team as a code owner June 16, 2024 13:51
@31
Copy link
Contributor

31 commented Jun 16, 2024

Filed/fixed #93240, and the description has some screenshots of a situation that Manifold handles way better than the existing meshing/CSG, even when there's an abnormality due to Snap being too high. The scene is also quite expensive but very simple because it relies on a generated mesh rather than assets--could be useful to look at performance. (I'm very impressed with the perf, but I was looking at 4.2.2 vs. a dev build and didn't use a stopwatch, so not apples to apples.)

@fire
Copy link
Member Author

fire commented Jul 21, 2024

Superseded by: #94321.

@fire fire closed this Jul 21, 2024
@fire fire deleted the vsk-csg-4.3 branch July 21, 2024 02:52
@AThousandShips AThousandShips removed this from the 4.x milestone Jul 21, 2024
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.

Improve CSG Quality with elalish/manifold
6 participants