-
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
[SofaDeformable] Fix RestShapeSpringsForceField #367
[SofaDeformable] Fix RestShapeSpringsForceField #367
Conversation
…external rest shape
@@ -62,7 +62,7 @@ void SceneCheckerVisitor::addHookInChangeSet(const std::string& version, ChangeS | |||
void SceneCheckerVisitor::installChangeSets() | |||
{ | |||
addHookInChangeSet("17.06", [](Base* o){ | |||
if(o->getClassName() == "RestShapeSpringsForceField" && o->findData("external_rest_shape")->isSet()) | |||
if(o->getClassName() == "RestShapeSpringsForceField" && o->findLink("external_rest_shape")->getSize() != 0) | |||
msg_warning(o) << "RestShapeSpringsForceField have changed since 17.06. The parameter 'external_rest_shape' is now a Link. To fix your scene you need to add and '@' in front of the provided path. See PR#315" ; |
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.
Just reading the code, it seems to me that we really want to search for a Data (and raise a warning if we find one).
@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.
What we want to check is if the object still have "something" with the name "external_rest_shape" and if this thing has be set by the user. Before the merge of PR 315 this thing was a Data, but since the PR it is a Link.
So I think searching for the link is the right thing to do.
|
||
msg_error(this)<< "external_rest_shape in node " << this-> getContext()->getName() << " not found"; | ||
//getContext()->removeObject(this); | ||
return; |
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.
@raffaellatrivisonne, you added this return in #315. Could you explain why?
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 just checked and I agree with @EulalieCoevoet.
The return wouldn't allow a default scenario with no external_rest_shape.
Sorry, my bad !!!
So i pass this PR to read ? Edit: if if the component should work then the correct way to emit the message is msg_warning instead of a msg_error. |
I fixed the conflict...Isn't this PR ready ? @hugtalbot @guparan |
I think the PR is ready then, right @damienmarchal ? |
[ci-build] All build & tests are passing... so let's merge it . |
…ringFF [SofaDeformable] Fix RestShapeSpringsForceField (cherry picked from commit adb5974) # Conflicts: # SofaKernel/modules/SofaDeformable/RestShapeSpringsForceField.inl
1- In SceneCheckerVisitor:
Fixed segfault. Please review, I'm not sure about the fix.
2- In RestShapeSpringForceField:
Fixed segfault when using the component without external_rest_shape, by removing a "return" that was unintentionally introduced in #315. (line 95)
And the error message should be print only if external_rest_shape is set by user. (line 93)
And minor cleaning.
This PR:
Reviewers will merge only if all these checks are true.