-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 technical definition for manifoldness #10419
Conversation
tutorials/3d/csg_tools.rst
Outdated
- it must be closed, | ||
- it must not self-intersect, | ||
- it must not contain internal faces, | ||
- every edge must connect to only two other faces. |
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.
We need to check that this layman's definition is still correct
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.
The triangle vertices must appear clockwise when viewed from outside the Godot Engine manifold mesh.
It can be split into two layperson explanations.
- The Godot Engine triangle mesh winding order should be clockwise.
- All the windings should be clockwise. There is no mixing between clockwise and counterclockwise.
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.
@elalish Do you know if we need to explain something about normals? I didn't see that in your explanation.
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.
Technically the three of these except for self-intersection are more precisely defined by the manifold definition below. Self-intersection should be avoided, but Manifold is robust to it at least, and it's difficult to test in practice.
As for winding direction - the manifoldness definition below already restricts us to an oriented 2-manifold, so there can't be any mixing of CW and CCW. Like self-intersection, CCW can be hard to test reliably. And manifold will handle inverted meshes just fine - it's simply that CW triangles will lead to negative volume. I would trust a positive volume check over checking CCW. Likewise, normals are not very trustworthy.
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 guess I would say the triangles are CCW - this is what defines the sign of the volume and inside vs outside. There's nothing you can do to make them CW.
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.
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.
REQUIRED
- it must be closed,
- every edge must connect to only two other faces.
- volume must exist
NOT RECOMMENDED
- negative volume
- self-intersection
- interior faces
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.
Hmm, Manifold is in the lower-right - Z-up and right-handed, so it looks like Godot has the same handedness? Or does Godot just use CW triangle winding as positive?
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.
https://docs.godotengine.org/en/latest/classes/class_arraymesh.html#arraymesh
Note: Godot uses clockwise winding order for front faces of triangle primitive modes.
What do we need to do to get this completed? |
b2b403c
to
948caba
Compare
If it's all correct to you now, this is mergable, but I'd like to leave it up for a day or so before merging to give others a chance to review, since I did write it myself. |
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.
LGTM
948caba
to
ea9fad5
Compare
Pushed a minor wording correction. |
Merged. Thanks fire and especially elalish for taking the time to review! |
Part of #10417. Also documents changes for godotengine/godot#94321.