Melodic robot model testing utils #1176

BryceStevenWilley · 2018-10-31T03:23:44Z

Description

Originally discussed in #1126, this PR provides a Builder class for making RobotModels. While it's aimed at making it very simple to make a specific RobotModel with the details you need for a test, I'm making it able to handle more complex Models as well.

I'm marking this as a WIP because there are a few tests that I haven't converted to use this yet, and I wanted to ask for feedback: does it look easy to use, etc. It's not going to be useful if we (the developers/community) think the interface is clunky or verbose.

Checklist

Required by CI: Code is auto formatted using clang-format
Extended the tutorials / documentation:
Inline docs are present, but a section under the tutorials would be nice too.
Document API changes relevant to the user in the moveit/MIGRATION.md notes
Created tests, which fail without this PR reference
Decide if this should be cherry-picked to other current ROS branches (should not)

rhaschke

Great contribution! Thanks a lot for this effort. I have only some minor remarks.

rhaschke · 2018-10-31T07:20:24Z

moveit_core/utils/include/moveit/utils/robot_model_builder.h

+   * \param[in] joint_origins The "parent to joint" origins for the joints connecting the links. If not used, all
+   * origins will default to the identity transform
+   */
+  void add(std::string section, std::string type, std::vector<geometry_msgs::Pose> joint_origins = {});


Why not using enum JointModel::JointType for type instead of string? Strings are always error-prone.

I like this idea, but JointModel::JointType doesn't have a CONTINUOUS value. Since this is writing directly to URDF, which does support continuous, we'd have to stick with strings.

moveit_core/utils/include/moveit/utils/robot_model_builder.h

davetcoleman · 2018-10-31T21:35:25Z

moveit_core/utils/CMakeLists.txt

-add_library(${MOVEIT_LIB_NAME} src/lexical_casts.cpp)
-set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES VERSION ${${PROJECT_NAME}_VERSION})
+if(CATKIN_ENABLE_TESTING)
+  add_library(${MOVEIT_LIB_NAME}


i don't like how this library changes scope based on whether testing is enabled or not. why not just have a separate library for things that are only for tests, and other utils?

I can do a separate library. My main concern was requiring moveit_resources to build moveit_core, which didn't seem right.

davetcoleman · 2018-10-31T21:37:40Z

moveit_core/utils/include/moveit/utils/robot_model_builder.h

@@ -0,0 +1,178 @@
+/*********************************************************************


is this intended to be used anywhere that is not test-specific? if not, rename file to something like robot_model_test_utils.h

if you intend to use it outside of test, i'm concerned it has duplicate functionality with
https://github.com/ros-planning/moveit/tree/melodic-devel/moveit_ros/planning/rdf_loader
https://github.com/ros-planning/moveit/tree/melodic-devel/moveit_ros/planning/robot_model_loader

Not intended to be used elsewhere, I'll clarify that. Also, wasn't aware of those other methods, neat.

I didn't do a deep dive, but do you think can we get rid of the rdf_loader after this is added? Or are they very different things? I haven't looked in a few years

I wouldn't say they're very different, rdf_loader seems to be a bit more general, looks like it handles Xacro files as well. It wouldn't be hard to move rdf_loader into moveit_core/utils and merge with loadTestingRobotModel() et al. though. Should that be another PR, or should it go it here?

Looking at it further, the rdf_loader has more ROS dependencies than allowed in moveit_core, e.g.

#include <ros/ros.h> #include <ros/package.h>

For example it uses the parameter server. While I am conflicted about moveit_core's level of ROS-agnotistism (not very, but still a little) I think that's a discussion for another issue/PR. I'd say let's move with this PR as-is

moveit_core/utils/include/moveit/utils/robot_model_builder.h

moveit_core/utils/src/robot_model_builder.cpp

davetcoleman · 2018-10-31T21:46:32Z

moveit_core/utils/src/robot_model_builder.cpp

+  if (link_names.empty())
+  {
+    ROS_ERROR_NAMED(LOGNAME, "No links specified (empty section?)");
+    return;


if this is going to be used for unit tests it seems the function should return false?

Hm, that's a good point. However, I don't think I like the tests looking like this:

bool is_valid = true; is_valid = is_valid && builder.add(....); is_valid = is_valid && builder.add(....); ... ASSERT_TRUE(is_valid);

The same thing can be done internally, so most of tests should just look like:

builder.add(...); builder.add(...); ... ASSERT_TRUE(builder.isValid());

moveit_core/utils/src/robot_model_builder.cpp

davetcoleman

This is a really great contribution that levels-up moveit!

davetcoleman · 2018-11-01T02:54:33Z

Before we merge this, can you make a short PR to the tests tutorial explaining how to use this and linking to an example file or two in the code base?

davetcoleman · 2018-11-01T03:58:07Z

Also please remove WIP when you're ready

BryceStevenWilley · 2018-11-01T12:33:24Z

Should be ready. Ended up making a conscious choice not to add enough features to converts bits of code like this, since it would make the interface too complicated to be used easily. Might tackle that in a later PR.

rhaschke

Some more comments. Otherwise lgtm.

moveit_core/collision_detection_fcl/test/test_fcl_collision_detection.cpp

moveit_core/constraint_samplers/test/test_constraint_samplers.cpp

moveit_core/kinematic_constraints/test/test_constraints.cpp

moveit_core/utils/CMakeLists.txt

davetcoleman · 2018-11-03T00:42:42Z

Whoo-hoo!

moveit#1176)

rhaschke approved these changes Oct 31, 2018

View reviewed changes

davetcoleman requested changes Oct 31, 2018

View reviewed changes

davetcoleman approved these changes Nov 1, 2018

View reviewed changes

BryceStevenWilley changed the title ~~WIP: Melodic robot model testing utils~~ Melodic robot model testing utils Nov 1, 2018

BryceStevenWilley mentioned this pull request Nov 1, 2018

Unit Tests Tutorial moveit/moveit_tutorials#253

Merged

rhaschke requested changes Nov 1, 2018

View reviewed changes

BryceStevenWilley added 10 commits November 2, 2018 00:14

Addition of robot_model_builder and integration into first test.

92f05d1

Extension to all tests, addition of additional features to do so

617d75c

Added inertial to builder, added docs.

cf90ed3

clang format

a80ac75

Fixed missing virtual joint param in test

5f76115

Addressed PR comments: better names, isValid, cleaner usage.

a5d581f

clang format

2d19fcd

Fixed spacemacs shortcut that ended up in the License

881040f

Left an extra bool.

2aa9e68

Reduce dead code, only compile test lib if testing enabled

fb60779

BryceStevenWilley force-pushed the melodic-robot-model-testing-utils branch from eb202cf to fb60779 Compare November 2, 2018 04:42

rhaschke approved these changes Nov 2, 2018

View reviewed changes

rhaschke merged commit 0e40658 into moveit:melodic-devel Nov 2, 2018

BryceStevenWilley deleted the melodic-robot-model-testing-utils branch November 3, 2018 13:51

sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024

Add launch file configurations for static tfs and spawning controllers (

81f10f1

moveit#1176)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Melodic robot model testing utils #1176

Melodic robot model testing utils #1176

BryceStevenWilley commented Oct 31, 2018

rhaschke left a comment

rhaschke Oct 31, 2018

BryceStevenWilley Nov 1, 2018

davetcoleman Oct 31, 2018

BryceStevenWilley Oct 31, 2018

davetcoleman Oct 31, 2018

BryceStevenWilley Oct 31, 2018

davetcoleman Nov 1, 2018

BryceStevenWilley Nov 1, 2018

davetcoleman Nov 1, 2018

davetcoleman Oct 31, 2018

BryceStevenWilley Nov 1, 2018

davetcoleman left a comment

davetcoleman commented Nov 1, 2018

davetcoleman commented Nov 1, 2018

BryceStevenWilley commented Nov 1, 2018

rhaschke left a comment

davetcoleman commented Nov 3, 2018

		@@ -0,0 +1,178 @@
		/*********************************************************************

Melodic robot model testing utils #1176

Melodic robot model testing utils #1176

Conversation

BryceStevenWilley commented Oct 31, 2018

Description

Checklist

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetcoleman left a comment

Choose a reason for hiding this comment

davetcoleman commented Nov 1, 2018

davetcoleman commented Nov 1, 2018

BryceStevenWilley commented Nov 1, 2018

rhaschke left a comment

Choose a reason for hiding this comment

davetcoleman commented Nov 3, 2018