Repository navigation
Conversation
There was a problem hiding this comment.
go2 replay realtime (arm64)
Details
| Benchmark suite | Current: aebb801 | Previous: c240b54 | Ratio |
|---|---|---|---|
peak memory |
1576.324 MB |
1827.461 MB |
0.86 |
peak threads |
361 threads |
364 threads |
0.99 |
network (transport) |
2727.33 MB |
2726.743 MB |
1.00 |
disk write |
1.828 MB |
1.809 MB |
1.01 |
instructions |
116.977 G |
136.831 G |
0.85 |
This comment was automatically generated by workflow using github-action-benchmark.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
bd424bb to
309e817
Compare
[Medium impact] The PR changes generated-message viewing across the browser and Rerun workflows. Fix source-timeline transform recording before merging; the external-schema compatibility issue is non-blocking. FindingsSummaryAdds generated CDR messages to Rerun and the browser SDK while keeping legacy rendering.
Both reported failures were reproduced. Neither is resolved by the supplied higher dependent PR revisions. Reviews (1) · Last reviewed commit: "fix(viewers): use globalThis for browser..." · Reviewed by Greptile |
| stamp = getattr(getattr(msg, "header", None), "stamp", None) | ||
| if stamp is not None: | ||
| nanos = int(stamp.sec) * 1_000_000_000 + int(stamp.nanosec) | ||
| rr.set_time("source", timestamp=np.datetime64(nanos, "ns")) |
There was a problem hiding this comment.
Transforms miss source playback
Generated TFMessage timestamps belong to individual transforms, but this callback sets source only from a top-level header. The resulting transforms are recorded on log_time and log_tick, not source, while stamped images and poses are recorded on source. During source playback, Rerun cannot retrieve the transform edges needed to place that sensor data correctly.
A real bridge recording confirmed that all three transform updates were unavailable at their source timestamps, including latest-at queries, while the corresponding images and poses remained available. Set source from each transform’s timestamp when logging it, then restore or clear the timeline state. This playback failure should be fixed before merging.
Knowledge Base Used: Rerun visualization bridge
Artifacts
Executed generated-message bridge reproduction
- The executed script sends generated TF, pose, and image updates through the production bridge and records current behavior plus an external timing control, without modifying production.
Executed source-timeline query and higher-snapshot checks
- The executed inspection script samples actual TF availability with latest-at filling, checks static and log-time controls, and compares scoped higher-PR functions, confirming the failure and absence of a supplied fix.
Executed inspection command wrapper
- The executed wrapper records the exact inspection command, working directory, exit code, and captured output, making the successful verification reproducible.
Observed TF queries and higher-PR comparison output
- The captured successful run shows null source-timeline TF values before, correct values with the timing control, working static/log-time controls, and identical relevant functions in all seven higher heads.
Real Rerun recording from current PR4733 bridge
- Actual generated messages were logged through the current bridge into this recording, whose TF values are unavailable on the source timeline.
Real Rerun recording with external TF timing control
- The same generated message sequence was recorded with source externally assigned around each TF update, making all three TF translations available at their source timestamps.
| const definitions = parse(schema.definition, { ros2: true }); | ||
| const reader = new MessageReader(definitions); |
There was a problem hiding this comment.
External schemas lose package context
cdrDecoder ignores schema.type when parsing the complete definition. Generated schemas leave the root unnamed, so the parser cannot use its package to resolve short field names. A valid external message containing a local Point and a nested geometry_msgs/Point throws Cannot unambiguously determine fully-qualified type name for 'Point'; the decoder factory then returns null, and the registry marks the channel unsupported.
This is a non-blocking compatibility gap for external packages with overlapping short type names. Supply the root identity from schema.type before type resolution and add a regression test covering two packages with a Point type.
Artifacts
Decoder and registry reject a valid complete schema
- Executed the unchanged SDK with the generated schema and binary controls, capturing the exact ambiguity error, null factory result, and undefined registry result.
Decoder and registry succeed with root identity supplied
- Executed the same fixture with an isolated in-memory root-identity change, showing successful decoding without editing production files.
Full reproduction source and generated binary fixture
- Captured the complete authored scripts, fixture, registry source, and generator output so the executed reproduction can be inspected and repeated.
Higher-stack source comparisons and exact source citations
- Compared all supplied heads and printed numbered resolution helpers, showing identical decoder, registry, and exporter blobs across the stack.
What this adds
How to test
From this PR's checkout:
Use a new output name. The demo runs the real bridge and creates a live RRD plus a source-scene MCAP. Check TF, geometry, images, depth, calibration, clouds and legacy rendering. Native MCAP import visualizes supported standard types; Path and custom EntityMarkers remain reflected fields.
Scene checklist and native MCAP export.
For the browser, run these in separate terminals:
cd web/sdk deno task fixture --host 127.0.0.1Open http://127.0.0.1:5174/demo_cdr.html?live=1. Expect
connected, all eight channels, increasing counts and zero decode errors. Browser checklist.Validation includes 38 focused tests and the installed Rerun example with native MCAP conversion. This layer leaves
dimos mem rerun, navigation anddimos.msgsunchanged.Shared CDR foundation
Standard ROS definitions are fetched and verified through the existing source lock and generation pipeline in #4366. Native SDK support is included in #4722. Shared Python value implementations and regressions are reviewed in #4800–#4803 before the #4734 caller migration. SDK and consumers use the same generated Rust message crate, including
Time,TransformStampedandTFMessage; there is no separate TF schema package or generation command. Camera/depth callers migrate in #4734 and mapping/navigation callers in #4737.The external CDR and transforms library pins remain unchanged. Local verification passed 122 SDK tests, all existing native consumer suites, and all 24 real native/DimSim/port acceptance tests without skips. Seven staged Nix source layouts resolve exactly one message crate with locked Cargo metadata. This is not a full Nix build or hardware validation. Fresh CI runs verify #4722 independently and the combined stack tip.
Stack #4804 contains the 12 original feature PRs and four separately reviewed adapter/buffer PRs (#4800–#4803). The redundant #4774 is superseded by #4722. No feature PR has been merged into main; existing conflicts with main and the WIP cutover failures in #4739 remain.