-
Notifications
You must be signed in to change notification settings - Fork 68
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
Removing exceptions #141
Comments
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. |
Returning an empty mesh is an non-reasonable result for all manifold input, but it technically satisfies the criteria of not crashing. |
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. |
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 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. |
From content creation testing.
If the mesh is not manifold, return an empty mesh.
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. |
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?
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? |
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
feels like writing old school C code and is very messy IMO... |
I am partial to |
|
Isn't optional part of c++17? I forget which version. |
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. |
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... |
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 |
I was easily able to cause the boolean operator to fail when mesh editing. |
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. |
I ran the Godot Engine cicd on godotengine/godot#62985. Here are the errors for the iphone.
|
@fire there is probably |
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 I don't want to base this on |
Create strategy to change exception code to return codes.
Some discussion is in the Godot issue.
The text was updated successfully, but these errors were encountered: