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

Patches and fixes for ros2 branch #461

Closed
wants to merge 6 commits into from
Closed

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented May 15, 2023

This PR contains a couple of fixes and improvements by @JafarAbdi that we've been using on a fork for quite some time but never got contributed upstream. With his consent, I am making that up now with this PR.

If desired I can divide this PR into smaller ones but I wanted to hear your feedback first.

@sjahr sjahr requested review from JafarAbdi and sea-bass May 15, 2023 11:30
Copy link
Contributor

@sea-bass sea-bass 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 a lot of the member variables recently switched from having underscores to not having underscores, which is causing build failures. Take a look at that?

@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Patch and project coverage have no change.

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

Additional details and impacted files
@@           Coverage Diff           @@
##             ros2     #461   +/-   ##
=======================================
  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.

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Thank you! Makes me wonder why these patches didn't make it here over the last year...

core/src/stage.cpp Outdated Show resolved Hide resolved
auto scene_diff_copy = scene_diff;
scene_diff_copy.robot_state.joint_state = sensor_msgs::msg::JointState();
scene_diff_copy.robot_state.multi_dof_joint_state = sensor_msgs::msg::MultiDOFJointState();
if (!context_->planning_scene_monitor_->newPlanningSceneMessage(scene_diff_copy))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Could you split this feature into a separate PR? It's fully independent of the tiny other cleanups and might break something in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a fixup for 72afdd2 , please merge the commits.

@v4hn Which commits are you referring to?

@sjahr sjahr requested a review from v4hn May 23, 2023 11:50
@rhaschke
Copy link
Contributor

Thanks for providing those improvements. I already merged 4001d32 and 1df04f9: 45ff3ea.
Note that 6ea4cd1 reverts 3b043dc!
Please file a new PR for 8326d32 and its cleanup 1ca5f9f and provide a motivation for using a separate thread.

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