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

Support disabling pose publisher from publishing top level model pose #1342

Merged
merged 5 commits into from Feb 24, 2022

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Feb 16, 2022

Signed-off-by: Ian Chen ichen@osrfoundation.org

🦟 Bug fix

Summary

The pose-publisher system uses only one parameter, publish_nested_model_pose, to determine whether or not top level model and nested model poses should be published. This PR adds another parameter, publish_model_pose, to let users decouple pose publishing behavior between nested models and top level models.

The changes should preserve existing behavior. That is, if publish_nested_model_pose is set to true, the top level model pose should still be published unless the user explicitly disables this by setting publish_model_pose to false. We should consider changing the behavior in garden.

A typical use case is to disable publishing top level model's poses (ground truth data) but still allow nested model poses to be available to the user.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Feb 16, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Feb 16, 2022
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@@ -197,6 +200,12 @@ void PosePublisher::Configure(const Entity &_entity,
_sdf->Get<bool>("publish_nested_model_pose",
this->dataPtr->publishNestedModelPose).first;

// for backward compatibility, publish_model_pose will be set to the
// same valuee as publish_nested_model_pose if it is not specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// same valuee as publish_nested_model_pose if it is not specified.
// same value as publish_nested_model_pose if it is not specified.

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Looks good with the one typo.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #1342 (2a56a75) into ign-gazebo6 (f7cd5aa) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 2a56a75 differs from pull request most recent head f768cda. Consider uploading reports for the commit f768cda to get more accurate results

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo6    #1342      +/-   ##
===============================================
+ Coverage        62.91%   62.92%   +0.01%     
===============================================
  Files              299      299              
  Lines            24151    24159       +8     
===============================================
+ Hits             15194    15203       +9     
+ Misses            8957     8956       -1     
Impacted Files Coverage Δ
src/systems/pose_publisher/PosePublisher.hh 100.00% <ø> (ø)
src/systems/pose_publisher/PosePublisher.cc 94.71% <100.00%> (+0.21%) ⬆️
src/EntityComponentManager.cc 88.38% <0.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7cd5aa...f768cda. Read the comment docs.

@iche033 iche033 merged commit 8b8b9f6 into ign-gazebo6 Feb 24, 2022
Core development automation moved this from In review to Done Feb 24, 2022
@iche033 iche033 deleted the pose_publisher_model branch February 24, 2022 21:32
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants