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

refactor #531

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 12 commits into from
Aug 16, 2023
Merged

refactor #531

merged 12 commits into from
Aug 16, 2023

Conversation

pca006132
Copy link
Collaborator
@pca006132 pca006132 commented Aug 15, 2023 •
edited
Loading

This does 5 things:

  1. Added bound check to vectors by default. I verified that the overhead is not large so we should probably enable this even when debug is off. Downstream projects can catch the exception and do whatever they like instead of segfault. (we don't have much shared states so it should be fine)
  2. Refactored VecDH to remove nearly all pointer usages, added VecView for the cases that requires pointer offsetting. I also renamed VecDH to Vec as we are no longer having differences between host and device.
  3. Moved sdf into a source file, and only expose the minimal public API to the user. We are no longer using CUDA so this should be fine
  4. Refactored cross_section.h so we no longer expose Clipper2 dependency to our users.
  5. Added CMake install target. Users can now use cmake install and get the following result:
result
├── include
│   └── manifold
│       ├── cross_section.h
│       ├── manifoldc.h
│       ├── manifold.h
│       ├── public.h
│       ├── sdf.h
│       └── types.h
└── lib
    ├── libcollider.so
    ├── libcross_section.so
    ├── libmanifoldc.so
    ├── libmanifold.so
    ├── libsdf.so
    └── manifold3d.cpython-39-x86_64-linux-gnu.so

Originally I only wanted to remove pointer usages, but as I was modifying things I suddenly want to do some refactor as the changes is already pretty large.

This change breaks the sdf API, as the original way of exposing sdf in header will make all our internal header files public.

Users of our library can now use compiled binaries and our header files. The only third party dependency they need is glm.

@pca006132 pca006132 requested a review from elalish August 15, 2023 17:58
@pca006132
Copy link
Collaborator Author

@elalish btw if the set of changes is too large, let me know and I can split them into separate PRs.

@codecov
Copy link
codecov bot commented Aug 15, 2023 •
edited
Loading

Codecov Report

Patch coverage: 94.08% and project coverage change: +0.51% 🎉

Comparison is base (8011fb6) 90.77% compared to head (582c85a) 91.29%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #531      +/-   ##
==========================================
+ Coverage   90.77%   91.29%   +0.51%     
==========================================
  Files          34       34              
  Lines        4304     4467     +163     
==========================================
+ Hits         3907     4078     +171     
+ Misses        397      389       -8
Files Changed Coverage Δ
src/collider/include/collider.h 66.66% <ø> (ø)
src/manifold/src/impl.h 72.72% <ø> (ø)
src/manifold/src/mesh_fixes.h 92.30% <ø> (+1.00%) ⬆️
src/utilities/include/sparse.h 54.38% <50.00%> (ø)
src/collider/src/collider.cpp 94.77% <81.81%> (+3.79%) ⬆️
src/manifold/src/properties.cpp 83.66% <85.71%> (-0.26%) ⬇️
src/utilities/include/vec.h 89.51% <89.51%> (ø)
src/cross_section/src/cross_section.cpp 79.94% <92.50%> (-0.12%) ⬇️
src/manifold/src/sort.cpp 90.58% <96.29%> (+2.15%) ⬆️
src/manifold/src/impl.cpp 96.44% <97.05%> (+0.40%) ⬆️
... and 13 more

... and 1 file with indirect coverage changes

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

@elalish
Copy link
Owner
elalish commented Aug 15, 2023

This looks amazing! I very much like the changes you've listed - I'll start reviewing now. I think we can probably leave it as one, but I'll let you know. Sounds like our next release will be v3.0 instead of v2.2, but I think that's fine.

elalish
elalish approved these changes Aug 15, 2023
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.

This is fantastic! Just some minor comments.

src/cross_section/include/cross_section.h
@@ -169,10 +166,10 @@ class CrossSection {
///@}

private:
mutable std::shared_ptr<const C2::PathsD> paths_;
mutable std::shared_ptr<const PathImpl> paths_;
8000
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @geoffder

bindings/c/box.cpp

#include "conv.h"
#include "manifold.h"
#include "public.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @geoffder

src/manifold/CMakeLists.txt
install(
DIRECTORY include/
DESTINATION ${CMAKE_INSTALL_FULL_INCLUDEDIR}/manifold
COMPONENT devel
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it! Are we testing the install somehow in our CI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the nix build uses install, but we are not checking the file contents

@elalish
Copy link
Owner
elalish commented Aug 15, 2023

@googlebot I signed it!

@elalish
Copy link
Owner
elalish commented Aug 16, 2023

Nice bug find - this all looks good to me; feel free to merge.

This change breaks the sdf API, as the original way of exposing sdf in header will make all our internal header files public.

I don't quite understand what you mean - looking at our samples and tests it doesn't appear there was any breaking change to the SDF API.

@pca006132
Copy link
Collaborator Author

The policy parameter is replaced with a boolean

@pca006132 pca006132 merged commit b4f583f into elalish:master Aug 16, 2023
@elalish
Copy link
Owner
elalish commented Aug 16, 2023

Ah, thanks, I see it now. That's pretty minor, I'm pretty comfortable glossing over that as far as a breaking change. Probably still good with 2.2.

@pca006132 pca006132 deleted the bound-check branch August 29, 2023 07:34
@elalish elalish mentioned this pull request Nov 3, 2023
Merged
cartesian-theatrics pushed a commit to cartesian-theatrics/manifold that referenced this pull request Mar 11, 2024
@pca006132
refactor (elalish#531)
0c211ed 
* added bound check for vec_dh

* fix cmakefile

* refactored VecDH

* renamed VecDH into Vec

* better packaging

* formatting

* include <algorithm>

* fix windows

* documentation and copyright fixes

* use shorter names and implicit conversions

* formatting

* minimum circular segments = 3
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.
2 participants
@pca006132 @elalish
0