Repository navigation
Conversation
|
run-ci |
|
There was a problem hiding this comment.
Isaac Lab Review Bot
The PR adds the flat Franka configurations, backend-aware rigid-body resolution, and non-contiguous Newton shape selection, but several issues need correction before merge: clone-plan direct matching uses the wrong namespace, one deprecated Franka config no longer preserves its prior asset path, the deprecation bridge lacks the repository’s warning/removal form, source no-match behavior regresses, and there are smaller style and test-naming inconsistencies.
- Design and architecture: The opt-in flat Franka split and use of
root_view.body_shapesare sound design choices. However,prefer_direct_matchescompares source prototype paths with destination-space expressions under clone plans, defeating the direct-body preference for prototypes outside the environment namespace. The compatibility helpers also interrupt the module’s constants-first structure by appearing before the remaining public Franka configuration. - API: The new flat and legacy Franka symbols are exported through both lazy-export stubs, and
prefer_direct_matches=Falsepreserves resolver behavior for unchanged callers. The new option is not correct for all clone-plan producer paths, and frame-transformer source resolution now lets the resolver’s singleton count check replace the existing descriptive no-matchValueErrorwith a genericRuntimeError. - Compatibility and deprecation: Breaking changes:
FRANKA_PANDA_MENAGERIE_CFGpreviously usedfranka_panda.usdabut now resolves tofranka_panda_nestedInstance.usda, despite documentation claiming its previous asset contract remains available. The deprecated Franka names emitFutureWarningwithout a removal release rather than the establishedDeprecationWarningform, leaving the deprecation transition incomplete. Frame-transformer source paths with no rigid-body match also change exception type and message becauseexpected_num_matchesis checked beforeraise_if_no_matches=Falsecan take effect. - Implementation: The Newton material-randomization change correctly selects each requested body’s actual shape indices without assuming contiguous shape storage. The direct-match resolver must instead track the prims that directly matched the destination expression before predicate-based descendant expansion; comparing prototype prim paths to
path_exprcannot work reliably under clone plans. Frame-transformer singleton validation should separately preserve the intended no-match error path. - Style consistency: Move
__getattr__,__dir__, and the private compatibility table below all public Franka configuration constants to restore the documented constants-before-functions layout. The newly added Newton test setup should also use the already importedclone(...)helper instead of new.copy()calls, matching the current configuration-operation guidance. - Test quality: The resolver, legacy-config, mimic-constraint, and PhysX/OVPhysX nested-body tests protect distinct changed contracts, and the backend-specific frame-transformer cases are justified because both implementations changed. Rename
test_franka_newton_mimic_constraint_tracks_passive_finger: it runs the OVPhysX fixture with the PhysX asset variant and does not enable Newton-native actuators, so its current name promises coverage it does not provide.
Significant concerns. Posted 7 actionable findings inline.
Automated review; human maintainers own approval decisions.
|
run-ci |
|
run-ci |
|
run-ci |
|
Removed prefer_direct_matches in 6a36528. The distinction is source versus target cardinality, not first versus all matches: the sensor requires one source body per environment and keeps every matched target. It now filters the declared path matches directly, without falling back to descendants. The shared resolver is unchanged from develop. The existing nested-body and regex-target tests pass in both backends, along with all 10 cloner tests. Restoring descendant expansion fails with 11 source bodies; keeping only the first match fails the all-targets test. No new tests; net -27 lines (-17 production). |
|
run-ci |
|
run-ci |
|
run-ci |
|
Backported to |
## Description Make the shared flat Franka asset the main `FRANKA_PANDA_CFG` and `FRANKA_PANDA_HIGH_PD_CFG`. The main configs use gripper-only collisions, Panda effort limits (87/12 Nm), a single `panda_arm` group, and a driven leader finger with a passive mimic follower. The high-PD config has stiffer arm gains and gravity disabled. Keep the original asset settings under `FRANKA_PANDA_LEGACY_CFG` and `FRANKA_PANDA_LEGACY_HIGH_PD_CFG`. Keep the Menagerie and Robotiq configs unchanged. Remove the deprecation lookup, warnings, and `FLAT` names. Existing tasks and controller examples that need the original settings explicitly select the legacy configs. **Breaking change:** code importing the original main config names now receives the flat asset and its actuator contract. To retain the old USD path, shoulder/forearm groups, gains, and collision settings, import the corresponding `LEGACY` config. Select the backend's `Physics` variant when using the main configs. The asset-package changelog includes migration guidance and a major-version marker. Frame transformers select rigid bodies matched by the declared path expression, without implicitly searching descendants. A source must match exactly one body per environment; target expressions retain all matching bodies. The shared resolver API stays unchanged. Preserve descriptive frame-transformer errors. Make Franka Pour compatible with the synced public asset: select the MuJoCo physics payload, use the main config, and remove the obsolete duplicate-mimic workaround. The development asset is already synced. The remaining task migrations are in #8170 and #8171; those branches need refreshing to use the new main names. ## Validation - Compared all six Franka configs with the previous commit: their settings are unchanged under the new names; verified exports from the asset package and robot package. - Existing Pour and Reach configuration tests: 39 passed. - Existing Franka Pour startup integration: passed against the public asset. - Existing OvPhysX joint-command test: passed, including passive-finger tracking. - Frame-transformer checks: 4 passed across PhysX and OvPhysX, covering nested Franka bodies and the existing all-targets case. The compact Franka regression fails if descendant expansion is restored or only the first target is returned. - Existing cloner suite: 10 passed; no generic resolver test additions. - Newton viscous-friction checks: 2 passed (index and mask). - Removing mimic coupling made the retained tracking assertion fail. - Formatting, lint, changelog validation, and `git diff --check` passed. ## Release backport - [x] <!-- backport-active-release --> Backport this pull request to the active release branch after it merges into `develop` --------- Co-authored-by: Mustafa Haiderbhai <mhaiderbhai@nvidia.com> Co-authored-by: Mustafa H <34825877+StafaH@users.noreply.github.com> Co-authored-by: Octi Zhang <zhengyuz@nvidia.com> (cherry picked from commit 02b779f)
Description
Make the shared flat Franka asset the main
FRANKA_PANDA_CFGandFRANKA_PANDA_HIGH_PD_CFG. The main configs use gripper-only collisions, Panda effort limits (87/12 Nm), a singlepanda_armgroup, and a driven leader finger with a passive mimic follower. The high-PD config has stiffer arm gains and gravity disabled.Keep the original asset settings under
FRANKA_PANDA_LEGACY_CFGandFRANKA_PANDA_LEGACY_HIGH_PD_CFG. Keep the Menagerie and Robotiq configs unchanged. Remove the deprecation lookup, warnings, andFLATnames. Existing tasks and controller examples that need the original settings explicitly select the legacy configs.Breaking change: code importing the original main config names now receives the flat asset and its actuator contract. To retain the old USD path, shoulder/forearm groups, gains, and collision settings, import the corresponding
LEGACYconfig. Select the backend'sPhysicsvariant when using the main configs. The asset-package changelog includes migration guidance and a major-version marker.Frame transformers select rigid bodies matched by the declared path expression, without implicitly searching descendants. A source must match exactly one body per environment; target expressions retain all matching bodies. The shared resolver API stays unchanged. Preserve descriptive frame-transformer errors.
Make Franka Pour compatible with the synced public asset: select the MuJoCo physics payload, use the main config, and remove the obsolete duplicate-mimic workaround. The development asset is already synced. The remaining task migrations are in #8170 and #8171; those branches need refreshing to use the new main names.
Validation
git diff --checkpassed.Release backport
develop