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

Check errors when loading scene geometry from files #1056

Merged
merged 7 commits into from
Oct 6, 2018

Conversation

jonbinney
Copy link
Contributor

@jonbinney jonbinney commented Sep 9, 2018

Description

Before this PR, there was no error checking when loading planning scene geometry from a text file. This meant the caller couldn't tell whether the planning scene had been successfully loaded.

Not sure if this should be cherry-picked. It changes the return type of two PlanningScene member functions, so I think it would require people to re-compile their code.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Created tests, which fail without this PR reference
  • Decide if this should be cherry-picked to other current ROS branches
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@felixvd
Copy link
Contributor

felixvd commented Sep 9, 2018

I like helpful error messages, so I like this. Is there anything to update in the visualization tab?

@jonbinney
Copy link
Contributor Author

@felixvd do you mean for the rviz plugin? I hadn't looked at that - I was actually using this function directly from a non-gui program.

Copy link
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.

Love it

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Not sure how critical this is but 1-2 unit tests would be nice. With or without, these are very welcome changes.

@jonbinney
Copy link
Contributor Author

@bmagyar good point - I'll add a couple unit tests..

@jonbinney
Copy link
Contributor Author

@bmagyar I've added a couple tests. Not exhaustive by any means, but should do a decent job of sanity checking the code.

@rhaschke
Copy link
Contributor

rhaschke commented Oct 5, 2018

I changed usages of the method in existing MoveIt! code to consider the return value.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I also approve this. Instead of returning a boolean, what about throwing an exception?
In this fashion we could also pass the error message to the caller. However, that's not the standard in ROS.

@jonbinney
Copy link
Contributor Author

@rhaschke thanks for updating the rviz plugin to use this! Switching to exceptions seems like a bigger change - other functions in PlanningScene like processAttachedCollisionObjectMsg use a return value (bool) to indicate success/failure and I figured it would be best to keep things similar.

@jonbinney
Copy link
Contributor Author

(if you do want exceptions here though, let me know and I'm happy to update the PR)

@rhaschke rhaschke merged commit 43993ca into moveit:melodic-devel Oct 6, 2018
rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Oct 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants