fix(sim): stamp camera output at render time and pace on the monotonic clock - #3690
Merged
Conversation
…c clock The camera publish loop stamped images, TF, and camera_info with time.time() sampled when the frame was *consumed*, while the frame itself carries the timestamp taken when it was *rendered* (MujocoEngine._sim_loop passes loop_start into _render_cameras). Everything published for one frame was therefore skewed later than the observation it describes by the render->publish latency, up to a full publish interval (~67ms at the default fps=15). That is the same order as the 0.1s TF lookup tolerance, so perception could match an image against a neighbouring TF sample and register object poses at the wrong depth. Stamp images, TF, and camera_info from frame.timestamp so all three agree on the instant the frame was rendered. Pacing now measures a monotonic loop_start rather than time.time() - ts. With ts sampled from the frame, wall-minus-stamp is no longer ~0, so reusing it would have driven sleep_time negative and dropped the loop to render rate instead of config.fps. Measuring the loop body directly keeps pacing correct whatever frame.timestamp means, including if the engine later moves to sim time. camera_info is published on its own subscription rather than per frame, so the latest frame stamp is tracked and reused, falling back to wall time only before the first frame has arrived. The get_color_camera_info/get_depth_camera_info RPCs stamp the same way; they were also on wall clock and would otherwise have disagreed with the published topic. Tests cover both halves: stamps equal frame.timestamp for images, TF, and camera_info, and pacing holds at config.fps for frame timestamps near zero and at 1e6, which fails if pacing is derived from the stamp.
mustafab0
requested review from
Dreamsorcerer,
leshy,
paul-nechifor and
spomichter
as code owners
August 26, 2026 03:00
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3690 +/- ##
==========================================
+ Coverage 77.79% 77.85% +0.06%
==========================================
Files 1289 1291 +2
Lines 123511 123747 +236
Branches 10823 10856 +33
==========================================
+ Hits 96091 96349 +258
+ Misses 24278 24247 -31
- Partials 3142 3151 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Contributor
Greptile SummaryThis update makes camera images, transforms, camera metadata, and camera-info responses use the rendered frame timestamp, while publication pacing uses a monotonic clock. Camera behavior was exercised across a reset. Although the duplicate-frame guard rejects decreasing timestamps in isolation, the MuJoCo renderer supplies increasing wall-clock render timestamps after reset, and the next frame continues to publish. No defects were found. Confidence Score: 5/5Safe to merge based on the exercised camera publication and reset behavior. Focused coverage verifies timestamp alignment, metadata behavior before and after rendering begins, and pacing independent of timestamp magnitude. An executed reset check also confirmed that the renderer's timestamp source remains increasing across a reset. Files Needing Attention: No files require follow-up.
 T-Rex LogsWhat T-Rex did
Reviews (1): Last reviewed commit: "fix(sim): stamp camera output at render ..." | Re-trigger Greptile |
github-actions
Bot
added
the
ready-to-merge
_camera_info_ts checked _latest_frame_ts for None and then read it again to return it. The attribute is written by the MujocoSimPublish thread and cleared by stop(), and the rx.interval subscription that drives _publish_camera_info is still live when stop() nulls it (super().stop() runs afterwards), so the second read could return None after the first saw a stamp, handing None to CameraInfo(ts=...) and to with_ts() on the camera_info RPCs. Read the attribute once into a local and branch on that. Each attribute load is atomic under the GIL, so a single read is sufficient and no lock is needed; the adjacent _publish_camera_info already uses this pattern for _camera_info_base. The regression test drives _latest_frame_ts with a PropertyMock that yields a stamp then None, so a check-then-use returns None and fails.
github-actions
Bot
added
ready-to-merge
Per review: _camera_info_base and _latest_frame_ts are touched by the MujocoSimPublish thread, the rx.interval that drives _publish_camera_info, the RPC thread, and stop(). Add a single threading.Lock covering both. This also fixes a pre-existing check-then-use that predates this branch: get_color_camera_info and get_depth_camera_info tested _camera_info_base for None and then read it again to call with_ts() on it, and _generate_pointcloud did the same before passing it to PointCloud2.from_rgbd. stop() clears that field, so either could raise AttributeError on None. Each site now snapshots under the lock and uses the local. The lock is held only across the field access. It is not reentrant and is never held while calling _camera_info_ts(), which takes it again, nor across the publish-thread join in stop(), which would deadlock until the 2s timeout. Smoke-checked on xarm-perception-sim: /color_image still publishes at 14.3Hz against config.fps=15 with 232/233 image stamps matching their camera TF stamp, so the added contention does not disturb the publish loop.
github-actions
Bot
removed
the
ready-to-merge
github-actions
Bot
added
the
ready-to-merge
paul-nechifor
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The camera publish loop stamped images, TF, and
camera_infowithtime.time()sampled when the frame was consumed, not with the timestamp the frame already carries from when it was rendered.Why it matters
Every message for a frame was skewed later than the observation it describes by the render-to-publish latency, measured live at 4.5-101.7 ms (mean 52.7 ms), which is the same order as the 0.1 s TF lookup tolerance and can register object poses against the wrong TF sample.