Skip to content

examples(mcap_record_replay): package under isaacteleop_examples - #1038

Merged
jiwenc-nv merged 8 commits into
mainfrom
jiwenc-nv/examples-mcap-record-replay
Sep 18, 2026
Merged

jiwenc-nv merged 8 commits into
mainfrom
jiwenc-nv/examples-mcap-record-replay

Conversation

@jiwenc-nv

@jiwenc-nv jiwenc-nv commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Part of #985. Stacked on #1037.

Nine modules imported common as a top-level module, which resolved only because the invoked script's own directory landed on sys.path. They broke under python -m, broke when copied into another project, and claimed the very generic bare name common. This moves the tree to the layout in examples/README.md.

uv pip install -e ./examples/mcap_record_replay
python -m isaacteleop_examples.mcap_record_replay.record_hand
python -m isaacteleop_examples.mcap_record_replay.replay_hand

Twelve co-equal scripts, so no __main__.py; the new README maps each channel to its live/record/replay module.

One behaviour change, forced by the move. Recordings defaulted to Path(__file__).resolve().parent.parent / "recordings", which resolved to the example directory. Three levels deeper that expression points inside the package, and for a pip-installed copy it would write into site-packages. Recordings now default to ./recordings/ relative to the working directory, and the replay scripts look in the same place. The old path only ever made sense from a source checkout.

References updated beyond docs/: rigs/full_body.yaml, src/plugins/noitom_mocap/README.md, src/plugins/vive_se3_tracker/README.md.

A second, deliberate behaviour change: all seven viser viewers bind 0.0.0.0 rather than 127.0.0.1, for the same reason — these run on a robot or workstation and get opened from a laptop. --host 127.0.0.1 restores the old behaviour. Each startup line now reports the bind address instead of always printing localhost.

The viser grid was a wall, not a floor. add_grid defaults to plane="xy", but the scene sets set_up_direction("+y") — so the grid stood vertical in every viewer. It is now an xz ground plane, 6 m with 0.25 m cells, and the camera starts centred on the origin at eye height so a tracked person fills the view on connect. That setup is a single setup_scene() helper in common.py rather than seven copies.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Testing

x86_64 / Ubuntu, Python 3.12. Wheel ships isaacteleop_examples/mcap_record_replay/ with no namespace __init__.py. uv pip install -e ./examples/mcap_record_replay into a clean venv, then all 12 modules import with the CWD outside the repo; import common fails, so the flat name is gone rather than relocated. pre-commit clean.

Not covered: recording needs a live OpenXR runtime. Replay against an existing .mcap is the cheapest check that the new ./recordings/ default behaves.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the linter and formatter with SKIP=check-copyright-year pre-commit run --all-files
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix/feature works (or explained why not)
  • I have signed off all my commits (git commit -s) per the DCO

No tests: this is a move, and this example has no automated coverage.

Summary by CodeRabbit

  • Documentation

    • Updated MCAP recording and replay instructions to use the relocated Python examples and module-based commands.
    • Added setup, usage, networking, supported-channel, runtime, and file-location guidance.
    • Updated tracker, plugin, and headless replay examples with the current commands.
  • New Features

    • Added automatic ground-grid and camera positioning for visualization scenes.
    • Recording and replay tools now default to ./recordings/ in the current working directory.
    • Improved package installation support for editable local development.

@coderabbitai

coderabbitai Bot commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

The saved review history does not include the base for the last reviewed commit. This saved history cannot establish the base for an incremental review. Comment @coderabbitai full review to establish a new review baseline. No full review was started, and the last reviewed checkpoint was preserved.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The MCAP record/replay example now installs as a Hatchling package and runs through Python module commands. Recording and replay default to the current working directory’s ./recordings/ directory. Shared Viser scene setup adds a ground grid that follows tracked body height and updates full-body visualization. Related device, plugin, rig, README, and reference documentation now uses the relocated package workflow.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PythonModule
  participant setup_scene
  participant GroundGrid
  participant FullBodyViz
  PythonModule->>setup_scene: initialize the Viser scene
  setup_scene->>GroundGrid: create the ground-grid controller
  PythonModule->>FullBodyViz: pass the GroundGrid
  FullBodyViz->>GroundGrid: forward body positions and validity
  GroundGrid->>GroundGrid: smooth floor height and reframe cameras
Loading

Merge Risk: 🟡 Moderate · up to a735d

The examples can unintentionally expose tracking data, stage generated recordings, select incompatible replay files, and direct users to failing commands. These issues should be corrected before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 13 files. (8 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: packaging the mcap_record_replay example under isaacteleop_examples.
Full details: Docstring Coverage

Explanation

Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 13 files. (8 skipped: 8 unsupported.)

✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jiwenc-nv/examples-mcap-record-replay

Comment @coderabbitai help to get the list of available commands.

@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from a205995 to d1bd5c8 Compare August 28, 2026 05:00
@jiwenc-nv
jiwenc-nv changed the base branch from jiwenc-nv/examples-deviceio-live-view to jiwenc-nv/examples-retargeting August 28, 2026 05:01
@jiwenc-nv
jiwenc-nv requested a review from ivany-nv August 28, 2026 05:11
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from d1bd5c8 to 560ec3b Compare August 28, 2026 05:22
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from 560ec3b to b136d28 Compare August 28, 2026 05:32
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from b136d28 to 601070f Compare August 28, 2026 05:42
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from 601070f to 6ecf63e Compare August 28, 2026 05:48
@ivany-nv

Copy link
Copy Markdown
Contributor

Two things riding along with the move.

The output directory is no longer gitignored. Path(__file__).parent.parent / "recordings" -> Path.cwd() / "recordings" is the right call for an installed copy, but the ignore rule is scoped to the example directory:

$ git check-ignore -v examples/mcap_record_replay/recordings/x.mcap
examples/mcap_record_replay/.gitignore:1:recordings   examples/mcap_record_replay/recordings/x.mcap
$ git check-ignore -v recordings/x.mcap
(not ignored)

The README tells you to run from the repo root (uv pip install -e ./examples/mcap_record_replay, then python -m ...), so following it drops untracked .mcap files at the root. Needs a /recordings/ entry in the root .gitignore. Same issue in #1043 with local_datasets/.

Seven more --host default flips. Same change I flagged on #1034; that makes eight files carrying default="0.0.0.0", # noqa: S104, and they are the only S104 suppressions in the repo. A behaviour change this broad reads better as its own PR than as a rider on eight packaging commits -- --host 0.0.0.0 already exists for the workflow it's meant to serve.

@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from 6ecf63e to 77e8b36 Compare August 28, 2026 16:10
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from 77e8b36 to e7bf116 Compare August 28, 2026 23:49
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from e7bf116 to 171b99c Compare August 29, 2026 00:21
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from 171b99c to ce025d1 Compare August 29, 2026 01:19
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from ce025d1 to 3b96b07 Compare August 30, 2026 16:23
jiwenc-nv added a commit that referenced this pull request Sep 12, 2026
common.py's GroundGrid (mcap_record_replay) and deviceio_viser.py's
(deviceio_live_view, landed on main via #1034) are byte-identical. Not
deduping: each example package is self-contained by design (see
examples/README.md), and a shared helper would make one example
depend on another. Leave a pointer in each copy instead of letting the
duplication look accidental.

Flagged by ivany-nv's verification pass on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
@ivany-nv

ivany-nv commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Verified this at 5faf201 with a clean build and install tree (not the source tree), plus a headless replay run driven by fixtures from teleop_ros2_mcap_generator. Install layout, generated pyproject, python -m from the install tree, legacy/converted split, clang-format and pre-commit all pass. Two behaviour fixes confirmed independently: the discovery filter picks hands_*.mcap over a newer controllers_*.mcap (before 0981f53 it picked the controller recording), and /recordings/ is anchored to the repo root without over-ignoring nested paths.

User-facing text defects, both in files this PR already touches:

** docs/source/references/mcap_record_replay.rst:177 and :192 say "From the example directory:" and then give a repo-root-relative path.**

$ cd examples/mcap_record_replay && uv pip install -e ./examples/mcap_record_replay
error: Distribution not found at: file://<repo>/examples/mcap_record_replay/examples/mcap_record_replay

$ cd <repo> && uv pip install -e ./examples/mcap_record_replay
Resolved 27 packages

Sphinx does not catch this and it ships in the rendered HTML. It also contradicts 887db46's own rationale ("its README has you run from the repo root") — if the rst heading is right, that .gitignore rule is unnecessary.

Unrelated to this PR, for whoever picks it up: #1039–#1047 are still stacked on the pre-rewrite 091b91c, so the /recordings/ fix has not propagated — #1041 still writes Path.cwd() / "recordings" with no root ignore rule.

jiwenc-nv added a commit that referenced this pull request Sep 14, 2026
'Live preview' and 'Recording' both said 'From the example directory:'
but gave uv pip install -e ./examples/mcap_record_replay, which is
repo-root-relative -- run from the example directory it resolves to a
nonexistent nested path. Say what the commands actually assume; it
also matches 887db46's rationale for the root .gitignore rule.

Also drops a stale .py suffix on a replay_full_body module reference
a few lines above.

Flagged by ivany-nv on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
@jiwenc-nv

Copy link
Copy Markdown
Collaborator Author

Verified this at 5faf201 with a clean build and install tree (not the source tree), plus a headless replay run driven by fixtures from teleop_ros2_mcap_generator. Install layout, generated pyproject, python -m from the install tree, legacy/converted split, clang-format and pre-commit all pass. Two behaviour fixes confirmed independently: the discovery filter picks hands_*.mcap over a newer controllers_*.mcap (before 0981f53 it picked the controller recording), and /recordings/ is anchored to the repo root without over-ignoring nested paths.

User-facing text defects, both in files this PR already touches:

** docs/source/references/mcap_record_replay.rst:177 and :192 say "From the example directory:" and then give a repo-root-relative path.**

$ cd examples/mcap_record_replay && uv pip install -e ./examples/mcap_record_replay
error: Distribution not found at: file://<repo>/examples/mcap_record_replay/examples/mcap_record_replay

$ cd <repo> && uv pip install -e ./examples/mcap_record_replay
Resolved 27 packages

Sphinx does not catch this and it ships in the rendered HTML. It also contradicts 887db46's own rationale ("its README has you run from the repo root") — if the rst heading is right, that .gitignore rule is unnecessary.

Unrelated to this PR, for whoever picks it up: #1039–#1047 are still stacked on the pre-rewrite 091b91c, so the /recordings/ fix has not propagated — #1041 still writes Path.cwd() / "recordings" with no root ignore rule.

good catch. fixed.

jiwenc-nv added a commit that referenced this pull request Sep 14, 2026
…y default

Split out per review on #1038: these run on a robot or workstation and
get opened from a laptop, same rationale as #1034's viser default flip.
127.0.0.1 stays available via --host.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
jiwenc-nv added a commit that referenced this pull request Sep 14, 2026
…ng type

resolve_mcap() picked the newest .mcap in the shared ./recordings/
directory regardless of source. If a replay's own recorder hasn't
produced a file yet, a newer recording from a different recorder can
be selected and fail with a missing-channel error. Prefer a
type-prefixed match, falling back to the newest .mcap of any kind --
matching replay_se3_vive.py's existing pattern.

Flagged by CodeRabbit on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from 84ddcd1 to d9ee810 Compare September 14, 2026 23:36
jiwenc-nv added a commit that referenced this pull request Sep 14, 2026
…po root

The recorders default to Path.cwd() / "recordings", and the README has
you run from the repo root, so the existing
examples/mcap_record_replay/.gitignore rule (scoped to that directory)
never covers the output. Add a root-level /recordings/ rule.

Flagged in review on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
jiwenc-nv added a commit that referenced this pull request Sep 14, 2026
…invocation

Usage lines, --help text (argparse reads it from the module docstring),
and error messages still told users to run these as scripts
(python replay_full_body.py ...) after the move to package/module
layout. Switch them to python -m
isaacteleop_examples.mcap_record_replay.<name>, matching the README
and rst; fix the same in the rst's Replaying section, which was the
one block left in script form.

Also updates the 'newest file' wording (docstrings, README, rst) for
the type-prefixed auto-discovery from the prior commit, warns instead
of silently degrading when resolve_mcap falls back to a wrong-type
recording, and documents the --python 3.11 requirement for uv run.

Flagged by ivany-nv's verification pass on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
jiwenc-nv added a commit that referenced this pull request Sep 14, 2026
common.py's GroundGrid (mcap_record_replay) and deviceio_viser.py's
(deviceio_live_view, landed on main via #1034) are byte-identical. Not
deduping: each example package is self-contained by design (see
examples/README.md), and a shared helper would make one example
depend on another. Leave a pointer in each copy instead of letting the
duplication look accidental.

Flagged by ivany-nv's verification pass on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
jiwenc-nv added a commit that referenced this pull request Sep 14, 2026
'Live preview' and 'Recording' both said 'From the example directory:'
but gave uv pip install -e ./examples/mcap_record_replay, which is
repo-root-relative -- run from the example directory it resolves to a
nonexistent nested path. Say what the commands actually assume; it
also matches 887db46's rationale for the root .gitignore rule.

Also drops a stale .py suffix on a replay_full_body module reference
a few lines above.

Flagged by ivany-nv on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
jiwenc-nv added a commit that referenced this pull request Sep 18, 2026
…_mcap

The fallback to the newest .mcap of any kind (added to fix picking the
wrong type outright) traded a hard failure for a silent one: a missed
log line still means the wrong recording plays. Fail instead -- point
at the recorder or an explicit path.

Flagged by aristarkhovNV on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
Nine modules imported `common` as a top-level module, which resolved only
because the invoked script's own directory landed on sys.path -- so they broke
under `python -m`, broke when copied into another project, and claimed the very
generic bare name `common`. Move the tree to the layout in examples/README.md.
Twelve co-equal scripts, so no __main__.py; the README maps channel to
live/record/replay.

Behaviour change forced by the move: recordings default to ./recordings/
relative to the working directory rather than a path derived from __file__,
which after the move points inside the package and for an installed copy would
have written into site-packages. Replay searches the same directory.

Three deliberate viewer fixes. The seven viser viewers bind 0.0.0.0 rather than
127.0.0.1, since they run where the hardware is and get opened from a laptop;
--host 127.0.0.1 restores it. Each startup line reports the bind instead of
always printing "localhost". And the grid lay in viser's default XY plane,
which stands up as a wall once the up direction is +y: it is now an xz ground
plane with the camera centred on it, as one helper in common.py rather than
seven copies.

Part of #985.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
…y default

Split out per review on #1038: these run on a robot or workstation and
get opened from a laptop, same rationale as #1034's viser default flip.
127.0.0.1 stays available via --host.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
…ng type

resolve_mcap() picked the newest .mcap in the shared ./recordings/
directory regardless of source. If a replay's own recorder hasn't
produced a file yet, a newer recording from a different recorder can
be selected and fail with a missing-channel error. Prefer a
type-prefixed match, falling back to the newest .mcap of any kind --
matching replay_se3_vive.py's existing pattern.

Flagged by CodeRabbit on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
…po root

The recorders default to Path.cwd() / "recordings", and the README has
you run from the repo root, so the existing
examples/mcap_record_replay/.gitignore rule (scoped to that directory)
never covers the output. Add a root-level /recordings/ rule.

Flagged in review on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
…invocation

Usage lines, --help text (argparse reads it from the module docstring),
and error messages still told users to run these as scripts
(python replay_full_body.py ...) after the move to package/module
layout. Switch them to python -m
isaacteleop_examples.mcap_record_replay.<name>, matching the README
and rst; fix the same in the rst's Replaying section, which was the
one block left in script form.

Also updates the 'newest file' wording (docstrings, README, rst) for
the type-prefixed auto-discovery from the prior commit, warns instead
of silently degrading when resolve_mcap falls back to a wrong-type
recording, and documents the --python 3.11 requirement for uv run.

Flagged by ivany-nv's verification pass on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
common.py's GroundGrid (mcap_record_replay) and deviceio_viser.py's
(deviceio_live_view, landed on main via #1034) are byte-identical. Not
deduping: each example package is self-contained by design (see
examples/README.md), and a shared helper would make one example
depend on another. Leave a pointer in each copy instead of letting the
duplication look accidental.

Flagged by ivany-nv's verification pass on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
'Live preview' and 'Recording' both said 'From the example directory:'
but gave uv pip install -e ./examples/mcap_record_replay, which is
repo-root-relative -- run from the example directory it resolves to a
nonexistent nested path. Say what the commands actually assume; it
also matches 887db46's rationale for the root .gitignore rule.

Also drops a stale .py suffix on a replay_full_body module reference
a few lines above.

Flagged by ivany-nv on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
…_mcap

The fallback to the newest .mcap of any kind (added to fix picking the
wrong type outright) traded a hard failure for a silent one: a missed
log line still means the wrong recording plays. Fail instead -- point
at the recorder or an explicit path.

Flagged by aristarkhovNV on #1038.

Signed-off-by: Jiwen Cai <jiwenc@nvidia.com>
@jiwenc-nv
jiwenc-nv force-pushed the jiwenc-nv/examples-mcap-record-replay branch from 8dc80b6 to 857aa2d Compare September 18, 2026 15:28

This branch was successfully deployed

1 active deployment
dev — 857aa2db Deployed Sep 18, 2026 by jiwenc-nv via publish-wheel #4840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants