[DRAFT]: New feature: sim_usd.py: a utility that reads a USD file, runs a Newton sim ba… #636
Conversation
📝 WalkthroughWalkthroughAdds several new USD-focused simulation utilities and helpers: two simulators (basic and advanced coupled MPM), a USD stage updater/recorder, a USD-import visual-shape branch, a quaternion-to-angular-velocity helper, and small integration/collision tweaks across solver and collision pipeline code. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Simulator
participant USD as "USD Stage"
participant Parser as "parse_usd()"
participant Model as "Newton Model"
participant Solver
participant Renderer
participant CUDA as "CUDA Graph (optional)"
User->>CLI: run simulator (args)
CLI->>Simulator: instantiate(input, output, integrator)
Simulator->>USD: load & flatten stage
Simulator->>Parser: parse_usd(stage)
Parser-->>Model: return path/body maps, transforms, builder results
Simulator->>Simulator: read stage attrs (fps, substeps, integrator, integrator params)
Simulator->>Solver: construct/configure solver & integrator
Simulator->>Model: initialize state and run FK pass
alt CUDA available & capture enabled
Simulator->>CUDA: capture graph for step()
end
User->>Simulator: simulate()
loop per frame
loop per substep
alt CUDA graph executed
Simulator->>CUDA: execute graph
else
Simulator->>Solver: step(substep)
end
Simulator->>Model: collision/impulse handling & swap state buffers
end
Simulator->>Renderer: render(frame)
end
Simulator->>Renderer: save(output)
Renderer-->>User: files written
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
newton/_src/utils/sim_usd.py (7)
20-26: Remove unused import and prep for ClassVar annotations.
numpyis unused. Also, you'll needClassVarfor the class-level dicts below.-from typing import Optional +from typing import Optional, ClassVar @@ -import numpy as np
115-117: Remove debug print of body orientation (noisy stdout).This prints every run and isn’t guarded by a verbosity flag.
- orig_orientation_1 = self.model.body_q.numpy().flatten()[3:] - print(f"__init__ orig_orientation_1 = {orig_orientation_1}")
121-129: Drop unused fields (self.body_remap,self.body_merged_parent,self.body_merged_transform).These are not used elsewhere in this file;
builder_resultsalready carries them for the renderer. Removing avoids drift and confusion.- collapse_results = self.builder_results["collapse_results"] - self.path_body_relative_transform = self.builder_results["path_body_relative_transform"] - if collapse_results: - self.body_remap = collapse_results["body_remap"] - self.body_merged_parent = collapse_results["body_merged_parent"] - self.body_merged_transform = collapse_results["body_merged_transform"] - else: - self.body_remap = None - self.body_merged_parent = None - self.body_merged_transform = None + collapse_results = self.builder_results["collapse_results"] + self.path_body_relative_transform = self.builder_results["path_body_relative_transform"]
165-176: Avoid writing to dict; use setattr for attribute assignment.This preserves potential descriptors/properties and is idiomatic.
- set_attrs = set(scene_attributes.keys()) - solver_attrs = set_attrs.intersection(self.SOLVER_ATTRIBUTES_KEYS) - - for attr in solver_attrs: - self.__dict__[self.SOLVER_ATTRIBUTES[attr]] = scene_attributes[attr] + for src_key, dest_key in self.SOLVER_ATTRIBUTES.items(): + if src_key in scene_attributes: + setattr(self, dest_key, scene_attributes[src_key]) @@ - set_attrs = set(scene_attributes.keys()) - model_attrs = set_attrs.intersection(self.MODEL_ATTRIBUTES_KEYS) - - for attr in model_attrs: - self.model.__dict__[self.MODEL_ATTRIBUTES[attr]] = scene_attributes[attr] + for src_key, dest_key in self.MODEL_ATTRIBUTES.items(): + if src_key in scene_attributes: + setattr(self.model, dest_key, scene_attributes[src_key]) @@ - set_attrs = set(scene_attributes.keys()) - cur_integrator = self.INTEGRATOR_ATTRIBUTES[self.integrator_type] - integrator_attrs = set_attrs.intersection(cur_integrator.keys()) - for attr in integrator_attrs: - self.integrator.__dict__[cur_integrator[attr]] = scene_attributes[attr] + cur_integrator = self.INTEGRATOR_ATTRIBUTES[self.integrator_type] + for src_key, dest_key in cur_integrator.items(): + if src_key in scene_attributes: + setattr(self.integrator, dest_key, scene_attributes[src_key])Also applies to: 189-191, 221-226
227-236: Make rigid contact margin configurable via scene attrs (and avoid magic constant).You already plumbed solver attributes; this uses
self.rigid_contact_marginwith a default of 0.1.- rigid_contact_margin = 0.1 + rigid_contact_margin = self.rigid_contact_margin
260-265: Guard empty profiler buckets and use f-strings (UP032).Avoid KeyError/ZeroDivision when timing is missing; use f-strings per Ruff.
-def print_time_profiler(simulator): - frame_times = simulator.profiler["step"] - render_times = simulator.profiler["render"] - print("\nAverage frame sim time: {:.2f} ms".format(sum(frame_times) / len(frame_times))) - print("\nAverage frame render time: {:.2f} ms".format(sum(render_times) / len(render_times))) +def print_time_profiler(simulator): + frame_times = simulator.profiler.get("step", []) + render_times = simulator.profiler.get("render", []) + if frame_times: + print(f"\nAverage frame sim time: {sum(frame_times) / len(frame_times):.2f} ms") + if render_times: + print(f"\nAverage frame render time: {sum(render_times) / len(render_times):.2f} ms")
282-289: Optional: make CLI integrator parsing case-insensitive and print cleaner choices.Current argparse shows enum reprs in help; this improves UX but preserves behavior.
- parser.add_argument( + parser.add_argument( "-i", "--integrator", help="Type of integrator", - type=IntegratorType, - choices=list(IntegratorType), + type=lambda s: IntegratorType(s.lower()), + choices=[t.value for t in IntegratorType], default=None, )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
newton/_src/utils/sim_usd.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
PR: newton-physics/newton#521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/utils/sim_usd.py
🧬 Code graph analysis (1)
newton/_src/utils/sim_usd.py (6)
newton/_src/utils/import_usd.py (1)
parse_usd(34-1297)newton/_src/sim/builder.py (3)
ModelBuilder(67-4215)collapse_fixed_joints(1801-2098)color(3781-3822)newton/_src/sim/model.py (1)
state(440-474)newton/_src/utils/render.py (2)
RendererUsd(957-1295)render_update_stage(1058-1062)newton/_src/solvers/mujoco/solver_mujoco.py (1)
SolverMuJoCo(1096-2569)newton/_src/sim/state.py (1)
clear_forces(64-75)
🪛 Ruff (0.12.2)
newton/_src/utils/sim_usd.py
25-25: numpy imported but unused
Remove unused import: numpy
(F401)
42-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
48-54: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
55-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
263-263: Use f-string instead of format call
Convert to f-string
(UP032)
264-264: Use f-string instead of format call
Convert to f-string
(UP032)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/_src/utils/sim_usd.py (2)
140-145: Confirm CUDA graph stability with changing contact topology.Graph-capturing the entire
simulate()once assumes a stable kernel launch pattern. If contact pair counts vary across frames (likely in domino/collapse), capture/launch may fail or run with stale allocations in some backends.Would you run two quick checks?
- Use a static scene (e.g., single box on ground) and confirm identical results with/without graphs.
- Use a highly dynamic scene (e.g., your collapse USD) and check for runtime errors or mismatches across frames when
--device cuda:0is set.If needed, switch to capturing only the inner integrator kernels while recomputing contacts outside the graph, or rebuild the graph when contact counts change (track last
len(self.contacts)).
146-154: RendererUsd wiring looks correct.
source_stageis passed as the input path, withpath_body_map,path_body_relative_transform, andbuilder_resultsset. This matches the expectations fromRendererUsd.__init__and should allow correct per-prim transform updates.
| MODEL_ATTRIBUTES = { | ||
| "newton:joint_attach_kd": "joint_attach_kd", | ||
| "newton:joint_attach_ke": "joint_attach_ke", | ||
| "newton:soft_contact_kd": "soft_contact_kd", | ||
| "newton:soft_contact_ke": "soft_contact_ke", | ||
| } | ||
| SOLVER_ATTRIBUTES = { | ||
| "newton:collide_on_substeps": "collide_on_substeps", | ||
| "newton:fps": "fps", | ||
| "newton:integrator": "integrator_type", | ||
| "newton:integrator_iterations": "integrator_iterations", | ||
| "newton:substeps": "substeps", | ||
| } | ||
| INTEGRATOR_ATTRIBUTES = { | ||
| IntegratorType.EULER: { | ||
| "newton:euler:angular_damping": "angular_damping", | ||
| "newton:euler:friction_smoothing": "friction_smoothing", | ||
| }, | ||
| IntegratorType.VBD: {"newton:vbd:friction_epsilon": "friction_epsilon"}, | ||
| IntegratorType.XPBD: { | ||
| "newton:xpbd:soft_body_relaxation": "soft_body_relaxation", | ||
| "newton:xpbd:soft_contact_relaxation": "soft_contact_relaxation", | ||
| "newton:xpbd:joint_linear_relaxation": "joint_linear_relaxation", | ||
| "newton:xpbd:joint_angular_relaxation": "joint_angular_relaxation", | ||
| "newton:xpbd:rigid_contact_relaxation": "rigid_contact_relaxation", | ||
| "newton:xpbd:rigid_contact_con_weighting": "rigid_contact_con_weighting", | ||
| "newton:xpbd:angular_damping": "angular_damping", | ||
| "newton:xpbd:enable_restitution": "enable_restitution", | ||
| }, | ||
| IntegratorType.MJWARP: { | ||
| "newton:mjwarp:use_mujoco_cpu": "use_mujoco_cpu", | ||
| "newton:mjwarp:solver": "solver", | ||
| "newton:mjwarp:integrator": "integrator", | ||
| "newton:mjwarp:iterations": "iterations", | ||
| "newton:mjwarp:ls_iterations": "ls_iterations", | ||
| "newton:mjwarp:save_to_mjcf": "save_to_mjcf", | ||
| "newton:mjwarp:contact_stiffness_time_const": "contact_stiffness_time_const", | ||
| }, | ||
| } | ||
| MODEL_ATTRIBUTES_KEYS = MODEL_ATTRIBUTES.keys() | ||
| SOLVER_ATTRIBUTES_KEYS = SOLVER_ATTRIBUTES.keys() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
*Annotate mutable class-level dicts with ClassVar and drop redundant _KEYS views.
This addresses Ruff RUF012 and simplifies lookups. Also, surface newton:rigid_contact_margin as a configurable solver attribute.
-class Simulator:
+class Simulator:
@@
- MODEL_ATTRIBUTES = {
+ MODEL_ATTRIBUTES: ClassVar[dict[str, str]] = {
"newton:joint_attach_kd": "joint_attach_kd",
"newton:joint_attach_ke": "joint_attach_ke",
"newton:soft_contact_kd": "soft_contact_kd",
"newton:soft_contact_ke": "soft_contact_ke",
}
- SOLVER_ATTRIBUTES = {
+ SOLVER_ATTRIBUTES: ClassVar[dict[str, str]] = {
"newton:collide_on_substeps": "collide_on_substeps",
"newton:fps": "fps",
"newton:integrator": "integrator_type",
"newton:integrator_iterations": "integrator_iterations",
"newton:substeps": "sim_substeps",
+ "newton:rigid_contact_margin": "rigid_contact_margin",
}
- INTEGRATOR_ATTRIBUTES = {
+ INTEGRATOR_ATTRIBUTES: ClassVar[dict[IntegratorType, dict[str, str]]] = {
IntegratorType.EULER: {
"newton:euler:angular_damping": "angular_damping",
"newton:euler:friction_smoothing": "friction_smoothing",
},
IntegratorType.VBD: {"newton:vbd:friction_epsilon": "friction_epsilon"},
IntegratorType.XPBD: {
"newton:xpbd:soft_body_relaxation": "soft_body_relaxation",
"newton:xpbd:soft_contact_relaxation": "soft_contact_relaxation",
"newton:xpbd:joint_linear_relaxation": "joint_linear_relaxation",
"newton:xpbd:joint_angular_relaxation": "joint_angular_relaxation",
"newton:xpbd:rigid_contact_relaxation": "rigid_contact_relaxation",
"newton:xpbd:rigid_contact_con_weighting": "rigid_contact_con_weighting",
"newton:xpbd:angular_damping": "angular_damping",
"newton:xpbd:enable_restitution": "enable_restitution",
},
IntegratorType.MJWARP: {
"newton:mjwarp:use_mujoco_cpu": "use_mujoco_cpu",
"newton:mjwarp:solver": "solver",
"newton:mjwarp:integrator": "integrator",
"newton:mjwarp:iterations": "iterations",
"newton:mjwarp:ls_iterations": "ls_iterations",
"newton:mjwarp:save_to_mjcf": "save_to_mjcf",
"newton:mjwarp:contact_stiffness_time_const": "contact_stiffness_time_const",
},
}
- MODEL_ATTRIBUTES_KEYS = MODEL_ATTRIBUTES.keys()
- SOLVER_ATTRIBUTES_KEYS = SOLVER_ATTRIBUTES.keys()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MODEL_ATTRIBUTES = { | |
| "newton:joint_attach_kd": "joint_attach_kd", | |
| "newton:joint_attach_ke": "joint_attach_ke", | |
| "newton:soft_contact_kd": "soft_contact_kd", | |
| "newton:soft_contact_ke": "soft_contact_ke", | |
| } | |
| SOLVER_ATTRIBUTES = { | |
| "newton:collide_on_substeps": "collide_on_substeps", | |
| "newton:fps": "fps", | |
| "newton:integrator": "integrator_type", | |
| "newton:integrator_iterations": "integrator_iterations", | |
| "newton:substeps": "substeps", | |
| } | |
| INTEGRATOR_ATTRIBUTES = { | |
| IntegratorType.EULER: { | |
| "newton:euler:angular_damping": "angular_damping", | |
| "newton:euler:friction_smoothing": "friction_smoothing", | |
| }, | |
| IntegratorType.VBD: {"newton:vbd:friction_epsilon": "friction_epsilon"}, | |
| IntegratorType.XPBD: { | |
| "newton:xpbd:soft_body_relaxation": "soft_body_relaxation", | |
| "newton:xpbd:soft_contact_relaxation": "soft_contact_relaxation", | |
| "newton:xpbd:joint_linear_relaxation": "joint_linear_relaxation", | |
| "newton:xpbd:joint_angular_relaxation": "joint_angular_relaxation", | |
| "newton:xpbd:rigid_contact_relaxation": "rigid_contact_relaxation", | |
| "newton:xpbd:rigid_contact_con_weighting": "rigid_contact_con_weighting", | |
| "newton:xpbd:angular_damping": "angular_damping", | |
| "newton:xpbd:enable_restitution": "enable_restitution", | |
| }, | |
| IntegratorType.MJWARP: { | |
| "newton:mjwarp:use_mujoco_cpu": "use_mujoco_cpu", | |
| "newton:mjwarp:solver": "solver", | |
| "newton:mjwarp:integrator": "integrator", | |
| "newton:mjwarp:iterations": "iterations", | |
| "newton:mjwarp:ls_iterations": "ls_iterations", | |
| "newton:mjwarp:save_to_mjcf": "save_to_mjcf", | |
| "newton:mjwarp:contact_stiffness_time_const": "contact_stiffness_time_const", | |
| }, | |
| } | |
| MODEL_ATTRIBUTES_KEYS = MODEL_ATTRIBUTES.keys() | |
| SOLVER_ATTRIBUTES_KEYS = SOLVER_ATTRIBUTES.keys() | |
| class Simulator: | |
| MODEL_ATTRIBUTES: ClassVar[dict[str, str]] = { | |
| "newton:joint_attach_kd": "joint_attach_kd", | |
| "newton:joint_attach_ke": "joint_attach_ke", | |
| "newton:soft_contact_kd": "soft_contact_kd", | |
| "newton:soft_contact_ke": "soft_contact_ke", | |
| } | |
| SOLVER_ATTRIBUTES: ClassVar[dict[str, str]] = { | |
| "newton:collide_on_substeps": "collide_on_substeps", | |
| "newton:fps": "fps", | |
| "newton:integrator": "integrator_type", | |
| "newton:integrator_iterations": "integrator_iterations", | |
| "newton:substeps": "sim_substeps", | |
| "newton:rigid_contact_margin": "rigid_contact_margin", | |
| } | |
| INTEGRATOR_ATTRIBUTES: ClassVar[dict[IntegratorType, dict[str, str]]] = { | |
| IntegratorType.EULER: { | |
| "newton:euler:angular_damping": "angular_damping", | |
| "newton:euler:friction_smoothing": "friction_smoothing", | |
| }, | |
| IntegratorType.VBD: { | |
| "newton:vbd:friction_epsilon": "friction_epsilon", | |
| }, | |
| IntegratorType.XPBD: { | |
| "newton:xpbd:soft_body_relaxation": "soft_body_relaxation", | |
| "newton:xpbd:soft_contact_relaxation": "soft_contact_relaxation", | |
| "newton:xpbd:joint_linear_relaxation": "joint_linear_relaxation", | |
| "newton:xpbd:joint_angular_relaxation": "joint_angular_relaxation", | |
| "newton:xpbd:rigid_contact_relaxation": "rigid_contact_relaxation", | |
| "newton:xpbd:rigid_contact_con_weighting": "rigid_contact_con_weighting", | |
| "newton:xpbd:angular_damping": "angular_damping", | |
| "newton:xpbd:enable_restitution": "enable_restitution", | |
| }, | |
| IntegratorType.MJWARP: { | |
| "newton:mjwarp:use_mujoco_cpu": "use_mujoco_cpu", | |
| "newton:mjwarp:solver": "solver", | |
| "newton:mjwarp:integrator": "integrator", | |
| "newton:mjwarp:iterations": "iterations", | |
| "newton:mjwarp:ls_iterations": "ls_iterations", | |
| "newton:mjwarp:save_to_mjcf": "save_to_mjcf", | |
| "newton:mjwarp:contact_stiffness_time_const": "contact_stiffness_time_const", | |
| }, | |
| } |
🧰 Tools
🪛 Ruff (0.12.2)
42-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
48-54: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
55-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd.py around lines 42 to 83, the review requests
annotating mutable class-level dicts with typing.ClassVar, removing the
redundant MODEL_ATTRIBUTES_KEYS and SOLVER_ATTRIBUTES_KEYS views, and exposing
"newton:rigid_contact_margin" as a solver attribute; update the dict
declarations to be typed as ClassVar[dict[str, str]] (import ClassVar if
missing), add the new "newton:rigid_contact_margin": "rigid_contact_margin"
entry to SOLVER_ATTRIBUTES, and delete the MODEL_ATTRIBUTES_KEYS and
SOLVER_ATTRIBUTES_KEYS variables and any code using them so lookups use the
dicts directly.
| # Defaults | ||
| self.fps = 60 | ||
| self.sim_substeps = 32 | ||
| self.integrator_type = IntegratorType.XPBD | ||
| self.integrator_iterations = 100 | ||
| self.collide_on_substeps = True | ||
|
|
||
| # Loading attributes | ||
| set_attrs = set(scene_attributes.keys()) | ||
| solver_attrs = set_attrs.intersection(self.SOLVER_ATTRIBUTES_KEYS) | ||
|
|
||
| for attr in solver_attrs: | ||
| self.__dict__[self.SOLVER_ATTRIBUTES[attr]] = scene_attributes[attr] | ||
|
|
||
| # Derived/computed attributes that depend on the above | ||
| self.frame_dt = 1.0 / self.fps | ||
| self.sim_dt = self.frame_dt / self.sim_substeps | ||
| self.integrator_type = IntegratorType(self.integrator_type) | ||
|
|
There was a problem hiding this comment.
Add validation for fps and substeps to avoid divide-by-zero and negative dt.
frame_dt = 1.0 / self.fps will crash if fps == 0; negative values also don’t make sense. Same for sim_substeps.
# Defaults
self.fps = 60
self.sim_substeps = 32
self.integrator_type = IntegratorType.XPBD
self.integrator_iterations = 100
self.collide_on_substeps = True
+ self.rigid_contact_margin = 0.1
@@
- self.frame_dt = 1.0 / self.fps
- self.sim_dt = self.frame_dt / self.sim_substeps
+ if self.fps <= 0:
+ raise ValueError(f"Invalid fps: {self.fps} (must be > 0)")
+ if self.sim_substeps <= 0:
+ raise ValueError(f"Invalid substeps: {self.sim_substeps} (must be > 0)")
+ self.frame_dt = 1.0 / self.fps
+ self.sim_dt = self.frame_dt / self.sim_substeps
self.integrator_type = IntegratorType(self.integrator_type)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Defaults | |
| self.fps = 60 | |
| self.sim_substeps = 32 | |
| self.integrator_type = IntegratorType.XPBD | |
| self.integrator_iterations = 100 | |
| self.collide_on_substeps = True | |
| # Loading attributes | |
| set_attrs = set(scene_attributes.keys()) | |
| solver_attrs = set_attrs.intersection(self.SOLVER_ATTRIBUTES_KEYS) | |
| for attr in solver_attrs: | |
| self.__dict__[self.SOLVER_ATTRIBUTES[attr]] = scene_attributes[attr] | |
| # Derived/computed attributes that depend on the above | |
| self.frame_dt = 1.0 / self.fps | |
| self.sim_dt = self.frame_dt / self.sim_substeps | |
| self.integrator_type = IntegratorType(self.integrator_type) | |
| # Defaults | |
| self.fps = 60 | |
| self.sim_substeps = 32 | |
| self.integrator_type = IntegratorType.XPBD | |
| self.integrator_iterations = 100 | |
| self.collide_on_substeps = True | |
| self.rigid_contact_margin = 0.1 | |
| # Loading attributes | |
| set_attrs = set(scene_attributes.keys()) | |
| solver_attrs = set_attrs.intersection(self.SOLVER_ATTRIBUTES_KEYS) | |
| for attr in solver_attrs: | |
| self.__dict__[self.SOLVER_ATTRIBUTES[attr]] = scene_attributes[attr] | |
| # Derived/computed attributes that depend on the above | |
| if self.fps <= 0: | |
| raise ValueError(f"Invalid fps: {self.fps} (must be > 0)") | |
| if self.sim_substeps <= 0: | |
| raise ValueError(f"Invalid substeps: {self.sim_substeps} (must be > 0)") | |
| self.frame_dt = 1.0 / self.fps | |
| self.sim_dt = self.frame_dt / self.sim_substeps | |
| self.integrator_type = IntegratorType(self.integrator_type) |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd.py around lines 158 to 176, validate that self.fps
and self.sim_substeps are positive non-zero numbers before computing frame_dt
and sim_dt; if either is missing or <= 0, raise a clear ValueError (or fallback
to safe defaults such as fps=60, sim_substeps=1) and only then compute
self.frame_dt = 1.0 / self.fps, self.sim_dt = self.frame_dt / self.sim_substeps,
keeping the integrator conversion afterward.
| # TODO: add Euler integrator | ||
| # if self.integrator_type == IntegratorType.EULER: | ||
| # self.integrator = newton.SemiImplicitIntegrator() | ||
| if self.integrator_type == IntegratorType.XPBD: | ||
| solver_args = {"iterations": scene_attributes.get("newton:xpbd:iterations", self.integrator_iterations)} | ||
| self.integrator = newton.solvers.XPBDSolver(self.model, **solver_args) | ||
| elif self.integrator_type == IntegratorType.MJWARP: | ||
| solver_args = { | ||
| "use_mujoco_cpu": scene_attributes.get("newton:mjwarp:use_mujoco_cpu", False), | ||
| "solver": scene_attributes.get("newton:mjwarp:solver", "newton"), | ||
| "integrator": scene_attributes.get("newton:mjwarp:integrator", "euler"), | ||
| "iterations": scene_attributes.get("newton:mjwarp:iterations", self.integrator_iterations), | ||
| "ls_iterations": scene_attributes.get("newton:mjwarp:ls_iterations", 5), | ||
| "save_to_mjcf": scene_attributes.get("newton:mjwarp:save_to_mjcf", "sim_usd_mjcf.xml"), | ||
| "contact_stiffness_time_const": scene_attributes.get( | ||
| "newton:mjwarp:contact_stiffness_time_const", 0.02 | ||
| ), | ||
| } | ||
| self.integrator = newton.solvers.SolverMuJoCo( | ||
| self.model, | ||
| **solver_args, | ||
| ) | ||
| else: | ||
| self.integrator = newton.solvers.VBDIntegrator(self.model, iterations=self.integrator_iterations) | ||
|
|
There was a problem hiding this comment.
Fix: requesting EULER integrator silently falls back to VBD.
If a stage or CLI requests euler, the current code ends up in the else: and uses VBD. Either implement Euler or raise a clear error to avoid silent misbehavior. Below raises until Euler is implemented.
- # TODO: add Euler integrator
- # if self.integrator_type == IntegratorType.EULER:
- # self.integrator = newton.SemiImplicitIntegrator()
- if self.integrator_type == IntegratorType.XPBD:
+ # TODO: add Euler integrator
+ if self.integrator_type == IntegratorType.EULER:
+ raise NotImplementedError(
+ "IntegratorType.EULER is not implemented yet. Use XPBD, VBD, or MJWARP."
+ )
+ elif self.integrator_type == IntegratorType.XPBD:
solver_args = {"iterations": scene_attributes.get("newton:xpbd:iterations", self.integrator_iterations)}
self.integrator = newton.solvers.XPBDSolver(self.model, **solver_args)
elif self.integrator_type == IntegratorType.MJWARP:
@@
- else:
- self.integrator = newton.solvers.VBDIntegrator(self.model, iterations=self.integrator_iterations)
+ elif self.integrator_type == IntegratorType.VBD:
+ self.integrator = newton.solvers.VBDIntegrator(self.model, iterations=self.integrator_iterations)
+ else:
+ raise ValueError(f"Unsupported integrator: {self.integrator_type}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # TODO: add Euler integrator | |
| # if self.integrator_type == IntegratorType.EULER: | |
| # self.integrator = newton.SemiImplicitIntegrator() | |
| if self.integrator_type == IntegratorType.XPBD: | |
| solver_args = {"iterations": scene_attributes.get("newton:xpbd:iterations", self.integrator_iterations)} | |
| self.integrator = newton.solvers.XPBDSolver(self.model, **solver_args) | |
| elif self.integrator_type == IntegratorType.MJWARP: | |
| solver_args = { | |
| "use_mujoco_cpu": scene_attributes.get("newton:mjwarp:use_mujoco_cpu", False), | |
| "solver": scene_attributes.get("newton:mjwarp:solver", "newton"), | |
| "integrator": scene_attributes.get("newton:mjwarp:integrator", "euler"), | |
| "iterations": scene_attributes.get("newton:mjwarp:iterations", self.integrator_iterations), | |
| "ls_iterations": scene_attributes.get("newton:mjwarp:ls_iterations", 5), | |
| "save_to_mjcf": scene_attributes.get("newton:mjwarp:save_to_mjcf", "sim_usd_mjcf.xml"), | |
| "contact_stiffness_time_const": scene_attributes.get( | |
| "newton:mjwarp:contact_stiffness_time_const", 0.02 | |
| ), | |
| } | |
| self.integrator = newton.solvers.SolverMuJoCo( | |
| self.model, | |
| **solver_args, | |
| ) | |
| else: | |
| self.integrator = newton.solvers.VBDIntegrator(self.model, iterations=self.integrator_iterations) | |
| # TODO: add Euler integrator | |
| if self.integrator_type == IntegratorType.EULER: | |
| raise NotImplementedError( | |
| "IntegratorType.EULER is not implemented yet. Use XPBD, VBD, or MJWARP." | |
| ) | |
| elif self.integrator_type == IntegratorType.XPBD: | |
| solver_args = { | |
| "iterations": scene_attributes.get( | |
| "newton:xpbd:iterations", self.integrator_iterations | |
| ) | |
| } | |
| self.integrator = newton.solvers.XPBDSolver(self.model, **solver_args) | |
| elif self.integrator_type == IntegratorType.MJWARP: | |
| solver_args = { | |
| "use_mujoco_cpu": scene_attributes.get("newton:mjwarp:use_mujoco_cpu", False), | |
| "solver": scene_attributes.get("newton:mjwarp:solver", "newton"), | |
| "integrator": scene_attributes.get("newton:mjwarp:integrator", "euler"), | |
| "iterations": scene_attributes.get( | |
| "newton:mjwarp:iterations", self.integrator_iterations | |
| ), | |
| "ls_iterations": scene_attributes.get("newton:mjwarp:ls_iterations", 5), | |
| "save_to_mjcf": scene_attributes.get( | |
| "newton:mjwarp:save_to_mjcf", "sim_usd_mjcf.xml" | |
| ), | |
| "contact_stiffness_time_const": scene_attributes.get( | |
| "newton:mjwarp:contact_stiffness_time_const", 0.02 | |
| ), | |
| } | |
| self.integrator = newton.solvers.SolverMuJoCo( | |
| self.model, | |
| **solver_args, | |
| ) | |
| elif self.integrator_type == IntegratorType.VBD: | |
| self.integrator = newton.solvers.VBDIntegrator( | |
| self.model, iterations=self.integrator_iterations | |
| ) | |
| else: | |
| raise ValueError(f"Unsupported integrator: {self.integrator_type}") |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd.py around lines 195-219, requesting the EULER
integrator currently falls through to the else branch and silently uses VBD; add
explicit handling for IntegratorType.EULER so behavior is not silent: either
implement the Euler integrator by constructing the appropriate integrator (e.g.,
newton.SemiImplicitIntegrator() or the correct newton integrator API) and assign
it to self.integrator, or if Euler is not ready, raise a clear error (e.g.,
raise NotImplementedError or ValueError with a descriptive message) instead of
letting execution fall into the VBD default.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
newton/_src/utils/sim_usd.py
Outdated
| @@ -0,0 +1,310 @@ | |||
| # Copyright (c) 2022 NVIDIA CORPORATION. All rights reserved. | |||
There was a problem hiding this comment.
This needs to be the Newton license header.
| EULER = "euler" | ||
| XPBD = "xpbd" | ||
| VBD = "vbd" | ||
| MJWARP = "mjwarp" |
| "newton:xpbd:enable_restitution": "enable_restitution", | ||
| }, | ||
| IntegratorType.MJWARP: { | ||
| "newton:mjwarp:use_mujoco_cpu": "use_mujoco_cpu", |
There was a problem hiding this comment.
Here it should also be newton:mujoco:....
| self.state_1 = self.model.state() | ||
|
|
||
| # NB: body_q will be modified, so initial state will be slightly altered | ||
| if self.model.joint_count: |
There was a problem hiding this comment.
You can remove the if condition, just eval_fk always. It will not do anything if there are no joints.
| if self.model.joint_count: | ||
| newton.eval_fk(self.model, self.model.joint_q, self.model.joint_qd, self.state_0, mask=None) | ||
|
|
||
| self.use_cuda_graph = wp.get_device().is_cuda |
There was a problem hiding this comment.
Can you match the API of the examples we have, i.e. using Example.capture(), step(), simulate(), render()?
| "newton:mjwarp:iterations": "iterations", | ||
| "newton:mjwarp:ls_iterations": "ls_iterations", | ||
| "newton:mjwarp:save_to_mjcf": "save_to_mjcf", | ||
| "newton:mjwarp:contact_stiffness_time_const": "contact_stiffness_time_const", |
There was a problem hiding this comment.
You probably also want to expose the cone property and maybe impratio here.
|
|
||
| # TODO: add Euler integrator | ||
| # if self.integrator_type == IntegratorType.EULER: | ||
| # self.integrator = newton.SemiImplicitIntegrator() |
| self.integrator.__dict__[cur_integrator[attr]] = scene_attributes[attr] | ||
|
|
||
| def simulate(self): | ||
| rigid_contact_margin = 0.1 |
There was a problem hiding this comment.
This should be configurable.
| self.simulate() | ||
| self.graph = capture.graph | ||
|
|
||
| self.renderer = newton.viewer.RendererUsd( |
There was a problem hiding this comment.
Can you use the new ViewerUSD class here instead? This should replace RendererUsd.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/utils/import_usd.py (1)
106-111: Fix argument order and remove invalid parameter inparse_usdcall.In
newton/_src/utils/sim_usd.py(lines 106-110), the call passes arguments in the wrong order:self.in_stageshould be the second argument, not the first. Additionally,invert_rotationsis not a valid parameter ofparse_usd. The correct order is:builderfirst, thensource(the stage).results = parse_usd( builder, self.in_stage, collapse_fixed_joints=True, )
♻️ Duplicate comments (1)
newton/_src/utils/sim_usd.py (1)
56-62: Mismatch betweenSOLVER_ATTRIBUTESkey and instance attribute name.
SOLVER_ATTRIBUTESmaps"newton:substeps"to"substeps", but line 168 initializesself.sim_substeps. Either the mapping value should be"sim_substeps"or the attribute should be namedsubsteps.SOLVER_ATTRIBUTES = { "newton:collide_on_substeps": "collide_on_substeps", "newton:fps": "fps", "newton:integrator": "integrator_type", "newton:integrator_iterations": "integrator_iterations", - "newton:substeps": "substeps", + "newton:substeps": "sim_substeps", }Also applies to: 166-172
🧹 Nitpick comments (8)
newton/_src/core/spatial.py (1)
332-374: Docstring return type mismatch.The docstring states the return is
ndarray shape (3,)but the function returns awp.vec3. This is a minor documentation inconsistency.- Returns: - omega : ndarray shape (3,) - Angular velocity vector in rad/s. + Returns: + omega : wp.vec3 + Angular velocity vector in rad/s.newton/_src/utils/import_usd.py (1)
212-216: Ambiguous variable namel.The variable
l(lowercase L) can be easily confused with the digit1or uppercaseI. Use a more descriptive name likelengthorquat_len.- l = wp.length(quat) - if np.isfinite(l) and l > 0.0: + quat_len = wp.length(quat) + if np.isfinite(quat_len) and quat_len > 0.0:newton/_src/utils/sim_usd.py (1)
28-28: Top-level import ofpxrmay cause import failures.Importing
pxrat module level will cause anImportErrorfor users who don't have USD installed. Based on learnings, PXR/USD modules should be lazily loaded within functions using try-except blocks.-from pxr import Usd +# pxr imported lazily within functions that need itThen import inside
__init__orcreate_stage_from_path:def create_stage_from_path(input_path) -> "Usd.Stage": from pxr import Usd # Lazy import stage = Usd.Stage.Open(input_path, Usd.Stage.LoadAll) ...newton/_src/utils/sim_usd_gtc.py (5)
49-58: Avoid function calls in default arguments.
Usd.TimeCode.Default()is evaluated once at function definition time, not at each call. Move it inside the function.-def parse_xform(prim, time=Usd.TimeCode.Default(), return_mat=False): +def parse_xform(prim, time=None, return_mat=False): from pxr import UsdGeom + if time is None: + time = Usd.TimeCode.Default() xform = UsdGeom.Xform(prim)
1008-1013: Avoid catching blindException.Catching all exceptions hides bugs and makes debugging difficult. Consider catching specific exceptions (e.g.,
RuntimeError) or at minimum log the exception type.try: wp.capture_launch(self.graph) - except Exception as e: - print(f"Error launching graph: {e}. Falling back to regular execution, which may be slower.") + except RuntimeError as e: + print(f"CUDA graph launch failed: {e}. Falling back to regular execution.") self.graph = None self.simulate()
1059-1060: Unused variablecid.The variable
cidis unpacked but never used. Prefix with underscore to indicate it's intentionally ignored.- impulses, pos, cid = self.integrator.mpm_solver.collect_collider_impulses( + impulses, pos, _cid = self.integrator.mpm_solver.collect_collider_impulses(
459-459: Avoid mutable default argument withwp.vec3.
wp.vec3(0.0, 0.0, 0.0)is evaluated once at definition time. UseNoneas default and create the vector inside the function.- usd_offset: wp.vec3 = wp.vec3(0.0, 0.0, 0.0), + usd_offset: wp.vec3 | None = None,Then in the function body:
if usd_offset is None: usd_offset = wp.vec3(0.0, 0.0, 0.0)
527-534: Addstrict=parameter tozip().For safety, specify
strict=Trueif the iterables must have equal length, orstrict=Falseto explicitly allow truncation.shape_indices = [ i - for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key)) + for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key, strict=True)) if flags & newton.ShapeFlags.COLLIDE_SHAPES
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/core/spatial.py(1 hunks)newton/_src/usd/schema_resolver.py(1 hunks)newton/_src/utils/import_usd.py(11 hunks)newton/_src/utils/sim_usd.py(1 hunks)newton/_src/utils/sim_usd_gtc.py(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/_src/utils/sim_usd.pynewton/_src/usd/schema_resolver.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-12-12T08:45:43.428Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:43.428Z
Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-09-22T21:03:18.367Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-09-02T13:59:02.072Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 701
File: CONTRIBUTING.md:7-7
Timestamp: 2025-09-02T13:59:02.072Z
Learning: The newton-physics/newton-governance repository's CONTRIBUTING.md file contains both "Legal Requirements" (line 73, anchor #legal-requirements) and "Project Members" (line 108, anchor #project-members) sections that can be properly referenced from other repositories' CONTRIBUTING.md files.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/utils/sim_usd_gtc.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-12-06T19:10:24.360Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_box_on_plane.py:322-371
Timestamp: 2025-12-06T19:10:24.360Z
Learning: In Newton Kamino examples under newton/_src/solvers/kamino/examples/sim, CLI arguments parsed via argparse (including flags like --test, --device, --headless, etc.) are passed as a complete args object to newton.examples.run(example, args). The run function internally consumes these flags to control execution behavior, so flags may appear "unused" in the example script itself even though they are actively used by the framework.
Applied to files:
newton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/tests/test_examples.py:329-333
Timestamp: 2025-08-25T21:41:45.795Z
Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.
Applied to files:
newton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/import_usd.py
🧬 Code graph analysis (4)
newton/_src/utils/sim_usd.py (2)
newton/_src/utils/import_usd.py (1)
parse_usd(38-1783)newton/_src/core/types.py (1)
Axis(69-166)
newton/_src/usd/schema_resolver.py (1)
newton/_src/usd/schemas.py (1)
SchemaResolverNewton(26-102)
newton/_src/utils/import_usd.py (2)
newton/_src/usd/schema_resolver.py (2)
PrimType(33-49)SchemaResolver(70-145)newton/_src/usd/utils.py (4)
get_attribute(40-40)get_attribute(44-44)get_attribute(47-62)has_attribute(83-95)
newton/_src/core/spatial.py (1)
newton/_src/sim/builder.py (1)
dot(4585-4586)
🪛 GitHub Actions: Pull Request
newton/_src/utils/import_usd.py
[error] 363-363: NameError: name 'load_visual_shapes_impl' is not defined. This occurred during USD import while loading visual shapes, causing subsequent tests to fail.
🪛 Ruff (0.14.8)
newton/_src/utils/sim_usd.py
50-55: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
56-62: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
63-88: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
newton/_src/utils/sim_usd_gtc.py
49-49: Do not perform function call Usd.TimeCode.Default in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
459-459: Do not perform function call wp.vec3 in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
529-529: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1010-1010: Do not catch blind exception: Exception
(BLE001)
1059-1059: Unpacked variable cid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1204-1204: Abstract raise to an inner function
(TRY301)
1204-1204: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/utils/import_usd.py
190-190: Undefined name Iterable
(F821)
213-213: Ambiguous variable name: l
(E741)
239-239: Undefined name invert_rotations
(F821)
363-363: Undefined name load_visual_shapes_impl
(F821)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
newton/_src/usd/schema_resolver.py
Outdated
| class SchemaResolverNewton(SchemaResolver): | ||
| name: ClassVar[str] = "newton" | ||
| mapping: ClassVar[dict[PrimType, dict[str, SchemaAttribute]]] = { | ||
| PrimType.SCENE: { | ||
| "time_step": SchemaAttribute("newton:timeStep", 0.002), | ||
| "max_solver_iterations": SchemaAttribute("newton:maxSolverIterations", 5), | ||
| "enable_gravity": SchemaAttribute("newton:enableGravity", True), | ||
| "rigid_contact_margin": SchemaAttribute("newton:rigidContactMargin", 0.0), | ||
| }, | ||
| PrimType.JOINT: { | ||
| "armature": SchemaAttribute("newton:armature", 1.0e-2), | ||
| "friction": SchemaAttribute("newton:friction", 0.0), | ||
| "limit_linear_ke": SchemaAttribute("newton:linear:limitStiffness", 1.0e4), | ||
| "limit_angular_ke": SchemaAttribute("newton:angular:limitStiffness", 1.0e4), | ||
| "limit_rotX_ke": SchemaAttribute("newton:rotX:limitStiffness", 1.0e4), | ||
| "limit_rotY_ke": SchemaAttribute("newton:rotY:limitStiffness", 1.0e4), | ||
| "limit_rotZ_ke": SchemaAttribute("newton:rotZ:limitStiffness", 1.0e4), | ||
| "limit_linear_kd": SchemaAttribute("newton:linear:limitDamping", 1.0e1), | ||
| "limit_angular_kd": SchemaAttribute("newton:angular:limitDamping", 1.0e1), | ||
| "limit_rotX_kd": SchemaAttribute("newton:rotX:limitDamping", 1.0e1), | ||
| "limit_rotY_kd": SchemaAttribute("newton:rotY:limitDamping", 1.0e1), | ||
| "limit_rotZ_kd": SchemaAttribute("newton:rotZ:limitDamping", 1.0e1), | ||
| "angular_position": SchemaAttribute("newton:angular:position", 0.0), | ||
| "linear_position": SchemaAttribute("newton:linear:position", 0.0), | ||
| "rotX_position": SchemaAttribute("newton:rotX:position", 0.0), | ||
| "rotY_position": SchemaAttribute("newton:rotY:position", 0.0), | ||
| "rotZ_position": SchemaAttribute("newton:rotZ:position", 0.0), | ||
| "angular_velocity": SchemaAttribute("newton:angular:velocity", 0.0), | ||
| "linear_velocity": SchemaAttribute("newton:linear:velocity", 0.0), | ||
| "rotX_velocity": SchemaAttribute("newton:rotX:velocity", 0.0), | ||
| "rotY_velocity": SchemaAttribute("newton:rotY:velocity", 0.0), | ||
| "rotZ_velocity": SchemaAttribute("newton:rotZ:velocity", 0.0), | ||
| }, | ||
| PrimType.SHAPE: { | ||
| "mesh_hull_vertex_limit": SchemaAttribute("newton:hullVertexLimit", -1), | ||
| # Use ShapeConfig.thickness default for contact margin | ||
| "rigid_contact_margin": SchemaAttribute("newton:rigidContactMargin", 1.0e-5), | ||
| }, | ||
| PrimType.BODY: { | ||
| "rigid_body_linear_damping": SchemaAttribute("newton:damping", 0.0), | ||
| }, | ||
| PrimType.MATERIAL: { | ||
| "priority": SchemaAttribute("newton:priority", 0), | ||
| "weight": SchemaAttribute("newton:weight", 1.0), | ||
| "stiffness": SchemaAttribute("newton:stiffness", 1.0e5), | ||
| "damping": SchemaAttribute("newton:damping", 1000.0), | ||
| }, | ||
| PrimType.ACTUATOR: { | ||
| # Mirror MuJoCo actuator defaults when applicable | ||
| "ctrl_low": SchemaAttribute("newton:ctrlRange:low", 0.0), | ||
| "ctrl_high": SchemaAttribute("newton:ctrlRange:high", 0.0), | ||
| "force_low": SchemaAttribute("newton:forceRange:low", 0.0), | ||
| "force_high": SchemaAttribute("newton:forceRange:high", 0.0), | ||
| "act_low": SchemaAttribute("newton:actRange:low", 0.0), | ||
| "act_high": SchemaAttribute("newton:actRange:high", 0.0), | ||
| "length_low": SchemaAttribute("newton:lengthRange:low", 0.0), | ||
| "length_high": SchemaAttribute("newton:lengthRange:high", 0.0), | ||
| "gainPrm": SchemaAttribute("newton:gainPrm", [1, 0, 0, 0, 0, 0, 0, 0, 0, 0]), | ||
| "gainType": SchemaAttribute("newton:gainType", "fixed"), | ||
| "biasPrm": SchemaAttribute("newton:biasPrm", [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]), | ||
| "biasType": SchemaAttribute("newton:biasType", "none"), | ||
| "dynPrm": SchemaAttribute("newton:dynPrm", [1, 0, 0, 0, 0, 0, 0, 0, 0, 0]), | ||
| "dynType": SchemaAttribute("newton:dynType", "none"), | ||
| # The following have no MuJoCo counterpart; keep unspecified defaults | ||
| "speedTorqueGradient": SchemaAttribute("newton:speedTorqueGradient", None), | ||
| "torqueSpeedGradient": SchemaAttribute("newton:torqueSpeedGradient", None), | ||
| "maxVelocity": SchemaAttribute("newton:maxVelocity", None), | ||
| "gear": SchemaAttribute("newton:gear", [1, 0, 0, 0, 0, 0]), | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for SchemaResolverNewton class in the repository
echo "=== Searching for SchemaResolverNewton in codebase ==="
rg -n "class SchemaResolverNewton" --type py
echo ""
echo "=== Checking file structure ==="
fd -t f "schema_resolver" --type py | head -20Repository: newton-physics/newton
Length of output: 496
🏁 Script executed:
#!/bin/bash
# Read the SchemaResolverNewton class from schema_resolver.py
echo "=== newton/_src/usd/schema_resolver.py (lines 270-350) ==="
wc -l newton/_src/usd/schema_resolver.py
cat -n newton/_src/usd/schema_resolver.py | sed -n '270,350p'
echo ""
echo "=== newton/_src/usd/schemas.py (lines 1-150) ==="
wc -l newton/_src/usd/schemas.py
cat -n newton/_src/usd/schemas.py | sed -n '1,150p'Repository: newton-physics/newton
Length of output: 14009
🏁 Script executed:
#!/bin/bash
# Check if schemas.py is imported or used elsewhere
echo "=== Where is schemas.py imported? ==="
rg -n "from.*schemas import\|import.*schemas" --type py
echo ""
echo "=== Where is schema_resolver.py imported? ==="
rg -n "from.*schema_resolver import\|import.*schema_resolver" --type py
echo ""
echo "=== Check if SchemaResolverNewton is used/registered ==="
rg -n "SchemaResolverNewton" --type pyRepository: newton-physics/newton
Length of output: 3551
🏁 Script executed:
#!/bin/bash
# Verify that schemas.py is truly unused by checking all Python files
echo "=== Checking ALL Python files for schemas.py imports ==="
rg "schemas" --type py -l | head -20
echo ""
echo "=== Detailed check for schemas.py references ==="
rg "from.*_src.*schemas|import.*schemas" --type pyRepository: newton-physics/newton
Length of output: 483
🏁 Script executed:
#!/bin/bash
# Check the exact imports from schemas.py in test file
echo "=== test_schema_resolver.py imports from schemas ==="
sed -n '1,70p' newton/tests/test_schema_resolver.py
echo ""
echo "=== usd.py schemas import (commented or not) ==="
sed -n '65,80p' newton/usd.pyRepository: newton-physics/newton
Length of output: 3524
🏁 Script executed:
#!/bin/bash
# Check what classes are actually defined in schemas.py
echo "=== Classes defined in schemas.py ==="
rg "^class " newton/_src/usd/schemas.py
echo ""
echo "=== Classes defined in schema_resolver.py ==="
rg "^class " newton/_src/usd/schema_resolver.py
echo ""
echo "=== Check if SchemaResolverMjc and SchemaResolverPhysx also duplicated ==="
rg "class SchemaResolverMjc|class SchemaResolverPhysx" --type pyRepository: newton-physics/newton
Length of output: 634
🏁 Script executed:
#!/bin/bash
# Check if SchemaResolverMjc and SchemaResolverPhysx exist in schema_resolver.py
echo "=== Search for all resolver classes in schema_resolver.py ==="
rg -n "class SchemaResolver" newton/_src/usd/schema_resolver.py
echo ""
echo "=== Full content of schemas.py to see all classes ==="
wc -l newton/_src/usd/schemas.py
tail -200 newton/_src/usd/schemas.pyRepository: newton-physics/newton
Length of output: 10366
🏁 Script executed:
#!/bin/bash
# Final check - which version of the resolvers are actually used where
echo "=== Import in import_usd.py ==="
grep -n "import.*Resolver\|from.*Resolver" newton/_src/utils/import_usd.py
echo ""
echo "=== Import in test_schema_resolver.py ==="
grep -n "import.*Resolver\|from.*Resolver" newton/tests/test_schema_resolver.py | head -10
echo ""
echo "=== Import in usd.py (main module) ==="
grep -n "import.*Resolver\|from.*Resolver" newton/usd.py | head -20Repository: newton-physics/newton
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Check test_schema_resolver imports more carefully
echo "=== test_schema_resolver.py full import section ==="
sed -n '40,70p' newton/tests/test_schema_resolver.py
echo ""
echo "=== Verify what's imported from schemas.py in test ==="
rg -A 5 "from newton._src.usd.schemas import" newton/tests/test_schema_resolver.pyRepository: newton-physics/newton
Length of output: 1141
🏁 Script executed:
#!/bin/bash
# Final clarification - check if SchemaResolverMjc and SchemaResolverPhysx exist in schema_resolver.py
echo "=== Full list of classes in schema_resolver.py ==="
rg "^class " newton/_src/usd/schema_resolver.py
echo ""
echo "=== Line count check on both files ==="
wc -l newton/_src/usd/schema_resolver.py newton/_src/usd/schemas.py
echo ""
echo "=== Verify schema_resolver.py doesn't have MjC or Physx resolvers ==="
rg "SchemaResolverMjc|SchemaResolverPhysx" newton/_src/usd/schema_resolver.pyRepository: newton-physics/newton
Length of output: 455
🏁 Script executed:
#!/bin/bash
# Check if there's a migration in progress - when was schemas.py last modified?
echo "=== Git history of both files (last 5 commits each) ==="
git log --oneline -5 -- newton/_src/usd/schema_resolver.py
echo "---"
git log --oneline -5 -- newton/_src/usd/schemas.py
echo ""
echo "=== Check if schemas.py was recently added or deprecated ==="
git log --all --pretty=format:"%h %s" -- newton/_src/usd/schemas.py | head -5Repository: newton-physics/newton
Length of output: 489
🏁 Script executed:
#!/bin/bash
# Final comprehensive check
echo "=== Production import path ==="
rg "SchemaResolverNewton" newton/_src/utils/import_usd.py
echo ""
echo "=== Test import path ==="
rg "from.*schemas import" newton/tests/test_schema_resolver.py
echo ""
echo "=== Check if any code actually uses the schemas.py imports in tests ==="
sed -n '55,110p' newton/tests/test_schema_resolver.py | grep -E "SchemaResolverNewton|SchemaResolverPhysx|SchemaResolverMjc" | head -5
echo ""
echo "=== Confirm: all three resolvers in schemas.py but only Newton in schema_resolver.py ==="
echo "In schema_resolver.py:"
rg "^class SchemaResolver" newton/_src/usd/schema_resolver.py | wc -l
echo "In schemas.py:"
rg "^class SchemaResolver" newton/_src/usd/schemas.py | wc -lRepository: newton-physics/newton
Length of output: 672
🏁 Script executed:
#!/bin/bash
# Final verification - SchemaResolverMjc and SchemaResolverPhysx locations
echo "=== Where SchemaResolverPhysx is defined ==="
rg -n "class SchemaResolverPhysx" --type py
echo ""
echo "=== Where SchemaResolverMjc is defined ==="
rg -n "class SchemaResolverMjc" --type py
echo ""
echo "=== Test imports and their usage ==="
sed -n '55,65p' newton/tests/test_schema_resolver.pyRepository: newton-physics/newton
Length of output: 567
Resolve import split between test and production code for schema resolvers.
The codebase has an architectural inconsistency: newton/_src/usd/schema_resolver.py defines SchemaResolverNewton, while newton/_src/usd/schemas.py defines all three resolvers (Newton, PhysX, MuJoCo). Production code (import_usd.py) imports from schema_resolver.py, but tests import from schemas.py. The schemas.py file is marked "Currently not used" yet contains the only definitions of SchemaResolverPhysx and SchemaResolverMjc, making this comment inaccurate. Additionally, SchemaResolverNewton is duplicated in both files. Consolidate all resolver classes into schema_resolver.py and update test imports accordingly, or clarify the intentional separation if one exists.
newton/_src/utils/import_usd.py
Outdated
| def parse_float_with_fallback(prims: Iterable[Usd.Prim], name: str, default: float = 0.0) -> float: | ||
| ret = default | ||
| for prim in prims: | ||
| if not prim: | ||
| continue | ||
| attr = get_attribute(prim, name) | ||
| if not attr or not attr.HasAuthoredValue(): | ||
| continue | ||
| val = attr.Get() | ||
| if np.isfinite(val): | ||
| ret = val | ||
| break | ||
| return ret |
There was a problem hiding this comment.
Missing import for Iterable.
The function parse_float_with_fallback uses Iterable[Usd.Prim] as a type hint, but Iterable is not imported. Add the import from collections.abc or typing.
from dataclasses import dataclass
-from typing import Any, Literal
+from typing import Any, Iterable, Literal📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def parse_float_with_fallback(prims: Iterable[Usd.Prim], name: str, default: float = 0.0) -> float: | |
| ret = default | |
| for prim in prims: | |
| if not prim: | |
| continue | |
| attr = get_attribute(prim, name) | |
| if not attr or not attr.HasAuthoredValue(): | |
| continue | |
| val = attr.Get() | |
| if np.isfinite(val): | |
| ret = val | |
| break | |
| return ret | |
| from dataclasses import dataclass | |
| from typing import Any, Iterable, Literal |
🧰 Tools
🪛 Ruff (0.14.8)
190-190: Undefined name Iterable
(F821)
🤖 Prompt for AI Agents
In newton/_src/utils/import_usd.py around lines 190 to 202, the type hint
Iterable[Usd.Prim] is used but Iterable is not imported; add an import for
Iterable (preferably from collections.abc import Iterable for Python 3.9+ or
from typing import Iterable for compatibility) at the top of the file, then run
a quick linter/type-check to ensure no other missing typing imports.
newton/_src/utils/import_usd.py
Outdated
| def parse_xform(prim, local=True): | ||
| xform = UsdGeom.Xform(prim) | ||
| if local: | ||
| mat = np.array(xform.GetLocalTransformation(), dtype=np.float32) | ||
| else: | ||
| mat = np.array(xform.ComputeLocalToWorldTransform(Usd.TimeCode.Default()), dtype=np.float32) | ||
| if invert_rotations: | ||
| rot = wp.quat_from_matrix(wp.mat33(mat[:3, :3].T.flatten())) | ||
| else: | ||
| rot = wp.quat_from_matrix(wp.mat33(mat[:3, :3].flatten())) | ||
| pos = mat[3, :3] | ||
| return wp.transform(pos, rot) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the import_usd.py file and search for invert_rotations occurrences
find . -name "import_usd.py" -type f 2>/dev/null
# If found, show all lines containing invert_rotations
if [ -f "newton/_src/utils/import_usd.py" ]; then
echo "=== All occurrences of invert_rotations ==="
rg -n "invert_rotations" newton/_src/utils/import_usd.py
echo -e "\n=== Context around lines 233-244 ==="
sed -n '200,260p' newton/_src/utils/import_usd.py | cat -n | sed 's/^[[:space:]]*200\t/233\t/'
fiRepository: newton-physics/newton
Length of output: 2562
🏁 Script executed:
#!/bin/bash
# Get broader context - lines 150-260 to see function definitions
sed -n '150,260p' newton/_src/utils/import_usd.py 2>/dev/null | cat -nRepository: newton-physics/newton
Length of output: 4619
🏁 Script executed:
#!/bin/bash
# Find the function that contains parse_xform
# Look for the main function definition and see if invert_rotations is a parameter
head -n 250 newton/_src/utils/import_usd.py | grep -n "^def " | tail -5
# Also search for the function signature that would contain invert_rotations as parameter
rg -n "def.*invert_rotations" newton/_src/utils/import_usd.py
# And search more broadly for where invert_rotations might be set
rg -n "invert_rotations\s*=" newton/_src/utils/import_usd.pyRepository: newton-physics/newton
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Show the first part of the file to find the parent function
head -n 100 newton/_src/utils/import_usd.py | tail -n +1 | cat -nRepository: newton-physics/newton
Length of output: 7451
🏁 Script executed:
#!/bin/bash
# Search the entire file for any definition of invert_rotations
grep -n "invert_rotations" newton/_src/utils/import_usd.py
# Also check if there's a default value or assignment anywhere
grep -n "invert" newton/_src/utils/import_usd.pyRepository: newton-physics/newton
Length of output: 130
Undefined variable invert_rotations. The parse_xform function references invert_rotations at line 239, but this variable is not defined in scope. This will cause a NameError at runtime. This variable needs to be either passed as a parameter to parse_usd or defined within the function body before use.
🧰 Tools
🪛 Ruff (0.14.8)
239-239: Undefined name invert_rotations
(F821)
|
|
||
| f_world = collider_impulses[i] / dt | ||
| max_f = 0.0 * m | ||
| if wp.length(f_world) > max_f: | ||
| f_world = f_world * max_f / wp.length(f_world) | ||
|
|
There was a problem hiding this comment.
Force clamping logic effectively zeros all forces.
max_f = 0.0 * m always equals 0.0, so the condition wp.length(f_world) > max_f is always true (unless force is exactly zero), and the force is scaled by max_f / wp.length(f_world) = 0. This zeros out all collider impulses. If this is intentional (disabled feature), add a comment; otherwise, fix the threshold.
- max_f = 0.0 * m
- if wp.length(f_world) > max_f:
- f_world = f_world * max_f / wp.length(f_world)
+ # Optionally clamp force magnitude
+ # max_f = 10.0 * m # Example: limit to 10x body weight
+ # if wp.length(f_world) > max_f:
+ # f_world = f_world * max_f / wp.length(f_world)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f_world = collider_impulses[i] / dt | |
| max_f = 0.0 * m | |
| if wp.length(f_world) > max_f: | |
| f_world = f_world * max_f / wp.length(f_world) | |
| f_world = collider_impulses[i] / dt | |
| # Optionally clamp force magnitude | |
| # max_f = 10.0 * m # Example: limit to 10x body weight | |
| # if wp.length(f_world) > max_f: | |
| # f_world = f_world * max_f / wp.length(f_world) | |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd_gtc.py around lines 376-381, the force clamping
sets max_f = 0.0 * m which is always zero and therefore scales every non-zero
f_world to zero; change this to a meaningful non-zero threshold (e.g., max_f =
SOME_MAX_FORCE * m or max_f = m * max_force_per_mass) or wire it to a
configurable parameter so real impulses are preserved, apply clamping only when
wp.length(f_world) > max_f, and if the zeroing was intentional (feature
disabled) replace the assignment with a clear comment explaining why and how to
re-enable non-zero clamping.
| results = parse_usd( | ||
| self.in_stage, | ||
| builder, | ||
| invert_rotations=True, | ||
| collapse_fixed_joints=True, | ||
| ) |
There was a problem hiding this comment.
Incorrect parse_usd argument order.
The call passes self.in_stage as the first argument and builder as the second, but based on the function signature in import_usd.py, builder should be the first positional argument and the source/stage should be the second. Also, invert_rotations is not a valid parameter.
results = parse_usd(
- self.in_stage,
builder,
+ self.in_stage,
- invert_rotations=True,
collapse_fixed_joints=True,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| results = parse_usd( | |
| self.in_stage, | |
| builder, | |
| invert_rotations=True, | |
| collapse_fixed_joints=True, | |
| ) | |
| results = parse_usd( | |
| builder, | |
| self.in_stage, | |
| collapse_fixed_joints=True, | |
| ) |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd.py around lines 106 to 111, the call to parse_usd
passes arguments in the wrong order and uses an invalid kwarg: swap the
positional args so builder is passed first and self.in_stage (the source/stage)
second, remove the unsupported invert_rotations parameter, and keep or rename
only the valid keyword args as defined in import_usd.py (e.g.,
collapse_fixed_joints=True) — adjust to call parse_usd(builder, self.in_stage,
collapse_fixed_joints=True) or the equivalent using the exact parameter names
from import_usd.py.
| orig_orientation_1 = self.model.body_q.numpy().flatten()[3:] | ||
| print(f"__init__ orig_orientation_1 = {orig_orientation_1}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug print statement.
This appears to be leftover debugging code that should be removed before merging.
- orig_orientation_1 = self.model.body_q.numpy().flatten()[3:]
- print(f"__init__ orig_orientation_1 = {orig_orientation_1}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| orig_orientation_1 = self.model.body_q.numpy().flatten()[3:] | |
| print(f"__init__ orig_orientation_1 = {orig_orientation_1}") |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd.py around lines 123 to 124, there's a leftover
debug print statement printing orig_orientation_1; remove that print statement
so no debugging output remains in production code and ensure only necessary
logic (assigning orig_orientation_1) remains.
apradhana
force-pushed
the
apradhana/sim_usd_pr
branch
from
78a6d62 to
7a4a833
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/_src/utils/sim_usd_gtc.py (1)
376-381: Force clamping effectively zeros all collider impulses.
max_f = 0.0 * malways evaluates to0.0, so all non-zero forces are scaled to zero. This appears to disable the coupled MPM force feedback entirely.If this is intentional (feature disabled), add a comment. Otherwise, set a meaningful threshold.
🧹 Nitpick comments (9)
newton/_src/usd/utils.py (1)
262-280: Inconsistent USD API usage betweenget_transform_matrixandget_transform.
get_transform_matrixusesUsdGeom.Xformable(prim)whileget_transform(line 294) usesUsdGeom.Xform(prim). While both work for Xform prims,UsdGeom.Xformableis the more general base class. Consider using the same API in both functions for consistency.-def get_transform(prim: Usd.Prim, local: bool = True) -> wp.transform: +def get_transform(prim: Usd.Prim, local: bool = True) -> wp.transform: """ Extract the transform (position and rotation) from a USD Xform prim. ... """ - xform = UsdGeom.Xform(prim) + xform = UsdGeom.Xformable(prim) if local: mat = np.array(xform.GetLocalTransformation(), dtype=np.float32)newton/_src/utils/sim_usd_gtc.py (8)
49-58: Mutable default argument in function signature.
Usd.TimeCode.Default()is evaluated at function definition time. While this is unlikely to cause issues for this specific case, it's flagged by static analysis (B008). Consider usingNoneas default and resolving inside the function.-def parse_xform(prim, time=Usd.TimeCode.Default(), return_mat=False): +def parse_xform(prim, time=None, return_mat=False): from pxr import UsdGeom + if time is None: + time = Usd.TimeCode.Default() xform = UsdGeom.Xform(prim)
459-459: Mutable default argumentwp.vec3(0.0, 0.0, 0.0)in function signature.This is flagged by static analysis (B008). Consider using
Noneas default and creating the vec3 inside the function if needed.def __init__( self, input_path, output_path, num_frames: int, integrator: Optional[IntegratorType] = None, sim_time: float = 0.0, sim_frame: int = 0, record_path: str = "", - usd_offset: wp.vec3 = wp.vec3(0.0, 0.0, 0.0), + usd_offset: wp.vec3 | None = None, use_unified_collision_pipeline: bool = True, use_coacd: bool = False, enable_timers: bool = False, ): + if usd_offset is None: + usd_offset = wp.vec3(0.0, 0.0, 0.0)
527-534: Add explicitstrict=parameter tozip().The
zip()call will silently truncate ifbuilder.shape_flagsandbuilder.shape_keyhave different lengths. Addstrict=Trueto catch length mismatches orstrict=Falseto explicitly allow truncation.shape_indices = [ i - for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key)) + for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key, strict=True)) if flags & newton.ShapeFlags.COLLIDE_SHAPES and "terrainMaincol" not in key and "ground_plane" not in key ]
1006-1015: Broad exception handling for CUDA graph fallback.Catching
Exceptionis flagged by static analysis (BLE001). While the fallback logic is reasonable, consider catching more specific exceptions (e.g.,RuntimeError) or logging the exception type for debugging.if self.graph is not None: try: wp.capture_launch(self.graph) - except Exception as e: - print(f"Error launching graph: {e}. Falling back to regular execution, which may be slower.") + except RuntimeError as e: + print(f"CUDA graph launch failed ({type(e).__name__}): {e}. Falling back to regular execution.") self.graph = None self.simulate()
1059-1059: Unused variablecid.Prefix with underscore to indicate it's intentionally unused.
- impulses, pos, cid = self.integrator.mpm_solver.collect_collider_impulses( + impulses, pos, _cid = self.integrator.mpm_solver.collect_collider_impulses( self.integrator.mpm_state_0 )
502-555: Demo-specific hardcoded logic in general utility.This section contains multiple hardcoded references to specific assets:
"BDXDroid"collision filters (lines 504-508)"chain"collision filters (lines 521-524)"terrainMaincol","ground_plane","HangingLantern"mesh handling (lines 531-555)These appear to be specific to a "BDX Lantern Demo" scene. Consider:
- Moving this demo-specific logic to a separate example script
- Making the collision filter patterns configurable via CLI arguments
- Adding a comment explaining this is demo-specific code that should be generalized
449-666: Consider refactoringSimulator.__init__for better separation of concerns.The constructor is quite long (~200 lines) and mixes:
- General simulation setup (model building, solver initialization)
- Demo-specific customizations (
_bdxlanterndemo_*methods, hardcoded asset names)- Recording/viewer setup
Consider extracting demo-specific logic into a subclass or callback system to make the base
Simulatorclass reusable for other USD scenes.
321-344: Hardcoded sand bed parameters in fallback case.When
particles_dictis empty, the code creates a hardcoded 1m × 2m × 0.5m sand bed at position[3.0, -1.0, 0.0]. This fallback appears demo-specific. Consider:
- Documenting this as example/fallback behavior
- Making the sand bed parameters configurable
- Raising an error instead if no particles are provided
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/core/spatial.py(1 hunks)newton/_src/usd/utils.py(2 hunks)newton/_src/utils/import_usd.py(6 hunks)newton/_src/utils/sim_usd_gtc.py(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/_src/core/spatial.pynewton/_src/usd/utils.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is re-exported from the spatial module via "from .spatial import transform_twist", making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/_src/core/spatial.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/usd/utils.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/sim_usd_gtc.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/sim_usd_gtc.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-12-06T19:10:24.360Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_box_on_plane.py:322-371
Timestamp: 2025-12-06T19:10:24.360Z
Learning: In Newton Kamino examples under newton/_src/solvers/kamino/examples/sim, CLI arguments parsed via argparse (including flags like --test, --device, --headless, etc.) are passed as a complete args object to newton.examples.run(example, args). The run function internally consumes these flags to control execution behavior, so flags may appear "unused" in the example script itself even though they are actively used by the framework.
Applied to files:
newton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/tests/test_examples.py:329-333
Timestamp: 2025-08-25T21:41:45.795Z
Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.
Applied to files:
newton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/utils/import_usd.py
🧬 Code graph analysis (1)
newton/_src/utils/import_usd.py (2)
newton/_src/utils/import_utils.py (1)
transform(139-140)newton/_src/usd/utils.py (2)
get_transform_matrix(262-280)get_transform(283-302)
🪛 Ruff (0.14.8)
newton/_src/utils/sim_usd_gtc.py
49-49: Do not perform function call Usd.TimeCode.Default in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
459-459: Do not perform function call wp.vec3 in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
529-529: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1010-1010: Do not catch blind exception: Exception
(BLE001)
1059-1059: Unpacked variable cid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1204-1204: Abstract raise to an inner function
(TRY301)
1204-1204: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (8)
newton/_src/core/spatial.py (2)
332-374: LGTM! Numerically stable angular velocity computation.The implementation correctly handles:
- Shortest-arc quaternion path via sign flip
- Small-angle stability with the
epsthreshold- Axis-angle extraction for larger rotations
Minor documentation nit: The docstring says
Returns: omega : ndarray shape (3,)but the function returnswp.vec3. Consider updating the docstring for consistency.
377-389: Export list correctly updated.newton/_src/usd/utils.py (1)
298-302: World transform extraction implementation looks correct.The matrix transposition and extraction of rotation/position components handle the USD-to-Warp coordinate convention properly.
newton/_src/utils/import_usd.py (4)
257-272: Matrix-based transform composition for proper scale propagation.The new approach correctly:
- Composes the incoming transform with scale into a 4x4 matrix
- Multiplies with the prim's local transform matrix
- Decomposes back to position, rotation, and scale
This properly handles hierarchical scale inheritance.
280-281: Recursive call correctly uses underscore prefix.The function name typo from a previous review (
load_visual_shapes_impl→_load_visual_shapes_impl) has been fixed.
1329-1331: Scale extraction from shape descriptor is appropriate.Using
shape_spec.localScaledirectly from the USD physics descriptor is the correct approach for collision shapes, as opposed to computing scale from the transform matrix.
1180-1180: Verify articulation transform composition logic.The computation
wp.mul(incoming_world_xform, usd.get_transform(articulation_prim, local=False))multiplies the incoming world transform (axis alignment + user xform) with the articulation prim's world transform (local=False). This may double-apply transformations from the scene hierarchy.If
incoming_world_xformrepresents only axis alignment and user offset (not the prim's ancestors), this is likely correct. Otherwise, consider usinglocal=Trueto avoid double-counting hierarchy transforms.newton/_src/utils/sim_usd_gtc.py (1)
1107-1243: CLI implementation is well-structured.The argument parsing handles:
- Required stage path
- Optional output with sensible default
- Integrator selection as enum
- USD offset parsing with error handling
The main loop is clean and straightforward.
…sed on the USD file, and outputs a USD. Signed-off-by: apradhana <[email protected]>
Signed-off-by: apradhana <[email protected]>
apradhana
force-pushed
the
apradhana/sim_usd_pr
branch
from
7a4a833 to
e8885ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
newton/_src/utils/sim_usd.py (7)
50-91: Annotate mutable class-level dicts withClassVar.The mutable class-level dictionaries should be annotated with
typing.ClassVarto satisfy Ruff RUF012 and clarify intent. TheMODEL_ATTRIBUTES_KEYSandSOLVER_ATTRIBUTES_KEYSviews are redundant since you can call.keys()directly on the dicts.
154-161: Consider usingViewerUSDclass.As noted in previous review feedback, consider using the new
ViewerUSDclass instead ofRendererUsdfor consistency with the updated API.
235-236: Makerigid_contact_marginconfigurable.The
rigid_contact_marginis hardcoded to0.1. This should be configurable via solver attributes or constructor parameter, as suggested in previous review feedback.
106-111: Incorrectparse_usdargument order and invalid parameter.Based on the
parse_usdsignature inimport_usd.py,buildershould be the first argument andsource(the stage) should be the second. Additionally,invert_rotationsis not a valid parameter.results = parse_usd( - self.in_stage, builder, - invert_rotations=True, + self.in_stage, collapse_fixed_joints=True, )
123-124: Remove debug print statement.This appears to be leftover debugging code that should be removed before merging.
- orig_orientation_1 = self.model.body_q.numpy().flatten()[3:] - print(f"__init__ orig_orientation_1 = {orig_orientation_1}")
180-183: Add validation forfpsandsim_substepsto avoid divide-by-zero.Computing
frame_dt = 1.0 / self.fpswill crash iffps == 0, and negative values don't make sense. Same applies tosim_substeps.+ if self.fps <= 0: + raise ValueError(f"Invalid fps: {self.fps} (must be > 0)") + if self.sim_substeps <= 0: + raise ValueError(f"Invalid sim_substeps: {self.sim_substeps} (must be > 0)") self.frame_dt = 1.0 / self.fps self.sim_dt = self.frame_dt / self.sim_substeps
203-227: Requesting EULER integrator silently falls back to VBD.If a user requests
euler, the current code falls through to theelsebranch and silently uses VBD. Either implement Euler or raise a clear error.- # TODO: add Euler integrator - # if self.integrator_type == IntegratorType.EULER: - # self.integrator = newton.SemiImplicitIntegrator() - if self.integrator_type == IntegratorType.XPBD: + if self.integrator_type == IntegratorType.EULER: + raise NotImplementedError( + "IntegratorType.EULER is not yet implemented. Use XPBD, VBD, or MJWARP." + ) + elif self.integrator_type == IntegratorType.XPBD: solver_args = {"iterations": scene_attributes.get("newton:xpbd:iterations", self.integrator_iterations)} self.integrator = newton.solvers.XPBDSolver(self.model, **solver_args) elif self.integrator_type == IntegratorType.MJWARP: @@ - else: - self.integrator = newton.solvers.VBDIntegrator(self.model, iterations=self.integrator_iterations) + elif self.integrator_type == IntegratorType.VBD: + self.integrator = newton.solvers.VBDIntegrator(self.model, iterations=self.integrator_iterations) + else: + raise ValueError(f"Unsupported integrator type: {self.integrator_type}")newton/_src/utils/sim_usd_gtc.py (1)
376-385: Force clamping logic effectively zeros all forces.
max_f = 0.0 * malways equals0.0, so all non-zero forces are scaled to zero. If this is intentional (disabled feature), add a comment; otherwise, fix the threshold or remove the clamping.f_world = collider_impulses[i] / dt - max_f = 0.0 * m - if wp.length(f_world) > max_f: - f_world = f_world * max_f / wp.length(f_world) + # Optionally clamp force magnitude (currently disabled) + # max_f = 10.0 * m # Example: limit to 10x body weight + # if wp.length(f_world) > max_f: + # f_world = f_world * max_f / wp.length(f_world)
🧹 Nitpick comments (5)
newton/_src/utils/sim_usd_gtc.py (5)
49-58: Avoid function call in argument default.
Usd.TimeCode.Default()is evaluated at function definition time. UseNoneas the default and resolve inside the function.-def parse_xform(prim, time=Usd.TimeCode.Default(), return_mat=False): +def parse_xform(prim, time=None, return_mat=False): from pxr import UsdGeom + if time is None: + time = Usd.TimeCode.Default() xform = UsdGeom.Xform(prim)
449-464: Avoid function call in default; remove unused parameter.
wp.vec3(0.0, 0.0, 0.0)is evaluated at function definition time. UseNoneas default.load_visual_shapesparameter is declared but never used in the method body.def __init__( self, input_path, output_path, num_frames: int, integrator: Optional[IntegratorType] = None, sim_time: float = 0.0, sim_frame: int = 0, record_path: str = "", - usd_offset: wp.vec3 = wp.vec3(0.0, 0.0, 0.0), + usd_offset: wp.vec3 | None = None, use_unified_collision_pipeline: bool = True, use_coacd: bool = False, enable_timers: bool = False, - load_visual_shapes: bool = False, ): + if usd_offset is None: + usd_offset = wp.vec3(0.0, 0.0, 0.0)
528-534: Addstrict=Truetozip()for safety.If
builder.shape_flagsandbuilder.shape_keyhave different lengths,zip()will silently truncate. Addingstrict=Trueensures length mismatch raises an error.shape_indices = [ i - for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key)) + for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key, strict=True)) if flags & newton.ShapeFlags.COLLIDE_SHAPES and "terrainMaincol" not in key and "ground_plane" not in key ]
1008-1014: Consider catching a more specific exception type.Catching bare
Exceptionis broad. If specific CUDA graph exceptions are known, catch those instead. If this is intentional for robustness, add a comment explaining why.if self.graph is not None: try: wp.capture_launch(self.graph) - except Exception as e: + except RuntimeError as e: # CUDA graph execution errors print(f"Error launching graph: {e}. Falling back to regular execution, which may be slower.") self.graph = None self.simulate()
1060-1062: Prefix unused variable with underscore.The
cidvariable is unpacked but never used.- impulses, pos, cid = self.integrator.mpm_solver.collect_collider_impulses( + impulses, pos, _cid = self.integrator.mpm_solver.collect_collider_impulses( self.integrator.mpm_state_0 )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/core/spatial.py(1 hunks)newton/_src/utils/import_usd.py(1 hunks)newton/_src/utils/sim_usd.py(1 hunks)newton/_src/utils/sim_usd_gtc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/core/spatial.py
- newton/_src/utils/import_usd.py
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-12-12T08:45:51.908Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:51.908Z
Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-09-22T21:03:18.367Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-09-02T13:59:02.072Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 701
File: CONTRIBUTING.md:7-7
Timestamp: 2025-09-02T13:59:02.072Z
Learning: The newton-physics/newton-governance repository's CONTRIBUTING.md file contains both "Legal Requirements" (line 73, anchor #legal-requirements) and "Project Members" (line 108, anchor #project-members) sections that can be properly referenced from other repositories' CONTRIBUTING.md files.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/sim_usd.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-12-06T19:10:24.360Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_box_on_plane.py:322-371
Timestamp: 2025-12-06T19:10:24.360Z
Learning: In Newton Kamino examples under newton/_src/solvers/kamino/examples/sim, CLI arguments parsed via argparse (including flags like --test, --device, --headless, etc.) are passed as a complete args object to newton.examples.run(example, args). The run function internally consumes these flags to control execution behavior, so flags may appear "unused" in the example script itself even though they are actively used by the framework.
Applied to files:
newton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/tests/test_examples.py:329-333
Timestamp: 2025-08-25T21:41:45.795Z
Learning: Newton examples use a centralized CLI argument parsing system in newton/examples/__init__.py. The create_parser() function defines common arguments like --device, --viewer, --output-path, and --num-frames, while init(parser) creates viewers based on parsed arguments. Individual example scripts don't need to define these flags themselves - they inherit them from the centralized system via the example_map and runpy execution.
Applied to files:
newton/_src/utils/sim_usd_gtc.py
🧬 Code graph analysis (1)
newton/_src/utils/sim_usd.py (5)
newton/_src/utils/import_usd.py (1)
parse_usd(38-1692)newton/_src/sim/builder.py (3)
ModelBuilder(70-6073)collapse_fixed_joints(2924-3304)joint_count(955-959)newton/_src/core/types.py (1)
Axis(69-166)newton/_src/sim/ik/ik_solver.py (1)
joint_q(362-363)newton/_src/sim/state.py (1)
clear_forces(68-79)
🪛 Ruff (0.14.8)
newton/_src/utils/sim_usd.py
50-55: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
56-62: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
63-88: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
newton/_src/utils/sim_usd_gtc.py
49-49: Do not perform function call Usd.TimeCode.Default in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
459-459: Do not perform function call wp.vec3 in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
463-463: Unused method argument: load_visual_shapes
(ARG002)
530-530: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1011-1011: Do not catch blind exception: Exception
(BLE001)
1060-1060: Unpacked variable cid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1211-1211: Abstract raise to an inner function
(TRY301)
1211-1211: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (13)
newton/_src/utils/sim_usd.py (5)
1-14: LGTM!License header is correct (Apache 2.0) and follows the Newton project standard.
36-43: LGTM!The
IntegratorTypeenum is well-defined with clear values and a proper__str__method for CLI integration.
148-152: Verify CUDA graph compatibility with collision detection.The CUDA graph captures the entire
simulate()method includingself.model.collide(). Ensure that collision detection is compatible with CUDA graph capture (i.e., no dynamic memory allocations during execution).
250-265: LGTM!The
step(),render(), andsave()methods are well-structured with appropriate CUDA graph handling and profiling.
275-318: LGTM!The CLI entry point is well-structured with appropriate argument parsing, default output path handling, and a clean simulation loop.
newton/_src/utils/sim_usd_gtc.py (8)
1-14: LGTM!License header is correct (Apache 2.0) and follows the Newton project standard.
61-74: LGTM!The
_build_solver_args_from_resolverfunction is a clean utility for dynamically building solver arguments from schema resolvers using introspection.
77-85: LGTM!The extended
IntegratorTypeenum correctly includesCOUPLED_MPMfor the MPM integration pathway.
88-180: LGTM!The
SchemaResolverclasses are well-structured with properClassVarannotations and clear attribute mappings for each integrator type.
183-216: LGTM!The
CoupledMPMIntegratorinitialization correctly separates MPM and rigid solver options, sets up both solver pipelines, and initializes the necessary state objects.
218-277: LGTM!The
stepmethod correctly implements the coupled MPM-rigid body simulation loop with proper force transfer between solvers.
412-429: LGTM!The QR-based rotation and scale extraction is a standard approach for decomposing transformation matrices.
1077-1083: LGTM!
print_time_profilercorrectly handles potentially missing profiler data with defensive.get()calls.
| @wp.kernel | ||
| def substract_body_force( |
There was a problem hiding this comment.
Typo in kernel name: substract → subtract.
The kernel name has a typo.
@wp.kernel
-def substract_body_force(
+def subtract_body_force(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @wp.kernel | |
| def substract_body_force( | |
| @wp.kernel | |
| def subtract_body_force( |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd_gtc.py around lines 388 to 389, the kernel function
name is misspelled as "substract_body_force"; rename it to "subtract_body_force"
(and update any internal references/calls and decorator usage if they rely on
the name) so the spelling is correct and consistent across the module.
| self.state_0.clear_forces() | ||
| self.viewer.apply_forces(self.state_0) | ||
| self.integrator.step(self.state_0, self.state_1, None, self.contacts, self.sim_dt) |
There was a problem hiding this comment.
simulate() unconditionally accesses self.viewer which may not exist.
self.viewer.apply_forces() is called unconditionally in simulate(), but self.viewer is only created when show_viewer=True. This will raise AttributeError if show_viewer=False.
self.state_0.clear_forces()
- self.viewer.apply_forces(self.state_0)
+ if self.show_viewer:
+ self.viewer.apply_forces(self.state_0)
self.integrator.step(self.state_0, self.state_1, None, self.contacts, self.sim_dt)🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd_gtc.py around lines 999 to 1001, simulate()
unconditionally calls self.viewer.apply_forces(...) which will raise
AttributeError when show_viewer=False because self.viewer is not created; guard
the call by either (a) checking for the attribute or non-None value (e.g., if
getattr(self, "viewer", None): self.viewer.apply_forces(self.state_0)) before
invoking apply_forces, or (b) ensure self.viewer is initialized to None in
__init__ so you can test if self.viewer is not None; apply the same guard
pattern wherever self.viewer is used.
| def save(self): | ||
| if self.usd_updater is not None: | ||
| self.usd_updater.close() | ||
| self.viewer.close() |
There was a problem hiding this comment.
Potential AttributeError if viewer is not initialized.
self.viewer is only created when self.show_viewer is True. If show_viewer is False, calling save() will raise AttributeError.
def save(self):
if self.usd_updater is not None:
self.usd_updater.close()
- self.viewer.close()
+ if self.show_viewer:
+ self.viewer.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def save(self): | |
| if self.usd_updater is not None: | |
| self.usd_updater.close() | |
| self.viewer.close() | |
| def save(self): | |
| if self.usd_updater is not None: | |
| self.usd_updater.close() | |
| if self.show_viewer: | |
| self.viewer.close() |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd_gtc.py around lines 1071 to 1074, save()
unconditionally calls self.viewer.close() but self.viewer is only created when
show_viewer is True, causing an AttributeError; update save() to defensively
check for the attributes (e.g., if hasattr(self, "usd_updater") and
self.usd_updater is not None: self.usd_updater.close(); if hasattr(self,
"viewer") and self.viewer is not None: self.viewer.close()) so both resources
are closed only when present.
| simulator = Simulator( | ||
| input_path=args.stage_path, | ||
| output_path=args.output, | ||
| integrator=args.integrator, | ||
| num_frames=args.num_frames, | ||
| sim_time=args.sim_time, | ||
| sim_frame=args.sim_frame, | ||
| record_path=args.record, | ||
| usd_offset=usd_offset, | ||
| use_unified_collision_pipeline=args.use_unified_collision_pipeline, | ||
| use_coacd=args.use_coacd, | ||
| ) |
There was a problem hiding this comment.
CLI argument enable_timers is parsed but not passed to Simulator.
The --enable_timers argument is parsed but not included in the Simulator constructor call.
simulator = Simulator(
input_path=args.stage_path,
output_path=args.output,
integrator=args.integrator,
num_frames=args.num_frames,
sim_time=args.sim_time,
sim_frame=args.sim_frame,
record_path=args.record,
usd_offset=usd_offset,
use_unified_collision_pipeline=args.use_unified_collision_pipeline,
use_coacd=args.use_coacd,
+ enable_timers=args.enable_timers,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| simulator = Simulator( | |
| input_path=args.stage_path, | |
| output_path=args.output, | |
| integrator=args.integrator, | |
| num_frames=args.num_frames, | |
| sim_time=args.sim_time, | |
| sim_frame=args.sim_frame, | |
| record_path=args.record, | |
| usd_offset=usd_offset, | |
| use_unified_collision_pipeline=args.use_unified_collision_pipeline, | |
| use_coacd=args.use_coacd, | |
| ) | |
| simulator = Simulator( | |
| input_path=args.stage_path, | |
| output_path=args.output, | |
| integrator=args.integrator, | |
| num_frames=args.num_frames, | |
| sim_time=args.sim_time, | |
| sim_frame=args.sim_frame, | |
| record_path=args.record, | |
| usd_offset=usd_offset, | |
| use_unified_collision_pipeline=args.use_unified_collision_pipeline, | |
| use_coacd=args.use_coacd, | |
| enable_timers=args.enable_timers, | |
| ) |
🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd_gtc.py around lines 1227 to 1238, the CLI flag
enable_timers is parsed but not forwarded into the Simulator constructor; add
the parameter enable_timers=args.enable_timers to the Simulator(...) call
(keeping argument ordering consistent or appended at end) so the Simulator
receives the parsed flag.
| def print_time_profiler(simulator): | ||
| frame_times = simulator.profiler["step"] | ||
| render_times = simulator.profiler["render"] | ||
| print("\nAverage frame sim time: {:.2f} ms".format(sum(frame_times) / len(frame_times))) | ||
| print("\nAverage frame render time: {:.2f} ms".format(sum(render_times) / len(render_times))) |
There was a problem hiding this comment.
Add defensive checks for missing or empty profiler data.
print_time_profiler will raise KeyError if "step" or "render" keys don't exist, and ZeroDivisionError if the lists are empty.
def print_time_profiler(simulator):
- frame_times = simulator.profiler["step"]
- render_times = simulator.profiler["render"]
- print("\nAverage frame sim time: {:.2f} ms".format(sum(frame_times) / len(frame_times)))
- print("\nAverage frame render time: {:.2f} ms".format(sum(render_times) / len(render_times)))
+ frame_times = simulator.profiler.get("step")
+ render_times = simulator.profiler.get("render")
+ if frame_times:
+ print("\nAverage frame sim time: {:.2f} ms".format(sum(frame_times) / len(frame_times)))
+ if render_times:
+ print("\nAverage frame render time: {:.2f} ms".format(sum(render_times) / len(render_times)))🤖 Prompt for AI Agents
In newton/_src/utils/sim_usd.py around lines 268 to 272, print_time_profiler
assumes simulator.profiler contains non-empty "step" and "render" lists; add
defensive checks to avoid KeyError and ZeroDivisionError by retrieving lists
with simulator.profiler.get("step", []) and .get("render", []) (or check "if
'step' in profiler" / "if 'render' in profiler"), verify each is a non-empty
sequence before computing the average, and if missing or empty print a clear
fallback message like "N/A" or "no samples" instead of performing the division.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
newton/_src/utils/update_usd.py (1)
380-404: Remove duplicate method definition.This is a duplicate of the method defined at line 93. The second definition will override the first in Python. See the earlier comment at lines 93-119 for details.
🧹 Nitpick comments (3)
newton/_src/utils/update_usd.py (3)
51-62: Simplify validation logic.The validation at lines 51-57 checks if
source_stageexists before validating related parameters, but lines 61-62 immediately raise an error ifsource_stageis not set. This meanssource_stageis always required, making the conditional validation redundant.Apply this diff to simplify:
- if source_stage: - if path_body_map is None: - raise ValueError("path_body_map must be set if you are providing a source_stage") - if path_body_relative_transform is None: - raise ValueError("path_body_relative_transform must be set if you are providing a source_stage") - if builder_results is None: - raise ValueError("builder_results must be set if you are providing a source_stage") - + if source_stage is None: + raise ValueError("source_stage must be set") + if path_body_map is None: + raise ValueError("path_body_map must be set when providing a source_stage") + if path_body_relative_transform is None: + raise ValueError("path_body_relative_transform must be set when providing a source_stage") + if builder_results is None: + raise ValueError("builder_results must be set when providing a source_stage") + self.source_stage = source_stage self.global_offset: wp.vec3 = global_offset if global_offset is not None else wp.vec3(0.0, 0.0, 0.0) - if not self.source_stage: - raise ValueError("source_stage must be set") - else: - self.stage = self._create_output_stage(self.source_stage, stage) - self.fps = fps - self.scaling = scaling - self.up_axis = up_axis - self.path_body_map = path_body_map - self.path_body_relative_transform = path_body_relative_transform - self.builder_results = builder_results - self._prepare_output_stage() - self._precompute_parents_xform_inverses() + + self.stage = self._create_output_stage(self.source_stage, stage) + self.fps = fps + self.scaling = scaling + self.up_axis = up_axis + self.path_body_map = path_body_map + self.path_body_relative_transform = path_body_relative_transform + self.builder_results = builder_results + self._prepare_output_stage() + self._precompute_parents_xform_inverses()
421-436: Consider renaming shadowed variable for clarity.Line 425 reuses the
source_stageparameter name when opening it as a USD stage, which shadows the parameter. While this works correctly, it could be clearer.Apply this diff:
def _create_output_stage(self, source_stage: str | Usd.Stage, output_stage: str | Usd.Stage) -> Usd.Stage: from pxr import Usd # noqa: PLC0415 if isinstance(output_stage, str): - source_stage = Usd.Stage.Open(source_stage, Usd.Stage.LoadAll) - flattened = source_stage.Flatten() + opened_source = Usd.Stage.Open(source_stage, Usd.Stage.LoadAll) + flattened = opened_source.Flatten() stage = Usd.Stage.Open(flattened.identifier) exported = stage.ExportToString()
438-451: Make exception handling more specific.Catching broad
Exceptionon line 448 can mask unexpected errors. Consider catching specific USD-related exceptions.Apply this diff:
def close(self): """ Finalize and save the USD stage. This should be called when all logging is complete to ensure the USD file is written. """ try: self.stage.Save() self.stage = None print("USD stage saved successfully") - except Exception as e: + except (OSError, RuntimeError) as e: print("Failed to save USD stage:", e) return FalseNote: You may need to verify the specific exception types raised by USD's
Save()method.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/utils/update_usd.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/update_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/update_usd.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
newton/_src/utils/update_usd.py
🪛 Ruff (0.14.8)
newton/_src/utils/update_usd.py
25-25: Undefined name Usd
(F821)
26-26: Undefined name Usd
(F821)
29-29: Undefined name AxisType
(F821)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
139-139: Undefined name Usd
(F821)
139-139: Undefined name Usd
(F821)
145-145: Undefined name Usd
(F821)
145-145: Undefined name Usd
(F821)
147-147: Undefined name Usd
(F821)
150-150: Undefined name Usd
(F821)
153-153: Undefined name Usd
(F821)
156-156: Prefer TypeError exception for invalid type
(TRY004)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
191-191: Avoid specifying long messages outside the exception class
(TRY003)
193-193: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Avoid specifying long messages outside the exception class
(TRY003)
212-212: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
215-215: Avoid specifying long messages outside the exception class
(TRY003)
306-306: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
328-328: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
354-354: Undefined name Usd
(F821)
355-355: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
385-385: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
388-388: Avoid specifying long messages outside the exception class
(TRY003)
407-407: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
410-410: Avoid specifying long messages outside the exception class
(TRY003)
421-421: Undefined name Usd
(F821)
421-421: Undefined name Usd
(F821)
421-421: Undefined name Usd
(F821)
422-422: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
436-436: Prefer TypeError exception for invalid type
(TRY004)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
448-448: Do not catch blind exception: Exception
(BLE001)
453-453: Undefined name Usd
(F821)
453-453: Undefined name Usd
(F821)
460-460: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (8)
newton/_src/utils/update_usd.py (8)
74-92: LGTM!The stage preparation logic correctly sets timing parameters and converts transform stacks to the canonical TQS (translate-quaternion-scale) format for all mapped prims.
120-137: LGTM!Frame management logic is correct. The
begin_framemethod properly converts simulation time to frame indices and updates the stage's end time code.
181-204: LGTM!The body mapping configuration correctly validates required parameters and prepares the stage for updates.
205-237: LGTM!The update logic correctly handles transform composition, relative transforms, parent-space conversion, and efficiently batches USD updates using
Sdf.ChangeBlock.
238-271: LGTM!The point instancer rendering correctly creates the necessary USD structures and handles time-sampled position, rotation, and scale data.
272-326: LGTM!The parent inverse transform logic is well-documented and correctly handles both dynamic (body) parents and static parents. The mathematical derivation in the docstring provides excellent context.
327-351: LGTM!The transform update logic correctly detects and preserves USD precision (float vs. double) and properly sets time-sampled transform values.
452-510: LGTM!The
_xform_to_tqsmethod is comprehensive and well-implemented. It correctly:
- Detects precision from existing transform ops
- Extracts translation, rotation, and scale from the local transformation matrix
- Handles both float and double precision
- Properly clears and rebuilds the transform stack in canonical TQS order
| from __future__ import annotations | ||
|
|
||
| import warp as wp | ||
|
|
There was a problem hiding this comment.
Add missing imports for type hints.
Type hints reference Usd.Stage and AxisType without importing them, causing undefined name errors. While USD modules are lazily loaded in function bodies (per project patterns), type hints at the module level require the types to be available.
Apply this diff to add the missing imports:
from __future__ import annotations
+from typing import TYPE_CHECKING
+
import warp as wp
+
+if TYPE_CHECKING:
+ from pxr import Usd
+ from newton import AxisTypeAlternatively, use string literals for forward references:
from __future__ import annotations
import warp as wpAnd update type hints to use strings like "Usd.Stage" and "AxisType".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from __future__ import annotations | |
| import warp as wp | |
| from __future__ import annotations | |
| from typing import TYPE_CHECKING | |
| import warp as wp | |
| if TYPE_CHECKING: | |
| from pxr import Usd | |
| from newton import AxisType |
🤖 Prompt for AI Agents
In newton/_src/utils/update_usd.py around lines 17 to 20, the module-level type
hints reference Usd.Stage and AxisType but those names are not imported, causing
NameError during type checking; either add the missing imports (e.g., import
from pxr import Usd and import or define AxisType) at the top of the file
guarded by TYPE_CHECKING if needed, or convert the annotations to
forward-reference strings (e.g., "Usd.Stage" and "AxisType") so the types are
not required at import time; ensure consistency with project lazy-import
patterns by using typing.TYPE_CHECKING to import actual types only for type
checkers if you choose imports.
| """ | ||
| Construct a UpdateUsd object. | ||
| Args: | ||
| model (newton.Model): The Newton physics model to render. | ||
| stage (str | Usd.Stage): The USD stage to render to. This is the output stage. | ||
| source_stage (str | Usd.Stage, optional): The USD stage to use as a source for the output stage. | ||
| scaling (float, optional): Scaling factor for the rendered objects. Defaults to 1.0. | ||
| fps (int, optional): Frames per second for the animation. Defaults to 60. | ||
| up_axis (newton.AxisType, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None. | ||
| show_joints (bool, optional): Whether to show joint visualizations. Defaults to False. | ||
| path_body_map (dict, optional): A dictionary mapping prim paths to body IDs. | ||
| path_body_relative_transform (dict, optional): A dictionary mapping prim paths to relative transformations. | ||
| builder_results (dict, optional): A dictionary containing builder results. | ||
| global_offset (wp.vec3, optional): A global offset to apply to the stage. | ||
| """ |
There was a problem hiding this comment.
Fix docstring to match actual parameters.
The docstring references parameters that don't exist (model, show_joints) in the actual method signature.
Apply this diff:
"""
Construct a UpdateUsd object.
Args:
- model (newton.Model): The Newton physics model to render.
stage (str | Usd.Stage): The USD stage to render to. This is the output stage.
source_stage (str | Usd.Stage, optional): The USD stage to use as a source for the output stage.
scaling (float, optional): Scaling factor for the rendered objects. Defaults to 1.0.
fps (int, optional): Frames per second for the animation. Defaults to 60.
up_axis (newton.AxisType, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None.
- show_joints (bool, optional): Whether to show joint visualizations. Defaults to False.
path_body_map (dict, optional): A dictionary mapping prim paths to body IDs.
path_body_relative_transform (dict, optional): A dictionary mapping prim paths to relative transformations.
builder_results (dict, optional): A dictionary containing builder results.
global_offset (wp.vec3, optional): A global offset to apply to the stage.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """ | |
| Construct a UpdateUsd object. | |
| Args: | |
| model (newton.Model): The Newton physics model to render. | |
| stage (str | Usd.Stage): The USD stage to render to. This is the output stage. | |
| source_stage (str | Usd.Stage, optional): The USD stage to use as a source for the output stage. | |
| scaling (float, optional): Scaling factor for the rendered objects. Defaults to 1.0. | |
| fps (int, optional): Frames per second for the animation. Defaults to 60. | |
| up_axis (newton.AxisType, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None. | |
| show_joints (bool, optional): Whether to show joint visualizations. Defaults to False. | |
| path_body_map (dict, optional): A dictionary mapping prim paths to body IDs. | |
| path_body_relative_transform (dict, optional): A dictionary mapping prim paths to relative transformations. | |
| builder_results (dict, optional): A dictionary containing builder results. | |
| global_offset (wp.vec3, optional): A global offset to apply to the stage. | |
| """ | |
| """ | |
| Construct a UpdateUsd object. | |
| Args: | |
| stage (str | Usd.Stage): The USD stage to render to. This is the output stage. | |
| source_stage (str | Usd.Stage, optional): The USD stage to use as a source for the output stage. | |
| scaling (float, optional): Scaling factor for the rendered objects. Defaults to 1.0. | |
| fps (int, optional): Frames per second for the animation. Defaults to 60. | |
| up_axis (newton.AxisType, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None. | |
| path_body_map (dict, optional): A dictionary mapping prim paths to body IDs. | |
| path_body_relative_transform (dict, optional): A dictionary mapping prim paths to relative transformations. | |
| builder_results (dict, optional): A dictionary containing builder results. | |
| global_offset (wp.vec3, optional): A global offset to apply to the stage. | |
| """ |
🤖 Prompt for AI Agents
In newton/_src/utils/update_usd.py around lines 35 to 50, the docstring lists
parameters that don't exist in the function signature (e.g., model and
show_joints) and therefore must be updated to match the actual parameters; edit
the docstring to remove nonexistent parameters and instead document the real
signature names and their meanings (ensure parameters, types, defaults, and any
optional ones correspond exactly to the function's parameter list such as stage,
source_stage, scaling, fps, up_axis, path_body_map,
path_body_relative_transform, builder_results, global_offset, etc.), keep
descriptions concise, and preserve formatting consistent with the rest of the
file.
| def _precompute_parents_xform_inverses(self): | ||
| """ | ||
| Convention: prefix c is for **current** prim. | ||
| Prefix p is for **parent** prim. | ||
| """ | ||
| from pxr import Sdf, Usd # noqa: PLC0415 | ||
|
|
||
| if self.path_body_map is None: | ||
| raise ValueError("self.path_body_map must be set before calling _precompute_parents_xform_inverses") | ||
|
|
||
| self.parent_translates = {} | ||
| self.parent_inv_Rs = {} | ||
| self.parent_inv_Rns = {} | ||
|
|
||
| with wp.ScopedTimer("prep_parents_xform"): | ||
| time = Usd.TimeCode.Default() | ||
| for prim_path in self.path_body_map.keys(): | ||
| current_prim = self.stage.GetPrimAtPath(Sdf.Path(prim_path)) | ||
| parent_path = str(current_prim.GetParent().GetPath()) | ||
|
|
||
| if parent_path not in self.parent_translates: | ||
| ( | ||
| self.parent_translates[parent_path], | ||
| self.parent_inv_Rs[parent_path], | ||
| self.parent_inv_Rns[parent_path], | ||
| ) = self._compute_parents_inverses(prim_path, time) | ||
|
|
There was a problem hiding this comment.
Remove duplicate method definition.
This method _precompute_parents_xform_inverses is defined twice (here at line 93 and again at line 380). In Python, the second definition will override the first, making this code unreachable.
The version at line 93 includes a ScopedTimer for performance profiling, while the version at line 380 does not. Determine which version should be kept and remove the other.
If the profiling is desired, keep this version and remove lines 380-404. Otherwise, remove lines 93-119.
🧰 Tools
🪛 Ruff (0.14.8)
98-98: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
newton/_src/utils/update_usd.py around lines 93-119: this method
_precompute_parents_xform_inverses is duplicated later (around lines 380-404) so
one definition should be removed; if you want performance profiling keep the
ScopedTimer version here (lines 93-119) and delete the duplicate at lines
380-404, otherwise remove lines 93-119 and keep the later non-profiling
implementation — ensure only one definition remains and run tests to confirm no
regressions.
| def attach_source_stage(self, source_stage: str | Usd.Stage, output_stage: str | Usd.Stage): | ||
| """ | ||
| Attach a USD stage created from a source stage (flattened copy) as the viewer's stage. | ||
| """ | ||
| if isinstance(output_stage, str): | ||
| stage = Usd.Stage.Open(source_stage, Usd.Stage.LoadAll) | ||
| flattened = stage.Flatten() | ||
| temp_stage = Usd.Stage.Open(flattened.identifier) | ||
| exported = temp_stage.ExportToString() | ||
|
|
||
| new_stage = Usd.Stage.CreateNew(output_stage) | ||
| new_stage.GetRootLayer().ImportFromString(exported) | ||
| self.stage = new_stage | ||
| elif isinstance(output_stage, Usd.Stage): | ||
| self.stage = output_stage | ||
| else: | ||
| raise ValueError("output_stage must be a string or a Usd.Stage") | ||
|
|
||
| # carry over fps and basic settings | ||
| self.stage.SetFramesPerSecond(self.fps) | ||
| self.stage.SetStartTimeCode(0) | ||
| return self.stage | ||
|
|
There was a problem hiding this comment.
Add type checking for source_stage parameter.
Line 145 attempts to open source_stage as a file path without first checking if it's already a Usd.Stage object. This will cause a runtime error if a stage object is passed.
Apply this diff:
def attach_source_stage(self, source_stage: str | Usd.Stage, output_stage: str | Usd.Stage):
"""
Attach a USD stage created from a source stage (flattened copy) as the viewer's stage.
"""
if isinstance(output_stage, str):
- stage = Usd.Stage.Open(source_stage, Usd.Stage.LoadAll)
+ if isinstance(source_stage, str):
+ stage = Usd.Stage.Open(source_stage, Usd.Stage.LoadAll)
+ else:
+ stage = source_stage
flattened = stage.Flatten()
temp_stage = Usd.Stage.Open(flattened.identifier)
exported = temp_stage.ExportToString()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def attach_source_stage(self, source_stage: str | Usd.Stage, output_stage: str | Usd.Stage): | |
| """ | |
| Attach a USD stage created from a source stage (flattened copy) as the viewer's stage. | |
| """ | |
| if isinstance(output_stage, str): | |
| stage = Usd.Stage.Open(source_stage, Usd.Stage.LoadAll) | |
| flattened = stage.Flatten() | |
| temp_stage = Usd.Stage.Open(flattened.identifier) | |
| exported = temp_stage.ExportToString() | |
| new_stage = Usd.Stage.CreateNew(output_stage) | |
| new_stage.GetRootLayer().ImportFromString(exported) | |
| self.stage = new_stage | |
| elif isinstance(output_stage, Usd.Stage): | |
| self.stage = output_stage | |
| else: | |
| raise ValueError("output_stage must be a string or a Usd.Stage") | |
| # carry over fps and basic settings | |
| self.stage.SetFramesPerSecond(self.fps) | |
| self.stage.SetStartTimeCode(0) | |
| return self.stage | |
| def attach_source_stage(self, source_stage: str | Usd.Stage, output_stage: str | Usd.Stage): | |
| """ | |
| Attach a USD stage created from a source stage (flattened copy) as the viewer's stage. | |
| """ | |
| if isinstance(output_stage, str): | |
| if isinstance(source_stage, str): | |
| stage = Usd.Stage.Open(source_stage, Usd.Stage.LoadAll) | |
| else: | |
| stage = source_stage | |
| flattened = stage.Flatten() | |
| temp_stage = Usd.Stage.Open(flattened.identifier) | |
| exported = temp_stage.ExportToString() | |
| new_stage = Usd.Stage.CreateNew(output_stage) | |
| new_stage.GetRootLayer().ImportFromString(exported) | |
| self.stage = new_stage | |
| elif isinstance(output_stage, Usd.Stage): | |
| self.stage = output_stage | |
| else: | |
| raise ValueError("output_stage must be a string or a Usd.Stage") | |
| # carry over fps and basic settings | |
| self.stage.SetFramesPerSecond(self.fps) | |
| self.stage.SetStartTimeCode(0) | |
| return self.stage |
🧰 Tools
🪛 Ruff (0.14.8)
139-139: Undefined name Usd
(F821)
139-139: Undefined name Usd
(F821)
145-145: Undefined name Usd
(F821)
145-145: Undefined name Usd
(F821)
147-147: Undefined name Usd
(F821)
150-150: Undefined name Usd
(F821)
153-153: Undefined name Usd
(F821)
156-156: Prefer TypeError exception for invalid type
(TRY004)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In newton/_src/utils/update_usd.py around lines 139 to 162, add type checking
for the source_stage parameter so we don't call Usd.Stage.Open on a Usd.Stage
object: if source_stage is a str, open it with Usd.Stage.Open(...) and proceed
to Flatten()/ExportToString(); if source_stage is a Usd.Stage, use it directly
(call Flatten()/ExportToString() on that stage); otherwise raise
ValueError("source_stage must be a string or a Usd.Stage"). Keep the existing
output_stage handling and the fps/timecode carry-over logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
newton/_src/utils/sim_usd_gtc.py (5)
379-381: Force clamping logic zeros all forces (already flagged).This issue was previously identified.
max_f = 0.0 * malways equals0.0, causing all forces to be zeroed. If this is intentional (disabled feature), please add a clarifying comment.
389-390: Typo in kernel name:substract→subtract(already flagged).This typo was previously identified and should be corrected along with the call site at line 260.
1033-1035:self.viewer.apply_forces()called without checkingshow_viewer(already flagged).This issue was previously identified.
self.vieweronly exists whenshow_viewer=True, causingAttributeErrorotherwise.
1132-1135:self.viewer.close()called without checkingshow_viewer(already flagged).This issue was previously identified. Add a guard:
if self.show_viewer: self.viewer.close().
1288-1300: CLI argumentsenable_timersandload_visual_shapesnot passed to Simulator (partially flagged).The
enable_timersissue was previously identified. Additionally,load_visual_shapes(line 1259-1264) is also parsed but not passed. Note thatload_visual_shapesis currently unused in the Simulator constructor anyway (see earlier comment).use_unified_collision_pipeline=args.use_unified_collision_pipeline, use_coacd=args.use_coacd, + enable_timers=args.enable_timers, )
🧹 Nitpick comments (6)
newton/_src/utils/sim_usd_gtc.py (6)
50-59: Avoid function call in default argument and remove redundant import.
Usd.TimeCode.Default()in the default argument is evaluated once at function definition time (Ruff B008).UsdGeomis already imported at module level (line 36), so the local import is redundant.Suggested fix
-def parse_xform(prim, time=Usd.TimeCode.Default(), return_mat=False): - from pxr import UsdGeom - +def parse_xform(prim, time=None, return_mat=False): + if time is None: + time = Usd.TimeCode.Default() xform = UsdGeom.Xform(prim)
461-466: Default argument issue and unused parameter.
- Line 461:
wp.vec3(0.0, 0.0, 0.0)as default is evaluated once at definition time (Ruff B008). UseNoneand create inside the function.- Line 465:
load_visual_shapesparameter is never used (Ruff ARG002).Suggested fix
- usd_offset: wp.vec3 = wp.vec3(0.0, 0.0, 0.0), + usd_offset: wp.vec3 | None = None, use_unified_collision_pipeline: bool = True, use_coacd: bool = False, enable_timers: bool = False, - load_visual_shapes: bool = False, ): + if usd_offset is None: + usd_offset = wp.vec3(0.0, 0.0, 0.0)
530-536: Consider addingstrict=Truetozip()for safety.Using
zip(..., strict=True)would catch potential length mismatches betweenbuilder.shape_flagsandbuilder.shape_keyearly.- for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key)) + for i, (flags, key) in enumerate(zip(builder.shape_flags, builder.shape_key, strict=True))
1043-1048: Consider catching more specific exceptions.Catching broad
Exception(Ruff BLE001) can mask unexpected errors. For CUDA graph launch failures, consider catchingwp.WarpExceptionor similar if available, or at minimum log the exception type for debugging.
1094-1096: Unused variablecid.Prefix with underscore to indicate intentionally unused:
- impulses, pos, cid = self.integrator.mpm_solver.collect_collider_impulses( + impulses, pos, _cid = self.integrator.mpm_solver.collect_collider_impulses(
779-789: Else branch catches all unmatched integrator types, not just VBD.The comment says
# VBDbut this else branch will handle anyIntegratorTypenot explicitly matched (e.g., ifEULERis added but not implemented). Consider usingelif self.integrator_type == IntegratorType.VBD:with an explicit else that raises an error for unsupported types.Suggested fix
- else: # VBD + elif self.integrator_type == IntegratorType.VBD: res = SchemaResolverVBD() ... self.integrator = newton.solvers.VBDIntegrator(self.model, **solver_args) + else: + raise ValueError(f"Unsupported integrator type: {self.integrator_type}")
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/sim/collide_unified.py(1 hunks)newton/_src/solvers/solver.py(1 hunks)newton/_src/utils/sim_usd_gtc.py(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
📚 Learning: 2025-12-12T17:45:26.847Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/geometry/primitives.py:0-0
Timestamp: 2025-12-12T17:45:26.847Z
Learning: In newton/_src/solvers/kamino/geometry/primitives.py, the narrow-phase kernel `_primitive_narrowphase` does not need to handle symmetric shape-pair orderings (e.g., both SPHERE-BOX and BOX-SPHERE) because `kamino.core.builder.ModelBuilder` and `kamino.geometry.primitive.broadphase` guarantee that explicit candidate geometry pairs are always defined with GIDs in ascending order.
Applied to files:
newton/_src/sim/collide_unified.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/collide_unified.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/_src/sim/collide_unified.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/sim/collide_unified.pynewton/_src/utils/sim_usd_gtc.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.
Applied to files:
newton/_src/sim/collide_unified.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/_src/sim/collide_unified.py
📚 Learning: 2025-09-10T10:09:05.676Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 747
File: newton/_src/viewer/kernels.py:0-0
Timestamp: 2025-09-10T10:09:05.676Z
Learning: In Newton physics picking implementation (newton/_src/viewer/kernels.py), damping is intentionally applied using center of mass velocity (vel_com) rather than velocity at the pick point. This is a deliberate design choice for stability improvements.
Applied to files:
newton/_src/solvers/solver.py
📚 Learning: 2025-09-22T21:03:39.624Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Applied to files:
newton/_src/solvers/solver.py
📚 Learning: 2025-09-22T21:03:18.367Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
Applied to files:
newton/_src/solvers/solver.py
📚 Learning: 2025-11-28T11:12:40.805Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1146
File: newton/examples/basic/example_basic_joints.py:206-213
Timestamp: 2025-11-28T11:12:40.805Z
Learning: In Newton's standard solvers (XPBD, SemiImplicit, SolverMuJoCo), spatial vectors in State.body_qd use the ordering (linear, angular): wp.spatial_top(qd) returns linear velocity and wp.spatial_bottom(qd) returns angular velocity. This is the opposite of typical Modern Robotics spatial twist notation where (angular, linear) is used.
Applied to files:
newton/_src/solvers/solver.py
📚 Learning: 2025-12-01T16:21:36.581Z
Learnt from: jvonmuralt
Repo: newton-physics/newton PR: 1160
File: newton/_src/sim/articulation.py:0-0
Timestamp: 2025-12-01T16:21:36.581Z
Learning: In Newton's Featherstone solver, during forward kinematics for FREE/DISTANCE joints, the computed spatial velocity v_wc follows Featherstone's spatial twist convention where the linear component (spatial_top) represents the velocity at the world origin (0,0,0), not at the joint anchor or COM. To convert to State.body_qd (which stores COM velocity), use v_com = v_origin + ω × x_com, where x_com is the absolute world position of the COM.
Applied to files:
newton/_src/solvers/solver.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/solvers/solver.py
📚 Learning: 2025-12-12T15:23:30.014Z
Learnt from: vastsoun
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_boxes_fourbar.py:149-163
Timestamp: 2025-12-12T15:23:30.014Z
Learning: In Kamino simulation examples under newton/_src/solvers/kamino/examples/sim/, the pattern where logging=True and use_cuda_graph=True creates a mismatch (SimulationLogger constructed but settings.compute_metrics and logger.log() calls guarded by `if not self.use_cuda_graph`) should not be flagged. These examples will be refactored in a future commit, and this pattern is intentionally deferred.
Applied to files:
newton/_src/utils/sim_usd_gtc.py
🪛 Ruff (0.14.8)
newton/_src/utils/sim_usd_gtc.py
50-50: Do not perform function call Usd.TimeCode.Default in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
461-461: Do not perform function call wp.vec3 in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
465-465: Unused method argument: load_visual_shapes
(ARG002)
532-532: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1045-1045: Do not catch blind exception: Exception
(BLE001)
1094-1094: Unpacked variable cid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1129-1129: Do not catch blind exception: Exception
(BLE001)
1272-1272: Abstract raise to an inner function
(TRY301)
1272-1272: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (2)
newton/_src/utils/sim_usd_gtc.py (2)
219-278: LGTM on CoupledMPMIntegrator step logic.The lazy initialization pattern and state management look correct. The MPM/rigid solver coupling with impulse collection and state swapping is well-structured.
831-876: LGTM on collider animation processing.The kernel closure pattern with
module="unique"and compile-time constants viawp.static()is appropriate. The velocity computation from frame-to-frame transforms looks correct.
| if broad_phase_mode == BroadPhaseMode.EXPLICIT: | ||
| self.shape_pairs_max = min(self.shape_pairs_max, len(shape_pairs_filtered)) |
There was a problem hiding this comment.
Redundant assignment: shape_pairs_max is overwritten at line 362.
This capping logic at lines 332-333 is immediately overwritten in the else branch at line 362:
self.shape_pairs_max = len(shape_pairs_filtered)Additionally, if shape_pairs_filtered is None (which is valid for NXN/SAP modes), line 333 will raise a TypeError when calling len(None).
Consider removing this early capping since the EXPLICIT mode branch handles it correctly:
Suggested fix
self.shape_pairs_max = (shape_count * (shape_count - 1)) // 2
- if broad_phase_mode == BroadPhaseMode.EXPLICIT:
- self.shape_pairs_max = min(self.shape_pairs_max, len(shape_pairs_filtered))
# Initialize broad phase🤖 Prompt for AI Agents
In newton/_src/sim/collide_unified.py around lines 332-333 (overwritten later at
line 362), remove the early capping assignment "self.shape_pairs_max =
min(self.shape_pairs_max, len(shape_pairs_filtered))" because it is redundant
and can raise TypeError when shape_pairs_filtered is None; rely on the
EXPLICIT/else branch at line 362 to set shape_pairs_max appropriately, or if you
must keep a guard here, check "if shape_pairs_filtered is not None:" before
computing len().
| # angular damping | ||
| w1 *= 1.0 - angular_damping * dt | ||
| v1 *= 1.0 - angular_damping * dt |
There was a problem hiding this comment.
Parameter naming mismatch: angular_damping now damps linear velocity.
The parameter is named angular_damping but line 99 applies it to linear velocity v1. This is semantically misleading and represents a behavioral change—existing simulations using angular_damping > 0 will now experience linear velocity damping as well.
Consider one of these approaches:
- Rename the parameter to
damping(breaking API change) - Add a separate
linear_dampingparameter - If this is intentional, update the docstring at lines 195-196 to clarify the behavior
- angular_damping (float, optional): The angular damping factor.
+ angular_damping (float, optional): Damping factor applied to both angular and linear velocities.Please confirm if applying angular damping to linear velocity is intentional behavior.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In newton/_src/solvers/solver.py around lines 97 to 99, the variable named
angular_damping is being applied to both angular (w1) and linear (v1) velocities
which is semantically incorrect; decide which approach you want and implement
it: either (A) rename the parameter to damping throughout the public API and
docs (breaking change) so it clearly applies to both, (B) introduce a separate
linear_damping parameter, apply angular_damping only to w1 and linear_damping to
v1, and update all call sites and defaults, or (C) if applying angular_damping
to linear velocity was intentional, update the docstring at lines 195-196 (and
any public docs) to explicitly state that angular_damping affects both angular
and linear velocities and add a unit test that asserts this behavior; also
confirm which option you choose before making the change.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
newton/_src/utils/add_bdx_physics_schema.py (5)
47-56: Consider usingstage.GetPrimAtPathdirectly for clarity.The
worldprim is fetched but thenGetPrimAtPathis called with absolute paths like"/World/{lantern}". While this works (absolute paths resolve against the stage), it's clearer to usestage.GetPrimAtPath()directly since you're not using relative paths.🔎 Proposed simplification
- world = stage.GetPrimAtPath("/World") - for lantern, chain in chains.items(): - lantern_prim = world.GetPrimAtPath(f"/World/{lantern}") + lantern_prim = stage.GetPrimAtPath(f"/World/{lantern}") UsdPhysics.RigidBodyAPI.Apply(lantern_prim) apply_collision_api(lantern_prim) - chain_geo_prim = world.GetPrimAtPath(f"/World/{chain}/geo") - chain_joints_prim = world.GetPrimAtPath(f"/World/{chain}/geo/joints") + chain_geo_prim = stage.GetPrimAtPath(f"/World/{chain}/geo") + chain_joints_prim = stage.GetPrimAtPath(f"/World/{chain}/geo/joints")
217-219: Fragile assumption about child ordering.Skipping the first child with
[1:]assumes"jointpositions"is always the first child of the geo prim. This could break if the USD structure changes.🔎 Proposed fix
- rod_link_prims = geo_prim.GetChildren()[1:] # skip "jointpositions" prim + rod_link_prims = [ + child for child in geo_prim.GetChildren() + if "jointpositions" not in str(child.GetPath()) + ]
289-290: Remove or document commented-out code.There are several commented-out code blocks that should either be removed or documented with a reason for keeping them:
- Lines 289-290: Commented mass API setup
- Line 332: Commented
breakstatementAlso applies to: 332-332
369-375: Magic number in path depth check.The condition
len(path) == 5is a magic number that's tightly coupled to a specific scene hierarchy. Consider adding a comment explaining the expected structure or making this configurable.- elif len(path) == 5 and any(name in path[-1] for name in ("gear", "piece", "piston")): + # Expected structure: /World/Objects/Category/PrimName (depth 5 with leading empty string) + elif len(path) == 5 and any(name in path[-1] for name in ("gear", "piece", "piston")):
383-384: Consider removing or documenting alternative implementation.The commented alternative
add_rod_joints(stage)should either be removed or documented to explain when each variant should be used.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/utils/add_bdx_physics_schema.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/utils/add_bdx_physics_schema.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/add_bdx_physics_schema.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/_src/utils/add_bdx_physics_schema.py
📚 Learning: 2025-10-27T18:45:18.446Z
Learnt from: Milad-Rakhsha-NV
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/schema_resolver.py:164-168
Timestamp: 2025-10-27T18:45:18.446Z
Learning: In newton/_src/utils/schema_resolver.py, when using USD's GetAuthoredPropertiesInNamespace() to collect solver-specific attributes, prop.GetName() already returns the full namespaced attribute name (e.g., "physxJoint:armature", not just "armature"), so no additional namespace prefix is needed when storing the keys.
Applied to files:
newton/_src/utils/add_bdx_physics_schema.py
🪛 GitHub Actions: License Check
newton/_src/utils/add_bdx_physics_schema.py
[error] 1-1: License header check failed: the file does not have a valid license header.
🪛 Ruff (0.14.8)
newton/_src/utils/add_bdx_physics_schema.py
211-211: Local variable world is assigned to but never used
Remove assignment to unused variable world
(F841)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (3)
newton/_src/utils/add_bdx_physics_schema.py (3)
8-20: LGTM - recursive collision API application.The function correctly applies
CollisionAPIto supported geometry types while skipping proxy prims. The recursive traversal is appropriate for typical USD scene hierarchies.
23-31: LGTM - transform parsing utility.The transformation extraction correctly handles the USD matrix layout and converts to Warp's transform representation. The transpose on the rotation matrix accounts for the convention differences between USD and Warp.
119-196: LGTM - lantern joint assembly.The function correctly builds articulated chains with fixed joints between intermediate links and spherical joints at anchors. The transform offset calculations look correct.
| import argparse | ||
|
|
||
| import numpy as np | ||
| import warp as wp | ||
| from pxr import Gf, Usd, UsdGeom, UsdPhysics | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and examine existing Python files in the newton directory
find . -name "*.py" -path "*/newton/_src/*" | head -20 | while read f; do
echo "=== File: $f ==="
head -15 "$f"
echo ""
doneRepository: newton-physics/newton
Length of output: 14074
🏁 Script executed:
#!/bin/bash
# Find license configuration files
find . -name ".licenserc*" -o -name "LICENSE*" -o -name "COPYING*" | head -20Repository: newton-physics/newton
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check for GitHub Actions CI config
find . -path "*/.github/workflows/*" -name "*.yml" -o -name "*.yaml" | head -10Repository: newton-physics/newton
Length of output: 447
Add missing Apache-2.0 license header.
The file lacks the required license header. Add the Apache-2.0 header at the top following the repository's standard format:
+# SPDX-FileCopyrightText: Copyright (c) 2025 The Newton Developers
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
import argparse
import numpy as np📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import argparse | |
| import numpy as np | |
| import warp as wp | |
| from pxr import Gf, Usd, UsdGeom, UsdPhysics | |
| # SPDX-FileCopyrightText: Copyright (c) 2025 The Newton Developers | |
| # SPDX-License-Identifier: Apache-2.0 | |
| # | |
| # Licensed under the Apache License, Version 2.0 (the "License"); | |
| # you may not use this file except in compliance with the License. | |
| # You may obtain a copy of the License at | |
| # | |
| # http://www.apache.org/licenses/LICENSE-2.0 | |
| # | |
| # Unless required by applicable law or agreed to in writing, software | |
| # distributed under the License is distributed on an "AS IS" BASIS, | |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| # See the License for the specific language governing permissions and | |
| # limitations under the License. | |
| import argparse | |
| import numpy as np | |
| import warp as wp | |
| from pxr import Gf, Usd, UsdGeom, UsdPhysics | |
🧰 Tools
🪛 GitHub Actions: License Check
[error] 1-1: License header check failed: the file does not have a valid license header.
🤖 Prompt for AI Agents
In newton/_src/utils/add_bdx_physics_schema.py lines 1-6, the file is missing
the repository's standard Apache-2.0 license header; add the project's standard
multi-line Apache-2.0 header comment block at the very top of the file (before
any imports), matching the exact wording/format used across the repo (including
copyright year and owner), and ensure there is a blank line after the header
before the first import.
| joints = [] | ||
| world = stage.GetPrimAtPath("/World") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused variables joints and world.
Both joints and world are assigned but never used. The joints list accumulates joint objects but is discarded when the function returns.
🔎 Proposed fix
- joints = []
- world = stage.GetPrimAtPath("/World")
for rod_idx, (chain_name, lantern_name) in enumerate(rods):Also remove the joints.append(...) calls on lines 235, 244, and 258, or return joints if they're needed by callers.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
211-211: Local variable world is assigned to but never used
Remove assignment to unused variable world
(F841)
| output_path = args.input_path.replace(".usd", "_physics.usd") | ||
|
|
||
| stage = Usd.Stage.Open(args.input_path) |
There was a problem hiding this comment.
Fragile output path generation and missing error handling.
The string replace could produce unexpected results if .usd appears in directory names (e.g., /data/my.usd.project/scene.usd → /data/my_physics.usd.project/scene.usd). Also, Usd.Stage.Open returns None if the file cannot be opened, which would cause an error later.
🔎 Proposed fix
+ import os
+
output_path = args.input_path.replace(".usd", "_physics.usd")
+ # Safer: only replace extension
+ base, ext = os.path.splitext(args.input_path)
+ output_path = f"{base}_physics{ext}"
stage = Usd.Stage.Open(args.input_path)
+ if not stage:
+ raise RuntimeError(f"Failed to open USD stage: {args.input_path}")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In newton/_src/utils/add_bdx_physics_schema.py around lines 344 to 346, the
output path construction using str.replace is fragile (it can change directory
names containing ".usd") and there is no check for Usd.Stage.Open failing (it
may return None). Replace the string replace with a robust extension change
using pathlib.Path (or os.path.splitext) to set the output filename to
input_path.with_suffix("_physics.usd") (or properly join original stem +
"_physics" + ".usd"), and add immediate error handling after Usd.Stage.Open to
check for a None return value and raise or log a clear error and exit/return
early; also ensure the output directory exists before writing.
3 participants
sim_usdis a utility that reads a USD file, runs a Newton simulation, and renders USD file as a result. For this version, we support rigid body. Here are a few examples of simulations that are run using Newton:Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.