-
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
[Mapping] Add checks at init in TopologicalMapping #3339
[Mapping] Add checks at init in TopologicalMapping #3339
Conversation
[ci-build][with-all-tests] |
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 for making this PR. It will improve the UX for sure.
However, I see a lot of duplicated code. I consider it as a technical debt. To discuss if we temporary merge your changes, or think bigger ;)
...mponent/Topology/Mapping/src/sofa/component/topology/mapping/Edge2QuadTopologicalMapping.cpp
Outdated
Show resolved
Hide resolved
I am open to think bigger, or merge it and then factorize |
Reviews taken into account 👍 @alxbilger |
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.
I think there is already some checks in TopologyMapping parse method. Checking fromModel and toModel.
Might be the good timing to unify it.
Also it is not possible to put the same message in a single method inside TopologyMapping which is called by the different components.
Using toModel.GetClassName, toModel.getName() etc... ?
Which part of the check is not generic?
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.
several changes are not compatible with the current api of the topology and some tests are already present in TopologicalMapping create and canCreate methods.
@hugtalbot if you agree I will push a new proposal directly in this PR.
if (!fromModel) | ||
{ | ||
// If the input topology link isn't set by the user, the TopologicalMapping::create method tries to find it. | ||
// If it is null at this point, it means no input mesh topology could be found. | ||
msg_error() << "No input mesh topology found. Consider setting the '" << fromModel.getName() << "' data attribute."; | ||
modelsOk = 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.
2 remarks regarding this check:
- This case is never possible because fromModel is already checked in the mother class TopologicalMapping in the canCreate method. If input and output don't link to valid BaseMeshTopology, the mapping is not create (canCreate return false)
- Hopefully this case is never possible because if fromModel is null, printing error message with fromModel.getName() will crash 😄
if (!dynamic_cast<container::dynamic::TriangleSetTopologyContainer *>(toModel.get())) | ||
{ | ||
msg_error() << "The input topology '" << toModel.getPath() << "' is not homogeneous with a TriangleSetTopologyContainer. The '" << toModel.getName() << "' data attribute must be linked to a valid component, among the following list of eligible components:" << msgendl | ||
<< sofa::core::ObjectFactory::getInstance()->listClassesDerivedFrom<container::dynamic::TriangleSetTopologyContainer>(); |
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.
I tested what would be the error message. In this mapping case, it is:
data attribute must be linked to a valid component, among the following list of eligible components: TetrahedronSetTopologyContainer, TriangleSetTopologyContainer
which doesn't work, because putting a TetrahedronSetTopologyContainer as output will result in a strange behavior. Not sure if it will crash or if it will be like a "TopologicalIdentityMapping" 🤔
I suggest to move the checks inside a commun method in TopologicalMapping and just test the topologyType of the containters
else | ||
{ | ||
// Making sure the input topology includes triangle elements | ||
if (fromModel.get()->getNbTriangles() != 0) |
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.
This could still be valid if we start from an empty mesh and we add progressively triangles with a mapping on the edges... btw I thought I had a test on that.
[ci-build][with-all-tests] |
...mponent/Topology/Mapping/src/sofa/component/topology/mapping/Edge2QuadTopologicalMapping.cpp
Outdated
Show resolved
Hide resolved
...mponent/Topology/Mapping/src/sofa/component/topology/mapping/Edge2QuadTopologicalMapping.cpp
Outdated
Show resolved
Hide resolved
New approach proposed in the TopologicalMapping to make the checks on input/output topologies. Some missing implementation and find a solution to get the topological element name from enum class. New review needed |
…TopologicalMapping.h to test input/output
65d06d6
to
6030f26
Compare
To be rebased once #3428 is merged, then ready |
et 💥 !! |
aw aw aw @epernod ! |
it seems mostly to be the case of a grid linked :
|
yes and it is "funny" because I had the same issue in #3368 |
Failing scenes were fixed in #3453 a fortiori, lets merge with master and have a final check |
Adding check on pointers and type of input/output topology (mostly input) before using them in TopologicalMapping
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