Removing exceptions about manifold

[elalish]

Okay, so the good news from #154 is I think I know how to make the triangulator exceptions optional (and thus the Boolean robust), which was the main blocker I was worried about. Most of the rest are really assertions, so making them compile out seems reasonable. The last one is exceptions in the Manifold(Mesh) constructor, where the input isn't valid. I'm wondering if we should just return an empty manifold and add a getError() method that'll return a string saying why you got an empty manifold. It'll be an empty string if the mesh is empty for good reason (decimated out of existence, non-overlapping intersection, etc).

I don't want to base this on DEBUG/RELEASE because thrust doesn't like DEGUB mode. I'm thinking I'll make a different flag to disable exceptions in the pre-processor. Then I'll also add a runtime option to disable them even when they've been compiled, so it's easier to test both ways. I definitely need to be able to compile with them to debug quickly, and they'll keep our tests catching more problems. Effectively enabling exceptions will be equivalent to "fail on self-overlapping manifolds and other bugs", rather than "do your best".

[pentacular]

I am partial to absl::StatusOr<T> myself.

[elalish]

So, I'm not yet convinced that return codes are superior. Please convince me here! The gist I have is that exceptions weren't supported well on old compilers, but mostly are now: #91 (comment), and that the STL is somehow okay even if exceptions are not, which I also don't understand: #91 (comment)

It sounds to me like the underlying issue is you'd like for Manifold to successfully return a reasonable result for all manifold input, which I would like as well. There are probably a few bugs left to fix, so the best thing there is to submit tests that reproduce them. The bigger issue is I don't currently make any guarantees about handling self-overlapping input meshes, and I don't have an efficient way to test for them either. I would like to make an algorithm to fix these, but that'll take considerable time. Until then, general input meshes aren't going to be safe.

[fire]

Returning an empty mesh is an non-reasonable result for all manifold input, but it technically satisfies the criteria of not crashing.

[fire]

STL is ok in the sense that it still crashes Godot Engine on exception throw but there's a feature to disable it. https://stackoverflow.com/questions/553103/can-i-disable-exceptions-in-stl

It's impractical to tell all third party libraries to port from STL to the host library's internal data structures. Especially if for example Manifold requires the stl multithreading operators on data structures.

[pca006132]

Not sure how game devs view this, but from Rust we categorize errors into two types: expected (input check, file exists etc.) and not expected (internal asserts that indicates bugs when failed)

For the first kind, we will use error code or Result<T> here, and users are expected to check for errors (or unwrap to abort if error). For the second kind, users are not expected to handle them and we can just abort.

I guess we can do something similar here. For user errors, we return error code. For assertion failure, we throw if exception is enabled and abort otherwise. I think using error code for all assert checks will make the API very complicated and these checks should be internal checks and not exposed to users.

[fire]

From content creation testing.

1. Expected (input check, file exists etc.)

If the mesh is not manifold, return an empty mesh.

2. Not expected

Boolean merge operation on two manifold meshes returns a non manifold mesh causing the next operator to fail too. I've also seen quad faces decomposing to the wrong faces [fixed]. See also #125

If the case that the boolean merge operator crashes the engine, the mean time to engine crash from the Manifold library was common about 10-30 seconds of editing.

I guess my concern is how to increase the average time of manifold crash to be around an hour.

For online 3d creation games, I can imagine a 2 day session.

[pca006132]

For #125, do you mean it fixed a crash or it causes a new crash?

For cases that are not expected, I mean the assertion failure is not expected by this library, we expect it to always hold. Violation implies a bug on our side and there is probably nothing the users can do to recover from it, apart from cancelling the operation. Do you think it would be good to abort in that case?

If the mesh is not manifold, return an empty mesh.

I'm not sure if this is a good way to handle errors in general. Perhaps we can return an error code and you convert the error code into an empty mesh?

[pca006132]

And I'm wondering if there is any library that can simplify error code handling, something similar to result type in Rust.

I found this one: https://github.com/ned14/outcome but not sure if it is a good fit.

Writing something like

error_t status = ...
if (status != 0) return status;

feels like writing old school C code and is very messy IMO...

[pca006132]

absl seems to be a fairly large dependency, idk if Godot Engine devs (@fire) will like it.

[fire]

Isn't optional part of c++17? I forget which version.

[makc]

fwiw emscripten has exceptions disabled by default because of the overhead - if we had no exceptions in manifold, it would run a tiny bit faster in js. so, not a big deal, but nice to have.

[pca006132]

Isn't optional part of c++17? I forget which version.

Optional is different from the result type we talk about above. The result type can be either the original return type or an error type (error code for example), with some convenient methods for handling them. This can make the interface a bit nicer than using error code and pass return values by parameters...

[elalish]

std::optional sounds like it could be a good way to go. I have no qualms about upgrading to C++17.

I still think the more fundamental problem is that self-overlapping meshes aren't allowed as valid input, but also aren't checked for; they just tend to sometimes make Boolean ops fail (and the error message isn't clear either). A good first step would be to add an isOverlapped method, and next the harder part: a water-tighting algorithm to fix self-overlaps.

[fire]

I was easily able to cause the boolean operator to fail when mesh editing.

[fire]

The test cases is in live editing where you duplicate the mesh. Let's say it's a sphere and then slowly merge with physical locations moving closer and closer.

[fire]

I ran the Godot Engine cicd on godotengine/godot#62985. Here are the errors for the iphone.

In file included from thirdparty/manifold/polygon/src/polygon.cpp:15:
In file included from thirdparty/manifold/polygon/include/polygon.h:18:
thirdparty/manifold/utilities/include/structs.h:59:5: error: cannot use 'throw' with exceptions disabled [2]
     throw Ex(output.str());
     ^
thirdparty/manifold/polygon/src/polygon.cpp:841:5: error: cannot use 'throw' with exceptions disabled [2]
     throw;
     ^
thirdparty/manifold/polygon/src/polygon.cpp:844:5: error: cannot use 'throw' with exceptions disabled [2]
     throw;
     ^
thirdparty/manifold/polygon/src/polygon.cpp:830:3: error: cannot use 'try' with exceptions disabled [2]
   try {
   ^
In file included from thirdparty/manifold/polygon/src/polygon.cpp:15:
In file included from thirdparty/manifold/polygon/include/polygon.h:18:
thirdparty/manifold/utilities/include/structs.h:59:5: error: cannot use 'throw' with exceptions disabled [2]
     throw Ex(output.str());
     ^
thirdparty/manifold/polygon/src/polygon.cpp:100:7: note: in instantiation of function template specialization 'manifold::AlwaysAssert<manifold::topologyErr>' requested here [2]
       ALWAYS_ASSERT(triangulator.NumTriangles() > 0, topologyErr,
       ^
thirdparty/manifold/utilities/include/structs.h:64:3: note: expanded from macro 'ALWAYS_ASSERT'
   AlwaysAssert<EX>(condition, __FILE__, __LINE__, #condition, msg);
   ^
thirdparty/manifold/utilities/include/structs.h:59:5: error: cannot use 'throw' with exceptions disabled [2]
     throw Ex(output.str());
     ^
thirdparty/manifold/polygon/src/polygon.cpp:391:5: note: in instantiation of function template specialization 'manifold::AlwaysAssert<std::logic_error>' requested here [2]
     ALWAYS_ASSERT(pair != activePairs_.end(), logicErr, "No pair to remove!");
     ^
thirdparty/manifold/utilities/include/structs.h:64:3: note: expanded from macro 'ALWAYS_ASSERT'
   AlwaysAssert<EX>(condition, __FILE__, __LINE__, #condition, msg);
   ^
thirdparty/manifold/utilities/include/structs.h:59:5: error: cannot use 'throw' with exceptions disabled [2]
     throw Ex(output.str());
     ^
thirdparty/manifold/polygon/src/polygon.cpp:596:7: note: in instantiation of function template specialization 'manifold::AlwaysAssert<manifold::geometryErr>' requested here [2]
       ALWAYS_ASSERT(
       ^
thirdparty/manifold/utilities/include/structs.h:64:3: note: expanded from macro 'ALWAYS_ASSERT'
   AlwaysAssert<EX>(condition, __FILE__, __LINE__, #condition, msg);
   ^
7 errors generated.
scons: *** [thirdparty/manifold/polygon/src/polygon.iphone.opt.arm64.o] Error 1

[makc]

@fire there is probably -fno-exceptions somewhere in the build command