CalculateNormals and Smooth #771
Conversation
|
I should mention this is technically a breaking change in that the behavior of |
bindings/python/manifold3d.cpp
Outdated
| .def("calculate_normals", &Manifold::CalculateNormals, | ||
| nb::arg("normal_idx"), nb::arg("min_sharp_angle"), | ||
| manifold__calculate_normals__normal_idx__min_sharp_angle) | ||
| .def("smooth", &Manifold::Smooth, nb::arg("min_sharp_angle"), |
There was a problem hiding this comment.
@wrongbad I appear to have messed up these python bindings somehow or maybe just the docs? Any thoughts on what I did wrong?
|
Will have a look tmr. Btw, any idea why the test failed? |
I messed up the python bindings somehow. |
There was a problem hiding this comment.
Just some little comments for now, has to sleep. Also, I think for the python issue you can use the following patch:
diff --git a/bindings/python/manifold3d.cpp b/bindings/python/manifold3d.cpp
index c85b61c..a4f0bf7 100644
--- a/bindings/python/manifold3d.cpp
+++ b/bindings/python/manifold3d.cpp
@@ -318,9 +318,13 @@ NB_MODULE(manifold3d, m) {
.def("calculate_normals", &Manifold::CalculateNormals,
nb::arg("normal_idx"), nb::arg("min_sharp_angle"),
manifold__calculate_normals__normal_idx__min_sharp_angle)
- .def("smooth", &Manifold::Smooth, nb::arg("min_sharp_angle"),
- nb::arg("min_smoothness"),
- manifold__smooth__min_sharp_angle__min_smoothness)
+ .def(
+ "smooth",
+ [](const Manifold &self, float minSharpAngle, float minSmoothness) {
+ return self.Smooth(minSharpAngle, minSmoothness);
+ },
+ nb::arg("min_sharp_angle") = 60, nb::arg("min_smoothness") = 0,
+ manifold__smooth__min_sharp_angle__min_smoothness)
.def("refine", &Manifold::Refine, nb::arg("n"), manifold__refine__n)
.def("refine_to_length", &Manifold::RefineToLength, nb::arg("length"),
manifold__refine_to_length__length)I think nanobind is just confused about the 3 overloads we have for Smooth.
| std::unordered_map<int, int> oldHalfedge2New; | ||
| for (int tri = 0; tri < NumTri(); ++tri) { | ||
| int oldTri = meshRelation_.triRef[tri].tri; | ||
| for (int i : {0, 1, 2}) oldHalfedge2New[3 * oldTri + i] = 3 * tri + i; |
There was a problem hiding this comment.
curious if a vector can be used instead, and maybe we can have parallelism here?
There was a problem hiding this comment.
I tried that first, but it's made difficult because there are potentially more triangles in the old indices than the new and I'm not sure how many.
src/manifold/src/smoothing.cpp
Outdated
| } | ||
| } | ||
| if (faceNeighbors > 1) { | ||
| triIsFlatFace[tri] = true; |
There was a problem hiding this comment.
can parallelize this as well?
if we want to avoid repeated work, we should check faceTris[tri] before the start of the j loop.
There was a problem hiding this comment.
I think for correctness we can't do the early-out.
| : vertNormal_[vert]; | ||
| int lastProp = -1; | ||
| do { | ||
| current = NextHalfedge(halfedge_[current].pairedHalfedge); |
There was a problem hiding this comment.
thinking about it, maybe we should just make a halfedge iterator that loops through all the halfedges connected to a vertex, to make this pattern more readable to beginners.
There was a problem hiding this comment.
Yeah, it's a good idea - I just feel like I'm often making small tweaks to it: stopping part-way instead of doing the full circle 8000 , or sometimes including vs excluding the first or last edge. Would love some thoughts on ways to make this pattern more generic, because I do use it all the time.
There was a problem hiding this comment.
It should be possible to make an iterator out of this. I will open an issue about this.
src/manifold/src/smoothing.cpp
Outdated
| for (int e = 0; e < halfedge_.size(); ++e) { | ||
| if (!halfedge_[e].IsForward()) continue; | ||
| const int pair = halfedge_[e].pairedHalfedge; | ||
| const float dihedral = glm::degrees( |
There was a problem hiding this comment.
maybe it will be slightly faster if we convert the angle into radian and only compare using radian, instead of doing conversion of degree to radian in a loop. same for the dihedral calculation below as well.
src/manifold/src/smoothing.cpp
Outdated
| if (prop == lastProp) continue; | ||
| lastProp = prop; | ||
| for (int i = 0; i < oldNumProp; ++i) | ||
| meshRelation_.properties[prop * numProp + i] = |
bindings/python/manifold3d.cpp
Outdated
| .def( | ||
| "smooth", | ||
| [](const Manifold &self, float minSharpAngle, float minSmoothness) { | ||
| return self.Smooth(minSharpAngle, minSmoothness); |
There was a problem hiding this comment.
@pca006132 Alas, no luck here. I wish the error wasn't such a pile of spew. Also, even when I build with MANIFOLD_PYBIND=ON I don't get the error - is something else required to make it build?
There was a problem hiding this comment.
weird... will debug this
There was a problem hiding this comment.
It seems that nanobind is unhappy with smooth being both a static and instance method for Manifold`.
Critical nanobind error: nb::detail::nb_func_new("smooth"): mismatched static/instance method flags in function overloads!
There was a problem hiding this comment.
Do we need to make the names different? Kind of annoying, but not the end of the world.
There was a problem hiding this comment.
yeah I think that can fix the issue.
There was a problem hiding this comment.
Okay, I'm going with SmoothOut - slightly more verb sounding.
|
When I try to build your branch it passes for me. Maybe you should push your branch with this in |
This is the next iteration on improving our mesh smoothing, now that we have
RefineToLength: we now have aSmoothmember function that is slightly higher-level than ourManifold::Smoothconstructor. Rather than specifying smoothness at particular edges, now you just specify an angle beyond which edges become sharpened. You can also specify aminSmoothnessto soften those sharp edges uniformly.Additionally, I've added
CalculateNormals, which works likeCalculateCurvature, but makes vert normals for smooth reflections and e.g. glTF export. It uses the same sharp angle specification as the aboveSmoothand automatically duplicates prop verts as necessary to create sharp edges.Both of these functions take special care with flat faces of three or more triangles - these are always considered flat and so have all their vert normals match their face normal. This also makes it easy to have proper curvature applied when e.g. a circular fillet meets a flat face, in which case traditional pseudo-normals give poor results.
I'm not considering quads to be flat as I plan to give them special handling in our
Smooth/Refinefunctions soon.