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

Perception screen added for the SA #969

Merged

Conversation

Projects
None yet
4 participants
@MohmadAyman
Copy link
Member

commented Jul 2, 2018

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)

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:perception_screen branch 4 times, most recently from c7d151c to 0b62878 Jul 2, 2018

@davetcoleman
Copy link
Member

left a comment

great work!

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 comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 10, 2018

Author Member

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

*/
struct OmplPlanningParameter
struct Parameter

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

rename to GenericParameter or something more descriptive

@@ -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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 10, 2018

Author Member

"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?

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

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

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 */

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

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

sensor_plugin_field_->setCurrentIndex(0);
}
}

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

great work with this file!!

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 comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

this should really be in a separate PR...

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

thanks for doing this!

@@ -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";

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

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

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_);

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 9, 2018

Member

*assistant (spelling)

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:perception_screen branch from 0b62878 to e8ba21c Jul 10, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

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

@@ -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.

This comment has been minimized.

Copy link
@Ridhwanluthra

Ridhwanluthra Jul 11, 2018

Contributor

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 11, 2018

Member

What name do you mean by "same name"?

This comment has been minimized.

Copy link
@Ridhwanluthra

Ridhwanluthra Jul 11, 2018

Contributor

The name of the yaml file "sensors_3d.yaml"

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 12, 2018

Author Member

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.

*/
struct OmplPlanningParameter
struct GenericParameter

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 11, 2018

Member

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 19, 2018

Author Member

Yes, i'll be working on that 👍

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 19, 2018

Author Member

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?

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 19, 2018

Author Member

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.

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 23, 2018

Author Member

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

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

This comment has been minimized.

Copy link
@Ridhwanluthra

Ridhwanluthra Jul 11, 2018

Contributor

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

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 12, 2018

Author Member

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

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:perception_screen branch from e8ba21c to af46fee Jul 23, 2018

@MohmadAyman MohmadAyman referenced this pull request Jul 23, 2018

Merged

ROS Controllers screen added to SA #994

3 of 5 tasks complete

@MohmadAyman MohmadAyman changed the title new screen for the SA for perception Perception screen added for the SA Jul 23, 2018

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

I need a second +1 from someone @mcevoyandy

@mcevoyandy
Copy link

left a comment

+1 These changes look good to me

@davetcoleman davetcoleman merged commit 30f3c3b into ros-planning:kinetic-devel Jul 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Perception screen added for the SA (#969)
* Added Perception screen

* perception used calss instead of struct
@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

cherry-picked to melodic

@MohmadAyman MohmadAyman referenced this pull request Aug 10, 2018

Closed

Setup Assistant 2 #894

6 of 6 tasks complete

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

Perception screen added for the SA (ros-planning#969)
* Added Perception screen

* perception used calss instead of struct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.