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

Crashes due to error in manifold::UnionFind. #1084

Closed
fire opened this issue Dec 1, 2024 · 15 comments · Fixed by #1092
Closed

Crashes due to error in manifold::UnionFind. #1084

fire opened this issue Dec 1, 2024 · 15 comments · Fixed by #1092

Comments

@fire
Copy link
Contributor

fire commented Dec 1, 2024

godotengine/godot#99888

https://github.com/elalish/manifold/releases/tag/v3.0.0

Was wondering how to narrow this bug.

Crashes due to error in manifold::UnionFind.

@pca006132
Copy link
Collaborator

Is it possible to turn this into a test case in boolean_complex_test? Export the mesh and the associated boolean operation.

@fire
Copy link
Contributor Author

fire commented Dec 1, 2024

We’re not able to export for test case recreation because the construction of the csg shape crashes.

@pca006132
Copy link
Collaborator

What would happen if you try to compile with MANIFOLD_DEBUG=ON?

@fire
Copy link
Contributor Author

fire commented Dec 1, 2024

Will try

@fire
Copy link
Contributor Author

fire commented Dec 1, 2024

Added bounds checking in unionfindxy

  void unionXY(I x, I y) {
    # Add check here.
    if (x < 0 || x >= parents.size() || y < 0 || y >= parents.size()) return;
    if (ranks[x] == 0) ranks[x] = 1;
    if (ranks[y] == 0) ranks[y] = 1;
    x = find(x);
    y = find(y);
    if (x == y) return;
    if (ranks[x] < ranks[y]) std::swap(x, y);
    if (ranks[x] == ranks[y]) ranks[x]++;
    parents[y] = x;
  }

@pca006132
Copy link
Collaborator

The issue is, why can we have invalid x and y?

@fire
Copy link
Contributor Author

fire commented Dec 1, 2024

node_3d.zip

It is unknown, but here's the mesh as a glb.

@pca006132
Copy link
Collaborator

I guess there is something wrong in CreateFaces. @elalish

@fire
Copy link
Contributor Author

fire commented Dec 1, 2024

The fix does not convince me, but here's the code commit. V-Sekai/godot@ad8d1d6

@elalish
Copy link
Owner

elalish commented Dec 1, 2024

Well, the good news is I'm already overhauling CreateFaces in #1040 to use a totally different algorithm. The bad news is it's going to need some work still.

@elalish
Copy link
Owner

elalish commented Dec 3, 2024

@fire I made a simple test with your object and it seems to work just fine:

TEST(Manifold, Merge2) {
  std::string file = __FILE__;
  std::string dir = file.substr(0, file.rfind('/'));
  MeshGL cylinder = ImportMesh(dir + "/models/" + "node_3d.glb");
  EXPECT_TRUE(cylinder.Merge());
  Manifold fixed(cylinder);
  EXPECT_NEAR(fixed.Volume(), 2.83, 0.01);
}

Can you send a PR with a test that actually fails so we can track down the problem?

@fire
Copy link
Contributor Author

fire commented Dec 3, 2024

I am having trouble. Can you supply code that dumps the entire triangle mesh before the two crash condition in unionxy if (x < 0 || x >= parents.size() || y < 0 || y >= parents.size()) return; and label2vert !(label >= 0 && label < label2vert.size() && label2vert[label] >= 0)?

So I can place the mesh dumper in this condition? V-Sekai/godot@ad8d1d6#diff-56b5b289049405153f8155e0989091eb012953e0775f43978dcb0fc020f7f696R154

I'll try to dump on the godot engine inputs.

@elalish
Copy link
Owner

elalish commented Dec 3, 2024

I believe @pca006132 was just working on a function for dumping an OBJ to the command prompt. There's also this (with MANIFOLD_DEUBG=ON):

vertPos_.Dump();
for(int tri=0; tri<NumTri(); ++tri){
  std::cout<<halfedge_[3*tri].startVert<<", "
    <<halfedge_[3*tri+1].startVert<<", "
    <<halfedge_[3*tri+2].startVert<<", "<<std::endl;
}

@pca006132
Copy link
Collaborator

Yeah I had that, but considering our meshio code seems to have difficulty loading the obj file and I have no idea if it can load 64 bit floats precisely, I may switch to some custom format which dump things like tolerances as well.

Also, the code will dump early if the boolean result is self-intersecting. Ideally we should be able to check if it is epsilon-valid, but it probably requires a solver if we want to be strict about it (compute some global perturbation such that the constraints as satisfied?).

@pca006132
Copy link
Collaborator

The plan is to get this done after the csg tree changes.

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 a pull request may close this issue.

3 participants