Skip to content

fix bug in perception screen#1013

Merged
davetcoleman merged 1 commit intomoveit:kinetic-develfrom
mayman99:perception_screen_fix
Aug 14, 2018
Merged

fix bug in perception screen#1013
davetcoleman merged 1 commit intomoveit:kinetic-develfrom
mayman99:perception_screen_fix

Conversation

@mayman99
Copy link
Copy Markdown
Contributor

@mayman99 mayman99 commented Aug 2, 2018

Description

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

Checklist

davetcoleman
davetcoleman requested changes Aug 5, 2018
View reviewed changes
moveit_setup_assistant/src/tools/moveit_config_data.cpp Outdated
void MoveItConfigData::clearSensorPluginConfig()
{
sensors_plugin_config_parameter_list_.clear();
// sensors_plugin_config_parameter_list_.clear();
Copy link
Copy Markdown
Member

@davetcoleman davetcoleman Aug 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

moveit_setup_assistant/src/tools/moveit_config_data.cpp Outdated
{
sensors_plugin_config_parameter_list_.clear();
// sensors_plugin_config_parameter_list_.clear();
for (int i = 0; i < sensors_plugin_config_parameter_list_.size(); ++i)
Copy link
Copy Markdown
Member

@davetcoleman davetcoleman Aug 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size_t

better var name e.g. param_id

Copy link
Copy Markdown
Contributor Author

@mayman99 mayman99 Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

moveit_setup_assistant/src/widgets/perception_widget.cpp Outdated
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 "
Copy link
Copy Markdown
Member

@davetcoleman davetcoleman Aug 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Look at" --> "Please see"

moveit_setup_assistant/src/widgets/perception_widget.cpp Outdated
"href='http://docs.ros.org/kinetic/api/moveit_tutorials/html/doc/"
"perception_configuration/"
"perception_configuration_tutorial.html'>Perception Documentation</a> "
"for more details",
Copy link
Copy Markdown
Member

@davetcoleman davetcoleman Aug 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "." after "details"

@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Aug 5, 2018

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

image

@davetcoleman
Copy link
Copy Markdown
Member

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

@mayman99
Copy link
Copy Markdown
Contributor Author

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

@mayman99
Copy link
Copy Markdown
Contributor Author

mayman99 commented Aug 6, 2018

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

@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Aug 6, 2018

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

@mayman99 mayman99 force-pushed the perception_screen_fix branch from 7a7f265 to 3718f32 Compare August 9, 2018 05:28
@mayman99
Copy link
Copy Markdown
Contributor Author

mayman99 commented Aug 9, 2018 •
edited
Loading

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

@mayman99 mayman99 mentioned this pull request Aug 9, 2018
Merged
5 tasks
@mayman99 mayman99 force-pushed the perception_screen_fix branch from 3718f32 to 48037a6 Compare August 9, 2018 22:30
@mayman99
Copy link
Copy Markdown
Contributor Author

mayman99 commented Aug 9, 2018

@davetcoleman Travis is happy.

davetcoleman
davetcoleman approved these changes Aug 13, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mcevoyandy
mcevoyandy approved these changes Aug 13, 2018
View reviewed changes
Copy link
Copy Markdown

@mcevoyandy mcevoyandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two minor comments

moveit_setup_assistant/src/tools/moveit_config_data.cpp Outdated

// 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)
Copy link
Copy Markdown

@mcevoyandy mcevoyandy Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not if(!sensors_plugin_config_parameter_list_.empty())

moveit_setup_assistant/src/tools/moveit_config_data.cpp Outdated
@@ -1518,6 +1518,7 @@ bool MoveItConfigData::input3DSensorsYAML(const std::string& default_file_path,
{
Copy link
Copy Markdown

@mcevoyandy mcevoyandy Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Copy Markdown
Contributor Author

@mayman99 mayman99 Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

@mayman99 mayman99 force-pushed the perception_screen_fix branch from 48037a6 to de2c521 Compare August 13, 2018 20:03
@mayman99 mayman99 force-pushed the perception_screen_fix branch from de2c521 to babd97b Compare August 13, 2018 22:26
davetcoleman
davetcoleman approved these changes Aug 14, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@davetcoleman davetcoleman merged commit 199eded into moveit:kinetic-devel Aug 14, 2018
davetcoleman pushed a commit to davetcoleman/moveit that referenced this pull request Aug 14, 2018
@mayman99 @davetcoleman
fix bug in perception screen (moveit#1013)
9dae7cd
@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Aug 14, 2018

cherry-picked to melodic

@mayman99 mayman99 mentioned this pull request Aug 20, 2018
Closed
6 tasks
pull bot pushed a commit to shadow-robot/moveit that referenced this pull request Sep 3, 2020
@mayman99 @pull
fix bug in perception screen (moveit#1013)
12b2259
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davetcoleman davetcoleman davetcoleman approved these changes

+1 more reviewer

@mcevoyandy mcevoyandy mcevoyandy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mayman99 @davetcoleman @mcevoyandy