-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add new component Regular Space #437
Conversation
Codecov Report
@@ Coverage Diff @@
## main #437 +/- ##
==========================================
+ Coverage 84.63% 85.37% +0.74%
==========================================
Files 160 187 +27
Lines 4516 5299 +783
==========================================
+ Hits 3822 4524 +702
- Misses 694 775 +81
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I wrote the basic operations, SplitFace, SplitFaceWithTag and SplitEdgeWithTag. I would rename the last two classes as SplitFaceWithTodoList and SplitEdgeWithTodoList. |
I found an interesting "bug" in the DEBUG_TriMesh. That is when you collapse an edge, it seems the maximum of the indices will not be modified. As a result, when you only have 4 vertices, you still have a vertex with idx = 4. |
I will first test the 1d case since the 2d case gets involved in the split result's direction. Something should be more clear about the result of the split operation in the document. |
I added the RegularSpace component API. Should be noted that this component DO NOT build the offset. Just make sure each different simplexity will not directly touch with the other. After using the RegularSpace function, one can directly get the offset by splitting every edge incident the input simplexity (just invoke the EdgeSplitWithTag and pass it to the scheduler, then all set). Because the design of the Option is a little messy, I will upload a small document under this component folder. |
This is not a bug. The mesh class does not yet have a garbage collection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach is definitely good. The code requires quite some clean-up though.
components/wmtk_components/regular_space/internal/RegularSpace.hpp
Outdated
Show resolved
Hide resolved
components/wmtk_components/regular_space/internal/RegularSpaceOptions.hpp
Outdated
Show resolved
Hide resolved
components/wmtk_components/regular_space/internal/RegularSpaceOptions.hpp
Outdated
Show resolved
Hide resolved
I still need to add the last few tests. Will be done soon. |
This component is done, wait to be reviewed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Most of my comments are on the tests. There are a few untested scenarios that should be covered.
Additionally, try to increase code coverage as much as possible.
I modified the code, but some codes I can't cover by tests, seems those codes are impossible to execute😅. however, for the safety of the code, those code is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. Just resolve my comments from the previous review.
tests/test_2d_operations.cpp
Outdated
wmtk::operations::tri_mesh::FaceSplitAtMidPoint op1(m, f1, settings); | ||
wmtk::operations::tri_mesh::FaceSplitAtMidPoint op2(m, f2, settings); | ||
CHECK(op0()); | ||
auto handle = m.get_attribute_handle<double>(std::string("position"), PV); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are still not using the handle and accessor
Just remove the variable never used in the test file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for now. It's ready to be merged.
Just a dummy skeleton now.