Skip to content

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

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
V-Sekai:vsk-csg-manifold-update-4.3
Nov 29, 2024
Merged

Fix mesh corruption of CSG by using elalish/manifold#94321
akien-mga merged 1 commit into
godotengine:masterfrom
V-Sekai:vsk-csg-manifold-update-4.3

Conversation

@fire

@fire fire commented Jul 13, 2024

Copy link
Copy Markdown
Member

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

The goal is to supersede #91748.

Work was done as part of the V-Sekai development community, which is focused on providing social VR functionality for the open-source Godot Engine.

  • 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 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

31 commented Jul 20, 2024

Copy link
Copy Markdown
Contributor

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?

@fire

fire commented Jul 21, 2024

Copy link
Copy Markdown
Member Author

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

fire commented Jul 21, 2024

Copy link
Copy Markdown
Member Author

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
Comment thread modules/csg/csg.h Outdated
@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

elalish commented Jul 21, 2024

Copy link
Copy Markdown

Exciting!

@fire

fire commented Nov 30, 2024

Copy link
Copy Markdown
Member Author

@zhangjiangen11 Can you create a new issue? I suggest clearing .godot because I can't recreate the crash.

@goatchurchprime

Copy link
Copy Markdown
Contributor

Here's a quick stress test of it. A few examples like this could feed into a very good blog post about it.
Screenshot from 2024-12-02 12-55-09

@fire

fire commented Dec 2, 2024

Copy link
Copy Markdown
Member Author

Can you post the script as an MIT-licensed GitHub project? The top portion of the sponge seems corrupted.

@goatchurchprime

Copy link
Copy Markdown
Contributor

Here it is. csgmanifoldtest.zip

Set mengerdimension = 4 and it takes 2 minutes instead of 0.2 seconds.
Set holelength = 1.001 to ensure that the holes go all the way through

The holes gaps on the rotated combiners of boxes might be misaligned if 90degree rotation is implemented using the axis vectors (sin(1.5707963), cos(1.5707963)) instead of (0,1) (radians are evil).

I can't explain why one the triangles of the middle hexagram star on each edge is too large.

@fire

fire commented Dec 2, 2024

Copy link
Copy Markdown
Member Author

I modified the CSG snap values, and it seemed to work ok.

image

csgmanifoldtest 2.zip

@goatchurchprime

goatchurchprime commented Dec 2, 2024

Copy link
Copy Markdown
Contributor

Is snap implemented entirely differently in manifold?

I think these bits are different when you compare to the online version. No idea why. In fact the whole thing is a bit squiff in terms of the spread of the little holes. Might be something wrong in my code, though can't see what.
image

@fire

fire commented Dec 2, 2024

Copy link
Copy Markdown
Member Author

I associated godot engine snap with tolerance

@elalish

elalish commented Dec 2, 2024

Copy link
Copy Markdown

There's no snapping in manifold itself. It almost looks like it's using 0.33 instead of a 1/3 or something. @fire even yours looks quite asymmetric and wrong to me.

I associated godot engine snap with tolerance

That might be dangerous... I'm not sure exactly how snap is used internally by Godot, but we use tolerance for mesh simplification, which can yield pretty unexpected results if the value gets too large.

@fire

fire commented Dec 2, 2024

Copy link
Copy Markdown
Member Author

We can deprecate snap and use the default manifold tolerance. How does that sound?

@elalish

elalish commented Dec 3, 2024

Copy link
Copy Markdown

Yeah, that sounds good to me.

@31

31 commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

We can deprecate snap and use the default manifold tolerance. How does that sound?

I already saw an issue with the default snap/tolerance a while back, example in this issue: #93240

I'm not sure how to rectify the meanings, but from what I've seen, it probably matters to expose it somehow. Maybe you can get around it by scaling/unscaling, but that doesn't sound all that nice to deal with for prototyping in-editor.

(Edit: That all said, if the default tolerance for Manifold is (effectively) massively smaller than what anyone would be using, I suppose that's fine. 😄)

@elalish

elalish commented Dec 3, 2024

Copy link
Copy Markdown

@31 - the issue you linked to is an example of the same problem: the mesh being broken by choosing too large a "snap" tolerance value. I still think @fire's approach will be the most reliable solution for that class of problems as well, particularly if snap is removed entirely.

@31

31 commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

the issue you linked to is an example of the same problem: the mesh being broken by choosing too large a "snap" tolerance value.

I understand that, but I don't have a sense for what the default Manifold tolerance behavior means. If I scaled the text 1/100, would the default behavior show a similar problem? 1/10000? Or is the default behavior close enough to floating point epsilon that it's difficult to even encounter that kind of problem?

I wasn't able to find any info in the Manifold docs that really told me what situation to expect here. 😕

@elalish

elalish commented Dec 3, 2024

Copy link
Copy Markdown

Yes, the default is floating point epsilon.

@ydeltastar

Copy link
Copy Markdown
Contributor

Looks like this also fixed #94174.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done