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

Push some warnings for CSG non manifold and other errors. #100361

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

fire
Copy link
Member

@fire fire commented Dec 13, 2024

@BastiaanOlij mentioned that seeing the error message for invalid CSGMesh inputs was complex.

This was also a leftover task from Fix mesh corruption of CSG by using #94321

Error Message Screenshot

I'm not perfectly happy with the warning wording. You are welcome to suggest changes.

Part of #100014

See also #100164

https://github.com/user-attachments/assets/f3080d1e

@fire fire requested a review from a team as a code owner December 13, 2024 13:27
@fire fire force-pushed the vsk-csg-warning-4.4 branch 3 times, most recently from d46cfeb to 4146070 Compare December 13, 2024 14:00
@fire
Copy link
Member Author

fire commented Dec 13, 2024

Question, should child nodes error if their root id broken?

@fire fire mentioned this pull request Dec 13, 2024
10 tasks
@Zireael07
Copy link
Contributor

I say yes, to make it SUPER clear to user that things are broken

@fire fire force-pushed the vsk-csg-warning-4.4 branch from 4146070 to beaf3e7 Compare December 13, 2024 18:13
@fire
Copy link
Member Author

fire commented Dec 13, 2024

@Zireael07 enhanced

@SpockBauru
Copy link

I don't think people will know what manifold means, maybe pointing a link to the CSG docs or just explaining that is not a "closed mesh" is better. "Manifold" is too cryptic.

@fire
Copy link
Member Author

fire commented Dec 13, 2024

Thanks for the review! I'll try to put the entire definition in a short way.

@fire
Copy link
Member Author

fire commented Dec 13, 2024

This is a draft error message. Will try it.

image

Edit 12.

@fire fire force-pushed the vsk-csg-warning-4.4 branch 6 times, most recently from ed6ec9d to e307cad Compare December 13, 2024 19:40
@SpockBauru
Copy link

Tested the windows artifact.

The warning does not appear when you set an invalid mesh, it only changes if there are changes in the tree. It also doesn't disappear when set a valid mesh.

Also when hovering the mouse the line in the in bottom is not splitting and goes over the screen limit of the monitor.

CSG_warning.mp4

@fire
Copy link
Member Author

fire commented Dec 13, 2024

The csg tree modification error is a different bug which has a pull request. #100340

@fire
Copy link
Member Author

fire commented Dec 13, 2024

Also when hovering the mouse the line in the in bottom is not splitting and goes over the screen limit of the monitor.

I wish I had some ideas with shorter wording, but I was stuck on the words.

My phrasing is too long, and the tooltip is not cutting the text.

@fire fire force-pushed the vsk-csg-warning-4.4 branch 3 times, most recently from 3f516c1 to b9b3d2d Compare December 14, 2024 02:46
@fire
Copy link
Member Author

fire commented Dec 14, 2024

@SpockBauru Can you retest after the build is done?

@SpockBauru
Copy link

SpockBauru commented Dec 14, 2024

Tested the Windows artifact and there are no changes on my end, the hint box still goes over the screen limit.

image

Edit: Warning box:
image

Comment on lines 449 to 452
"A manifold mesh forms a solid object without gaps, holes, or loose edges. "
"For a mesh to be manifold, every edge of every triangle must contain the same two vertices (by index) as exactly one other triangle edge, "
"and the start and end vertices must switch between these two edges. The Godot Engine triangle vertices must appear in clockwise order "
"when viewed from the outside of the mesh.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"A manifold mesh forms a solid object without gaps, holes, or loose edges. "
"For a mesh to be manifold, every edge of every triangle must contain the same two vertices (by index) as exactly one other triangle edge, "
"and the start and end vertices must switch between these two edges. The Godot Engine triangle vertices must appear in clockwise order "
"when viewed from the outside of the mesh.";
"A manifold mesh forms a solid object without gaps, holes, or loose edges. Each edge must connect to exactly two faces."

or

Suggested change
"A manifold mesh forms a solid object without gaps, holes, or loose edges. "
"For a mesh to be manifold, every edge of every triangle must contain the same two vertices (by index) as exactly one other triangle edge, "
"and the start and end vertices must switch between these two edges. The Godot Engine triangle vertices must appear in clockwise order "
"when viewed from the outside of the mesh.";
"A manifold mesh forms a solid object without gaps, holes, or loose edges. Each edge must be a member of exactly two faces."

I feel like this is about the right amount of information to put in a warning message: a one or two sentence summary, in friendly and relatively non-technical language.

Even as someone who understands all the concepts, the full-length message is very intimidating. My eyes start to glaze over as I read it. If we need a long and technical explanation like that, it should be in the docs somewhere, not in a warning popup.

Maybe we can put it in the manual, along with some tips on how to troubleshoot non-manifold meshes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tetrapod00 I have a draft of the process for making manifold shapes using Blender's fixing tool. https://docs.google.com/document/d/11KPDwMuZl3c4NSwanhatekAIvUxktURmXJVPkP_a17o/edit?tab=t.0#heading=h.r41iui30hwv7

@AThousandShips AThousandShips added this to the 4.4 milestone Dec 14, 2024
@fire fire force-pushed the vsk-csg-warning-4.4 branch 2 times, most recently from b8f2438 to c5615a0 Compare December 14, 2024 13:25
@fire

This comment was marked as outdated.

@fire fire force-pushed the vsk-csg-warning-4.4 branch from c5615a0 to 4d91691 Compare December 14, 2024 14:04
@SpockBauru
Copy link

SpockBauru commented Dec 14, 2024

Tested the windows artifact, the new wording seems to be good enough, it roughly explains the issue and the hint box does not goes over the screen limit anymore :D

Seems like #100340 has been merged. Is that applied to this PR? Because I'm still not triggering the error message when selecting an invalid mesh. The error icon only appears when there are changes in the tree, not in the mesh itself.

@fire
Copy link
Member Author

fire commented Dec 14, 2024

Setting a mesh on csgmesh3d still is broken with respect to updating war ings

@fire fire force-pushed the vsk-csg-warning-4.4 branch 4 times, most recently from 9354db2 to 75f5547 Compare December 14, 2024 18:45
@fire
Copy link
Member Author

fire commented Dec 14, 2024

I can now move the CSG nodes around and keep the warnings, but I lost the specific error message, and setting a mesh on a CSGMesh3D still does not update until the scene reloads.

@fire fire force-pushed the vsk-csg-warning-4.4 branch from 75f5547 to 5684a1e Compare December 14, 2024 18:54
modules/csg/csg_shape.cpp Outdated Show resolved Hide resolved
modules/csg/csg_shape.cpp Outdated Show resolved Hide resolved
@fire fire force-pushed the vsk-csg-warning-4.4 branch from c69e368 to 441c440 Compare December 16, 2024 20:49
@SpockBauru
Copy link

Tested the Windows artifact, the new tooltip is a way more readable:

image

@0xafbf
Copy link
Contributor

0xafbf commented Dec 17, 2024

For the explanation of the two edges, I think it can be useful to refer to the normals of the faces for explaining the concept of manifold. I suggest something like this:

Every edge of every triangle should link exactly two triangles, and the normals of the triangles should always face outwards

Although I don't know if all-inward faces are allowed. but still, saying "all inward or all outward" should be clear I think.

@fire
Copy link
Member Author

fire commented Dec 17, 2024

The nuance about all the triangles being in the same winding order (like clockwise) from outside the object was removed because it was too technical. The 3mf manifold definition doesn’t require normals.

Edited:

https://github.com/elalish/manifold/wiki/Manifold-Library#manifoldness

@akien-mga akien-mga merged commit 8385a12 into godotengine:master Dec 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@fire fire deleted the vsk-csg-warning-4.4 branch December 17, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

9 participants