Point clouds are not cleared on time jump

[Kettenhoax]

I'm using galactic with the released binary.

When displaying a point cloud topic during a time jump, for example when looping a ROS bag, subsequent point clouds stack until the original time point is reached again.
The decay time is disabled, i.e. only one cloud should be displayed.

Looking at the point cloud Display implementation on the current master, I've verified it's sufficient to decay clouds with receive time in the future in
PointCloudCommon::cloudInfoIsDecayed. However, I'm not sure if that's desirable in all cases. Alternatively the Display could install a ROS clock jump handler that clears data on jump.

if (now < cloud_info->receive_time_) {
  return true;
}

I'm happy to provide a PR if a core team member could comment on the preferred solution. I've attached the test data publisher and Rviz config to obtain the situation in the GIF.

[gbalke]

Heya! I just got around to this and reproduced the issue locally with sim time:

rviz2 -d point_decay_time_jump.rviz --ros-args -p use_sim_time:=true

To note for later: The "Detected jump back in time" that's implemented in Rolling isn't in Foxy. Foxy never clears the TF buffer.

[WARN] [1633651498.409042849] [tf2_buffer]: Detected jump back in time. Clearing TF buffer.

My confusion is that, before the first TF buffer clear, the system operates as expected. Also hitting the "Reset" button seems to fix this state. It may be more reasonable to do a full reset on clock regression as we likely don't want any persistent state! I could be wrong here in the case of static objects that are loaded in at the beginning of a recording? @jacobperron any thoughts?

[jacobperron]

Is this an issue with the point cloud display specifically, or does it affect other displays?

[gbalke]

I just tested this with the LaserScan message and am observing identical results. This appears to affect multiple displays so it may be assumed that this is an issue with how displays generally handle negative time jumps. I think a reset may be reasonable in this scenario.

Here's @Kettenhoax's code with my modifications:
with_laserscan.zip

[gbalke]

@Kettenhoax if you still want to implement a fix, I think the best approach to ensure this captures all displays is to execute the same call as the reset button on detecting a time reset. Do you think that will work for you/can you think of an issue with why that wouldn't work for your use case?

[Kettenhoax]

Thanks for verifying with LaserScan!

I can't think of issues with the global reset approach off the top of my head. I'll look into it and provide a PR soon.

[jacobperron]

Fixed in #791 for Rolling.

Proposing to backport for Humble in #854.