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

Improve handling of invalid shape construction. #385

Merged
merged 9 commits into from
Mar 23, 2023
Merged

Improve handling of invalid shape construction. #385

merged 9 commits into from
Mar 23, 2023

Conversation

geoffder
Copy link
Collaborator

Fixes #379

  • Add Error::InvalidConstruction, and add checks to Manifold constructors that return empty manifolds with this error when given invalid arguments (such as negative radius)
  • Add similar checks to CrossSection, returning empty cross-sections
  • As part of testing the behaviour of Cube with negative dimensions, I noticed the semantic is to return mirrored cubes. To match this in CrossSection::Square, I added a check that reverses the winding of the path to make sure the shape is still positive.

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Patch coverage: 98.73% and project coverage change: +0.12 🎉

Comparison is base (2a63971) 85.91% compared to head (0f24d0d) 86.04%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   85.91%   86.04%   +0.12%     
==========================================
  Files          36       36              
  Lines        4444     4485      +41     
==========================================
+ Hits         3818     3859      +41     
  Misses        626      626              
Impacted Files Coverage Δ
src/manifold/src/csg_tree.cpp 92.33% <98.11%> (+0.25%) ⬆️
src/cross_section/src/cross_section.cpp 71.76% <100.00%> (+0.73%) ⬆️
src/manifold/src/constructors.cpp 94.70% <100.00%> (+0.29%) ⬆️
src/manifold/src/manifold.cpp 92.24% <100.00%> (+0.09%) ⬆️
src/manifold/src/sort.cpp 90.37% <100.00%> (ø)
src/utilities/include/par.h 94.28% <100.00%> (+0.53%) ⬆️

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.

src/cross_section/src/cross_section.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Show resolved Hide resolved
test/cross_section_test.cpp Outdated Show resolved Hide resolved
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! Would you mind updating our other bindings with this new enum value?

src/cross_section/src/cross_section.cpp Outdated Show resolved Hide resolved
@geoffder
Copy link
Collaborator Author

Thanks! Would you mind updating our other bindings with this new enum value?

Right, will do.

@geoffder
Copy link
Collaborator Author

I added an entry for InvalidConstruction in the WASM bindings, but it seems like pybind takes care of Error on its own and there is nothing to do there.

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.

Great, thank you!

@geoffder geoffder merged commit cdce9e7 into elalish:master Mar 23, 2023
@geoffder geoffder deleted the invalid-construction branch March 24, 2023 03:51
@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
Add `InvalidConstruction` to `Manifold::Error` and return empty manifolds with it set when manifold construction is attempted with invalid parameters

- zero / negative dimensions for Cube
- zero / negative radius for Sphere
- zero / negative height for Extrude
- empty CrossSection for Extrude/Revolve

Also, Circle and Square have been updated to return empty CrossSections for similar arguments.
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 this pull request may close these issues.

More sanity check
2 participants