Perception screen added for the SA #969

mayman99 · 2018-07-02T20:10:34Z

Description

This perception screen works as the following:
if (package has a 3d sensors config file) loads it into the screen
else load the default values from moveit_setup_assistanat/resources/config/sensors_rgbd.yaml -name is not final-.
when the user leaves the screen, saves the new config values to the sensors_rgbd.yaml to the package, if the user selected none from the plugin type drop down then no config file is saved.

Checklist

Required: Code is auto formatted using clang-format
Extended the tutorials / documentation, if necessary reference
Include a screenshot if changing a GUI
Optional: Created tests, which fail without this PR reference
Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

davetcoleman

great work!

davetcoleman · 2018-07-09T20:57:06Z

moveit_setup_assistant/CMakeLists.txt

-  catkin_add_gtest(test_writing_ros_controllers test/moveit_config_data_test.cpp)
-  target_link_libraries(test_writing_ros_controllers  ${PROJECT_NAME}_tools
+  catkin_add_gtest(test_reading_writing_config test/moveit_config_data_test.cpp)
+  target_link_libraries(test_reading_writing_config  ${PROJECT_NAME}_tools


this doesn't belong in this pull request, can you separate it please?

This PR adds a test to the moveit_config_data_test.cpp, so I renamed the test to be sth more generic.

davetcoleman · 2018-07-09T20:58:09Z

moveit_setup_assistant/include/moveit/setup_assistant/tools/moveit_config_data.h

 */
-struct OmplPlanningParameter
+struct Parameter


rename to GenericParameter or something more descriptive

davetcoleman · 2018-07-09T20:58:26Z

moveit_setup_assistant/include/moveit/setup_assistant/tools/moveit_config_data.h

@@ -79,9 +79,9 @@ struct GroupMetaData
 };

 /**
- * Planning parameters which may be set in the config files
+ * Parameters which may be set in the config files


"Reusable parameter struct which may be set in various configuration files"

"Reusable parameter struct which may be used in reading and writing configuration files"

as that is the main usage of this struct, is that okay?

davetcoleman · 2018-07-09T21:00:30Z

moveit_setup_assistant/include/moveit/setup_assistant/tools/moveit_config_data.h

+   *
+   * @param parameter name
+   * @param parameter value
+   * @param parameter comment


remove 'parameter' from each line

also, either add a real description for each parameter or remove the @param's part as it doesn't serve a purpose currently

davetcoleman · 2018-07-09T21:00:55Z

moveit_setup_assistant/include/moveit/setup_assistant/tools/moveit_config_data.h

+  void addParameterToSensorPluginConfig(const std::string& name, const std::string& value = "",
+                                        const std::string& comment = "");
+
+  /** \brief This Function for clearing the sensor plugin configuration parameter list */


More concise:
"This Function for clearing the..." -> "Clear the..."

davetcoleman · 2018-07-09T21:12:06Z

moveit_setup_assistant/src/widgets/perception_widget.cpp

+    sensor_plugin_field_->setCurrentIndex(0);
+  }
+}
+


great work with this file!!

davetcoleman · 2018-07-09T21:12:40Z

moveit_setup_assistant/src/widgets/setup_assistant_widget.cpp

-  connect(ssw_, SIGNAL(readyToProgress()), this, SLOT(progressPastStartScreen()));
-  connect(ssw_, SIGNAL(loadRviz()), this, SLOT(loadRviz()));
-  main_content_->addWidget(ssw_);
+  start_screen_widget_ = new StartScreenWidget(this, config_data_);


this should really be in a separate PR...

davetcoleman · 2018-07-09T21:13:00Z

moveit_setup_assistant/src/widgets/setup_assistant_widget.h

+  PassiveJointsWidget* passive_joints_widget_;
+  PerceptionWidget* perception_widget_;
+  AuthorInformationWidget* author_information_widget_;
+  ConfigurationFilesWidget* configuration_files_widget_;


thanks for doing this!

davetcoleman · 2018-07-09T21:13:46Z

moveit_setup_assistant/src/widgets/start_screen_widget.cpp

@@ -421,6 +421,23 @@ bool StartScreenWidget::loadExistingFiles()
  fs::path kinematics_yaml_path = config_data_->config_pkg_path_;
  kinematics_yaml_path /= "config/kinematics.yaml";

+  // Load sensors_3d yaml file if available --------------------------------------------------
+  fs::path sensors_3d_yaml_path = config_data_->config_pkg_path_;
+  sensors_3d_yaml_path /= "config/sensors_3d.yaml";


add note inside the sensors_3d.yaml files that the filename should not be changed or else the MSA will not detect it

davetcoleman · 2018-07-09T21:14:24Z

moveit_setup_assistant/test/moveit_config_data_test.cpp

+  moveit_setup_assistant::MoveItConfigDataPtr config_data_;
+  config_data_.reset(new moveit_setup_assistant::MoveItConfigData());
+
+  boost::filesystem::path setup_assistnat_path(config_data_->setup_assistant_path_);


*assistant (spelling)

mayman99 · 2018-07-10T20:12:20Z

@davetcoleman Taken care of all the comments, feel free to review :)

Ridhwanluthra · 2018-07-11T21:24:27Z

moveit_setup_assistant/resources/default_config/sensors_3d.yaml

@@ -0,0 +1,22 @@
+# This YAML configuration file holds the default values used for configuring 3D sensors.
+# The name of this file shouldn't be changed, or else the Setup Assistant won't detect it.


If this is the case maybe we need to keep the same name of the yaml file or is it not needed?

What name do you mean by "same name"?

The name of the yaml file "sensors_3d.yaml"

I'm not sure if I understand correctly but the name of the yaml file "sensors_3d.yaml" doesn't change if thats what you're asking.

davetcoleman · 2018-07-11T21:28:01Z

moveit_setup_assistant/include/moveit/setup_assistant/tools/moveit_config_data.h

 */
-struct OmplPlanningParameter
+struct GenericParameter


this should actually be a class since it has member functions (see GStyle guide)

Yes, i'll be working on that 👍

I think this struct is just passive.

I mean the member functions that uses this struct are addGenericParameterToSensorPluginConfig() and SensorPluginConfig is a member of the moveit_config_data, so if we made a class all it functions will be just get and set functions and the addGenericParamereter function would still be a function in the movit_config_data class.
Same with getSensorPluginConfig().
Did I get it correctly?

The controllers screen is also ready, but if this one really needs the struct to be class i'll modify it before I open a PR for it.

I've made two commits, the first one uses a struct and in the second one I've converted it into a class.

Ridhwanluthra · 2018-07-11T21:43:10Z

moveit_setup_assistant/resources/default_config/sensors_3d.yaml

+    padding_scale: 1.0
+    max_update_rate: 1.0
+    filtered_cloud_topic: filtered_cloud
+  - sensor_plugin: occupancy_map_monitor/DepthImageOctomapUpdater


Won't have both of them in the same file end up loading both of them?

Yes, but we don't put this file on the parameter server, we only use it for holding the default values.

davetcoleman · 2018-07-27T23:06:16Z

I need a second +1 from someone @mcevoyandy

mcevoyandy

+1 These changes look good to me

* Added Perception screen * perception used calss instead of struct

davetcoleman · 2018-07-31T23:19:38Z

cherry-picked to melodic

* Added Perception screen * perception used calss instead of struct

gavanderhoorn · 2019-09-27T12:04:24Z

moveit_setup_assistant/templates/moveit_config_pkg_template/launch/sensor_manager.launch.xml

@@ -2,6 +2,9 @@

  <!-- This file makes it easy to include the settings for sensor managers -->

+  <!-- Params for 3D sensors config -->
+  <rosparam command="load" file="$(find [GENERATED_PACKAGE_NAME])/config/sensors_3d.yaml" />


Late, but what was the rationale for not using the "robot specific sensor manager" .launch.xml infrastructure used further on in this file to load the .yaml?

This seems to have deviated from how this worked previously, and seems to make [ROBOT_NAME]_moveit_sensor_manager.launch.xml redundant (but that file is still generated by the Setup Assistant).

* Added Perception screen * perception used calss instead of struct

mayman99 force-pushed the perception_screen branch 4 times, most recently from c7d151c to 0b62878 Compare July 9, 2018 12:51

davetcoleman requested changes Jul 9, 2018

View reviewed changes

davetcoleman mentioned this pull request Jul 10, 2018

Perception tutorial update moveit/moveit_tutorials#178

Closed

mayman99 force-pushed the perception_screen branch from 0b62878 to e8ba21c Compare July 10, 2018 20:11

Ridhwanluthra reviewed Jul 11, 2018

View reviewed changes

davetcoleman approved these changes Jul 11, 2018

View reviewed changes

Ridhwanluthra reviewed Jul 11, 2018

View reviewed changes

mayman99 mentioned this pull request Jul 14, 2018

Perception screen for configuring 3D sensors #959

Closed

mayman99 added 2 commits July 19, 2018 08:07

Added Perception screen

74dc5ff

perception used calss instead of struct

af46fee

mayman99 force-pushed the perception_screen branch from e8ba21c to af46fee Compare July 23, 2018 12:43

mayman99 mentioned this pull request Jul 23, 2018

ROS Controllers screen added to SA #994

Merged

5 tasks

mayman99 changed the title ~~new screen for the SA for perception~~ Perception screen added for the SA Jul 23, 2018

mcevoyandy approved these changes Jul 30, 2018

View reviewed changes

davetcoleman merged commit 30f3c3b into moveit:kinetic-devel Jul 31, 2018

davetcoleman pushed a commit that referenced this pull request Jul 31, 2018

Perception screen added for the SA (#969)

3196715

* Added Perception screen * perception used calss instead of struct

mayman99 mentioned this pull request Aug 10, 2018

Setup Assistant 2 #894

Closed

6 tasks

mayman99 added a commit to mayman99/moveit that referenced this pull request Aug 25, 2018

Perception screen added for the SA (moveit#969)

aed2da0

* Added Perception screen * perception used calss instead of struct

gavanderhoorn reviewed Sep 27, 2019

View reviewed changes

This was referenced Sep 27, 2019

MSA: config/sensors_3d.yaml always populated #1679

Closed

MSA: incorrect 3D sensor configuration if no sensor option selected #1680

Open

pull bot pushed a commit to shadow-robot/moveit that referenced this pull request Sep 3, 2020

Perception screen added for the SA (moveit#969)

36cd70d

* Added Perception screen * perception used calss instead of struct

rhaschke mentioned this pull request Nov 5, 2021

Fix handling of sensors_3d.yaml #2946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perception screen added for the SA #969

Perception screen added for the SA #969

mayman99 commented Jul 2, 2018 •

edited

Loading

davetcoleman left a comment

davetcoleman Jul 9, 2018

mayman99 Jul 10, 2018

davetcoleman Jul 9, 2018

davetcoleman Jul 9, 2018

mayman99 Jul 10, 2018

davetcoleman Jul 9, 2018

davetcoleman Jul 9, 2018

davetcoleman Jul 9, 2018

davetcoleman Jul 9, 2018

davetcoleman Jul 9, 2018

davetcoleman Jul 9, 2018

davetcoleman Jul 9, 2018

mayman99 commented Jul 10, 2018

Ridhwanluthra Jul 11, 2018

davetcoleman Jul 11, 2018

Ridhwanluthra Jul 11, 2018

mayman99 Jul 12, 2018

davetcoleman Jul 11, 2018

mayman99 Jul 19, 2018

mayman99 Jul 19, 2018

mayman99 Jul 19, 2018

mayman99 Jul 23, 2018

Ridhwanluthra Jul 11, 2018

mayman99 Jul 12, 2018

davetcoleman commented Jul 27, 2018

mcevoyandy left a comment

davetcoleman commented Jul 31, 2018

gavanderhoorn Sep 27, 2019

		@@ -0,0 +1,22 @@
		# This YAML configuration file holds the default values used for configuring 3D sensors.
		# The name of this file shouldn't be changed, or else the Setup Assistant won't detect it.

Perception screen added for the SA #969

Perception screen added for the SA #969

Conversation

mayman99 commented Jul 2, 2018 • edited Loading

Description

Checklist

davetcoleman 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

Choose a reason for hiding this comment

mayman99 commented Jul 10, 2018

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 commented Jul 27, 2018

mcevoyandy left a comment

Choose a reason for hiding this comment

davetcoleman commented Jul 31, 2018

Choose a reason for hiding this comment

mayman99 commented Jul 2, 2018 •

edited

Loading