ViewerUSD improvements #818
Conversation
* Use non-instanced prims by default for easier post editing * Add root xform + scaling * Add default prim for referencing * Allow use of default output path for "output.usd" for examples * Set default num_frames=100 Signed-off-by: Miles Macklin <[email protected]>
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 4
🧹 Nitpick comments (6)
newton/examples/__init__.py (2)
125-128: Align help text with behavior and consider overwrite/dir handling for the default path.
- The new default makes --output-path no longer “required” for usd viewer; please update the help text accordingly.
- Consider proactively creating the parent directory (if any) and warning on overwrite to avoid surprising users/tests.
Apply this diff to update the help string:
- parser.add_argument( - "--output-path", type=str, default="output.usd", help="Path to the output USD file (required for usd viewer)." - ) + parser.add_argument( + "--output-path", + type=str, + default="output.usd", + help="Path to the output USD file (used by the usd viewer).", + )
169-174: Runtime check is now stale given the default; add light validation instead.With a non-None default, this check won’t trigger anymore. Replace it with a simple sanity check (non-empty string) and optional parent dir creation.
Apply this diff:
- elif args.viewer == "usd": - if args.output_path is None: - raise ValueError("--output-path is required when using usd viewer") - viewer = newton.viewer.ViewerUSD(output_path=args.output_path, num_frames=args.num_frames) + elif args.viewer == "usd": + if not args.output_path or not isinstance(args.output_path, str): + raise ValueError("--output-path must be a non-empty string for the usd viewer") + # create parent directory if provided + out_dir = os.path.dirname(args.output_path) + if out_dir: + os.makedirs(out_dir, exist_ok=True) + viewer = newton.viewer.ViewerUSD(output_path=args.output_path, num_frames=args.num_frames)newton/_src/viewer/viewer_usd.py (4)
270-357: PointInstancer pathing and attributes look good; handle instance count changes and unused arg.
- If the number of instances changes across frames, update ids and protoIndices accordingly.
materialsis currently unused.Apply this diff to update ids/protoIndices on size changes:
num_instances = len(xforms) @@ - instancer = self._instancers[name] + instancer = self._instancers[name] @@ - if scales is not None: + if scales is not None: instancer.GetScalesAttr().Set(scales, self._frame_index) @@ if colors is not None: # Promote colors to proper numpy array format colors_np = self._promote_colors_to_array(colors, num_instances) @@ displayColor.SetIndices(indices, self._frame_index) + + # Keep ids/protoIndices in sync if count changes + existing_ids = instancer.GetIdsAttr().Get() + if existing_ids is None or len(existing_ids) != num_instances: + instancer.GetIdsAttr().Set(list(range(num_instances))) + instancer.GetProtoIndicesAttr().Set([0] * num_instances, self._frame_index)Optionally prefix the unused param to silence linters:
- def log_instances_point_instancer(self, name, mesh, xforms, scales, colors, materials): + def log_instances_point_instancer(self, name, mesh, xforms, scales, colors, _materials):
40-56: Prefer consistent precision for Xform ops (float vs double).You’re authoring translate/scale as Vec3d but orient as Quatf. Consider using Vec3f for translate/scale to match precision for a smaller file footprint.
Apply this diff:
- if pos is not None: - xform_ops[0].Set(Gf.Vec3d(float(pos[0]), float(pos[1]), float(pos[2])), time) + if pos is not None: + xform_ops[0].Set(Gf.Vec3f(float(pos[0]), float(pos[1]), float(pos[2])), time) @@ - if scale is not None: - xform_ops[2].Set(Gf.Vec3d(float(scale[0]), float(scale[1]), float(scale[2])), time) + if scale is not None: + xform_ops[2].Set(Gf.Vec3f(float(scale[0]), float(scale[1]), float(scale[2])), time)
229-229: Unused parametermaterial.
materialis not used. Either implement material binding or prefix it with_to silence linters.Apply this diff:
- def log_instances(self, name, mesh, xform, scale, color, material): + def log_instances(self, name, mesh, xform, scale, color, _material):
271-271: Unused parametermaterials.Same as above; prefix with
_until material support is implemented.Apply this diff:
- def log_instances_point_instancer(self, name, mesh, xforms, scales, colors, materials): + def log_instances_point_instancer(self, name, mesh, xforms, scales, colors, _materials):
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/viewer/viewer_usd.py(9 hunks)newton/examples/__init__.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T21:41:45.795Z
Learnt from: dylanturpin
PR: newton-physics/newton#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/examples/__init__.py
🧬 Code graph analysis (1)
newton/_src/viewer/viewer_usd.py (4)
newton/_src/viewer/viewer_gl.py (1)
log_instances(233-255)newton/_src/viewer/viewer.py (1)
log_instances(377-378)newton/_src/viewer/viewer_rerun.py (1)
log_instances(133-203)newton/_src/viewer/viewer_null.py (1)
log_instances(68-80)
🪛 Ruff (0.13.1)
newton/_src/viewer/viewer_usd.py
229-229: Unused method argument: material
(ARG002)
271-271: Unused method argument: materials
(ARG002)
🔇 Additional comments (4)
newton/_src/viewer/viewer_usd.py (4)
98-106: Root Xform/scaling looks good; set default prim is a nice touch.This nicely centralizes scene transforms and gives a stable anchor for references/editing.
197-201: Good: Pre-create scopes before defining mesh.Ensures stable hierarchy and avoids overs in parent scopes later.
224-226: Visibility handling is correct.Setting per-frame visibility on the prototype is consistent.
369-384: LGTM: Capsules as line segments with instancing.The hierarchy creation and proto wiring are correct.
There was a problem hiding this comment.
@mmacklin can you check the CodeRabbit comments? They seem reasonable to me.
Signed-off-by: Miles Macklin <[email protected]>
Think these should all be addressed now. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
newton/_src/viewer/viewer_usd.py (6)
40-57: Guard against missing xform ops to avoid index errors.If
_usd_set_xformis ever called without prior_usd_add_xform,xform_ops[0..2]will IndexError. Add a defensive check.Apply this diff:
def _usd_set_xform( xform, pos: tuple | None = None, rot: tuple | None = None, scale: tuple | None = None, time: float = 0.0, ): xform = UsdGeom.Xform(xform) - xform_ops = xform.GetOrderedXformOps() + xform_ops = xform.GetOrderedXformOps() + if len(xform_ops) < 3: + # Ensure ops exist (in case caller forgot to initialize) + _usd_add_xform(xform.GetPrim()) + xform_ops = xform.GetOrderedXformOps() if pos is not None: xform_ops[0].Set(Gf.Vec3d(float(pos[0]), float(pos[1]), float(pos[2])), time) if rot is not None: xform_ops[1].Set(Gf.Quatf(float(rot[3]), float(rot[0]), float(rot[1]), float(rot[2])), time) if scale is not None: xform_ops[2].Set(Gf.Vec3d(float(scale[0]), float(scale[1]), float(scale[2])), time)
247-248: Avoid “/scope” suffix hack when ensuring parents.Ensure parents for the actual base path instead; clearer and avoids stray ‘scope’.
Apply this diff:
- self._ensure_scopes_for_path(self.stage, self._get_path(name) + "/scope") + base_path = self._get_path(name) + self._ensure_scopes_for_path(self.stage, base_path)
261-263: Build instance paths with Sdf.Path.AppendChild, not string concat.Safer and normalized.
Apply this diff:
- instance_path = self._get_path(name) + f"/instance_{i}" + base = Sdf.Path(self._get_path(name)) + instance_path = base.AppendChild(f"instance_{i}").pathString instance = self.stage.GetPrimAtPath(instance_path)
351-357: Update ids/protoIndices each frame (handles changing instance counts).Without this, changing num_instances across frames can desync data.
Apply this diff:
instancer.GetPositionsAttr().Set(positions, self._frame_index) instancer.GetOrientationsAttr().Set(orientations, self._frame_index) if scales is not None: instancer.GetScalesAttr().Set(scales, self._frame_index) + # Keep ids and protoIndices in sync per frame + instancer.GetProtoIndicesAttr().Set([0] * num_instances, self._frame_index) + instancer.GetIdsAttr().Set(list(range(num_instances)), self._frame_index)
241-241: Silence lint on unused material(s) argument or wire it up.Either use the parameter(s) or prefix with “_” to satisfy linters until implemented.
Apply one of these quick fixes:
- def log_instances(self, name, mesh, xforms, scales, colors, materials): + def log_instances(self, name, mesh, xforms, scales, colors, _materials):- def log_instances_point_instancer(self, name, mesh, xforms, scales, colors, materials): + def log_instances_point_instancer(self, name, mesh, xforms, scales, colors, _materials):Also applies to: 284-284
209-213: Normalize dictionary keys to absolute Sdf.Path strings to avoid duplicate entries.Currently keys are raw
namevalues; mixing "mesh" vs "/root/mesh" will duplicate.Consider:
- Store keys as
self._get_path(name)(absolute path strings) for meshes/instancers.- Convert call sites accordingly.
Also applies to: 241-246, 300-303
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/viewer/viewer_usd.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
newton/_src/viewer/viewer_usd.py (4)
newton/_src/viewer/viewer_gl.py (1)
log_instances(233-255)newton/_src/viewer/viewer.py (1)
log_instances(377-378)newton/_src/viewer/viewer_rerun.py (1)
log_instances(133-203)newton/_src/viewer/viewer_null.py (1)
log_instances(68-80)
🪛 Ruff (0.13.1)
newton/_src/viewer/viewer_usd.py
241-241: Unused method argument: material
(ARG002)
284-284: Unused method argument: materials
(ARG002)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (11)
newton/_src/viewer/viewer_usd.py (11)
31-38: Good: Centralized xform op creation.Clear, minimal helpers to standardize translate/orient/scale authoring.
68-78: Constructor + docstring updates look good.Scaling parameter is documented, num_frames default updated to 100.
106-114: Good: Root Xform with uniform scaling and defaultPrim.Consistent scene root and scale authored at t=0 is a solid choice.
209-213: Good: Define mesh at normalized path and precompute topology once.Scope creation + Define + time-sampled points is correct.
236-238: Visibility properly time-sampled.This avoids overriding authored visibility at other times.
264-270: Good: Reference prototype via GetReferences and initialize xform ops once.Correct API usage and avoids per-frame op redefinition.
308-319: Good: Normalized paths for instancer and prototype target.Targets use _get_path(name) consistently.
382-386: Good: Normalized path usage for line instancer creation.Consistent with the path helper.
457-462: Resolved: Points.Get now uses normalized path to avoid re-defining each frame.This addresses the earlier bug.
172-178: Fix path joining: use Sdf.Path to avoid “/rootmesh” and double “/root” bugs.String concat breaks relative/absolute inputs and Sdf.Path inputs.
Apply this diff:
- def _get_path(self, name): - # Handle both absolute and relative paths correctly - if name.startswith("/"): - return "/root" + name - else: - return "/root/" + name + def _get_path(self, name): + root = Sdf.Path("/root") + p = name if isinstance(name, Sdf.Path) else Sdf.Path(str(name)) + if not p.IsAbsolutePath(): + return root.AppendPath(p).pathString + if p.HasPrefix(root): + return p.pathString + return root.AppendPath(p.MakeRelativePath(Sdf.Path.absoluteRootPath)).pathString
279-281: Setting displayColor on an Xform does nothing; author on the referenced Mesh prim.Create/set the primvar on the child mesh inside the instance namespace.
Apply this diff:
- if color is not None: - displayColor = UsdGeom.PrimvarsAPI(instance).GetPrimvar("displayColor") - displayColor.Set(color[i], self._frame_index) + if colors is not None: + mesh_leaf = self._meshes[mesh].GetPrim().GetName() + mesh_child_path = Sdf.Path(instance_path).AppendChild(mesh_leaf) + mesh_child = self.stage.GetPrimAtPath(mesh_child_path) + if mesh_child: + pv = UsdGeom.PrimvarsAPI(mesh_child).CreatePrimvar( + "displayColor", Sdf.ValueTypeNames.Color3fArray + ) + pv.SetInterpolation(UsdGeom.Tokens.constant) + c = colors[i] + pv.Set([Gf.Vec3f(float(c[0]), float(c[1]), float(c[2]))], self._frame_index)
Signed-off-by: Eric Heiden <[email protected]>
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Chores