Backporting Joint Properties back to Noetic #109
Conversation
- To set planar joints as differential drive, moveit2 uses joint properties. - This is a backport of the joint properties supported by moveit2 that currently exists on the foxy branch.
- Added tests for joint properties and applied refactors to model that use the new isValidJoint() function
|
Please refer to the original PR/commits (and if possible the authors) in the commit. |
| @@ -279,6 +314,11 @@ class Model | |||
| std::vector<CollisionPair> enabled_collision_pairs_; | |||
| std::vector<CollisionPair> disabled_collision_pairs_; | |||
| std::vector<PassiveJoint> passive_joints_; | |||
| std::map<std::string, std::vector<JointProperty>> joint_properties_; | |||
There was a problem hiding this comment.
Hm. My actual comment was lost. Here it is again:
I think this is the wrong data structure to store properties:
- First, the joint name is stored redundantly both, in this map and in the JointProperty struct.
- Second, the vector should actually be a map too (property name -> property value).
It doesn't make sense to have multiple properties with same name but different value.
|
I agree there is a lot to criticize in this patch, but it *is* supposed to be a backport. As such I think it's sufficient.
I would propose to keep the backport and add improvements on top to keep patches more compatible.
Do you want to insist on improving the datastructures in this PR to merge?
The CI failure is actually a missing clang-format-10 package and reads like a gha-config issue to me.
|
Sorry, I'm somewhat frustrated about the poor quality of patches (and their reviews) applied to ROS2 branches.
Ok. I will handle that. |
Done: 177742b |
|
Sorry, I'm somewhat frustrated about the poor quality of patches (and their reviews) applied to ROS2 branches.
Yes.
If we don't insist on improvements immediately, when will they ever happen?
If someone is willing to do the work to backport, do we want to force them to cleanup the original patches?
If we just gate the `master` branch with "higher standards", I'm afraid we will just stay even more stale.
|
@v4hn I am actually preparing a PR with a backport of moveit/moveit2#390 right now, just doing some final testing before submitting the PR.
@rhaschke I understand and share your concerns with the redundant storage of joint_names and agree that using a map would streamline accessing the joint properties. My goal with this PR was to do a 1-to-1 backport of the ROS2 implementation to avoid introducing additional bugs when integrating with the backport of MoveIt2's diff drive implementation. |
I filed a corresponding issue: #110 😉 |
|
Unfortunately, there were still formatting issues. I fixed them and force-pushed. Please update your branch to the current master. |
This PR is a backport of #77 from Foxy to Noetic.
I am currently working on using MoveIt! for a differential drive robot in ROS1 and have been backporting components necessary for differential drive robots that was implemented for ROS2 back into ROS1.
This PR enables joint properties, which allows properties such as
angular_distance_weightto be defined for joints.These properties can then be used by the differential drive kinematics.
We also do the same code cleanup as seen in #77 and replace verbose joint validity checking with the newly defined
isValidJoint()function.