Removing QuickHull dependency

[Kushal-Shah-03]

I have removed the dependency of quickhull, I was not sure whether we wanted to delete the Makefile for third-party so, I have kept that for now. I also removed the Vector3 struct of the original implementation,replacing it with glm::vec3, and some functions that acted on Vector3. I wanted to know how to use squaredLength() functions and length() functions that show up in editor.d.ts, but are not a part of glm, currently I just added those functions in the namespace of mathutils, but was wondering whether there was a way to access them?

[starseeker]

Looks like some nanobind changes are in the PR - are they needed?

[Kushal-Shah-03]

No they aren't, I didn't realize that changes to nanobind were made, I am not sure how that happened. Also, I am unsure as to how to revert those, any ideas?

[starseeker]

Not completely sure - maybe start here? https://stackoverflow.com/questions/5669358/can-i-do-a-partial-revert-in-git

[Kushal-Shah-03]

Thanks, for the help, seems like I've fixed that issue.

[pca006132]

also please give us a summary on what you have changed comparing to the upstream version

[Kushal-Shah-03]

Ok, so basically I compiled all the header files into quickhull.hpp, and then modified some things to make the code compile. After, that I just worked on replacing Vector3 struct and some of it's functions with glm::vec3. I also removed the dependency from the Makefile, now will remove the directory as suggested.

[elalish]

Any troubler resolving conflicts?

[elalish]

Can we do something like halfedges.push_back({b, 6, 0, 1}) instead?

[elalish]

Or even just list them all together: halfEdges = {{b, 6, 0, 1}, {...}, ...};

[elalish]

Considering we have our own halfedge mesh data structure in Impl already, is there a chance we can use that instead of this one?

[Kushal-Shah-03]

I've made the requested changes, and resolved them for clarity. As discussed I'll tackle the Half edge mesh data structure in another PR.

[Kushal-Shah-03]

Ok, so the tests are failing because

CMake Error at bindings/python/CMakeLists.txt:17 (nanobind_add_module):
Unknown CMake command "nanobind_add_module".


-- Configuring incomplete, errors occurred!

Any ideas on how I can fix it?

[pca006132]

revert this line.

[pca006132]

you removed both the third-party directory inside src and bindings/python, but they are two different things.

[Kushal-Shah-03]

So, I think I'm encountering this error now, although I am not sure

lcov: ERROR: 'exclude' pattern '*/third_party/*' is unused.
(use "lcov --ignore-errors unused ..." to bypass this error)

Based on a search, I think it could be caused by
https://github.com/Kushal-Shah-03/manifold/blob/00dd951c03d0b1117c1efe3f86d27f6f6df5dd22/.github/workflows/manifold.yml#L68
or
https://github.com/Kushal-Shah-03/manifold/blob/00dd951c03d0b1117c1efe3f86d27f6f6df5dd22/Doxyfile#L941

But, since I'm not sure could you suggest how to solve this issue?

[pca006132]

yeah just the manifold.yml, remove the third_party pattern.

[codecov]

Codecov Report

Attention: Patch coverage is 72.56098% with 135 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (d437097) to head (95f9721).
Report is 70 commits behind head on master.

Files	Patch %	Lines
src/manifold/src/quickhull.h	64.46%	97 Missing ⚠️
src/manifold/src/quickhull.cpp	81.99%	38 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #881      +/-   ##
==========================================
- Coverage   91.84%   89.19%   -2.65%     
==========================================
  Files          37       66      +29     
  Lines        4976     9691    +4715     
  Branches        0     1051    +1051     
==========================================
+ Hits         4570     8644    +4074     
- Misses        406     1047     +641     

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

[Kushal-Shah-03]

I've sent a request for re-review, could you go through it once, and tell me if I should change something.

[pca006132]

I think you should add the copyright header from the upstream library (or make one for them), as well as change the define to pragma once that we typically use.

[Kushal-Shah-03]

Should I add both Manfiold copyright header and the upstream header? (also they did mention that we don't need to add anything so what should I add)

[pca006132]

I think we can include both.

[pca006132]

why the additional bracket?

[Kushal-Shah-03]

I missed it, I'll change that.

[pca006132]

Not sure if this is the best way to put it, I think we typically just acknowledge the source, put an URL as well as state its license. From my understanding, the code is under a certain license doesn't mean the author has no copyright over it.

[Kushal-Shah-03]

This was what he had included in his code. I can include the link, but I don't know which license to acknowledge, could you just tell me what exactly do I need to include I will add that below our license.

[elalish]

Public domain does mean there is no longer a copyright holder. We should modify this line, since our version is no longer public domain (see the apache license above). I would change this to "Derived from the public domain work of Antti Kuukka at github/..."

[Kushal-Shah-03]

I've made the change to the license, I hope it's alright now.

[Kushal-Shah-03]

Thank you for merging #875, now for the next steps, I was wondering what changes should I make in this pull request?

[elalish]

Start by merging master and re-enabling your tests - they should work in this branch, right? In fact, that's probably all that's needed to merge this - I believe the rest we're planning for follow-on PRs, right?

[Kushal-Shah-03]

So, yeah ideally it should pass the tests, but I realized a flaw, since while calculating in the algorithm we use a variable epsilon which changes with scale, we won't be able to find the exact epsilon to check whether the point is positive or not, and since when we store it in a mesh or a manifold, the pts are converted back to floats rather than the double used to calculate the hull.

I am thinking we can either shift the check for valid hull inside the algorithm implementation itself, so we can check relative to the variable epsilon, or we would need to check using a floating epsilon if we check outside the implementation.

What would you suggest?
Also, I checked despite the fact that the tests don't "pass", the outputs of both the "failing tests" are infact convex, so we would need to modify how we check for convexity

[elalish]

We'll be switching to double soon, so as a stopgap, can you just put epsilon as a parameter to IsConvex() and then pick values for each test that makes sense?

[Kushal-Shah-03]

I've made the change you suggested, currently I'm using the m_epsilon value that the algorithm generates as the epsilon for the two tests. I am looking into how I can revert the nanobind change I made by mistake

[elalish]

Good work - when the tests pass, let's merge it! I look forward to seeing this merge deeper into our own data structures next.

[Kushal-Shah-03]

Good work - when the tests pass, let's merge it! I look forward to seeing this merge deeper into our own data structures next.

Yeah, I think this seems good for this PR, I look forward to integrating it further into our data structures, I will start working on it from tomorrow.