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

Fix mesh corruption of CSG by using elalish/manifold #94321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix mesh corruption of CSG by using elalish/manifold #94321

wants to merge 1 commit into from
+19,884 −1,624

Conversation

fire
Copy link
Member

@fire fire commented Jul 13, 2024 •
edited
Loading

Fixes: godotengine/godot-proposals#9711
Fixes: elalish/manifold#91

The goal is to supersede #91748.

  • Remove exceptions
  • Remove thrust
  • Remove glm
  • Singlethread is on
  • Materials are not transferring

Ok to review for style. Trying to debug why the materials are broken.

Fixes: #43755
Fixes: #58637
Fixes: #41140

Future work

@AThousandShips AThousandShips added this to the 4.x milestone Jul 13, 2024
@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 4 times, most recently from 5a2e7e0 to ccdfea7 Compare July 13, 2024 17:43
@fire fire mentioned this pull request Jul 13, 2024
Closed
@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 6 times, most recently from eb82d74 to e28f331 Compare July 13, 2024 18:24
@31
Copy link
Contributor

31 commented Jul 20, 2024 •
edited
Loading

Thanks for the ping on Discord about this. I finally got a chance to take a look. 🙂

I got materials working (it seems), but all I did was put my old code from #91748 (review) back in and change some names to fit the new place where this implementation lives: 31@5eaf7cb.

Admittedly, my testing is still quite limited. Here's a simple scene that is looking good to me: https://gist.github.com/31/a3e597b32ab31b132ddbc488d8eb0324. The stress-test-ish scene with text also still seems to be working fine: https://gist.github.com/31/c3c4bace42d4ca550ae6badf0f2b7cf9. (Maybe even faster? Been a while.)

I like the change of where this logic lives (vs. #91748). However, I'm not following why the implementation details were changed. It seems significantly more complex and I can't spot what it's fixing. Do you have a test case that this helps with?

@31 31 mentioned this pull request Jul 21, 2024
Open
@fire
Copy link
Member Author

fire commented Jul 21, 2024

As far as I know the implementation details changed because Mesh became MeshGL in manifold. The rest of the changes were a result of this. I didn't like that fact that we are passing an increasingly growing material dictionary so that was removed, but I caused an error.

Thanks for fixing!

@fire
Copy link
Member Author

fire commented Jul 21, 2024 •
edited
Loading

The developers working at manifold worked hard to remove exceptions and thrust, so we were able to vastly reduce the dependencies by 1/3. The changes here were due to api updates.

@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 3 times, most recently from f4c93df to 3391f6b Compare July 21, 2024 01:38
@fire fire marked this pull request as ready for review July 21, 2024 01:51
@fire fire requested review from a team as code owners July 21, 2024 01:51
@fire fire mentioned this pull request Jul 21, 2024
Closed
5 tasks
fire
fire commented Jul 21, 2024
View reviewed changes
modules/csg/csg.h Outdated Show resolved Hide resolved
@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 2 times, most recently from e77e71d to db8d880 Compare July 21, 2024 03:55
@elalish
Copy link

elalish commented Jul 21, 2024

Exciting!

@fire fire marked this pull request as ready for review November 8, 2024 10:09
@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 2 times, most recently from b42affa to 71ffc33 Compare November 8, 2024 16:34
@fire
Copy link
Member Author

fire commented Nov 8, 2024

Test cases for the optimization revert.

csg_flicker.zip

Screenshot 2024-11-08 at 8 36 10 AM

image

editor_screenshot_2024-11-08T083835 copy

@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 2 times, most recently from 95d749f to f02cb93 Compare November 8, 2024 17:41
@fire
Copy link
Member Author

fire commented Nov 8, 2024

Switched the code to lazily collect manifolds of the same type and if the operation changes we apply the collection with manifold::Manifold::BatchBoolean(manifolds, current_op);.

Moved the _unpack_manifold to the very end.

@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 4 times, most recently from 80ff30e to 7146227 Compare November 8, 2024 18:37
fire
fire commented Nov 8, 2024
View reviewed changes
thirdparty/README.md Outdated Show resolved Hide resolved
@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch from 64a498e to 3d0a1de Compare November 8, 2024 20:35
This was referenced Nov 8, 2024
Open
Open
aaronfranke
aaronfranke reviewed Nov 11, 2024
View reviewed changes
Copy link
Member

@aaronfranke aaronfranke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@fire asked me to review this PR. First up, testing to see if it works:

  • Holes in CSG meshes with coplanar faces and engine freeze when moving them #58637
    • The test project was made with Godot 4.0 so it may be outdated.
    • Cannot reproduce the bug in master, it looks visually correct already. However, moving the CSG box causes the editor to freeze and stop responding.
    • With this PR, there is still no bug, and the editor doesn't freeze anymore. ✅
  • CSG flicker / missing triangles / holes in the geometry at certain translations #43755
    • The test project is made with Godot 3.x so it had to be converted to Godot 4.x.
    • I am able to reproduce the bug in master.
    • With this PR, the bug is fixed. ✅
  • CSG box and sphere subtract displays cracking from missing triangle  #41140
    • The MRP is a scene file, not a project, but it seems to load fine in master.
    • I am able to reproduce the bug in master.
    • With this PR, the bug is fixed. ✅ Plus the topology looks cleaner overall. ✅
    • In both master and this PR, the outline of the CSG geometry becomes outdated when moving meshes. Clicking on the mesh with outdated outlines causes the outlines to refresh.
  • All test cases:
    • In both master and this PR, there is massive Z-fighting when a CSG box is selected.
    • I did not test materials, the MRPs did not include any.

20 thousand new lines of code is a lot, unfortunately, but @fire has assured me that this is as cut down as it can be. It makes sense, calculating CSG manifolds is not a simple operation. All of this 'bloat' can be disabled by disabling the CSG module, so it's a non-issue for people making a minimal build of Godot. Plus, it removes a lot of broken first-party code that we don't have to maintain anymore, replacing it with working third-party code, which is great.

As for a code review, I'm not an expert in the CSG module so I cannot give it an in-depth review. However, it looks pretty good overall, it's all self-contained in the CSG module, but I have two notes:

modules/csg/csg_shape.h Outdated Show resolved Hide resolved
modules/csg/csg_shape.cpp Outdated Show resolved Hide resolved
@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 2 times, most recently from 196bbec to 12bb0a6 Compare November 11, 2024 13:28
@fire
Copy link
Member Author

fire commented Nov 11, 2024 •
edited
Loading

Quick-hull is removable in a newer manifold git revision to reduce the code size, but @31 wanted to make a hull operator on top of this pr. godotengine/godot-proposals#9933

Updating to a newer revision is blocked on elalish/manifold#1027

Since upgrading manifold forward has some problems, I've kept this pr on an old revision to do more testing.

@akien-mga akien-mga self-requested a review November 12, 2024 14:55
@elalish
Copy link

elalish commented Nov 12, 2024

Quick-hull is removable in a newer manifold git revision to reduce the code size, but @31 wanted to make a hull operator on top of this pr. godotengine/godot-proposals#9933

Not so much removable as integrated. We removed the quick-hull dependency in favor of making it a native component with quite a bit less code. So you should be good to go updating and still building your hull operator on top.

@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch 2 times, most recently from aca3a40 to c5a92e1 Compare November 12, 2024 18:36
@fire
Copy link
Member Author

fire commented Nov 12, 2024 •
edited
Loading

Renamed MANIFOLD_MAX to MANIFOLD_PROPERTY_MAX.

Removed unused:

MANIFOLD_PROPERTY_NORMAL_X,
MANIFOLD_PROPERTY_NORMAL_Y,
MANIFOLD_PROPERTY_NORMAL_Z,

@fire @31 @cloudofoz
Add csg boolean operators using elalish/manifold.
336af24 
Uses MeshGL64 for more floating point precision.

Co-Authored-By: 31 <[email protected]>
Co-Authored-By: Claudio Z <[email protected]>
@fire fire force-pushed the vsk-csg-manifold-update-4.3 branch from c5a92e1 to 336af24 Compare November 12, 2024 18:39
This was referenced Nov 13, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@elalish elalish elalish left review comments

@aaronfranke aaronfranke aaronfranke approved these changes

@31 31 31 left review comments

@Calinou Calinou Calinou left review comments

@smix8 smix8 smix8 approved these changes

@lyuma lyuma lyuma approved these changes

@akien-mga akien-mga Awaiting requested review from akien-mga

Assignees
No one assigned
Labels
feature proposal topic:3d
Projects
Asset Pipeline Issue Triage
Status: Ready for review
Milestone
4.x
10 participants
@fire @31 @elalish @cloudofoz @PiCode9560 @smix8 @Calinou @aaronfranke @lyuma @AThousandShips