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 Axes display use latest transform #892

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Aug 30, 2022

This makes the Axes display use the latest transform data, just like it did in ROS 1.

The problem with trying to get the transform at now() is the transform message was created in the past. Looking up the transform at the current time produces an error message about extrapolating into the future (assuming clocks are synchronized and the transform isn't future-dated).

To reproduce the bug this fixes:

  • Start RViz and add the Axes display
  • Set the "Reference Frame" to via_tf in the Axes display
  • Run this python file:
from geometry_msgs.msg import TransformStamped
import rclpy
from std_msgs.msg import Header
from tf2_ros import TransformBroadcaster


def main():
    rclpy.init()

    node = rclpy.create_node("rviz_pub")
    tf_broadcast = TransformBroadcaster(node)

    while True:
        header = Header()
        header.stamp = node.get_clock().now().to_msg()
        header.frame_id = "map"

        message = TransformStamped()
        message.header = header
        message.child_frame_id = "via_tf"
        message.transform.translation.x = 0.0
        message.transform.translation.y = 0.0
        message.transform.translation.z = 0.0
        message.transform.rotation.x = 0.0
        message.transform.rotation.y = 0.0
        message.transform.rotation.z = 0.0
        message.transform.rotation.w = 1.0
        tf_broadcast.sendTransform(message)


if __name__ == "__main__":
    main()

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz added the bug Something isn't working label Aug 30, 2022
@sloretz sloretz self-assigned this Aug 30, 2022
Copy link

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Woot! Just a small comment about existing docstrings.

Also, I'm surprised the ros2 docs mention a special tf2::TimePointZero, but it seems like other uses either use rclcpp::Time() or rclcpp::Time(0, 0, clock_->get_clock_type()) (as is done in FrameManager::getTransform.
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Learning-About-Tf2-And-Time-Cpp.html#tf2-and-time

What is the correct things to do? I see usages of both when searching through vcs import < ros2.repos for Humble.

EDIT: Yeah, I see tf2::TimePointZero, rclcpp::Time(), and various flavors of rclcpp::Time(0, 0...)

Is there already an issue for this? If not, probably good to file.

@@ -102,7 +102,7 @@ void AxesDisplay::update(float dt, float ros_dt)
Ogre::Vector3 position;
Ogre::Quaternion orientation;
if (context_->getFrameManager()->getTransform(
frame, context_->getClock()->now(), position, orientation))
frame, position, orientation))

Choose a reason for hiding this comment

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

nit (pre-existing) Documentation for this function seems insufficient:

/// Return the pose for a frame relative to the fixed frame, in Ogre classes, at a given time.
/**
* \param[in] frame The frame to find the pose of.
* \param[out] position The position of the frame relative to the fixed frame.
* \param[out] orientation The orientation of the frame relative to the
* fixed frame.
* \return true on success, false on failure.
*/
virtual
bool
getTransform(
const std::string & frame,
Ogre::Vector3 & position,
Ogre::Quaternion & orientation) = 0;

Is it possible to update that docstring to mention it's effectively using TimePointZero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated docs in #893

@EricCousineau-TRI
Copy link

Also, is there a way to encourage / ensure all plugins generally use the time-point-zero overload?
e.g. adding to docstring of function, "If you are writing a visualization plugin, you should prefer TimePointZero, so use this overload."

@sloretz
Copy link
Contributor Author

sloretz commented Aug 31, 2022

Also, I'm surprised the ros2 docs mention a special tf2::TimePointZero, but it seems like other uses either use rclcpp::Time() or rclcpp::Time(0, 0, clock_->get_clock_type()) (as is done in FrameManager::getTransform.

I think there are a couple things leading to rclcpp::Time being used instead of tf2::TimePointZero. The first is RViz's FrameManager is a pluggable interface. It defaults to using tf2, but allows people to provide their own plugin to do something else. (See #346 for more info). The second is tf2 has two parts: tf2 and tf2_ros. The former has it's own time typdefs using std::chrono types, and that's where tf2::TimePointZero comes from. Anything above tf2_ros is probably using rclcpp, and so will default to using rclcpp::Time especially since tf2_ros adds APIs that use it.

Also, is there a way to encourage / ensure all plugins generally use the time-point-zero overload?

I can document that plugins without a Header should use the time-point-zero overload, and from what I can tell that seems to already be the case. Plugins operating on data with a Header will want to use the transform at the time the data was measured.

@EricCousineau-TRI
Copy link

EricCousineau-TRI commented Sep 1, 2022

I think there are a couple things leading to [...]

Gotcha! Is that documented somewhere discoverable on docs.ros2.org?
Also, why do some sites use rclcpp::Time(0, 0) vs. rclcpp::Time(0, 0, clock->get_clock_type())?
The latter seems more precise?

I can document that plugins [...]

Sounds good!

@sloretz
Copy link
Contributor Author

sloretz commented Sep 1, 2022

Gotcha! Is that documented somewhere discoverable on docs.ros2.org?

RViz's Frame manager being pluggable is documented at https://index.ros.org/r/rviz/github-ros2-rviz/#pluggable-transformation-library . I didn't find any documentation for it on docs.ros2.org. It also links to an RViz plugin development guide here: https://github.com/ros2/rviz/blob/humble/docs/plugin_development.md

@sloretz
Copy link
Contributor Author

sloretz commented Sep 1, 2022

CI (build: --packages-above-and-dependencies rviz_default_plugins test: --packages-above rviz_default_plugins)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

Ok on green CI! Remember to merge this along #893 !

@sloretz sloretz merged commit 8dc9f8a into rolling Sep 1, 2022
@delete-merged-branch delete-merged-branch bot deleted the sloretz__rviz2__axes_use_latest_transform branch September 1, 2022 20:53
@EricCousineau-TRI
Copy link

thanks for the links! filed issue for time being -> #894

@sloretz
Copy link
Contributor Author

sloretz commented Sep 12, 2022

https://github.com/Mergifyio backport humble galactic foxy

mergify bot pushed a commit that referenced this pull request Sep 12, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 8dc9f8a)
mergify bot pushed a commit that referenced this pull request Sep 12, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 8dc9f8a)
mergify bot pushed a commit that referenced this pull request Sep 12, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 8dc9f8a)
@mergify
Copy link

mergify bot commented Sep 12, 2022

backport humble galactic foxy

✅ Backports have been created

sloretz added a commit that referenced this pull request Sep 12, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 8dc9f8a)

Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz added a commit that referenced this pull request Sep 12, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 8dc9f8a)

Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz added a commit that referenced this pull request Sep 12, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 8dc9f8a)

Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants