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

fix bug in perception screen #1013

Merged

Conversation

Projects
None yet
3 participants
@MohmadAyman
Copy link
Member

commented Aug 2, 2018

Description

  1. Fixing a bug that makes the SA crashes when switching from the perception screen.
  2. Improve documentation.

Checklist

@@ -1652,7 +1656,11 @@ std::vector<std::map<std::string, GenericParameter>> MoveItConfigData::getSensor
// ******************************************************************************************
void MoveItConfigData::clearSensorPluginConfig()
{
sensors_plugin_config_parameter_list_.clear();
// sensors_plugin_config_parameter_list_.clear();

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

remove comment

@@ -1652,7 +1656,11 @@ std::vector<std::map<std::string, GenericParameter>> MoveItConfigData::getSensor
// ******************************************************************************************
void MoveItConfigData::clearSensorPluginConfig()
{
sensors_plugin_config_parameter_list_.clear();
// sensors_plugin_config_parameter_list_.clear();
for (int i = 0; i < sensors_plugin_config_parameter_list_.size(); ++i)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

size_t

better var name e.g. param_id

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Aug 9, 2018

Author Member

Well I do really apologize for this mistake, its not my first time doing it.

new HeaderWidget("3D Perception Sensor Configuration", "Configure your 3D sensors to work with Moveit! ", this);
HeaderWidget* header = new HeaderWidget("3D Perception Sensor Configuration",
"Configure your 3D sensors to work with Moveit! "
"Look at <a "

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

"Look at" --> "Please see"

"href='http://docs.ros.org/kinetic/api/moveit_tutorials/html/doc/"
"perception_configuration/"
"perception_configuration_tutorial.html'>Perception Documentation</a> "
"for more details",

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

Add "." after "details"

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

I still do not see any default values when I choose a sensor plugin:

image

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

Bug: if I go to the "3D Perception" screen and choose "Depth Map" from the drop down list, then switch to the "Passive Joints" screen, the MSA segfaults.

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2018

@davetcoleman
I do confirm the other bug about the segfaults but I can still not reproduce the no default values.
Would you tell me what do you see if you run the catkin test in the setup assistant? there is actually a test case on reading the default values.

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2018

I also cant reproduce the segmentation fault either on this branch.

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Run on a new moveit_config package, not an existing one.

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:perception_screen_fix branch from 7a7f265 to 3718f32 Aug 9, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

Fixed the bug the occurs when using on a new package by calling the input3DSensors() even if the package is new.

@MohmadAyman MohmadAyman referenced this pull request Aug 9, 2018

Merged

ROS Controllers screen added to SA #994

3 of 5 tasks complete

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:perception_screen_fix branch from 3718f32 to 48037a6 Aug 9, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

@davetcoleman Travis is happy.

@davetcoleman
Copy link
Member

left a comment

Tested locally and it works!
Should be merged asap due to hotfix

@mcevoyandy
Copy link

left a comment

two minor comments

@@ -859,7 +859,7 @@ bool MoveItConfigData::output3DSensorPluginYAML(const std::string& file_path)
emitter << YAML::BeginMap;

// Make sure sensors_plugin_config_parameter_list_ is not empty
if (sensors_plugin_config_parameter_list_.size() == 1)
if (sensors_plugin_config_parameter_list_.size() > 0)

This comment has been minimized.

Copy link
@mcevoyandy

mcevoyandy Aug 13, 2018

why not if(!sensors_plugin_config_parameter_list_.empty())

@@ -1518,6 +1518,7 @@ bool MoveItConfigData::input3DSensorsYAML(const std::string& default_file_path,
{

This comment has been minimized.

Copy link
@mcevoyandy

mcevoyandy Aug 13, 2018

not really in the scope of this PR, but why not combine these if statements?
if (const YAML::Node& sensors_node = doc["sensors"] && sensors_node.IsSequence())

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Aug 13, 2018

Author Member

I think can do sth like this

    // Get sensors node
    const YAML::Node& sensors_node = doc["sensors"];
  
    // Make sure that the sensors are written as a sequence
    if (sensors_node && sensors_node.IsSequence())

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:perception_screen_fix branch from 48037a6 to de2c521 Aug 13, 2018

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:perception_screen_fix branch from de2c521 to babd97b Aug 13, 2018

@davetcoleman
Copy link
Member

left a comment

Great job!

@davetcoleman davetcoleman merged commit 199eded into ros-planning:kinetic-devel Aug 14, 2018

1 check passed

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

davetcoleman added a commit to davetcoleman/moveit that referenced this pull request Aug 14, 2018

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

cherry-picked to melodic

@MohmadAyman MohmadAyman referenced this pull request Aug 20, 2018

Closed

Setup Assistant 2 #894

6 of 6 tasks complete
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.