Allow shadow visualizers to load reversed-joint assets - #7597
Conversation
github-actions
Bot
added
bug
Greptile SummaryThis PR makes render-only Newton shadow models tolerate legacy reversed USD joints by excluding constraints and collision-mesh approximation during import, and adds clone-source regression coverage.
Confidence Score: 4/5The startup fix appears mergeable, but it introduces a non-blocking regression in joint-debug visualization for all PhysX-backed shadow models. Reversed-joint clone imports are concretely covered and rigid-body synchronization remains body-path based, but globally ignoring every joint leaves the visualizer’s joint display without topology even for normally supported articulations. Files Needing Attention: source/isaaclab_newton/isaaclab_newton/physics/visualization_builder.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[PhysX USD stage] --> B[Shadow Newton import]
B --> C[Bodies and visual shapes]
B -. all joints ignored .-> D[No shadow constraints]
C --> E[SceneData pose synchronization]
E --> F[Newton / Rerun / Viser consumers]
D --> G[Reversed joints no longer block startup]
D --> H[Joint debug visualization unavailable]
Reviews (1): Last reviewed commit: "Allow shadow visualizers to load reverse..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| state, so the shadow model needs bodies and shapes but not their constraints. | ||
| Omitting joints also lets renderers display legacy assets whose authored joint | ||
| direction is unsupported by Newton's simulation importer. | ||
| """ |
There was a problem hiding this comment.
This collects every USD physics joint and excludes all of them from each PhysX-backed shadow import, rather than excluding only unsupported reversed joints. As a result, normal articulations no longer provide joint topology to the Newton visualizer, so its “Show Joints” control cannot display joints that were previously available. This is a non-blocking regression that makes joint debugging less useful.
Knowledge Base Used:
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Addressed in 71b68ef. The shadow import now detects and omits only reversed tree joints (plus dependent loop constraints in the affected graph). Supported joints remain in the Newton model, and the regression test now asserts that a supported joint is retained while the reversed joint is absent.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The visualization-only import consistently omits joint constraints and skips mesh approximation while preserving the authoritative simulation import path. One documentation inconsistency should be corrected before merge.
- Design and architecture: The change keeps joint omission confined to the render-only shadow-model path, where rigid-body poses are supplied by the active backend and constraints are not required. The simulation import behavior remains unchanged.
- API: The new keyword-only
skip_mesh_approximationparameter has a behavior-preserving default, so existing callers remain compatible. However,build_source_buildersstill unconditionally documents that authoredphysics:approximationvalues are applied, which is false when the new option is enabled; the docstring should describe that override. - Implementation: The joint ignore expressions and
skip_mesh_approximation=Truesetting are applied across standalone, world, and clone-source visualization imports. Regression coverage exercises a reversed-joint clone source, and the package changelog fragment is present.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| load_visual_shapes: Whether to import visual-only geometry. Importing it costs | ||
| USD parse time and memory that only pays off when the shapes are rendered | ||
| or ray cast. | ||
| skip_mesh_approximation: Whether to skip collision mesh approximation during import. |
There was a problem hiding this comment.
🔵 Suggestion · Api — Docstring contradicts new skip_mesh_approximation option
The docstring still states that "the cloner approximates nothing" and that Newton's importer applies each shape's physics:approximation, directing callers to change it where it is authored. With skip_mesh_approximation=True (now passed by every visualization import) the authored token is bypassed, so the documented contract no longer holds for this public function. Update the opening paragraph to note that the new flag overrides authored approximation.
There was a problem hiding this comment.
Addressed in 71b68ef. The opening paragraph now documents the default authored-approximation behavior and explicitly notes that render-only callers can override it with skip_mesh_approximation.
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
| Omitting joints also lets renderers display legacy assets whose authored joint | ||
| direction is unsupported by Newton's simulation importer. | ||
| """ | ||
| return [f"^{re.escape(str(prim.GetPath()))}$" for prim in stage.Traverse() if prim.IsA(UsdPhysics.Joint)] |
There was a problem hiding this comment.
[P1] Agent review: this emits one pattern per joint prim across the whole stage, so the list is num_envs × joints_per_robot. Newton scans it with any(re.match(...)), and Python's re cache holds 512 compiled patterns — past that, every call recompiles.
Measured per-pattern scan cost:
| Patterns | Per pattern |
|---|---|
| 512 | 0.64 µs |
| 600 | 15.08 µs |
A ~70× jump at the cache boundary. The threshold is num_envs × joints > 512 — about 43 envs for a 12-joint arm and 17 for a 30-joint humanoid, which is inside normal usage.
Suggested: collapse to one pattern per env-relative joint path, since every env is a clone.
patterns = {
"^" + re.sub(r"/World/envs/env_\d+", "/World/envs/env_[^/]*",
re.escape(str(prim.GetPath()))) + "$"
for prim in stage.Traverse()
if prim.IsA(UsdPhysics.Joint)
}
return sorted(patterns)Measured on a 3-env × 2-joint stage: 6 patterns → 2, with an identical model (bodies=9, joints=9, all FREE). [^/]* rather than .*, which matches / and would over-match across path separators.
One trap worth recording for whoever picks this up: replacing the anchors with plain SdfPaths silences the Ill-formed SdfPath warnings but changes the model — bodies end up with no joints, plus Invalid ArticulationDesc warnings at scale. The anchored form is load-bearing.
There was a problem hiding this comment.
Addressed in 71b68ef. Detection is scoped to the roots actually imported, and every incompatible joint path is combined into one anchored alternation rather than emitted as a separate regex. This keeps the anchored exact-match behavior while avoiding the 512-pattern cache cliff. Supported joints are preserved, and bodies_follow_joint_ordering=False retains bodies disconnected by the omitted constraints.
|
run-ci |
isaaclab-bot
Bot
added
ci:run-docker
2 participants
Description
PhysX-backed Newton visualizers build a shadow Newton model solely for rendering. Newton rejects legacy USD joints whose body relationships are authored in the reverse direction, so the Dofbot tutorial fails even though PhysX can simulate the asset.
This change detects and omits only constraints incompatible with the render-only shadow import, while retaining supported joints for debug visualization. Affected paths are combined into one anchored expression to avoid Python's regex-cache scaling cliff, and body ordering is decoupled from the remaining joint graph so every visual body is preserved. Collision-mesh approximation is also skipped for these shadow imports. Rigid-body poses remain synchronized from SceneData, and authoritative Newton simulation imports are unchanged.
Fixes NVBug 6708340
No new dependencies.
Type of change
Release backport
Validation
ValueError: Reversed joints are not supportedand passes with this change.add_new_robot.pyinvocation withkit,viser,rerun, andnewton_glreachedSetup complete; the infinite simulation loop was then stopped.uv run isaaclab -fpassed all hooks against upstream/develop.Screenshots
Not applicable; this fixes initialization behavior.
Checklist
uv run isaaclab -fisaaclab_newtonchangelog fragment