Skip to content
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.

Sign up for GitHub

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

[IRC] Add a Link for the mandatory FixedConstraint #152

Merged
merged 3 commits into from
Aug 26, 2024
+27 −11
Merged

[IRC] Add a Link for the mandatory FixedConstraint #152

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ceb0ca7
Change ptr to fixedConstraint to Link and Avoid crashing if no FixedC…
fredroy Jul 10, 2024
1726eba
Apply suggestions from code review (renaming with FixedProjectiveCons…
fredroy Jul 12, 2024
67c3087
remove creation of fixedconstraint
fredroy Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/BeamAdapter/component/controller/InterventionalRadiologyController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,12 @@ class InterventionalRadiologyController : public MechanicalStateController<DataT

bool m_useBeamActions = false;
bool m_FF, m_RW, m_sensored;
FixedProjectiveConstraint<DataTypes> * m_fixedConstraint;

SingleLink<
InterventionalRadiologyController, FixedProjectiveConstraint<DataTypes>,
BaseLink::FLAG_STOREPATH | BaseLink::FLAG_STRONGLINK> l_fixedConstraint;
DeprecatedAndRemoved m_fixedConstraint;

type::vector<Vec3d> m_sensorMotionData;
unsigned int m_currentSensorData;
type::vector<Real> m_nodeCurvAbs;
Expand Down
39 changes: 29 additions & 10 deletions src/BeamAdapter/component/controller/InterventionalRadiologyController.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ InterventionalRadiologyController<DataTypes>::InterventionalRadiologyController(
, d_rigidCurvAbs(initData(&d_rigidCurvAbs, "rigidCurvAbs", "pairs of curv abs for beams we want to rigidify"))
, d_motionFilename(initData(&d_motionFilename, "motionFilename", "text file that includes tracked motion from optical sensor"))
, d_indexFirstNode(initData(&d_indexFirstNode, (unsigned int) 0, "indexFirstNode", "first node (should be fixed with restshape)"))
, l_fixedConstraint(initLink("fixedConstraint", "Path to the FixedConstraint"))
fredroy marked this conversation as resolved.
Show resolved Hide resolved
{
m_fixedConstraint = nullptr;
m_sensored =false;
}

Expand Down Expand Up @@ -134,9 +134,29 @@ void InterventionalRadiologyController<DataTypes>::init()
for(unsigned int i=0; i<m_instrumentsList.size(); i++)
m_instrumentsList[i]->setControlled(true);

context->get(m_fixedConstraint);
if(m_fixedConstraint==nullptr)
msg_error()<<"No fixedConstraint found.";
if (!l_fixedConstraint)
{
typename FixedProjectiveConstraint<DataTypes>::SPtr fixedConstraint{};
context->get(fixedConstraint);
if (fixedConstraint)
{
l_fixedConstraint.set(fixedConstraint);
}
else
{
msg_error() << "No FixedConstraint found. One will be created on the spot but most likely it will not behave as attended.";
fredroy marked this conversation as resolved.
Show resolved Hide resolved

fixedConstraint = sofa::core::objectmodel::New<FixedProjectiveConstraint<DataTypes>>();
context->addObject(fixedConstraint);
fixedConstraint->addConstraint(0);

l_fixedConstraint.set(fixedConstraint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about initialization of the FixedProjectiveConstraint? Even if you call it, FixedProjectiveConstraint::init can also fail, and there is no automatic mechanism to prevent invalidity of the scene. So, I am in favor of triggering an error without backup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is, if you dont create a fixedconstraint (i.e is null), as the IRC code is... let's say, not really solid 🤔, it will crash later on (not test on pointer here and there etc)
The creation here of FixedConstraint is not really intended to serve as a backup actually ; more as a way to not crash by itself 😅 Thats why there is a msg_error (and not a warning).
Or instead of crashing (if we dont do anything) we can do a fatal() or something like that... a graceful exit

And as for the breaking, I agree and I assume that I wont do deprecation and stuff 🙊

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with both of you : @alxbilger for the analysis and @fredroy about the IRC component

Hard to decide which would be the better option.
I would vote for the simplest one (thinking about our future us debugging) : just an error, even if this leads to crashes (why not the fatal if it is slightly nicer)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me the component should be emit an error and be set in invalid state.
Then the component state should be checked if it is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me the component should be emit an error and be set in invalid state. Then the component state should be checked if it is valid.

This is what I did few lines after (commented) but only a few functions in IRC does the componentState testing.
And I dont really want to update all the functions to add the test 🥸


//Instead of create one, it may be more correct to set this component to Invalid
//but the component state is almost never tested....
//this->d_componentState.setValue(sofa::core::objectmodel::ComponentState::Invalid);
}
}

// the controller must listen to the event (in particular BeginAnimationStep event)
if (!f_listening.isSet())
Expand Down Expand Up @@ -703,7 +723,6 @@ void InterventionalRadiologyController<DataTypes>::applyInterventionalRadiologyC
{
const Real& threshold = d_threshold.getValue();


// Create vectors with the CurvAbs of the noticiable points and the id of the corresponding instrument
type::vector<Real> newCurvAbs;
type::vector<type::vector<int>> idInstrumentTable;
Expand Down Expand Up @@ -907,11 +926,11 @@ void InterventionalRadiologyController<DataTypes>::applyInterventionalRadiologyC
if (!rigid)
{
rigid=true;
m_fixedConstraint->addConstraint(firstSimulatedNode+i);
l_fixedConstraint->addConstraint(firstSimulatedNode+i);
}
else
{
m_fixedConstraint->addConstraint(firstSimulatedNode+i);
l_fixedConstraint->addConstraint(firstSimulatedNode+i);
rigid=false;
}
it++;
Expand All @@ -922,7 +941,7 @@ void InterventionalRadiologyController<DataTypes>::applyInterventionalRadiologyC
else
{
if(rigid)
m_fixedConstraint->addConstraint(firstSimulatedNode+i);
l_fixedConstraint->addConstraint(firstSimulatedNode+i);
}
}
}
Expand Down Expand Up @@ -1030,12 +1049,12 @@ void InterventionalRadiologyController<DataTypes>::fixFirstNodesWithUntil(unsign

// set the position to startingPos for all the nodes that are not simulated
// and add a fixedConstraint
m_fixedConstraint->clearConstraints();
l_fixedConstraint->clearConstraints();
for(unsigned int i=0; i<firstSimulatedNode-1 ; i++)
{
xMstate[i]=d_startingPos.getValue();
vMstate[i].clear();
m_fixedConstraint->addConstraint(i);
l_fixedConstraint->addConstraint(i);
}
d_indexFirstNode = firstSimulatedNode-1 ;
}
Expand Down
Loading