search

moveit / srdfdom

Semantic Robot Description Format
BSD 3-Clause "New" or "Revised" License
13 stars 68 forks source link

Backporting Joint Properties back to Noetic #109

Closed scchow closed 1 year ago

scchow commented 1 year ago

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_weight to 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.

v4hn commented 1 year ago

Please refer to the original PR/commits (and if possible the authors) in the commit.

v4hn commented 1 year ago

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.

rhaschke commented 1 year ago

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?

Sorry, I'm somewhat frustrated about the poor quality of patches (and their reviews) applied to ROS2 branches. If we don't insist on improvements immediately, when will they ever happen?

The CI failure is actually a missing clang-format-10 package and reads like a gha-config issue to me.

Ok. I will handle that.

rhaschke commented 1 year ago

The CI failure is actually a missing clang-format-10 package and reads like a gha-config issue to me.

Ok. I will handle that.

Done: 177742b707c2b5da77b1ffa1049f59953e897ee7

v4hn commented 1 year ago

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.

scchow commented 1 year ago

@scchow Do you plan to provide a follow-up PR that adds ports main feature to MoveIt as well?

@v4hn I am actually preparing a PR with a backport of https://github.com/ros-planning/moveit2/pull/390 right now, just doing some final testing before submitting the PR.

I understand that this is a backport. Nevertheless, I suggest rethinking the data structure to store joint properties to better reflect the intended semantics. Before merging, you definitely need to fix the formatting errors (see instructions in CI job).

@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. Once I have that working, I'll be happy to come back and see if I can submit another PR cleaning up the JointProperty data structures.

rhaschke commented 1 year ago

I'll be happy to come back and see if I can submit another PR cleaning up the JointProperty data structures.

I filed a corresponding issue: #110 :wink:

rhaschke commented 1 year ago

Unfortunately, there were still formatting issues. I fixed them and force-pushed. Please update your branch to the current master.