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.

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

allow EnvironmentBodyRemover not to restore a grabbed body when the grabbing link does not exist #1359

Open
wants to merge 11 commits into
base: production
Choose a base branch
from

Conversation

eisoku9618
Copy link
Collaborator

@eisoku9618 eisoku9618 commented Feb 27, 2024

Currently we allow EnvironmentBodyRemover not to restore active manipulator on restore if the active manipulator does not exist anymore in the body in case of CBAS change.

I think that we should do the same thing for grabbed bodies because a grabbing link might not exist in case of CBAS change.

  • a grabbing link might not exist
  • a link in _setIgnoreRobotLinkNames might not exist
  • grabbed body might not exist

@eisoku9618 eisoku9618 requested review from kanbouchou and yoshikikanemoto and removed request for kanbouchou and yoshikikanemoto February 27, 2024 09:50
@eisoku9618 eisoku9618 marked this pull request as draft February 27, 2024 10:38
@eisoku9618 eisoku9618 marked this pull request as ready for review February 28, 2024 04:51
}
}
if( !_pGrabbedInfos.empty() ) {
// it might be ok with grabbing link doesn't exist if ConnectedBody acitve state changes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eisoku9618 I think you should not change the behavior of EnvironmentBodyRemover. There is a complication like this: If there is a grabbed body that has _setIgnoreRobotLinkNames and some of links are removed by the CBAS change, it is not obvious how this class should be doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yoshikikanemoto Eventually we need to take care of grabbed bodies restoring failures either by
EnvironmentBodyRemover itself or by callers of EnvironmentBodyRemover.

My only concern is an inconsistency of current EnvironmentBodyRemover between active manipulator and grabbed bodies. EnvironmentBodyRemover tries to restore both active manipulator and grabbed bodies, and the inconsistency is that it allows not restoring active manipulator while it terminates the program (c++11 and later) if unable to restore grabbed bodies.

it is not obvious how this class should be doing.

How about adding the input arguments to EnvironmentBodyRemover to clarify how this class should behave?

EnvironmentBodyRemover(KinBodyPtr pBody, bool abortOnActiveManipulatorLost=false, bool abortOnGrabbedBodiesLost=true);

If this is still not good and we can leave the inconsistent behavior of EnvironmentBodyRemover for now, I can make another utility function SetConnectedBodyActiveStatesKeepingGrabbedBodiesAsMuchAsPossible and move the implementation to this.

void SetConnectedBodyActiveStatesKeepingGrabbedBodiesAsMuchAsPossible(pRobot, requestedConnectedBodyActiveStates) {
    std::vector<KinBody::GrabbedInfoPtr> pGrabbedInfos;
    pRobot->GetGrabbedInfo(pGrabbedInfos);

    // manually restore grabbed state instead of using EnvironmentBodyRemover because a grabbing link might get removed on connected body active state change.
    pRobot->ReleaseAllGrabbed();
    {
        EnvironmentBodyRemover robotremover(pRobot);
        pRobot->SetConnectedBodyActiveStates(requestedConnectedBodyActiveStates);
    }

    std::vector<KinBody::GrabbedInfoPtr>::iterator itRemoveFirst = pGrabbedInfos.begin();
    for (std::vector<KinBody::GrabbedInfoPtr>::const_iterator itGrabbedInfo = pGrabbedInfos.begin(); itGrabbedInfo != pGrabbedInfos.end(); ++itGrabbedInfo) {
        const KinBody::GrabbedInfoPtr pGrabbedInfo = *itGrabbedInfo;
        bool needToDelete = false;
        if( !pRobot->GetLink(pGrabbedInfo->_robotlinkname) ) {
            RAVELOG_WARN_FORMAT("env=%s, body=%s, cannot re-grab '%s' because grabbing link '%s' does not exist anymore.", pRobot->GetEnv()->GetNameId()%pRobot->GetName()%pGrabbedInfo->_grabbedname%pGrabbedInfo->_robotlinkname);
            needToDelete = true;
        }
        else if( !pRobot->GetEnv()->GetKinBody(pGrabbedInfo->_grabbedname) ) {
            RAVELOG_WARN_FORMAT("env=%s, body=%s, cannot re-grab '%s' because it does not exist in the environment.", pRobot->GetEnv()->GetNameId()%pRobot->GetName()%pGrabbedInfo->_grabbedname);
            needToDelete = true;
        }
        else {
            for( std::set<std::string>::const_iterator itLinkName = pGrabbedInfo->_setIgnoreRobotLinkNames.begin(); itLinkName != pGrabbedInfo->_setIgnoreRobotLinkNames.end(); ++itLinkName ) {
                if( !pRobot->GetLink(*itLinkName) ) {
                    RAVELOG_WARN_FORMAT("env=%s, body=%s, cannot re-grab '%s' because '%s' in _setIgnoreRobotLinkNames does not exist anymore.", pRobot->GetEnv()->GetNameId()%pRobot->GetName()%pGrabbedInfo->_grabbedname%(*itLinkName));
                    needToDelete = true;
                    break;
                }
            }
        }
        if( needToDelete ) {
            continue;
        }
        // will regrasp this info
        if( itRemoveFirst != itGrabbedInfo ) {
            *itRemoveFirst = std::move(*itGrabbedInfo);
        }
        ++itRemoveFirst;
    }
    const std::vector<KinBody::GrabbedInfoConstPtr> pConstGrabbedInfos(pGrabbedInfos.begin(), itRemoveFirst);
    pRobot->ResetGrabbed(pConstGrabbedInfos);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

discussed with @yoshikikanemoto
At least, it is better to use a bitmask instead of two booleans for the argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yoshikikanemoto As we discussed, I changed this PR to use a bit mask for the options. Could you check this PR and the changes in the related internal repositories? pipelineid=456553

@rdiankov
Copy link
Owner

@snozawa can you take a look at this MR? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants