-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaMiscTopology] Add component TopologyChecker #1594
Conversation
modules/SofaMiscTopology/src/SofaMiscTopology/TopologyChecker.cpp
Outdated
Show resolved
Hide resolved
[ci-build][with-all-tests] |
Is there any ... test 🤓 ? |
test the component that will do the tests. Could be possible I think |
[ci-build][with-all-tests] |
[ci-build][with-all-tests] |
…to be able to test container and cross containes seperatly
[ci-build][with-all-tests] |
@fredroy you wanted some tests.... I have added "some"... I know 2 are failing but I think the mesh is wrong. I will fix that in another PR |
EXPECT_MSG_NOEMIT(Error); | ||
EXPECT_EQ(checker->checkTriangleToEdgeCrossContainer(), true); | ||
|
||
sofa::helper::WriteAccessor< sofa::core::objectmodel::Data<sofa::helper::vector<Topology::Edge> > > edges = topoCon->d_edge; |
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.
Advice: for super long line like that, use auto.
And in this case: auto edges = sofa::helper::getWriteAccessor(topoCon->d_edge);
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.
thanks, done!
const auto& edge = my_edges[i]; | ||
if (edge[0] == edge[1]) { | ||
msg_error() << "CheckEdgeTopology failed: edge " << i << " has 2 identical vertices: " << edge; | ||
res = false; |
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.
Maybe cut the loop once res = false ? (or add the test i < nbE && !res in the for loop)
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.
yes, I hesitated, the idea was to catch all the errors... but it quickly become a mess..
} | ||
} | ||
|
||
if (simulation::AnimateEndEvent::checkEventType(event) && d_eachStep.getValue()) |
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.
empty statement if animateEnd, so I guess you can remove this
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.
in fact it was not supposed to be empty
modules/SofaMiscTopology/src/SofaMiscTopology/TopologyChecker.h
Outdated
Show resolved
Hide resolved
Could you identify them in the code (in a comment, maybe add the url or the number of this PR) |
In fact the checker had a bug. All the tests are now working. Thanks for the review, the tests were worth it! |
This PR add a component called TopologyChecker to be added in the scene and link to a MeshTopology / TopologyContainer in order to check the topology either on demand or every end of step.
The methods used to check the topology is similar to the code inside the method xxxSetTopologyContainer::CheckTopology()
which will be removed.
This PR use changes from PR #1593 and won't compile until the previous one is merged
Fix #1367
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if