[Newton] Delegate homogeneous world prefix generation to ModelBuilder.replicate() - #7453
[Newton] Delegate homogeneous world prefix generation to ModelBuilder.replicate()#7453camevor wants to merge 3 commits into
ModelBuilder.replicate()#7453Conversation
Rewriting every merged label after replication is a second full pass over what replication just produced, and it scales with environment count. `replicate()` now takes a per-world label prefix, so the prototype's labels are rebased once and each copy comes out named for the environment it lands in; `ClonePlan` records `env_template` to supply that boundary. A prototype the boundary cannot express keeps the old path, and the string custom attributes are still rewritten here.
Greptile SummaryThe PR propagates the clone plan’s environment template into backend replication and delegates homogeneous Newton entity naming to
Confidence Score: 5/5The PR appears safe to merge, with no actionable changed-code defect identified. The new template and label-prefix data are propagated consistently through clone planning, backend construction, site registration, replication, and fallback rewriting, while body bindings and custom-attribute rewriting remain intact. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[ClonePlan with env_template] --> B[Backend replication context]
B --> C[Build Newton source prototypes]
C --> D{Homogeneous and labels rebase cleanly?}
D -- Yes --> E[ModelBuilder.replicate with per-env prefixes]
D -- No --> F[Replicate then rewrite labels]
E --> G[Rewrite string custom attributes]
F --> G
B --> H[Register per-world sites with destination template]
H --> I[Environment-qualified site labels]
G --> J[Final Newton model]
I --> J
Reviews (1): Last reviewed commit: "Update newton pin" | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The replication-prefix optimization is well scoped, but two compatibility issues need correction: the fast path mutates retained prototype labels, and NewtonReplicateContext.__init__ silently rebinds existing positional arguments by inserting env_template before device.
- Design and architecture: Delegating homogeneous label prefixing to
ModelBuilder.replicate()with a guarded fallback is sound, but rebasing the source builder in place makes retained prototype state depend on the replication path. Rebase a copy or restore labels after replication. - API:
ClonePlan.env_templateis defaulted and propagated consistently to in-tree contexts. However, insertingenv_templatebefore existing positional parameters inNewtonReplicateContext.__init__breaks positional-call compatibility and the new argument is omitted from the docstring. Preserve the existing parameter order or make the new argument keyword-only. - Implementation: The batched and per-world site paths and three-value replication result are wired through their consumers and tests. The remaining implementation defect is that
_rebase_to_envpermanently changes builders later retained inNewtonManager._cl_protos, exposing environment-relative labels to single-model consumers.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| # env root, instead of rewriting every label in every world afterwards. | ||
| label_prefixes = None | ||
| prototype_env = clone_path.match(sources[0], env_template) if env_ids is not None else None | ||
| if prototype_env is not None and _rebase_to_env(source_builder, env_template.format(prototype_env.instance)): |
There was a problem hiding this comment.
🟡 Warning · Implementation — Rebase mutates retained prototype builders
_rebase_to_env rewrites source_builder labels in place, and that same object is returned in source_builders, stored as NewtonManager._cl_protos, and handed out by copy_newton_clone_source for single-model consumers. On the delegated path the retained prototype now carries env-relative labels (Robot/base) instead of the clone-plan source path, so prototype label state differs between the two cloning paths. Rebase a copy, or restore the labels after replicate().
| self, | ||
| stage: Usd.Stage, | ||
| global_paths: tuple[str, ...] = (), | ||
| env_template: str = DEFAULT_ENV_TEMPLATE, |
There was a problem hiding this comment.
🟡 Warning · Api — New parameter inserted before existing positional arguments
env_template is inserted between global_paths and device in NewtonReplicateContext.__init__, so an existing positional call such as NewtonReplicateContext(stage, paths, "cuda:0") silently binds the device string to the template and falls back to device="cpu". The USD and OvPhysX contexts append the parameter last. Append it after the existing parameters (or make it keyword-only) and document it in the Args block, which currently omits it.
StafaH
left a comment
There was a problem hiding this comment.
Since this is moving the newton pin, we won't be able to backport this to our release branch, it'll have to stay in develop until GA. Is that okay?
| from pxr import Gf, Sdf, Usd, UsdGeom, Vt | ||
|
|
||
| from ._fabric_notices import disabled_fabric_change_notifies | ||
| from .cloner_cfg import DEFAULT_ENV_TEMPLATE |
There was a problem hiding this comment.
Please use absolute imports here and elsewhere (feel free to change other imports in the file to match)
| xforms = _compose_world_xforms(positions_np, quaternions_np, source_xform_inv) | ||
| builder.replicate(source_builder, num_worlds, xforms=xforms) | ||
|
|
||
| # One source populating every world is the shape replication can name itself: rebase the |
There was a problem hiding this comment.
The wording here is a bit confusing "every world is the shape replication can name itself:"
Description
Note: This depends on newton-physics/newton#4012, which is first released with Newton 1.6.
Env-local Newton sites were labelled the same in every environment, and the homogeneous cloning path renamed every replicated entity in a second pass after replication.
Env name in env-local site labels. A bodyless per-environment site carried the label
ft_0in every environment.cl_register_sitenow takes thedestination_templateof the clone-plan row that requested the site, so the label names the environment it lands in —/World/envs/env_3/ft_0. Sites are still resolved by index, so consumers that look them up by label and index are unaffected.ClonePlan.env_template. The destination template for one environment was a constructor argument the plan discarded, leaving a consumer holding a row unable to recover where the environment ends. It is now recorded on the plan and handed to backend replication contexts alongsideglobal_paths.Replication names its own copies. The homogeneous Newton path rebases the prototype's labels once — a few hundred entries — and passes the per-env roots to
ModelBuilder.replicate(), instead of rewriting every label in every world afterwards. That pass cost 215 ms onIsaac-Velocity-Flat-G1at 4096 environments. The resulting labels are identical either way; a prototype whose labels a per-world prefix cannot spell keeps the previous path, and the string custom attributes are still rewritten inrename_builder_labels.Type of change
Release backport
developChecklist
Docker and GPU tests run on demand. Push the commits you want tested, then
comment
run-cion the pull request.pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there