-
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
[All] Overridden 'canCreate' methods should always log an error message when they fail #1294
[All] Overridden 'canCreate' methods should always log an error message when they fail #1294
Conversation
…age when they fail.
[ci-build][with-all-tests] |
{ | ||
// we should refuse to create mappings with the same input and output model, which may happen if a State object is missing in the child node | ||
msg_error(context) << "Creation of " << className(obj) << " topology mapping failed as the same object \"" << stin->getName() << "\" is linked as input and output."; | ||
arg->logError("Both the input mesh and the output mesh points to the same mesh topology ('"+stin->getName()+"')."); | ||
return 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.
what is the difference betweem msg_error
and arg->logError
?
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.
Hi Hugo,
The idea is to let the calling function to detect and print the error. This is why the errors are logged in the object instead of sent via the msg_error API. I think this is not a satisfactory solution because it is an approach to error management that is very invasive.
I think a much better way of doing that would be to use a frame based message handler (as what was done with the EXPECT_MSG_ and EXPECT_NO_MSG_ for testing tests).
This would allow to write:
{
MessageCatcher mc;
tito();
canCreate();
if( mc.hasErrorMessage() )
{
msg_error(mc) << "reformat and re-emit the messsages " < mc.msg();
}
}
which is actually very close to
try {
} catch(Error& m){
throw Error("reformat and re-emit the messsages "+m.msg());
}
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 @damienmarchal
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.
Hi @damienmarchal and @hugtalbot
The object creation can be a little bit confusing. Here the arg->logError
does not necessary mean that an error occurred. It is there to specify why a given object with a given template and a given context cannot be created.
For example, look at the following scene:
<Node>
<MechanicalObject template="Vec3" />
<TriangleFEMForcefield />
</Node>
And here is what the object creation for the TriangleFEMForcefield could look like:
- TriangleFEMForcefield<Vec2Types>
Does it have a mechanical object with Vec2Types in the current context?
NO
logError("No mechanical object with vec2Types found in the current context")
- TriangleFEMForcefield<Vec3Types>
Does it have a mechanical object with Vec3Types in the current context?
YES
-> Instanciate a new TriangleFEMForcefield<Vec3Types>
-> No error shown since at least one combination of the object/template is valid
Now, let's change the mechanical object by giving him an incompatible template:
<Node>
<MechanicalObject template="Vec2Rigid" />
<TriangleFEMForcefield />
</Node>
And here is what the object creation for the TriangleFEMForcefield could look like:
- TriangleFEMForcefield<Vec2Types>
Does it have a mechanical object with Vec2Types in the current context?
NO
logError("No mechanical object with vec2Types found in the current context")
- TriangleFEMForcefield<Vec3Types>
Does it have a mechanical object with Vec3Types in the current context?
NO
logError("No mechanical object with vec2Types found in the current context")
-> The object factory cannot instantiate a TriangleFEMForcefield object because:
a. No mechanical object with vec2Types found in the current context
b. No mechanical object with vec3Types found in the current context
Here the real error is that no TriangleFEMForcefield can be instantiated
The logError
are there to specify what combinations of object/template were tried and why they failed.
Here's a real example of the failed creation of a component:
Hopefully that clear things up a little bit
@@ -90,8 +90,11 @@ class SOFA_GENERAL_LOADER_API ReadTopology: public core::objectmodel::BaseObject | |||
template<class T> | |||
static bool canCreate(T* obj, core::objectmodel::BaseContext* context, core::objectmodel::BaseObjectDescription* arg) | |||
{ | |||
if (context->getMeshTopologyLink() == nullptr) | |||
if (context->getMeshTopologyLink() == nullptr) { |
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 don't get why sometimes getMeshTopologyLink
and some other getMeshTopology
which seem to be two interface for both static and dynamic topologies but they are identitical !
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.
They are both doing the same thing, but getMeshTopology
is more frequent, so I changed this one and hopefully at some point no one will be using getMeshTopologyLink
and we will be able to safely remove it.
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.
Ok agreed. I created an issue to highlight this work in progress: #1297
[ci-build][with-all-tests] |
Now pretty much all overridden 'canCreate' found in the kernel or the modules behave as they should, which is, always specify why a component with the given template cannot be created in the given context.
This is the continuation of the PR #1266
This PR:
Reviewers will merge only if all these checks are true.