Skip to content
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 MinGap function #765

Merged
merged 49 commits into from
Apr 2, 2024
Merged

Add MinGap function #765

merged 49 commits into from
Apr 2, 2024

Conversation

mleleszi
Copy link
Contributor

@mleleszi mleleszi commented Mar 6, 2024

implements #186 / opencax/GSoC#82

@elalish
Copy link
Owner

elalish commented Mar 6, 2024

Looking at the CI - is there a chance you're working from an out-of-date branch? You might want to try git pull origin master. Let us know when you're ready for some review.

@pca006132
Copy link
Collaborator

the CI seems fine, the function TriangleDistance indeed is only declared but undefined.

@mleleszi
Copy link
Contributor Author

mleleszi commented Mar 6, 2024

Added the missing definition, so the build should work fine now. (tests are still gonna fail though)

@mleleszi mleleszi changed the title MinGap POC Add MinGap function Mar 11, 2024
@mleleszi
Copy link
Contributor Author

@elalish @pca006132
I probably still need to refactor/clean up a bit but I think It's ready for an initial review.

@mleleszi mleleszi marked this pull request as ready for review March 14, 2024 15:51
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 97.53086% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 88.53%. Comparing base (d437097) to head (3786209).
Report is 10 commits behind head on master.

Files Patch % Lines
bindings/c/manifoldc.cpp 0.00% 3 Missing ⚠️
src/utilities/include/tri_dist.h 98.27% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
- Coverage   91.84%   88.53%   -3.31%     
==========================================
  Files          37       56      +19     
  Lines        4976     8096    +3120     
  Branches        0      871     +871     
==========================================
+ Hits         4570     7168    +2598     
- Misses        406      928     +522     

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

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is looking pretty good! Since the public API looks stable and good you can also go ahead and add the various bindings as well - look at one of our recent PRs that added an API and you'll see which files to change.

src/manifold/include/manifold.h Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/sort.cpp Outdated Show resolved Hide resolved
src/utilities/include/tri_dist.h Outdated Show resolved Hide resolved
test/mingap_test.cpp Outdated Show resolved Hide resolved
test/mingap_test.cpp Outdated Show resolved Hide resolved
@pca006132
Copy link
Collaborator

but maybe you want to apply for another project considering how fast you are doing!

src/manifold/src/impl.cpp Outdated Show resolved Hide resolved
@mleleszi mleleszi requested a review from elalish March 29, 2024 08:41
Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor things, but this looks good to me. Let's see what @pca006132 thinks.

test/manifold_test.cpp Outdated Show resolved Hide resolved
test/manifold_test.cpp Show resolved Hide resolved
test/manifold_test.cpp Outdated Show resolved Hide resolved
test/manifold_test.cpp Show resolved Hide resolved
src/utilities/include/tri_dist.h Outdated Show resolved Hide resolved
src/utilities/include/tri_dist.h Show resolved Hide resolved
@elalish elalish requested a review from pca006132 March 29, 2024 16:26
@elalish
Copy link
Owner

elalish commented Mar 29, 2024

By the way, excellent work on this feature! How would you like to approach your GSOC application? Do you know what you'd like to work on next?

@pca006132
Copy link
Collaborator

Overall this looks great!

@mleleszi
Copy link
Contributor Author

mleleszi commented Mar 29, 2024

@elalish @pca006132
Thank you, I've had a lot of fun working on this! However, I've realized that I might not have enough time over the summer to commit to GSOC. But that doesn't mean I'll stop contributing, I plan to keep at this long term! I'm already looking at what to pick up next. I might take a look at #723 or one of the GSOC tasks that don't get picked for GSOC (maybe #667). I'll take a look at the other open issues as well. (I'm open to suggestions as well)

@elalish
Copy link
Owner

elalish commented Mar 29, 2024

@mleleszi It's great to hear that you have a realistic understanding of your time commitments. Good expectation setting is one of the most important and least common traits of a software engineer. We would certainly welcome more contributions as your time allows!

I think #723 would be great - it's nice that we have such a clear example where we know there are orders of magnitude speed increase available. And since you already have some familiarity with our Collider from this PR, that should give you a leg up. Perf PRs are nice because you already have an algorithm that's getting the right answer with lots of tests - all you need to do is not let its answers change.

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job parallelizing! One minor change and let's merge this.

src/manifold/src/impl.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, thank you!

@elalish elalish merged commit 3258152 into elalish:master Apr 2, 2024
18 checks passed
@elalish elalish mentioned this pull request Apr 2, 2024
@elalish elalish mentioned this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants