-
Notifications
You must be signed in to change notification settings - Fork 283
Description
rviz/rviz_common/src/rviz_common/visualization_manager.cpp
Lines 341 to 360 in 2f82645
| const uint64_t wall_dt = std::chrono::duration_cast<std::chrono::nanoseconds>(wall_diff).count(); | |
| const auto ros_now = clock_->now(); | |
| const uint64_t ros_dt = ros_now.nanoseconds() - last_update_ros_time_.nanoseconds(); | |
| last_update_ros_time_ = ros_now; | |
| last_update_wall_time_ = wall_now; | |
| if (ros_dt < 0.0) { | |
| resetTime(); | |
| } | |
| executor_->spin_some(std::chrono::milliseconds(10)); | |
| Q_EMIT preUpdate(); | |
| frame_manager_->update(); | |
| root_display_group_->update(wall_dt, ros_dt); | |
| if (nullptr != view_manager_) { | |
| view_manager_->update(wall_dt, ros_dt); |
Compared to rviz1, the update dt was suddenly changed from seconds to nanoseconds, which is not documented anywhere I could find even after finding out.
There are multiple issues with this:
- It is not documented despite being a substantial semantic change (neither the change nor in the method documentation)
On the contrary, the method documentation even still explicitly states it would be in seconds:
rviz/rviz_common/include/rviz_common/display.hpp
Lines 152 to 159 in 2f82645
/// Called periodically by the visualization manager. /** * \param wall_dt Wall-clock time, in seconds, since the last time the update list was run through. * \param ros_dt ROS time, in seconds, since the last time the update list was run through. */ virtual void update(float wall_dt, float ros_dt); - The datatype did not change, which would at least have been an indication of change, but more importantly, a float makes no sense for nanoseconds since there are no decimal places and a significant loss of precision compared to keeping the uint64_t.
In line with all the other changes in ROS 2 using standard library methods where appropriate, this should have been changed to astd::chrono::durationanyway, which would also make the unit explicit.
Proposed solution
Change the update method signature to a chrono duration.
This will require changing existing displays, but the alternative solution to create a second update method with the duration and keeping this method would be unclean and might still cause issues for anyone porting rviz1 displays that use the dt.
This way, this error would move from a semantic hard-to-spot error to a compilation error, which is easy to find and quick to fix.
And neither solution is ABI compatible, so they can only be made for the upcoming distros anyway.
For existing distributions at least the documentation should be changed.