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 static pose publisher and support pose_v msg type in pose_publisher system #65

Merged
merged 9 commits into from
May 5, 2020

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Apr 17, 2020

Added two features to help reduce bandwidth of pose-publisher's pose topic

  • Static pose publisher:

    • When the <static_publisher> sdf element is set to true, a static pose publisher is created that publishes static pose transform for entities in the model that do not change pose. Links connected by joints are considered as dynamic entities (except for canonical link) and their pose is published to the original /model/[model_name]/pose topic, while the rest are published to the new /model/[model_name]/pose_static topic.
    • Ideally the static pose publisher should be latched like the /tf_static topic so we don't have to keep publishing the data but I don't think ign transport has latched support yet. So I added <static_update_frequency> sdf element to let users configure / reduce the publisher's update rate
  • Pose_V message:

    • Added the <use_pose_vector_msg> sdf element to support publishing ignition::msgs::Pose_V msgs (vector of poses at one sim iteration) as opposed to individual ignition::msgs::Pose msgs. This helps to reduce the number of published msgs with the trade-off of larger msg size. The default pose publisher's publish rate should now be equal to the simulation's update rate, whereas before the update rate was equal to num_entities_in_model * sim_update_rate

related ros_ign pull request

src/systems/pose_publisher/PosePublisher.cc Outdated Show resolved Hide resolved
src/systems/pose_publisher/PosePublisher.cc Outdated Show resolved Hide resolved
src/systems/pose_publisher/PosePublisher.cc Outdated Show resolved Hide resolved
@nkoenig nkoenig mentioned this pull request Apr 17, 2020
iche033 and others added 2 commits April 17, 2020 11:11
doxygen fixes

Co-Authored-By: Nate Koenig <nkoenig@users.noreply.github.com>
@chapulina
Copy link
Contributor

@osrf-jenkins run tests

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@mjcarroll mjcarroll self-requested a review April 27, 2020 18:27
/// performance.
public: ignition::msgs::Pose_V poseVMsg;


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra vertical whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed. 4255f22

{
auto entityIt = this->entitiesToPublish.find(entity);
if (entityIt == this->entitiesToPublish.end())
continue;

if (this->usePoseV)
msg = this->poseVMsg.add_pose();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe curly braces around one line of logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added. 4255f22

// frame_id: parent entity name
// child_frame_id = entity name
// pose is the transform from frame_id to child_frame_id
auto header = this->poseMsg.mutable_header();
auto header = msg->mutable_header();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance of msg being null here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should never be null. Added IGN_ASSERT to be sure. 4255f22

}
mutex.unlock();


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra vertical whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed. 4255f22

<static_publisher>true</static_publisher>
<use_pose_vector_msg>true</use_pose_vector_msg>
</plugin>

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed. 4255f22

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.

Just a few style nits, but otherwise looks good.

iche033 added 2 commits May 4, 2020 14:14
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 merged commit dd5c72b into ign-gazebo2 May 5, 2020
@iche033 iche033 deleted the static_pose_v branch May 5, 2020 01:00
@adlarkin adlarkin mentioned this pull request Apr 1, 2021
8 tasks
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.

4 participants