Usd viewer update#629
Conversation
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
📝 WalkthroughTip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
newton/_src/viewer/viewer_rerun.py (3)
71-77: server_uri is undefined when server=False.If server=False and launch_viewer=True, connect_to references an undefined name. Initialize server_uri to None and guard its use.
- if self.server: - server_uri = rr.serve_grpc() + server_uri = None + if self.server: + server_uri = rr.serve_grpc() ... - if self.launch_viewer: - rr.serve_web_viewer(connect_to=server_uri) + if self.launch_viewer: + if server_uri: + rr.serve_web_viewer(connect_to=server_uri) + else: + rr.serve_web_viewer()Also consider honoring the address argument if rerun supports binding to a specific host/port.
155-171: vertex_colors may be uninitialized when colors is None.vertex_colors is referenced unconditionally; if colors is None, this will raise UnboundLocalError.
- # Handle colors - ReRun doesn't support per-instance colors + # Handle colors - ReRun doesn't support per-instance colors # so we just use the first instance's color for all instances - if colors is not None: + vertex_colors = None + if colors is not None: colors_np = colors.numpy().astype(np.float32) # Take the first instance's color and apply to all vertices first_color = colors_np[0] color_rgb = np.array(first_color * 255, dtype=np.uint8) num_vertices = len(mesh_data["points"]) vertex_colors = np.tile(color_rgb, (num_vertices, 1)) # Log the base mesh with optional colors mesh_3d = rr.Mesh3D( vertex_positions=mesh_data["points"], triangle_indices=mesh_data["indices"], vertex_normals=mesh_data["normals"], - vertex_colors=vertex_colors, + vertex_colors=vertex_colors, )
278-289: Align log_points signatures across viewer implementationsTo keep
ViewerBase.log_pointspolymorphic and type-checker–friendly, remove the unusedwidthparameter from bothviewer_rerun.pyandviewer_null.py, and correct the docstring parameter name from “radius” to “radii”. No call sites passwidth, so this refactor is safe.• newton/_src/viewer/viewer_rerun.py
– Removewidth: float = 0.01from the signature
– Update docstringradius: Point radii.→radii: Point radii.• newton/_src/viewer/viewer_null.py
– Removewidth: float = 0.01from the signature
– Update docstringradius: Point radii.→radii: Point radii.Suggested diffs:
--- a/newton/_src/viewer/viewer_rerun.py @@ -278,7 +278,6 @@ - def log_points(self, name, points, radii, colors, width: float = 0.01, hidden=False): + def log_points(self, name, points, radii, colors, hidden=False): """ Placeholder for logging points to rerun. @@ -283,7 +282,7 @@ Args: name (str): Name of the point batch. points: Point positions. - radius: Point radii. + radii: Point radii. colors: Point colors. hidden (bool): Whether the points are hidden. """--- a/newton/_src/viewer/viewer_null.py @@ -126,7 +126,6 @@ - def log_points(self, name, points, radii, colors, width: float = 0.01, hidden=False): + def log_points(self, name, points, radii, colors, hidden=False): """ No-op implementation for logging points. @@ -130,7 +129,7 @@ Args: name (str): Name of the point batch. points: Point positions. - radius: Point radii. + radii: Point radii. colors: Point colors. hidden (bool): Whether the points are hidden. """newton/_src/viewer/viewer_null.py (1)
126-137: Removewidthparameter fromlog_pointsinViewerNull
The no-op implementation should exactly mirror the abstract signature inViewerBase, which omitswidthforlog_points.Affected location:
newton/_src/viewer/viewer_null.py, around line 126Suggested diff:
- def log_points(self, name, points, radii, colors, width: float = 0.01, hidden=False): + def log_points(self, name, points, radii, colors, hidden=False):
🧹 Nitpick comments (7)
newton/_src/viewer/gl/opengl.py (3)
382-389: Width parameter from public API is unused in the GL path.ViewerBase/ViewerGL expose width for log_lines, and USD backend uses it. GL backend ignores it. Two options:
- Minimal: document that GL path ignores width.
- Better: plumb width through LinesGL.update(..., width) and apply it in render (e.g., glLineWidth, or shader-based thick lines).
Proposed cross-file minimal plumb and usage:
- def update(self, starts, ends, colors): + def update(self, starts, ends, colors, width: float | None = None): ... - self.num_lines = len(starts) + self.num_lines = len(starts) + self._width = width if width is not None else getattr(self, "_width", 1.0)And in viewer_gl.py:
- self.lines[name].update(starts, ends, colors) + self.lines[name].update(starts, ends, colors, width)And in render():
gl.glBindVertexArray(self.vao) +if hasattr(self, "_width") and self._width: + gl.glLineWidth(max(1.0, float(self._width) * 1000.0)) # NOTE: many drivers clamp >1.0 current_vertices = self.num_lines * 2 gl.glDrawArrays(gl.GL_LINES, 0, current_vertices)If you don’t want to risk portability issues with glLineWidth (>1.0 often unsupported), consider a shader-based screen-space line thickness pass instead.
Would you like me to wire this through end-to-end or keep GL as “width-agnostic” and document it?
Also applies to: 393-396, 398-405, 413-416
402-405: Message wording: “begins” → “starts” for consistency.Error strings still say “line begins” after the rename to “starts”.
- raise RuntimeError("Number of line ends does not match line begins") + raise RuntimeError("Number of line ends does not match line starts") ... - raise RuntimeError("Number of line colors does not match line begins") + raise RuntimeError("Number of line colors does not match line starts")
425-428: Avoid reallocating the whole VBO each update on CPU path.glBufferData every frame can cause frequent reallocations. Prefer orphaning + glBufferSubData or map/flush for just the used vertices.
gl.glBindBuffer(gl.GL_ARRAY_BUFFER, self.vbo) -gl.glBufferData(gl.GL_ARRAY_BUFFER, host_vertices.nbytes, host_vertices.ctypes.data, gl.GL_DYNAMIC_DRAW) +current_bytes = (self.num_lines * 2) * self.vertex_byte_size +# Orphan the buffer, then upload only active vertex region +gl.glBufferData(gl.GL_ARRAY_BUFFER, self.vbo_size, None, gl.GL_DYNAMIC_DRAW) +gl.glBufferSubData(gl.GL_ARRAY_BUFFER, 0, current_bytes, host_vertices.ctypes.data)newton/examples/basic/example_basic_viewer.py (1)
51-80: Nit: align variable naming with new API (“begins” → “starts”).To keep the example consistent with the starts/ends/colors terminology:
- self.axes_begins = wp.array( + self.axes_starts = wp.array( ... - self.viewer.log_lines("/coordinate_axes", self.axes_begins, self.axes_ends, self.axes_colors) + self.viewer.log_lines("/coordinate_axes", self.axes_starts, self.axes_ends, self.axes_colors)I can push this rename across the file if desired.
Also applies to: 175-175
newton/_src/viewer/viewer_rerun.py (1)
264-276: Signature updated: fine as a placeholder; consider documenting width handling.log_lines matches the new API surface with width. Given rerun backend is stubbed, note in the docstring whether width will be used when implemented.
newton/_src/viewer/viewer.py (1)
152-163: Comment/color mismatch for contacts; pick the intended color.Comment says “orange-red” but sets green. Suggest use a warm color and keep tuple form for expansion.
- # Use orange-red color for contact normals - colors = (0.0, 1.0, 0.0) + # Use orange-red color for contact normals + colors = (1.0, 0.35, 0.0)newton/_src/viewer/viewer_usd.py (1)
79-79: Premature set_model call in constructorCalling
set_model(None)in the constructor seems unnecessary. The parent ViewerBase class doesn't require this, and it's typically called by the user when they have a model ready.Consider removing this line as it's not needed in the constructor:
- self.set_model(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 (7)
newton/_src/viewer/gl/opengl.py(4 hunks)newton/_src/viewer/viewer.py(3 hunks)newton/_src/viewer/viewer_gl.py(4 hunks)newton/_src/viewer/viewer_null.py(1 hunks)newton/_src/viewer/viewer_rerun.py(1 hunks)newton/_src/viewer/viewer_usd.py(7 hunks)newton/examples/basic/example_basic_viewer.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
newton/_src/viewer/viewer_null.py (4)
newton/_src/viewer/viewer.py (2)
log_lines(350-351)log_points(354-355)newton/_src/viewer/viewer_gl.py (2)
log_lines(217-271)log_points(273-291)newton/_src/viewer/viewer_rerun.py (2)
log_lines(265-276)log_points(278-289)newton/_src/viewer/viewer_usd.py (2)
log_lines(274-345)log_points(347-389)
newton/examples/basic/example_basic_viewer.py (5)
newton/_src/viewer/viewer.py (1)
log_lines(350-351)newton/_src/viewer/viewer_gl.py (1)
log_lines(217-271)newton/_src/viewer/viewer_null.py (1)
log_lines(113-124)newton/_src/viewer/viewer_rerun.py (1)
log_lines(265-276)newton/_src/viewer/viewer_usd.py (1)
log_lines(274-345)
newton/_src/viewer/viewer_rerun.py (4)
newton/_src/viewer/viewer.py (2)
log_lines(350-351)log_points(354-355)newton/_src/viewer/viewer_gl.py (2)
log_lines(217-271)log_points(273-291)newton/_src/viewer/viewer_null.py (2)
log_lines(113-124)log_points(126-137)newton/_src/viewer/viewer_usd.py (2)
log_lines(274-345)log_points(347-389)
newton/_src/viewer/viewer_gl.py (5)
newton/_src/viewer/gl/opengl.py (4)
update(236-283)update(382-427)update(901-910)update_from_points(701-718)newton/_src/viewer/viewer.py (3)
update(409-418)log_points(354-355)log_lines(350-351)newton/_src/viewer/viewer_null.py (2)
log_points(126-137)log_lines(113-124)newton/_src/viewer/viewer_rerun.py (2)
log_points(278-289)log_lines(265-276)newton/_src/viewer/viewer_usd.py (2)
log_points(347-389)log_lines(274-345)
newton/_src/viewer/viewer.py (4)
newton/_src/viewer/viewer_gl.py (2)
log_lines(217-271)log_points(273-291)newton/_src/viewer/viewer_null.py (2)
log_lines(113-124)log_points(126-137)newton/_src/viewer/viewer_rerun.py (2)
log_lines(265-276)log_points(278-289)newton/_src/viewer/viewer_usd.py (2)
log_lines(274-345)log_points(347-389)
newton/_src/viewer/viewer_usd.py (2)
newton/_src/viewer/viewer.py (6)
set_model(64-72)begin_frame(74-75)end_frame(369-370)close(373-374)log_lines(350-351)log_points(354-355)newton/_src/viewer/viewer_gl.py (7)
set_model(142-155)begin_frame(357-364)end_frame(366-381)is_running(430-437)close(448-452)log_lines(217-271)log_points(273-291)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (26)
newton/_src/viewer/gl/opengl.py (1)
119-136: Renamed kernel inputs look correct; good alignment with upstream API.Switching to starts/ends/colors and duplicating color per-vertex is correct and matches viewer_gl.log_lines’ expansion of list/tuple colors to per-line arrays. No functional concerns here.
newton/examples/basic/example_basic_viewer.py (1)
175-175: Path rename LGTM.Using “/coordinate_axes” matches the style elsewhere (e.g., “/contacts”, “/model/joints”).
newton/_src/viewer/viewer.py (1)
639-645: Radii plumbed through correctly.switch from widths → radii is consistent with USD GL backends and base API. LGTM.
newton/_src/viewer/viewer_null.py (1)
113-124: Signature matches base; docstring aligned.No issues; placeholder is fine.
newton/_src/viewer/viewer_gl.py (8)
220-224: Parameter renaming aligned with PR objectivesThe parameter renaming from
line_begins/line_ends/line_colorstostarts/ends/colorsimproves API clarity and consistency across viewer backends. The addition of thewidthparameter provides flexibility for line rendering, though it's not yet utilized in this GL backend.
231-234: Comprehensive docstring updateThe docstring properly reflects the new parameter names and clearly describes the expected formats, including support for None values to handle empty logs.
237-240: Proper empty log handlingGood defensive programming by checking for None values and correctly resetting the LinesGL object state when any input is None. This ensures clean state management when clearing lines.
242-245: Input validation maintains robustnessThe assertions and validation logic properly enforce that starts and ends have matching lengths, maintaining data consistency.
248-255: Efficient color expansion for tuple/list inputsThe implementation correctly handles tuple/list colors by expanding them to a per-line color array using
fill_for efficient GPU-based filling. The zero-lines edge case is also properly handled.
273-273: Parameter name consistency for log_pointsThe parameter renaming from
widthstoradiiin the log_points signature improves semantic clarity, as radii is more intuitive for point/sphere representations.
350-355: Updated _render_picking_line to use new parameter namesThe picking line rendering correctly uses the new
starts/ends/colorsparameter names when callinglog_lines.
271-271: LinesGL.update signature matches usageThe
updatemethod innewton/_src/viewer/gl/opengl.py(line 382) is defined as:def update(self, starts, ends, colors):which exactly matches the call in
newton/_src/viewer/viewer_gl.py(line 271). No further changes are needed.newton/_src/viewer/viewer_usd.py (14)
3-4: Added import for os moduleThe
osimport is needed for the newos.path.abspath()call in theclose()method to display the absolute USD output path.
16-29: Well-structured helper function for segment transformsThe
_compute_segment_xformhelper function cleanly encapsulates the logic for computing a cylinder transform between two points. It properly calculates the midpoint, orientation quaternion, and scale for USD capsule primitives used in line rendering.
76-77: Proper frame tracking initializationThe addition of
_frame_indexand_frame_countenables proper time-sampled data recording in USD.
81-95: Robust frame management implementationThe
begin_framemethod properly manages frame indices and updates the USD stage's end time code as needed. The frame index calculation based on FPS is correct.
104-113: Frame limit checking for recording controlThe
is_runningmethod correctly implements frame limit checking whennum_framesis specified, providing control over recording duration.
115-125: Comprehensive close method with user feedbackThe close method properly saves the USD stage and provides helpful feedback by printing the absolute output path. This improves the user experience.
157-172: Improved mesh topology handlingThe mesh creation now correctly sets topology data (face vertex counts and indices) only once during initial creation, while vertex positions are time-sampled per frame. This is the proper USD pattern for animated meshes.
203-205: Clearer error message for missing mesh prototypeThe error message now provides helpful guidance to call
log_mesh()first, improving developer experience.
231-245: Proper quaternion format conversion for USDThe quaternion conversion from Warp format (x,y,z,w) to USD format (w,(x,y,z)) is correctly implemented. The use of Gf.Quath with explicit float conversions ensures proper USD compatibility.
249-253: Scales handling with proper numpy conversionThe code properly handles None scales by providing identity scaling and correctly converts Warp arrays to numpy when needed.
262-271: Proper primvar setup with explicit indicesThe color primvar setup correctly includes explicit identity indices, which is necessary for some USD viewers (like Omniverse) to properly pick up per-instance colors.
274-345: Comprehensive log_lines implementation using capsulesThe implementation cleverly uses USD capsules with PointInstancer to represent line segments. The per-line transform computation and color handling are properly implemented with time sampling support.
347-389: Complete log_points implementation with UsdGeomPointsThe log_points method properly uses UsdGeomPoints primitive with correct radius-to-width conversion (radii * 2.0) and flexible color interpolation handling. The implementation correctly determines interpolation types based on input formats.
407-433: Versatile color promotion helperThe
_promote_colors_to_arrayhelper method provides flexible color format handling, supporting Warp arrays, single colors as tuples/lists, and numpy arrays. The tiling logic for single colors is correct.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
LGTM. I left some really minor comments that can also be addressed in a future PR. |
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Signed-off-by: Miles Macklin <mmacklin@nvidia.com>
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
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
Bug Fixes
Refactor
Examples