8000 Misc. changes by pca006132 · Pull Request #940 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Misc. changes #940

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Sep 21, 2024
Merged

Misc. changes #940

merged 7 commits into from
Sep 21, 2024

Conversation

pca006132
Copy link
Collaborator
@pca006132 pca006132 commented Sep 19, 2024 •
edited by elalish
Loading

Fixes #744.

  1. Propagate errors in mesh boolean, transform and compose. Probably there are still some other operations that we should propagate the error.
  2. Use 64-bit integers for MeshGL64. Note that this is not yet tested, I just checked it can compile. (ideally nothing else can go wrong considering we don't do raw pointer operations...). We should check if the APIs make sense.

I don't plan to do 64-bit run ID or mesh ID, as I don't think users will ever want that...? It might make sense in some scientific computation situation to handle large meshes, but it doesn't make sense to do boolean on several hundred million meshes, I guess...

Checklist for error propagation, feel free to add to this list, so I can add them in this PR:

  1. Warp

@pca006132 pca006132 requested a review from elalish September 19, 2024 07:45
@codecov Codecov
Copy link
codecov bot commented Sep 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 62.71186% with 22 lines in your changes missing coverage. Please review.

Project coverage is 88.25%. Comparing base (d437097) to head (31d1588).
Report is 99 commits behind head on master.

Files with missing lines Patch % Lines
src/manifold/src/boolean_result.cpp 33.33% 6 Missing ⚠️
src/manifold/src/manifold.cpp 76.00% 6 Missing ⚠️
src/manifold/src/csg_tree.cpp 50.00% 3 Missing ⚠️
bindings/c/manifoldc.cpp 33.33% 2 Missing ⚠️
src/manifold/src/impl.cpp 33.33% 2 Missing ⚠️
src/utilities/include/manifold/iters.h 0.00% 2 Missing ⚠️
src/manifold/src/impl.h 85.71% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #940      +/-   ##
==========================================
- Coverage   91.84%   88.25%   -3.59%     
==========================================
  Files          37       62      +25     
  Lines        4976     8668    +3692     
  Branches        0     1049    +1049     
==========================================
+ Hits         4570     7650    +3080     
- Misses        406     1018     +612

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

elalish
elalish approved these changes Sep 19, 2024
View reviewed changes
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.

As far as a checklist, I guess it's everything non-static in manifold.h that returns a Manifold?

@pca006132
Copy link
Collaborator Author

Thinking about it, some functions in Manifold should also return 64-bit integer for this.

@pca006132
Copy link
Collaborator Author

Notably, OriginalID is now returning std::optional<size_t>, and is empty when the mesh is not an original.

@pca006132
Copy link
Collaborator Author

I was looking into other functions that do unary operations on a manifold. Apparently, they copy the Impl data structure, so I think they should have the error code copied with them together. I forgot who can we have those error conditions and whether our internal data structures are still safe to operate on when an error occurs though, so not sure if I should add guard in those operations to early return.

@pca006132
Copy link
Collaborator Author

maybe I will fix the wasm issue tmr

@elalish
Copy link
Owner
elalish commented Sep 20, 2024

IIRC, the manifold is set to empty when any of the error conditions occur - at least that was the intention. So I believe that yes, they should all be in a safe state.

elalish
elalish requested changes Sep 20, 2024
View reviewed changes
elalish
elalish approved these changes Sep 21, 2024
View reviewed changes
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!

@elalish elalish merged commit cff8d90 into elalish:master Sep 21, 2024
22 checks passed
@elalish elalish mentioned this pull request Nov 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elalish elalish elalish approved these changes

Assignees
No one assigned
Labels
None yet
-->
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

better error handling
2 participants
@pca006132 @elalish
0