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

Replaced collision checking from MoveIt by a direct interface to FCL #125

Merged
merged 25 commits into from Sep 29, 2017

Conversation

VladimirIvan
Copy link
Collaborator

  • Moved CollisionScene to a separate header and turned it into an abstract class.
  • Moved KinematicElement into a separate header.
  • Moved all FCL dependent code into a plugin extending the CollisionScene class.
  • Implemented a direct interface to FCL using collision objects and pointers to KinematicElements.
  • Added SceneInitializer property to set the collision scene (selecting a plugin to load).
  • Added fcl_catkin package compiling the FCL 0.6.0 from source (submodue).
  • Added CollisionSceneFCLLatest correctly computing correct distances between shapes using FCL 0.6.0 (insteas of the outdated moveit version).
  • Added CollisionProxy class capturing distances between shapes and publishProxies method to publish them as markers to Rviz.

When computing distances/proxies:

  • Collision normal is the direction from one contact point to the other.
  • When the shapes are colliding, centres of shapes are used due to insufficient contact points provided by the FCL.
  • Distance computation only works with FCL 0.6.0.

* @return The collision world links.
*/
virtual std::vector<std::string> getCollisionWorldLinks()
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are e.g. these two methods fully defined in the header while the others are in the implementation cpp?

* @return The collision robot links.
*/
virtual std::vector<std::string> getCollisionRobotLinks()
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

see above


namespace fcl_convert
{
void fcl2Eigen(const fcl::Vector3d & fcl, Eigen::Vector3d & eigen)
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of the fcl2Eigen are unused in the project as of now - remnant?

}
else
{
new_object = cache_entry->second;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure but this may be a copy operation, so perhaps move new_object into the if(cache_entry==end() and the L99 fixed in both the if and else block with the corresponding right-hand-side, i.e. the else-block would be fcl_objects_[i++] = cache_entry->second.get(); and the if-block fcl_objects_[i++] = constructFclCollisionObject(object.second).get();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, just copying the pointer, not the data.

}
}

// This function was copied from 'moveit_core/collision_detection_fcl/src/collision_common.cpp'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Provide link to the file in the ros-planning/moveit GitHub repository so people can check for license, changes, etc.

@@ -10,7 +10,6 @@

<PlanningScene>
<Scene>
<!-- Computing distances is enabled by default, disable for a Sampling problem by setting the PlanningMode to speed up Scene updates -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this comment was helpful for people using the example configs to learn about how the system works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a deprecated parameter. I removed it from the XML.

<< joint_index_.size() << " but " << x.rows()
<< " is provided");

if (BaseType == BASE_TYPE::FIXED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this was such a major pain to fix, just to make sure: does the new CollisionScene work correctly for a) fixed base, b) floating base, c) mixed base, i.e. model is floating base but planning group is fixed base?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no base handling in the collision scene. All of that is in the KinematicTree.

@@ -552,6 +553,19 @@ PYBIND11_MODULE(_pyexotica, module)
samplingProblem.def("getSpaceDim", &SamplingProblem::getSpaceDim);
samplingProblem.def("getBounds", &SamplingProblem::getBounds);

py::class_<CollisionProxy, std::shared_ptr<CollisionProxy>> proxy(module, "Proxy");
proxy.def(py::init());
proxy.def_readonly("Contact1", &CollisionProxy::contact1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit at unease with the inconsistency of using Uppercase at the start of variables - sometimes it's lower case (most common), then in other places suddenly uppercase...

}
return py::make_tuple(d, p1, p2);
});
scene.def("isStateValid", [](Scene* instance, bool self) {return instance->getCollisionScene()->isStateValid(self);}, py::arg("self")=true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like the margin/safe distance back - we used it heavily in the past (both in C++ and in Python).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above.

instance->getCollisionScene()->getRobotDistance(link, self, d, p1, p2, norm, c1, c2, safeDist);
return py::make_tuple(d, norm);
});
scene.def("getDistance", [](Scene* instance, std::string o1, std::string o2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should bring this one back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. The return is a vector of proxies.

@wxmerkt
Copy link
Collaborator

wxmerkt commented Sep 29, 2017

  • PlanningMode is now deprecated

Vladimir Ivan added 2 commits September 29, 2017 16:27
- Removed CollisionData.Done
- Separated narrow phace routine
- Added collision/distance check overloads taking object names
- Fixed typose, comments, and removed depricated XML tags
data->Proxies.push_back(p);
}

bool CollisionSceneFCLLatest::collisionCallbackDistance(fcl::CollisionObjectd* o1, fcl::CollisionObjectd* o2, void* data, double& dist)
Copy link
Collaborator

Choose a reason for hiding this comment

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

dist is now being passed in as a reference but not assigned a value/used as part of the function - computeDistance changes the values in data_ but dist does not get assigned a value.

@wxmerkt wxmerkt merged commit b9cea2c into master Sep 29, 2017
@wxmerkt wxmerkt deleted the vi-collisionscene branch September 29, 2017 16:16
wxmerkt added a commit that referenced this pull request Oct 16, 2017
- Addresses #149
- Broken in #125

Conflicts:
	exotica_python/src/Exotica.cpp
wxmerkt added a commit that referenced this pull request Oct 16, 2017
- Addresses and resolves #149
- Broken in #125
wxmerkt added a commit that referenced this pull request Oct 16, 2017
wxmerkt added a commit that referenced this pull request Oct 16, 2017
- Addresses and resolves #149
- Broken in #125
world_joint_ + "/trans_y", x(1));
ps_->getCurrentStateNonConst().setVariablePosition(
world_joint_ + "/trans_z", x(2));
KDL::Rotation rot = KDL::Rotation::EulerZYX(x(3), x(4), x(5));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is why never noticed the ZYX <> RPY bug in Kinematica in practice - Kinematica was never used for floating base and the MoveIt planning scene for sampling based planning was correct due to this.

Cf. #138

wxmerkt added a commit that referenced this pull request Oct 16, 2017
- Broken in #125
- Adds method that updates the model state from Kinematica just before publishing
- Does not handle attached objects (addObject/attachObject/detachObject)
- Addresses #150
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.

None yet

2 participants