-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Fix mesh corruption of CSG by using elalish/manifold #94321
base: master
Are you sure you want to change the base?
Conversation
5a2e7e0
to
ccdfea7
Compare
eb82d74
to
e28f331
Compare
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? |
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! |
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. |
f4c93df
to
3391f6b
Compare
e77e71d
to
db8d880
Compare
Exciting! |
e0697ae
to
f4eb21a
Compare
Horray Manifold recently replaced the GLM dependency with linalg.h elalish/manifold#984 |
468c689
to
9fb7797
Compare
Looking for team approval and style approval. |
9fb7797
to
983b945
Compare
5c7147b
to
e0f20e4
Compare
Tested this PR a little and while it in general works fine I had a few random instances where for whatever reason certain parts of the mesh would turn out with wrong normals or smoothing. It would look like this here with the sphere mesh that touches the box mesh with union. When I change csg properties or move the objects a little it usually disappears. It is hard to reproduce because when I undo all the the changes and have the csg at the original bug state it does not show the issues again. When I try to save the scene to create an MRP it does not show the bug on loading. Any guess what could cause this? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
It's off by one because our runIndex has an extra element to store the total size. I'd recommend using |
As for |
e0f20e4
to
0beceb2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
0beceb2
to
56394ac
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
It's in this thread: #91748 (comment) |
The difference seems to be that the CSGSphere3D and CSGBox3D don't automatically calculate tangents, but CSGMesh3D does. |
@smix8 The artifact where the shading changes when the mesh goes from touching to intersection is because the below list doesn't calculate tangents from the faces. It is possible and would be highly wanted by others and me, but it could be a different feature request after this is merged.
What do you think about approving this for now? A workaround is to use CSGMesh3D which does calculate tangents or add a StandardMaterial and set it to vertex shading. |
So you mean this is not a regression with this PR? Just something that was already the case in master with those CSG nodes and I just did not encounter it by luck? In that case, yes I think it would be fine to improve that later in another PR. |
I can confirm it happens in Godot Engine 4.3.stable. Screen.Recording.2024-10-18.at.2.26.22.PM.mov |
When testing I mean both versions stutter with a Mesh resource but with this PR I can barely control the mouse while dragging. Is this performance difference expected? What is causing it? |
Will need to go over the code with a profiler to see what is the slowdown, but it's late Friday today, so will be delayed for a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encountered a few CSG issues while testing this but apparently they all exist in the current master and are not regressions from this PR.
E.g. specific node types had issues or dont work with certain ops which need to be looked at (e.g. hide the non-working options from the inspector if they can't be fixed), same with some performance differences.
Everything outside of that so far worked fine. I would be ok with pushing the other fixes to followup PRs to keep the ball rolling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit I don't know much about the manifold library itself, but the change seems to largely consolidate code around manifold packing and unpacking, which is good.
The thirdparty code addition is not too crazy, but that's more a question for maintainers to answer.
The main issue I'm worried about is that the process of converting to/from manifold looks at first glance to be a bit costly, and I'm worried it's doing the conversion too often.
@@ -844,7 +844,7 @@ void TextureStorage::texture_2d_layered_initialize(RID p_texture, const Vector<R | |||
} | |||
} | |||
|
|||
Texture texture; | |||
Texture texture = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Texture does not have default initializers, which is bad form, but this should be a separate PR.
I don't think we should be adding a single = {} just in this one place since it doesn't improve the overall code quality.
If there is a bug that we're working around or fixing by doing this, I'd like to be explicit about what it is. I double checked all the class members without default initializers and they should all be getting initialized here, so I'm skeptical that this change is needed.
manifold::Manifold brush_a; | ||
_pack_manifold(&p_brush_a, brush_a, mesh_materials, p_default_material, p_vertex_snap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called every time the CSGShape3D is dirty, right?
My main feedback here is that I am worried about continually packing and unpacking the manifold for each operation within CSGShape3D::_get_brush() . this seems like it could be a performance bottleneck.
I'd think that the point of this that we can keep each CSGBrush in manifold form and then a merge operation is now as simple as + or ^ or -.
(and separately, we do unpack once per frame if it's dirty just to submit to the graphics API.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
^
are commutative-
is not commutative
I did some cleanup but wasn't able to rewrite the expressions.
mesh.runIndex.push_back(mesh.triVerts.size()); | ||
|
||
// Associate the material with an ID. | ||
uint32_t reserved_id = r_manifold.ReserveIDs(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a static function? If we do run this once per CSG operation, if I have 1000 CSG brushes at 60 Hz, it will overflow the static uint32_t after just 20 hours of gameplay. If it's once per face, this uint32_t could overflow within minutes.
Is an overflow of the static ID generator expected / normal?
Is there a way to manage the allocation ourselves? I'm just feeling a bit uneasy about using a global static function even if it isn't hammered every frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we generating 60 000 * 60 * 60 * 20 boolean operations in a nested tree csg operations?
Also this might be an upstream limit until its changed. @elalish Not sure if doubles changed the run id limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand why you need to generate that many IDs. My first instinct is they're probably not being used as intended if you create that many. Their point is generally to be a way to map back to e.g. materials. I doubt you're creating that many unique materials, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyuma As far as I know we only generate one runid per material in a mesh. So I don't know how you are calculating this.
modules/csg/csg.cpp
Outdated
if (err != manifold::Manifold::Error::NoError) { | ||
print_error(String("Manifold creation from mesh failed:" + itos((int)err))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this return a boolean or Error if it fails? Or is it impossible to trigger an error here (this is more like an assertion)
Might be good to use one of the ERR_ macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the geometry is not manifold able I prefer it disappears rather to crash godot. An error message every frame is not fun either.
Uses MeshGL64 for more floating point precision. Co-Authored-By: 31 <[email protected]> Co-Authored-By: Claudio Z <[email protected]>
56394ac
to
1020518
Compare
The goal is to supersede #91748.
Materials are not transferringOk to review for style. Trying to debug why the materials are broken.Fixes: #43755
Fixes: #58637
Fixes: #41140