Cleaning Hull Algorithm Code

[Kushal-Shah-03]

I've removed some redundant functions and structs, I also removed some comments. I had made some changes in the previous PR, which also removed some redundancy in the code. Should I make the same changes here? (like modifying the for_each loop to merge two for loops)

I was thinking I can pass the vertPos_ and halfedge_ to the function as arguments and then modify them inside the quickhull implementation to reduce the size of the impl Hull function and also reduce the ConvexHull class. What do you think?

[elalish]

You should make your changes in one PR or the other - you don't want to generate a bunch of merge conflicts for yourself. Do whatever feels easiest to keep track of.

[elalish]

Unnecessary?

[elalish]

Let's get rid of CCW too - CCW is always positive in Manifold.

[elalish]

You might just merge getConvexHullAsMesh into Impl::Hull and put this combined implementation in quickhull.cpp. Then you can remove ConvexHull and directly construct halfedge_ and vertPos_.

[Kushal-Shah-03]

I have merged getConvexHullAsMesh into Impl::Hull and removed ConvexHull. I think it's better to keep the Hull function inside Impl, since it follows a similar flow to the other functions inside Impl. Although if you think it's better to put it into quickhull.cpp, I can make that change.

[elalish]

I mean both - notice how our Impl functions are implemented across many files? You can implement Impl::Hull in quickhull.cpp if you want, so long as it imports impl.h.

[Kushal-Shah-03]

I was going through the Face struct to see how we could break it into struct of vectors, I noticed that
isVisibleFaceOnCurrentIteration, visibilityCheckedOnIteration and horizonEdgesOnCurrentIteration are accessed close to each other.
and
inFaceStack, mostDistantPoint, mostDistantPointDist and pointsOnPositiveSide are also accessed close to each other.
he and P are accessed close to all the variables.

So maybe we can break the Face struct into these three parts?

[codecov]

Codecov Report

Attention: Patch coverage is 76.47059% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.68%. Comparing base (d437097) to head (6a7582b).
Report is 80 commits behind head on master.

Files	Patch %	Lines
src/manifold/src/quickhull.cpp	76.47%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
- Coverage   91.84%   89.68%   -2.16%     
==========================================
  Files          37       66      +29     
  Lines        4976     9696    +4720     
  Branches        0     1042    +1042     
==========================================
+ Hits         4570     8696    +4126     
- Misses        406     1000     +594     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pca006132]

I was going through the Face struct to see how we could break it into struct of vectors, I noticed that isVisibleFaceOnCurrentIteration, visibilityCheckedOnIteration and horizonEdgesOnCurrentIteration are accessed close to each other. and inFaceStack, mostDistantPoint, mostDistantPointDist and pointsOnPositiveSide are also accessed close to each other. he and P are accessed close to all the variables.

So maybe we can break the Face struct into these three parts?

I think you probably want to do this in the optimization PR.

[elalish]

This is looking like great progress to me - @pca006132 any changes you want to see before we merge?

[pca006132]

LGTM