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

Add parameter default_planning_scene to load planning scene geometry on startup #2949

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

pac48
Copy link
Contributor

@pac48 pac48 commented Aug 5, 2024

Description

This PR adds a new parameter to the move_group node that allows users to specify a planning scene file to load on startup. The parameter is a string containing the absolute file path. The parameter can be set with either $(find-pkg-share pkg-wth-planning-file) for XML or ament_index_python.packages.get_package_share_directory in Python to facilitate specifying the path. If the parameter is not specified, the move_group node will behave as it previously did. If there is an error loading the file, the node will exit with a fatal error.

Checklist

  • Extend the tutorials / documentation reference
  • Create tests, which fail without this PR reference

pac48 added 9 commits August 5, 2024 10:42
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
@sjahr sjahr self-requested a review August 6, 2024 08:54
std::string path = nh->get_parameter("default_planning_scene").as_string();
std::fstream file_stream;
file_stream.open(path, std::fstream::in);
if (!file_stream.is_open() || !ps->loadGeometryFromStream(file_stream))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is too dangerous because you're not locking the planning scene when writing to it (see

).
You can get thread safe access like this:

{
  planning_scene_monitor::LockedPlanningSceneRW ps = planning_display_->getPlanningSceneRW();
  if (ps)
     FANCY MODIFICATIONS
} // End scope to release lock

Copy link

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Sep 23, 2024
@sjahr
Copy link
Contributor

sjahr commented Oct 11, 2024

@pac48 Should we close this?

@github-actions github-actions bot removed the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Oct 14, 2024
@pac48
Copy link
Contributor Author

pac48 commented Oct 21, 2024

@sjahr We should try to merge this. I think the change to the test should either be deleted or more effort has to be put in to make it not flaky.

@pac48 pac48 force-pushed the pr-load-default-planning-scene branch from 2355089 to 4eef9ee Compare November 5, 2024 17:02
pac48 added 2 commits November 5, 2024 10:02
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
@pac48 pac48 requested a review from sjahr November 5, 2024 17:04
@pac48 pac48 marked this pull request as ready for review November 5, 2024 17:04
Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

I think it is a useful addition but it could be hardened a bit to make it harder to misuse. Also can you add the PR description as a documentation comment on top of this function call? At the moment there is nothing explaining what this is doing or how it is supposed to be used

@@ -290,8 +290,26 @@ int main(int argc, char** argv)
const auto moveit_cpp = std::make_shared<moveit_cpp::MoveItCpp>(nh, moveit_cpp_options);
const auto planning_scene_monitor = moveit_cpp->getPlanningSceneMonitorNonConst();

if (planning_scene_monitor->getPlanningScene())
if (auto ps = planning_scene_monitor->getPlanningScene())
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that ps is never used, would make sense to revert this change?

{
if (nh->has_parameter("default_planning_scene"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit concerned about tying this to only the existence of the parameter. How about checking whether or not the parameter is set.

const auto default_planning_scene_file = std::string();
nh->get_parameter_or("default_planning_scene", default_planning_scene_file, std::string());
// Maybe add function to validate the string pattern if it actually representing  path
if(validateFilePath(default_planning_scene_file))
{
...
}

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.

2 participants