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

Make mtc execute solution capability apply scene diffs #467

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented May 23, 2023

This PR contains a fix for #353 by @JafarAbdi that we've been using on a fork for quite some time but never got contributed upstream. With his consent, I opened this PR to bring it upstream

/* RCLCPP_DEBUG_STREAM(LOGGER, "apply effect of " << description); */
scene_diff.robot_state.joint_state = sensor_msgs::msg::JointState();
scene_diff.robot_state.multi_dof_joint_state = sensor_msgs::msg::MultiDOFJointState();
if (!context_->planning_scene_monitor_->newPlanningSceneMessage(scene_diff))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment of @v4hn:

This is a fixup for 72afdd2 , please merge the commits.
Also, you should set robot_state.is_diff = true explicitly if you empty the robot_state fields and do it before you check for isEmpty because the set robot state will often be the only reason why the scene diff is not empty.

…ng trajectory

Don't override joint state in planning scene monitor when executing task's solution - Fix: moveit#353

Refactor creating an executable trajectory in a helper lambda

Make mtc execute solution capability apply scene diffs before executing trajectory

Fix compiler issues

Clang-tidy fixes

Copy planning scene to modify it
@sjahr sjahr force-pushed the pr-make_mtc_execute_solution_capability_apply_scene_diffs branch from 5e83f72 to c580584 Compare May 23, 2023 08:53
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a0befc5) 41.80% compared to head (c580584) 41.80%.

Additional details and impacted files
@@           Coverage Diff           @@
##             ros2     #467   +/-   ##
=======================================
  Coverage   41.80%   41.80%           
=======================================
  Files          78       78           
  Lines        7846     7846           
=======================================
  Hits         3279     3279           
  Misses       4567     4567           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sjahr sjahr requested review from v4hn and JafarAbdi May 23, 2023 11:50
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.

The changes are rather large and I don't see immediately what was actually changed.
Please add some more details to the main comment of this PR describing the actual problem and your solution for it.

@rhaschke
Copy link
Contributor

Given my feedback here, I would have expected a much simpler solution, namely removing the corresponding RobotState members.

@sjahr
Copy link
Contributor Author

sjahr commented Oct 23, 2023

Closed in favor of #498

@sjahr sjahr closed this Oct 23, 2023
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.

3 participants