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 Merge function #394

Merged
merged 25 commits into from
Apr 10, 2023
Merged

Add Merge function #394

merged 25 commits into from
Apr 10, 2023

Conversation

elalish
Copy link
Owner

@elalish elalish commented Mar 28, 2023

Fixes #363
Fixes #398

This adds a Merge function to MeshGL that will heuristically fill in mergeFromVert and mergeToVert vectors, the purpose of which is to create manifold meshes from GL-style meshes that appear manifold but are not due to unshared verts where materials, UVs, normals, etc. have a boundary.

This will not attempt to fix meshes that are more broken - I'll leave that to external auto-manifold tools for now, since that is more complicated and arbitrary.

@elalish elalish self-assigned this Mar 28, 2023
@elalish elalish marked this pull request as ready for review April 4, 2023 17:20
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Patch coverage: 90.81% and project coverage change: +0.03 🎉

Comparison is base (fbe4cc6) 85.90% compared to head (afd81db) 85.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
+ Coverage   85.90%   85.94%   +0.03%     
==========================================
  Files          36       38       +2     
  Lines        4492     4574      +82     
==========================================
+ Hits         3859     3931      +72     
- Misses        633      643      +10     
Impacted Files Coverage Δ
meshIO/include/meshIO.h 100.00% <ø> (ø)
src/collider/include/collider.h 66.66% <ø> (ø)
src/utilities/include/public.h 55.44% <ø> (-4.04%) ⬇️
src/manifold/src/shared.h 55.05% <66.66%> (+0.40%) ⬆️
src/manifold/src/sort.cpp 90.26% <90.00%> (-0.12%) ⬇️
src/collider/src/collider.cpp 95.00% <100.00%> (+0.08%) ⬆️
src/manifold/include/manifold.h 100.00% <100.00%> (ø)
src/manifold/src/impl.cpp 95.49% <100.00%> (+0.22%) ⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@elalish
Copy link
Owner Author

elalish commented Apr 6, 2023

This also involved moving MeshGL from public.h to manifold.h. I also updated to CI to link with Assimp on Mac.

@elalish
Copy link
Owner Author

elalish commented Apr 6, 2023

Here's the output of the Boolean.Normals test, which has lovely smooth reflections, despite the relatively course meshes involved, which you can see in the facets of the intersections.
image

@@ -580,6 +503,21 @@ void Dump(const std::vector<T>& vec) {
}
std::cout << std::endl;
}

template <typename T>
void Diff(const std::vector<T>& a, const std::vector<T>& b) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

I added an extra debug helper to quickly see just which parts of two vectors are not identical.

template <typename T>
SparseIndices Collisions(const VecDH<T>& queriesIn) const;
SparseIndices Collisions(const VecDH<T>& queriesIn,
bool selfCollision = false) const;
Copy link
Owner Author

Choose a reason for hiding this comment

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

I updated the Collider so that we can collide a vector with itself and not spend a bunch of time reporting that every index collides with itself. This might come in handy down the road too.

#ifndef GSK_GRAPH_LITE_H
#define GSK_GRAPH_LITE_H

#include <algorithm>
#include <cassert>
#include <ciso646> // Added to fix MSVC error
Copy link
Owner Author

Choose a reason for hiding this comment

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

There's not actually much change here - I think it's just formatting. I added this line to make MSVC able to compile and in addition to &&. The only update I pulled in was to not emit warnings about duplicate edges.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting! According to cppreference

This means that in a conforming implementation, including this header has no effect.

Thought MSVC is a conforming implementation :P

@@ -242,20 +242,17 @@ struct CoplanarEdge {
const int edgeIdx = thrust::get<2>(inOut);

const Halfedge edge = halfedge[edgeIdx];
if (!edge.IsForward()) return;
Copy link
Owner Author

Choose a reason for hiding this comment

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

This fixed a bug I found - I was not deduplicating property verts correctly before.

@elalish elalish requested a review from pca006132 April 6, 2023 19:25
* multi-material MeshGL was produced, but its merge vectors were lost due to a
* round-trip through a file format.
*/
bool MeshGL::Merge() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering the result may not be a manifold, the users may want to use IsManifold to check for manifoldness. Should we move the method out of the testing hooks group?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, that reminds me, I've been meaning to get rid of that function. They can just use the Manifold constructor to check if it's manifold.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add this to the documentation and then merge this?

bool MeshGL::Merge() {
std::multiset<Edge> openEdges;

std::vector<int> merge(NumVert());
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me that when mergeFromVert.size() is much smaller than NumVert(), it may be more beneficial to use a sparse hashmap. (just a nitpick)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well, it might save a bit of memory, but not compute. I figure if I'm not allocating more memory than I already have stored in the structure I'm operating on, then the savings probably isn't worth the trouble.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It depends on the size and sparsity. For large data set, saving memory equals to better cache utilization which means better performance. But yeah this may not worth the trouble.

src/manifold/src/sort.cpp Outdated Show resolved Hide resolved
@elalish elalish merged commit aeba267 into master Apr 10, 2023
@elalish elalish deleted the weld2 branch April 10, 2023 00:16
@elalish elalish mentioned this pull request Nov 3, 2023
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
* start Merge

* started Merge function

* added collider

* refactor now builds

* connect vectors

* finished Merge, updated Collider

* added tests

* added sorting, updated graph_lite

* revert submodule change

* fixed indexing

* fixed prop dedupe bug

* fixed test

* update meshIO linking

* fix MSVC

* use assimp on Mac

* add precision handling

* improved tests

* revert test tweak

* update collider

* clean up multiset

* updated docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants