removed face from halfedge #939
Conversation
src/manifold/src/impl.cpp
Outdated
| @@ -496,8 +498,9 @@ void Manifold::Impl::CreateHalfedges(const Vec<ivec3>& triVerts) { | |||
| if (h0.startVert != h1.endVert || h0.endVert != h1.startVert) break; | |||
| if (halfedge_[NextHalfedge(pair0)].endVert == | |||
| halfedge_[NextHalfedge(pair1)].endVert) { | |||
| h0.face = -1; | |||
| h1.face = -1; | |||
| // FIXME: this actually does nothing | |||
There was a problem hiding this comment.
This is an interesting one, I was trying to use a vector instead for this and was having weird issues. It turns out we declared h0 and h1 as Halfedge, so these assignments aren't doing anything. Changing them to Halfedge& breaks things, so I just commented them out for now.
There was a problem hiding this comment.
Ha, I think I put this in to try to debug something and then forgot about it. Let's remove.
| @@ -78,13 +78,15 @@ size_t MeshBuilder::addHalfedge() { | |||
| return index; | |||
| } | |||
| halfedges.push_back({}); | |||
| halfedgeToFace.push_back(0); | |||
There was a problem hiding this comment.
Face index is needed in quickhull, so I keep a halfedgeToFace vector for that.
| extrema = reduce(halfedge_.begin(), halfedge_.end(), extrema, Extrema()); | ||
| #endif | ||
|
|
||
| for (size_t i = 0; i < halfedge_.size(); i++) { |
There was a problem hiding this comment.
Simplified the code a bit as well. Was thinking about using pair iterators, but it is too much work. We don't need parallelization here anyway.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #939 +/- ##
==========================================
- Coverage 91.84% 88.45% -3.39%
==========================================
Files 37 62 +25
Lines 4976 8650 +3674
Branches 0 1042 +1042
==========================================
+ Hits 4570 7651 +3081
- Misses 406 999 +593 ☔ View full report in Codecov by Sentry. |
src/manifold/src/impl.cpp
Outdated
| @@ -496,8 +498,9 @@ void Manifold::Impl::CreateHalfedges(const Vec<ivec3>& triVerts) { | |||
| if (h0.startVert != h1.endVert || h0.endVert != h1.startVert) break; | |||
| if (halfedge_[NextHalfedge(pair0)].endVert == | |||
| halfedge_[NextHalfedge(pair1)].endVert) { | |||
| h0.face = -1; | |||
| h1.face = -1; | |||
| // FIXME: this actually does nothing | |||
There was a problem hiding this comment.
Ha, I think I put this in to try to debug something and then forgot about it. Let's remove.
src/manifold/src/impl.cpp
Outdated
| // if (halfedge_[pair0].face < 0) { | ||
| // halfedge_[pair0] = {-1, -1, -1, -1}; | ||
| // halfedge_[pair1] = {-1, -1, -1, -1}; | ||
| // } else { |
There was a problem hiding this comment.
will remove tmr when I wake up.
src/manifold/src/quickhull.cpp
Outdated
| halfedges[index].face = index / 3; | ||
| } | ||
| // for (size_t index = 0; index < halfedges.size(); index++) { | ||
| 8000 | // halfedges[index].face = index / 3; |
|
Measurable overall, like 26k ms to 25k ms in total. Another interesting idea: We don't really need both start and end vertex, as we can get the opposite one from the paired halfedge. The problem is whether or not it is worth the changes (less readable perhaps?) and uncertain performance implication (cache misses?). |
|
Yeah, I thought about that when I made |
Just some cleanup, I think we talked about
facebeing redundant because we can just compute it from the index, so I took the time to remove it.