Improve sdf creation performance#2475
Conversation
📝 WalkthroughWalkthroughAdds a parity-based sign-query fast path selectable via Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host
participant MeshQuery as MeshQueryDispatcher
participant Winding as WindingKernel
participant Parity as ParityKernel
participant Populate as Populate/LinearityKernels
participant Texture as TextureStorage
rect rgba(120,180,240,0.5)
Note over Host,Texture: Winding-number path (use_parity = false)
Host->>MeshQuery: request signed-distance samples (use_parity=false)
MeshQuery->>Winding: perform winding-number queries
Winding-->>MeshQuery: signed-distance & sign
MeshQuery->>Populate: deliver samples
Populate->>Texture: write texels & indirection slots
end
rect rgba(200,150,100,0.5)
Note over Host,Texture: Parity fast path (use_parity = true)
Host->>MeshQuery: request signed-distance samples (use_parity=true)
MeshQuery->>Parity: perform parity sign + distance queries
Parity-->>MeshQuery: signed-distance & sign
MeshQuery->>Populate: deliver samples
Populate->>Populate: accumulate linearity errors -> apply thresholds
Populate->>Texture: write texels, update subgrid occupancy/linearity
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 (3)
newton/_src/geometry/sdf_utils.py (1)
394-418:⚠️ Potential issue | 🟠 MajorDon't drop
shape_marginon the texture-backed mesh path.
create_texture_sdf_from_mesh()never receivesshape_margin, and the texture extraction path does not compensate for it later. After this change,Mesh.build_sdf(shape_margin=...)behaves like zero margin for mesh SDFs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 394 - 418, The texture-backed path never applies the requested shape_margin because create_texture_sdf_from_mesh is called without it, so Mesh.build_sdf(shape_margin=...) behaves like zero margin; update the call sites that use create_texture_sdf_from_mesh (the call with tex_mesh, margin=margin, narrow_band_range=..., etc.) to pass the shape_margin argument through (or, if create_texture_sdf_from_mesh cannot accept it, apply the margin to the returned texture/coarse/subgrid textures before constructing the SDF), and ensure the SDF constructed (SDF(..., shape_margin=shape_margin, ...)) is consistent with the texture data adjustments so the requested margin is honored.newton/_src/geometry/types.py (1)
796-816:⚠️ Potential issue | 🟠 MajorInvalidate
is_watertightafter in-place mesh edits.This cache is only cleared in the property setters, but
Mesh.verticesandMesh.indicesstill expose the backing writablendarrays. Oncemesh.is_watertighthas been computed, an in-place edit likemesh.vertices[0] = ...leaves the cached classification stale and can routebuild_sdf()through the wrong watertight fast path.Also applies to: 855-882
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/types.py` around lines 796 - 816, The current vertices and indices getters return the mutable backing ndarrays (_vertices/_indices), so in-place edits (e.g., mesh.vertices[0]=...) bypass cache invalidation for _cached_hash and _is_watertight; fix by returning read-only views from the getters (e.g., call .view() and set .flags.writeable = False or return a copy) so attempts to mutate will raise and force use of the property setters which clear _cached_hash, _edges and _is_watertight; apply the same change for the indices getter and the analogous properties referenced around lines 855-882 to ensure no writable backing arrays are exposed.newton/_src/geometry/sdf_texture.py (1)
1880-1913:⚠️ Potential issue | 🟠 MajorExpose texture SDF API in public module and document parameter changes in CHANGELOG.
The
create_texture_sdf_from_meshfunction is innewton/_src/(internal module) and is not exposed in the publicnewton/geometry.pyAPI. Per the coding guidelines, user-facing symbols must be exposed via public modules—do not keep them in_src.If this function is intended as a user-facing entry point (as the docstring suggests), it should be:
- Exposed in
newton/geometry.pyby adding it to__all__- Documented in
CHANGELOG.mdunder[Unreleased]with details on the new parameters (watertightanduse_parity) and their migration guidance (e.g., when to use each path)If the function is internal-only, move its docstring into an internal comment since the current docstring signals public API intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` around lines 1880 - 1913, The function create_texture_sdf_from_mesh is currently in an internal module but has a public-facing docstring; either promote it to the public API by importing and exporting it from the public geometry module (add create_texture_sdf_from_mesh to the module's namespace and include it in __all__) and add a CHANGELOG.md entry under [Unreleased] documenting the new parameters watertight and use_parity and migration guidance, or, if it must remain internal, remove the public docstring and convert it into an internal comment to avoid advertising it as part of the public API; update references to the symbol accordingly (create_texture_sdf_from_mesh) to ensure callers use the public import path if promoted.
🧹 Nitpick comments (2)
newton/_src/geometry/sdf_texture.py (1)
1355-1387: Add validation to prevent conflictingwatertightanduse_parityoptions.The
watertightparameter enables a completely separate fast-path that uses scanline ray-march for sign classification. Theuse_parityparameter is only meaningful whenwatertight=False(standard path). However, there's no validation to prevent users from passingwatertight=True, use_parity=True, which would silently ignoreuse_parity.Suggested validation
Args: mesh: Warp mesh with ``support_winding_number=True`` (or without when *watertight* is ``True``). ... Returns: Dictionary with all sparse SDF data. """ + if watertight and use_parity: + raise ValueError( + "watertight=True and use_parity=True are mutually exclusive. " + "The watertight fast-path uses scanline ray-march for sign, not parity queries." + ) + # Ceiling division ensures the subgrid grid fully covers the fine grid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` around lines 1355 - 1387, The function that builds the sparse SDF texture (the one with parameters watertight: bool and use_parity: bool) must validate that the two flags are not both True; update the start of that function to check if watertight and use_parity are both True and raise a ValueError (or appropriate exception) with a clear message indicating that use_parity is only valid when watertight is False and will be ignored otherwise; reference the parameters watertight and use_parity in the check and error to make the failure obvious to callers.newton/tests/test_sdf_compute.py (1)
1523-1528: Consider adding a comment explaining the cache manipulation pattern.The pattern of setting
mesh._is_watertight = Falsethenmesh._is_watertight = Noneto force different code paths is a test-specific workaround. A brief comment would clarify this for future maintainers.Suggested comment
# Build SDF via normal (winding) path by forcing is_watertight to False + # We directly set the private cache to bypass the lazy computation + # and force the non-watertight code path, then reset to None to + # allow fresh computation for the next SDF build. mesh._is_watertight = False sdf_winding = SDF.create_from_mesh(mesh, max_resolution=32, texture_format="float32") mesh._is_watertight = None # reset cache🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_sdf_compute.py` around lines 1523 - 1528, Add a brief inline comment above the test's cache manipulation that explains why the test sets mesh._is_watertight = False before calling SDF.create_from_mesh(...) and then resets mesh._is_watertight = None: state that this is a test-only cache override to force the non-watertight code-path for SDF.create_from_mesh and then clear the cached value so subsequent assertions (mesh.is_watertight) exercise the normal watertight fast path; reference mesh._is_watertight, SDF.create_from_mesh, and mesh.is_watertight to make intent clear to future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/sdf_jfa.py`:
- Around line 50-56: The code samples voxel boundaries instead of centers;
update the ray origin and voxel sampling so all three axes use cell centers by
adding 0.5 * cell_size offsets: when constructing ray_o use min_corner[1] +
(float(iy) + 0.5) * cell_size[1] and min_corner[2] + (float(iz) + 0.5) *
cell_size[2], and when evaluating voxel_t (x sampling for ix) use min_corner[0]
+ (float(ix) + 0.5) * cell_size[0]; apply the same +0.5*cell_size adjustment to
the analogous samples in the block around lines 77–92 to ensure all voxel
classifications use centers (refer to variables ray_o, ray_d, voxel_t, ix/iy/iz,
min_corner, cell_size, size_x).
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 328-330: The Mesh SDF path currently returns an SDF with empty
data, sparse_volume, and coarse_volume which breaks callers like
Mesh.build_sdf(), to_kernel_data(), and
compute_sdf_from_shape(GeoType.MESH,...); restore the legacy payload or add a
deprecation: either populate SDF.data/sparse_volume/coarse_volume from the
previous NanoVDB construction when building from a Mesh, or add a clear
deprecation wrapper on Mesh.build_sdf() that still fills those fields while
emitting a DeprecationWarning and documenting the future removal; also add a
[Unreleased] CHANGELOG.md entry with migration guidance and reference to the new
texture-based path so users know to migrate callers that inspect NanoVDB fields.
- Around line 364-404: The code currently accepts use_parity=True for
non-watertight meshes; add a guard that rejects this case by raising a clear
ValueError when use_parity is True but mesh.is_watertight is False (or computed
is_watertight variable is False). Update the logic near where is_watertight and
use_parity are checked (the block that constructs tex_mesh / computes
winding_threshold and calls create_texture_sdf_from_mesh) to perform this
validation before constructing tex_mesh or calling create_texture_sdf_from_mesh,
with a message referencing that parity classification requires a watertight
mesh.
---
Outside diff comments:
In `@newton/_src/geometry/sdf_texture.py`:
- Around line 1880-1913: The function create_texture_sdf_from_mesh is currently
in an internal module but has a public-facing docstring; either promote it to
the public API by importing and exporting it from the public geometry module
(add create_texture_sdf_from_mesh to the module's namespace and include it in
__all__) and add a CHANGELOG.md entry under [Unreleased] documenting the new
parameters watertight and use_parity and migration guidance, or, if it must
remain internal, remove the public docstring and convert it into an internal
comment to avoid advertising it as part of the public API; update references to
the symbol accordingly (create_texture_sdf_from_mesh) to ensure callers use the
public import path if promoted.
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 394-418: The texture-backed path never applies the requested
shape_margin because create_texture_sdf_from_mesh is called without it, so
Mesh.build_sdf(shape_margin=...) behaves like zero margin; update the call sites
that use create_texture_sdf_from_mesh (the call with tex_mesh, margin=margin,
narrow_band_range=..., etc.) to pass the shape_margin argument through (or, if
create_texture_sdf_from_mesh cannot accept it, apply the margin to the returned
texture/coarse/subgrid textures before constructing the SDF), and ensure the SDF
constructed (SDF(..., shape_margin=shape_margin, ...)) is consistent with the
texture data adjustments so the requested margin is honored.
In `@newton/_src/geometry/types.py`:
- Around line 796-816: The current vertices and indices getters return the
mutable backing ndarrays (_vertices/_indices), so in-place edits (e.g.,
mesh.vertices[0]=...) bypass cache invalidation for _cached_hash and
_is_watertight; fix by returning read-only views from the getters (e.g., call
.view() and set .flags.writeable = False or return a copy) so attempts to mutate
will raise and force use of the property setters which clear _cached_hash,
_edges and _is_watertight; apply the same change for the indices getter and the
analogous properties referenced around lines 855-882 to ensure no writable
backing arrays are exposed.
---
Nitpick comments:
In `@newton/_src/geometry/sdf_texture.py`:
- Around line 1355-1387: The function that builds the sparse SDF texture (the
one with parameters watertight: bool and use_parity: bool) must validate that
the two flags are not both True; update the start of that function to check if
watertight and use_parity are both True and raise a ValueError (or appropriate
exception) with a clear message indicating that use_parity is only valid when
watertight is False and will be ignored otherwise; reference the parameters
watertight and use_parity in the check and error to make the failure obvious to
callers.
In `@newton/tests/test_sdf_compute.py`:
- Around line 1523-1528: Add a brief inline comment above the test's cache
manipulation that explains why the test sets mesh._is_watertight = False before
calling SDF.create_from_mesh(...) and then resets mesh._is_watertight = None:
state that this is a test-only cache override to force the non-watertight
code-path for SDF.create_from_mesh and then clear the cached value so subsequent
assertions (mesh.is_watertight) exercise the normal watertight fast path;
reference mesh._is_watertight, SDF.create_from_mesh, and mesh.is_watertight to
make intent clear to future maintainers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 31fcec94-6492-4488-ac95-a77466bfacd5
📒 Files selected for processing (6)
newton/_src/geometry/sdf_jfa.pynewton/_src/geometry/sdf_texture.pynewton/_src/geometry/sdf_utils.pynewton/_src/geometry/types.pynewton/tests/test_sdf_compute.pynewton/tests/test_sdf_texture.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
newton/_src/geometry/sdf_texture.py (2)
538-549: Consider consolidating duplicated parity distance function.
_parity_signed_distanceis identical toget_distance_to_mesh_parityimported fromsdf_utils.py(line 27). If this duplication is intentional (e.g., to avoid cross-modulewp.funcindirection overhead), consider adding a comment explaining why. Otherwise, the watertight kernels could callget_distance_to_mesh_paritydirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` around lines 538 - 549, The function _parity_signed_distance duplicates get_distance_to_mesh_parity from sdf_utils.py; either remove the duplicate and have watertight kernels call get_distance_to_mesh_parity directly, or if the duplication is deliberate to keep a local `@wp.func` for performance/indirection reasons, add a short comment above _parity_signed_distance explaining that intent (referencing get_distance_to_mesh_parity and why the local copy is required). Ensure the fix updates imports/usages accordingly so there are no unresolved references.
878-893: Watertight path allocates texture for subsequently-demoted linear subgrids.The fused populate kernels write texture data for all occupied subgrids, then
_apply_linearity_kerneldemotes some to linear. The standard path runs linearity demotion before populate, avoiding texture writes for linear subgrids.This is a trade-off: the fused kernel simplifies GPU work (single pass) at the cost of writing unused texture data. If memory is a concern for large narrow bands, consider a two-pass approach for the watertight path as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` around lines 878 - 893, The fused watertight path currently populates texture for all occupied subgrids and then runs _apply_linearity_kernel which demotes some via subgrid_is_linear/subgrid_required, causing unnecessary texture writes; fix by performing linearity demotion before writing texture (or convert the fused kernel into a two-pass sequence): first run the linearity computation and invoke _apply_linearity_kernel to set subgrid_required = 0 for linear subgrids (use linearity_errors, subgrid_occupied, subgrid_is_linear, subgrid_required), then run the populate/write kernel only for subgrid_required != 0 (or modify the fused populate kernel to check subgrid_required before writing). Ensure the watertight path mirrors the standard path ordering so texture allocations/writes are skipped for demoted linear subgrids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@newton/_src/geometry/sdf_texture.py`:
- Around line 538-549: The function _parity_signed_distance duplicates
get_distance_to_mesh_parity from sdf_utils.py; either remove the duplicate and
have watertight kernels call get_distance_to_mesh_parity directly, or if the
duplication is deliberate to keep a local `@wp.func` for performance/indirection
reasons, add a short comment above _parity_signed_distance explaining that
intent (referencing get_distance_to_mesh_parity and why the local copy is
required). Ensure the fix updates imports/usages accordingly so there are no
unresolved references.
- Around line 878-893: The fused watertight path currently populates texture for
all occupied subgrids and then runs _apply_linearity_kernel which demotes some
via subgrid_is_linear/subgrid_required, causing unnecessary texture writes; fix
by performing linearity demotion before writing texture (or convert the fused
kernel into a two-pass sequence): first run the linearity computation and invoke
_apply_linearity_kernel to set subgrid_required = 0 for linear subgrids (use
linearity_errors, subgrid_occupied, subgrid_is_linear, subgrid_required), then
run the populate/write kernel only for subgrid_required != 0 (or modify the
fused populate kernel to check subgrid_required before writing). Ensure the
watertight path mirrors the standard path ordering so texture allocations/writes
are skipped for demoted linear subgrids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5da1a1b8-b1c3-49f1-ab16-74393ceb5109
📒 Files selected for processing (1)
newton/_src/geometry/sdf_texture.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/sdf_texture.py`:
- Around line 1549-1697: The watertight path is incurring extra host/device
stalls by calling subgrid_texture_gpu.numpy() (and later wp.synchronize())
before running _apply_linearity_kernel; move the wp.launch call for
_apply_linearity_kernel (the block guarded by "if watertight and check_linearity
!= 0") so it executes before any .numpy() reads of subgrid_texture_gpu, then
remove the explicit wp.synchronize() right before
subgrid_start_slots_gpu.numpy(); keep the same kernel inputs (linearity_errors,
subgrid_occupied_gpu, subgrid_is_linear, subgrid_required,
linearization_error_threshold) and preserve the watertight/check_linearity
checks.
- Around line 581-592: The parity occupancy sample currently uses integer
midpoint via "half = cells_per_subgrid // 2" which floors the center and can
land on a vertex for odd subgrid sizes; change to a true midpoint by computing
half as a float (e.g., half = cells_per_subgrid * 0.5) and ensure gx, gy, gz are
computed as floats (keep the later float(...) casts or make gx/gy/gz floats
directly) before building sample_pos and calling
get_distance_to_mesh_parity(mesh, sample_pos, ...); update the code around
_id_to_xyz, cells_per_subgrid, gx/gy/gz, min_corner and cell_size accordingly so
the parity check samples the true subgrid center.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2cf78888-b1a6-4922-8240-8b9fa3f55d6d
📒 Files selected for processing (1)
newton/_src/geometry/sdf_texture.py
# Conflicts: # newton/_src/geometry/sdf_texture.py
There was a problem hiding this comment.
♻️ Duplicate comments (4)
newton/_src/geometry/sdf_utils.py (2)
394-400:⚠️ Potential issue | 🟠 MajorReject
use_parity=Truefor non-watertight meshes.The docstring states parity classification requires a watertight mesh, but the current branching at line 395 still enables it unconditionally when
use_parity=True. On an open mesh this silently produces wrong inside/outside signs rather than falling back to winding-number classification. Raise aValueError(or fall back to winding) whenuse_parityis set butis_watertightisFalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 394 - 400, The code currently allows parity-based classification when use_parity=True even if is_watertight is False; update the logic around winding_threshold/tex_mesh to reject or fallback: if use_parity is True and is_watertight is False, raise a ValueError (or explicitly set use_parity=False to fall back to winding-number) before constructing tex_mesh, ensuring parity mode is never used on non-watertight meshes; adjust the branch that creates wp.Mesh (the tex_mesh assignment) and any subsequent use of compute_mesh_signed_volume/winding_threshold accordingly to reflect the enforced behavior.
337-339:⚠️ Potential issue | 🟠 MajorBreaking change in mesh SDF path:
sparse_volume/coarse_volume/block_coordsare now always empty.Callers that route mesh shapes through
Mesh.build_sdf(),to_kernel_data(), orcompute_sdf_from_shape(GeoType.MESH, ...)and then sample via NanoVDB (or iterateblock_coords) will silently lose that data. Either preserve the legacy NanoVDB payload for the mesh path or gate this behind a deprecation with a CHANGELOG entry and migration guidance.As per coding guidelines: "Breaking changes require a deprecation first. Do not remove or rename public API symbols without deprecating them in a prior release."
Also applies to: 416-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 337 - 339, The mesh SDF path now drops NanoVDB-related payloads (sparse_volume, coarse_volume, block_coords) which breaks callers of Mesh.build_sdf(), Mesh.to_kernel_data(), and compute_sdf_from_shape(GeoType.MESH,...); restore backward compatibility by either (A) preserving and populating the legacy NanoVDB fields when building mesh SDFs (reusing the previous NanoVDB construction codepaths so sparse_volume/coarse_volume/block_coords remain populated and available to downstream sampling), or (B) if intentionally removing them, add a formal deprecation: emit runtime warnings in Mesh.build_sdf()/to_kernel_data()/compute_sdf_from_shape when those fields would be emptied, add a CHANGELOG entry and migration guidance explaining how to migrate callers to the new texture-based SDF (and provide helper conversion utilities), and keep the old fields intact for at least one release behind the deprecation flag; reference the symbols sparse_volume, coarse_volume, block_coords, Mesh.build_sdf, Mesh.to_kernel_data, compute_sdf_from_shape, and NanoVDB when making the change.newton/_src/geometry/sdf_texture.py (2)
1548-1698:⚠️ Potential issue | 🟡 MinorRedundant sync/stall in watertight fused path.
In the watertight branches the
.numpy()readback onsubgrid_texture_gpuat lines 1587 / 1632 / 1677 forces a full device sync before_apply_linearity_kernelis launched at 1684, and the explicitwp.synchronize()at 1697 immediately precedessubgrid_start_slots_gpu.numpy()at 1698. The explicit synchronize is redundant (the following.numpy()already blocks), and the linearity kernel can be launched before any host readback so it overlaps with the D2H copy. Reorder the launch above the first.numpy()and drop line 1697.As per coding guidelines: "Never call
wp.synchronize()orwp.synchronize_device()right before.numpy()on a Warp array, as.numpy()performs a synchronous device-to-host copy that completes all outstanding work."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` around lines 1548 - 1698, The watertight path forces a premature device sync by calling subgrid_texture_gpu.numpy() (for FLOAT32/UINT16/UINT8 in the branches using subgrid_texture_gpu) before launching _apply_linearity_kernel and also calls an unnecessary wp.synchronize() before subgrid_start_slots_gpu.numpy(); move the wp.launch(...) of _apply_linearity_kernel (the call using linearity_errors, subgrid_occupied_gpu, subgrid_is_linear, subgrid_required, linearization_error_threshold) to occur immediately after the quantization branch finishes but before any subgrid_texture_gpu.numpy() readback, and remove the explicit wp.synchronize() prior to subgrid_start_slots_gpu.numpy() so the linearity kernel can overlap with the device-to-host copy.
581-598:⚠️ Potential issue | 🟠 MajorParity occupancy check samples a vertex for odd
subgrid_size.
half = cells_per_subgrid // 2floors the midpoint. For oddsubgrid_size(e.g. 7) this samples a subgrid corner rather than its center, making thesubgrid_radiusthreshold non‑conservative — narrow‑band subgrids that the standard kernel would retain (which uses the true float center at lines 215‑217) can be dropped. Use the same float-center formulation here for consistency with_check_subgrid_occupied_kernel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` around lines 581 - 598, The parity occupancy check is sampling the subgrid using integer half = cells_per_subgrid // 2 which floors the midpoint and mis-centers odd-sized subgrids; update the sample_pos calculation in the parity kernel to use the same floating midpoint formula as the standard kernel (compute the center as coords * cells_per_subgrid + 0.5 * float(cells_per_subgrid) for gx/gy/gz or otherwise add 0.5*cells_per_subgrid when converting to world space) so get_distance_to_mesh_parity(mesh, sample_pos, ...) uses the true float center and _is_in_narrow_band/subgrid_required decisions match _check_subgrid_occupied_kernel.
🧹 Nitpick comments (1)
newton/_src/geometry/sdf_texture.py (1)
1300-1301: Docstring: also mentionuse_parity=Trueas a case where winding-number support is not required.The same note should be added to the
create_texture_sdf_from_meshdocstring at lines 1819‑1820.Proposed wording
- mesh: Warp mesh with ``support_winding_number=True`` (or without - when *watertight* is ``True``). + mesh: Warp mesh with ``support_winding_number=True`` (or without + when *watertight* is ``True`` or *use_parity* is ``True``).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` around lines 1300 - 1301, Update the docstrings for the mesh parameter in both the SDF texture function and create_texture_sdf_from_mesh to also call out that when use_parity=True winding-number support is not required; specifically, in the sentence mentioning "support_winding_number=True (or without when *watertight* is True)" add an alternative clause noting "or when use_parity=True" so the mesh description for the functions (including create_texture_sdf_from_mesh) clearly documents that use_parity=True obviates the need for winding-number support.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@newton/_src/geometry/sdf_texture.py`:
- Around line 1548-1698: The watertight path forces a premature device sync by
calling subgrid_texture_gpu.numpy() (for FLOAT32/UINT16/UINT8 in the branches
using subgrid_texture_gpu) before launching _apply_linearity_kernel and also
calls an unnecessary wp.synchronize() before subgrid_start_slots_gpu.numpy();
move the wp.launch(...) of _apply_linearity_kernel (the call using
linearity_errors, subgrid_occupied_gpu, subgrid_is_linear, subgrid_required,
linearization_error_threshold) to occur immediately after the quantization
branch finishes but before any subgrid_texture_gpu.numpy() readback, and remove
the explicit wp.synchronize() prior to subgrid_start_slots_gpu.numpy() so the
linearity kernel can overlap with the device-to-host copy.
- Around line 581-598: The parity occupancy check is sampling the subgrid using
integer half = cells_per_subgrid // 2 which floors the midpoint and mis-centers
odd-sized subgrids; update the sample_pos calculation in the parity kernel to
use the same floating midpoint formula as the standard kernel (compute the
center as coords * cells_per_subgrid + 0.5 * float(cells_per_subgrid) for
gx/gy/gz or otherwise add 0.5*cells_per_subgrid when converting to world space)
so get_distance_to_mesh_parity(mesh, sample_pos, ...) uses the true float center
and _is_in_narrow_band/subgrid_required decisions match
_check_subgrid_occupied_kernel.
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 394-400: The code currently allows parity-based classification
when use_parity=True even if is_watertight is False; update the logic around
winding_threshold/tex_mesh to reject or fallback: if use_parity is True and
is_watertight is False, raise a ValueError (or explicitly set use_parity=False
to fall back to winding-number) before constructing tex_mesh, ensuring parity
mode is never used on non-watertight meshes; adjust the branch that creates
wp.Mesh (the tex_mesh assignment) and any subsequent use of
compute_mesh_signed_volume/winding_threshold accordingly to reflect the enforced
behavior.
- Around line 337-339: The mesh SDF path now drops NanoVDB-related payloads
(sparse_volume, coarse_volume, block_coords) which breaks callers of
Mesh.build_sdf(), Mesh.to_kernel_data(), and
compute_sdf_from_shape(GeoType.MESH,...); restore backward compatibility by
either (A) preserving and populating the legacy NanoVDB fields when building
mesh SDFs (reusing the previous NanoVDB construction codepaths so
sparse_volume/coarse_volume/block_coords remain populated and available to
downstream sampling), or (B) if intentionally removing them, add a formal
deprecation: emit runtime warnings in
Mesh.build_sdf()/to_kernel_data()/compute_sdf_from_shape when those fields would
be emptied, add a CHANGELOG entry and migration guidance explaining how to
migrate callers to the new texture-based SDF (and provide helper conversion
utilities), and keep the old fields intact for at least one release behind the
deprecation flag; reference the symbols sparse_volume, coarse_volume,
block_coords, Mesh.build_sdf, Mesh.to_kernel_data, compute_sdf_from_shape, and
NanoVDB when making the change.
---
Nitpick comments:
In `@newton/_src/geometry/sdf_texture.py`:
- Around line 1300-1301: Update the docstrings for the mesh parameter in both
the SDF texture function and create_texture_sdf_from_mesh to also call out that
when use_parity=True winding-number support is not required; specifically, in
the sentence mentioning "support_winding_number=True (or without when
*watertight* is True)" add an alternative clause noting "or when
use_parity=True" so the mesh description for the functions (including
create_texture_sdf_from_mesh) clearly documents that use_parity=True obviates
the need for winding-number support.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8fd1b99a-3281-4c7c-af81-f8e6036d8b85
📒 Files selected for processing (2)
newton/_src/geometry/sdf_texture.pynewton/_src/geometry/sdf_utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/geometry/sdf_utils.py (1)
416-429:⚠️ Potential issue | 🟠 MajorBreaking change to
SDF.create_from_meshpayload — add migration guidance.The mesh path now unconditionally returns
data=create_empty_sdf_data(),sparse_volume=None,coarse_volume=None,block_coords=[]. Any caller that previously inspectedsdf.sparse_volume/sdf.coarse_volume, sampled viato_kernel_data()(e.g.,sample_sdf_extrapolated), or routed throughcompute_sdf_from_shape(GeoType.MESH, …)(lines 967-977 returns the emptydataandNonevolumes) will silently get an empty SDF.The CHANGELOG
Changedentry on line 20 notes the removal but does not include the migration guidance required by the coding guideline forChangedentries (e.g., point users toModel.texture_sdf_data/SDF.to_texture_kernel_data()and explain thatsparse_volume/coarse_volumeare now alwaysNonefor mesh SDFs).As per coding guidelines: "For
Deprecated,Changed, andRemovedCHANGELOG entries, include migration guidance".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 416 - 429, Update the CHANGELOG `Changed` entry to include explicit migration guidance for the breaking change in SDF.create_from_mesh: state that mesh SDFs now always return data=create_empty_sdf_data(), sparse_volume=None, coarse_volume=None and block_coords=[]. Advise callers that any code inspecting SDF.sparse_volume or SDF.coarse_volume, sampling via to_kernel_data()/sample_sdf_extrapolated, or relying on compute_sdf_from_shape(GeoType.MESH, …) must migrate to the texture-based APIs; point them to Model.texture_sdf_data and SDF.to_texture_kernel_data() as the replacement, and give a short note that callers should sample from the texture kernel data instead of sparse/coarse volumes and update call sites accordingly. Ensure the changelog text explicitly warns that previously available volumes will be None for mesh SDFs and include a minimal example reference to using Model.texture_sdf_data -> SDF.to_texture_kernel_data() for migration.
♻️ Duplicate comments (1)
newton/_src/geometry/sdf_utils.py (1)
394-413:⚠️ Potential issue | 🟠 MajorReject
use_parity=Truefor non-watertight meshes.The docstring states parity classification requires a watertight mesh, but lines 395-398 still take the parity branch whenever
use_parity=True, regardless ofis_watertight. Notice also that on line 411watertight=is_watertightis forwarded tocreate_texture_sdf_from_meshindependently ofuse_parity, so a forceduse_parity=Trueon an open mesh would build withwatertight=Falsewhile still using the parity sign query — silently producing wrong signs instead of falling back to winding-number classification.🛡️ Suggested guard
is_watertight = mesh.is_watertight + if use_parity and not is_watertight: + raise ValueError("use_parity=True requires a watertight mesh.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 394 - 413, The code currently allows use_parity=True for non-watertight meshes; change the logic so parity classification is rejected for open meshes: if use_parity and not is_watertight, either raise a ValueError or reset use_parity=False (and optionally warn) before constructing tex_mesh and before calling create_texture_sdf_from_mesh; ensure the winding_threshold / tex_mesh branch uses the corrected use_parity and that watertight=is_watertight is consistent with the chosen classification mode (affecting the tex_mesh creation, winding_threshold computation, and the watertight/use_parity args passed into create_texture_sdf_from_mesh).
🧹 Nitpick comments (1)
newton/_src/geometry/sdf_utils.py (1)
772-775: Redundant initialization.
winding_thresholdis unconditionally reassigned on line 775, so the= 0.5on line 772 is dead. You can drop it (or, mirroringcreate_from_mesh, gate thewp.Mesh(..., support_winding_number=True)construction on a watertight check too).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 772 - 775, The initial assignment winding_threshold = 0.5 is redundant because winding_threshold is immediately reassigned after computing signed_volume; remove the dead assignment (or alternatively mirror create_from_mesh by only constructing wp.Mesh(..., support_winding_number=True) and computing compute_mesh_signed_volume(pos, indices) when the mesh is watertight) — update the block involving winding_threshold, wp.Mesh, and compute_mesh_signed_volume accordingly so winding_threshold is only set once based on signed_volume and avoid unnecessary wp.Mesh construction when not needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 20: Update the CHANGELOG "Changed" entry to include migration guidance:
explain that SDF.create_from_mesh no longer fills NanoVDB-backed
sdf.sparse_volume or sdf.coarse_volume and that callers should switch to the
texture-backed sampling path (use the SDFData texture buffers and the
texture-based sampling helpers) or explicitly convert/backfill a NanoVDB volume
via the project's conversion utility if NanoVDB APIs are still required;
reference the symbols SDF.create_from_mesh, sdf.sparse_volume,
sdf.coarse_volume, SDFData, and NanoVDB so users know what to change and where
to look for texture-based sampling helpers or conversion utilities.
---
Outside diff comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 416-429: Update the CHANGELOG `Changed` entry to include explicit
migration guidance for the breaking change in SDF.create_from_mesh: state that
mesh SDFs now always return data=create_empty_sdf_data(), sparse_volume=None,
coarse_volume=None and block_coords=[]. Advise callers that any code inspecting
SDF.sparse_volume or SDF.coarse_volume, sampling via
to_kernel_data()/sample_sdf_extrapolated, or relying on
compute_sdf_from_shape(GeoType.MESH, …) must migrate to the texture-based APIs;
point them to Model.texture_sdf_data and SDF.to_texture_kernel_data() as the
replacement, and give a short note that callers should sample from the texture
kernel data instead of sparse/coarse volumes and update call sites accordingly.
Ensure the changelog text explicitly warns that previously available volumes
will be None for mesh SDFs and include a minimal example reference to using
Model.texture_sdf_data -> SDF.to_texture_kernel_data() for migration.
---
Duplicate comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 394-413: The code currently allows use_parity=True for
non-watertight meshes; change the logic so parity classification is rejected for
open meshes: if use_parity and not is_watertight, either raise a ValueError or
reset use_parity=False (and optionally warn) before constructing tex_mesh and
before calling create_texture_sdf_from_mesh; ensure the winding_threshold /
tex_mesh branch uses the corrected use_parity and that watertight=is_watertight
is consistent with the chosen classification mode (affecting the tex_mesh
creation, winding_threshold computation, and the watertight/use_parity args
passed into create_texture_sdf_from_mesh).
---
Nitpick comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 772-775: The initial assignment winding_threshold = 0.5 is
redundant because winding_threshold is immediately reassigned after computing
signed_volume; remove the dead assignment (or alternatively mirror
create_from_mesh by only constructing wp.Mesh(..., support_winding_number=True)
and computing compute_mesh_signed_volume(pos, indices) when the mesh is
watertight) — update the block involving winding_threshold, wp.Mesh, and
compute_mesh_signed_volume accordingly so winding_threshold is only set once
based on signed_volume and avoid unnecessary wp.Mesh construction when not
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: af6b48c3-80ca-4f7a-a467-164ee45f12ce
📒 Files selected for processing (4)
CHANGELOG.mddocs/concepts/collisions.rstnewton/_src/geometry/sdf_texture.pynewton/_src/geometry/sdf_utils.py
✅ Files skipped from review due to trivial changes (2)
- docs/concepts/collisions.rst
- newton/_src/geometry/sdf_texture.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/geometry/sdf_utils.py (1)
438-459:⚠️ Potential issue | 🟠 MajorThread
shape_margininto the texture build.
shape_marginis documented as being baked into the SDF values, but this path never passes it intocreate_texture_sdf_from_mesh(), andTextureSDFDatahas no other way to carry it. The result is that texture sampling/collision uses the raw mesh distance whileshape_marginonly survives on the Python wrapper, so mesh SDFs built with a non-zero margin change behavior on the new path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 438 - 459, The texture SDF build is not receiving the shape_margin, so create_texture_sdf_from_mesh is called without shape_margin and TextureSDFData never records it; update the call to create_texture_sdf_from_mesh (the invocation that sets texture_data, coarse_texture, subgrid_texture, tex_block_coords) to pass the existing shape_margin argument through, and ensure whatever TextureSDFData produced by create_texture_sdf_from_mesh stores/propagates shape_margin so the SDF constructor (SDF(..., texture_data=texture_data, _coarse_texture=coarse_texture, _subgrid_texture=subgrid_texture, ...)) receives texture data baked with the same shape_margin used for the main SDF.newton/_src/geometry/sdf_texture.py (1)
1-1:⚠️ Potential issue | 🟡 MinorRestore the original SPDX creation year.
This file predates this PR, so updating the header to 2026 makes the SPDX line inconsistent with the repository rule for modified files.
As per coding guidelines, "In SPDX copyright lines, use the year the file was first created. Do not create date ranges or update the year when modifying a file."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_texture.py` at line 1, Update the SPDX header at the top of the file: replace the "2026" in the SPDX-FileCopyrightText line with the file’s original creation year (do not create a range or update the year when modifying the file); ensure the SPDX line remains a single-year value reflecting the file's creation year.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 450-455: The SDF instances for texture-backed meshes are being
initialized as empty by always calling create_empty_sdf_data() and clearing
sparse_volume/coarse_volume, so update the SDF construction in the mesh path
(where texture_block_coords and texture_data are available) to populate real SDF
data when texture_data exists: if texture_data (or texture_block_coords) is
present, construct or copy the appropriate SDF.data from the texture mesh source
instead of create_empty_sdf_data(), and keep or set the
sparse_volume/coarse_volume to the texture-backed NanoVDB objects (not None) so
that SDF.is_empty() correctly reflects a populated texture-backed SDF; modify
the SDF(...) call sites that currently pass create_empty_sdf_data(),
sparse_volume=None and coarse_volume=None to use the real data/volumes for
texture-backed cases.
In `@newton/tests/test_sdf_compute.py`:
- Around line 1735-1813: The test
test_sign_method_parity_override_on_non_watertight_mesh pins undefined behavior
by expecting specific signs when calling SDF.create_from_mesh(...,
sign_method="parity") on a non-watertight mesh; change the test to avoid
asserting concrete inside/outside values for that unsupported case — either mark
the test as expecting undefined/conditional behavior (e.g., assert that
texture_data is present but skip sign assertions) or limit the assertions to
behavior guaranteed by the API (for example, only check mesh.is_watertight is
False and that SDF.texture_data exists), and remove or relax assertions that
validate sign of sampled points so the test no longer enforces
parity-on-open-mesh semantics.
---
Outside diff comments:
In `@newton/_src/geometry/sdf_texture.py`:
- Line 1: Update the SPDX header at the top of the file: replace the "2026" in
the SPDX-FileCopyrightText line with the file’s original creation year (do not
create a range or update the year when modifying the file); ensure the SPDX line
remains a single-year value reflecting the file's creation year.
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 438-459: The texture SDF build is not receiving the shape_margin,
so create_texture_sdf_from_mesh is called without shape_margin and
TextureSDFData never records it; update the call to create_texture_sdf_from_mesh
(the invocation that sets texture_data, coarse_texture, subgrid_texture,
tex_block_coords) to pass the existing shape_margin argument through, and ensure
whatever TextureSDFData produced by create_texture_sdf_from_mesh
stores/propagates shape_margin so the SDF constructor (SDF(...,
texture_data=texture_data, _coarse_texture=coarse_texture,
_subgrid_texture=subgrid_texture, ...)) receives texture data baked with the
same shape_margin used for the main SDF.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2b6a30e0-515b-4f8c-b0b6-63ba4b646c3c
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/geometry/sdf_texture.pynewton/_src/geometry/sdf_utils.pynewton/_src/geometry/types.pynewton/tests/test_sdf_compute.py
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/geometry/types.py
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
newton/tests/test_sdf_compute.py (2)
1517-1733: Consider a helper for the_is_watertightcache poking.The pattern
mesh._is_watertight = False / True / Noneis repeated in five tests to force each code path. Touching the private cache directly is fragile (it couples tests to the current caching scheme and will silently no-op if the attribute is ever renamed). A small helper — e.g.,_build_sdf_with_sign_method(mesh, method, ...)that callsSDF.create_from_mesh(..., sign_method="parity" | "winding")— would exercise the same paths through the public API and avoid the cache mutation entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_sdf_compute.py` around lines 1517 - 1733, Tests repeatedly mutate the private mesh._is_watertight cache to force code paths; add a small test helper (e.g., _build_sdf_with_sign_method(mesh, method, max_resolution=..., texture_format=...)) that calls SDF.create_from_mesh(mesh, max_resolution=..., texture_format=..., sign_method="parity" or "winding") and return the SDF, then update the tests (those using mesh._is_watertight = False/True/None and SDF.create_from_mesh calls) to use this helper for both fast and winding paths so tests no longer poke the private _is_watertight attribute while preserving existing assertions and calls to mesh.clear_sdf().
1497-1508: Hard-coded"cuda:0"vs the class-levelget_cuda_test_devices()pattern.
_sample_texture_sdf_at_pointshard-codesdevice="cuda:0"while the rest of the module usesget_cuda_test_devices()/add_function_testfor multi-GPU coverage. If CI ever runs these tests on a non-default CUDA device, the sampling helper will mismatch the SDF's device. Consider sampling on the SDF's own device (e.g.,sdf_obj.texture_data.coarse_texture.deviceor an explicit device argument) to keep the helper portable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_sdf_compute.py` around lines 1497 - 1508, The helper _sample_texture_sdf_at_points currently hard-codes device="cuda:0"; change it to use the SDF object's actual device or accept a device argument so tests using get_cuda_test_devices()/add_function_test don't mismatch devices — e.g., derive the device from sdf_obj.texture_data.coarse_texture.device (or from sdf_obj.texture_data.device if present) and use that when constructing wp.array and wp.zeros, or add an optional device parameter to _sample_texture_sdf_at_points and pass the test harness device through add_function_test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 403-447: The code currently ignores target_voxel_size when
building the texture SDF; compute an effective_max_resolution from
target_voxel_size and the mesh AABB instead of silently falling back to 64: use
mesh.vertices to get min/max bounds, compute the longest axis length L, compute
resolution = ceil(L / target_voxel_size), convert to int and use that as
effective_max_resolution (respecting any explicit max_resolution override), and
pass that resolution into create_texture_sdf_from_mesh (replace the hardcoded
res = effective_max_resolution if effective_max_resolution is None else 64
logic); alternatively, if you prefer not to auto-convert, raise a ValueError
from the code path when target_voxel_size is provided to prevent silent
downgrades.
---
Nitpick comments:
In `@newton/tests/test_sdf_compute.py`:
- Around line 1517-1733: Tests repeatedly mutate the private mesh._is_watertight
cache to force code paths; add a small test helper (e.g.,
_build_sdf_with_sign_method(mesh, method, max_resolution=...,
texture_format=...)) that calls SDF.create_from_mesh(mesh, max_resolution=...,
texture_format=..., sign_method="parity" or "winding") and return the SDF, then
update the tests (those using mesh._is_watertight = False/True/None and
SDF.create_from_mesh calls) to use this helper for both fast and winding paths
so tests no longer poke the private _is_watertight attribute while preserving
existing assertions and calls to mesh.clear_sdf().
- Around line 1497-1508: The helper _sample_texture_sdf_at_points currently
hard-codes device="cuda:0"; change it to use the SDF object's actual device or
accept a device argument so tests using
get_cuda_test_devices()/add_function_test don't mismatch devices — e.g., derive
the device from sdf_obj.texture_data.coarse_texture.device (or from
sdf_obj.texture_data.device if present) and use that when constructing wp.array
and wp.zeros, or add an optional device parameter to
_sample_texture_sdf_at_points and pass the test harness device through
add_function_test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0f5f9f95-248a-4aac-9e4b-093028a1366c
📒 Files selected for processing (2)
newton/_src/geometry/sdf_utils.pynewton/tests/test_sdf_compute.py
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Nice refactor — centralizing sign queries through _query_mesh_sdf, parallelizing the 9³ linearity inner loop with atomic_max, and moving slot writes / linear-demotion to GPU gives a real per-voxel BVH-query reduction on the hot path. The sign_method enum (auto/parity/winding) is a better public API than the earlier watertight + use_parity booleans, and test_inverted_winding_sphere + test_sign_method_parity_override_on_non_watertight_mesh are exactly the tests I'd want to pin the contract.
One real semantics question inline (shape_margin no longer baked on the texture path), a redundant wp.synchronize() that the AGENTS.md guide explicitly forbids, and a CHANGELOG migration-guidance ask.
Nits worth raising but non-blocking:
Mesh.is_watertightuses 100 nm quantized float32 tolerance — false negatives fall through to the winding path, which is the right disposition, but atolerance: float = 1e-7knob for power users who weld at sub-μm would be nice. Similarly, a one-linefilter degenerate-trianglestep before the edge-count build would avoid spurious watertightness failures for assets carrying degenerate triangles (benign — winding fallback still correct, just a 1.1× perf hit).Mesh.build_sdfdoesn't forwardsign_methodtoSDF.create_from_mesh; users can't pick the strategy from the convenience wrapper. One-line pass-through.- No
texture_format="uint8"test on the watertight fast path (uint16 is covered). A twin or parametrized test would exercise_populate_subgrid_texture_uint8_kernel. Mesh._is_watertightcache has the same stale-after-in-place-edit gap as the existing_cached_hash/_edgescaches — out of scope for this PR; worth hardening all three together later if we ever do.- The
# Dilate the non-linear subgrid set by one ring of occupied neighborscomment on theblock_coords_from_subgrid_requiredcall insdf_texture.py(~line 1451, preexisting) no longer matches the implementation (which now usessubgrid_occupieddirectly). Not touched in this PR, but worth a follow-up pass. compute_sdf_from_shape(GeoType.MESH, ...)public tuple shape is preserved but for MESH now returns(kernel_data, None, None, []). Belongs in the migration note (see inline on CHANGELOG).
Mostly LGTM pending C1.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 20: Update the CHANGELOG entry to remove internal implementation details:
rewrite the paragraph about SDF.create_from_mesh to use imperative present tense
(e.g., "Build mesh SDFs via the texture-based sparse path only") and remove
references to internal symbols and tuple slot behavior for
compute_sdf_from_shape(...) and the private helper _compute_sdf_from_shape_impl;
keep user-facing guidance that SDF.sparse_volume, SDF.coarse_volume, and
SDF.block_coords are no longer populated for mesh SDFs and direct readers to use
SDF.texture_data and Model.texture_sdf_data for sampling, but do not describe
internal return tuples or call patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 80c4065f-6baf-4b25-be7f-1011fb9cc3e4
📒 Files selected for processing (2)
CHANGELOG.mdnewton/_src/geometry/sdf_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/geometry/sdf_utils.py
mzamoramora-nvidia
left a comment
There was a problem hiding this comment.
LGTM. Solid speedup on FastBuildSdf (L4, ~5k-tri icosphere): 14×–63× across resolutions 32–256.
| max_resolution | main | this PR | speedup |
|---|---|---|---|
| 32 | 2.73 s | 0.044 s | 62× |
| 64 | 2.36 s | 0.037 s | 63× |
| 128 | 2.50 s | 0.076 s | 33× |
| 256 | 4.16 s | 0.306 s | 14× |
Two non-blocking notes:
- #2464 fixes
target_voxel_sizebeing dropped in the samesdf_utils.py:423-467block — worth coordinating landing order. Mesh.build_sdf(shape_margin=X).extract_isomesh(isovalue=0.0)silently returns the base surface now instead of the X-offset surface. Docstring at
sdf_utils.py:230-251covers it, but aChangedentry would help external callers.
# Conflicts: # newton/_src/geometry/sdf_utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/geometry/sdf_utils.py (1)
346-358:⚠️ Potential issue | 🟡 MinorThread
sign_methodthroughMesh.build_sdf().
SDF.create_from_mesh()exposes thesign_methodparameter (selecting between"auto","parity", and"winding"for mesh-to-SDF conversion), butMesh.build_sdf()does not, preventing users from controlling sign strategy through the main user-facing API.🛠️ Proposed API passthrough
def build_sdf( self, *, device: Devicelike | None = None, narrow_band_range: tuple[float, float] | None = None, target_voxel_size: float | None = None, max_resolution: int | None = None, margin: float | None = None, shape_margin: float = 0.0, scale: tuple[float, float, tuple] | None = None, texture_format: str = "uint16", + sign_method: SignMethod = "auto", ) -> "SDF": @@ self.sdf = SDF.create_from_mesh( self, device=device, narrow_band_range=narrow_band_range if narrow_band_range is not None else (-0.1, 0.1), target_voxel_size=target_voxel_size, max_resolution=max_resolution, margin=margin if margin is not None else 0.05, shape_margin=shape_margin, scale=scale, texture_format=texture_format, + sign_method=sign_method, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 346 - 358, The Mesh.build_sdf sign computation isn't configurable because Mesh.build_sdf doesn't accept a sign_method and therefore can't pass it to SDF.create_from_mesh; add a sign_method: SignMethod = "auto" parameter to Mesh.build_sdf's signature and propagate it into the call to SDF.create_from_mesh (and any internal helpers that build the SDF) so the same "auto"/"parity"/"winding" choice exposed by SDF.create_from_mesh is available on the Mesh API and forwarded unchanged to SDF.create_from_mesh.
🧹 Nitpick comments (2)
newton/_src/geometry/sdf_utils.py (1)
426-431: Avoid watertightness checks for forced sign modes.
mesh.is_watertightcan be an O(mesh) cached edge-sharing test, but it is only needed for"auto". Forced"parity"/"winding"should skip it.♻️ Proposed refactor
- is_watertight = mesh.is_watertight - if sign_method == "auto": - use_parity = is_watertight + use_parity = mesh.is_watertight else: use_parity = sign_method == "parity"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 426 - 431, Currently mesh.is_watertight is always accessed which can be O(mesh); change the logic so you only evaluate mesh.is_watertight when sign_method == "auto". Concretely, avoid reading mesh.is_watertight at top-level; instead, inside the branch if sign_method == "auto" compute is_watertight and set use_parity = is_watertight, and for other branches set use_parity = (sign_method == "parity") without touching mesh.is_watertight; refer to the existing symbols sign_method, mesh.is_watertight and use_parity to locate where to move the check.newton/tests/test_sdf_texture.py (1)
884-898: Reuse_build_nanovdb_data()here.This duplicates the new NanoVDB setup helper and local import. Using the helper keeps the volume-lifetime pattern centralized.
♻️ Proposed cleanup
def test_texture_sdf_from_volume(test, device): """Build texture SDF from NanoVDB volumes and verify sampling.""" - from newton._src.geometry.sdf_utils import _compute_sdf_from_shape_impl # noqa: PLC0415 - mesh = _create_box_mesh() - sdf_data, sparse_volume, coarse_volume, _ = _compute_sdf_from_shape_impl( - shape_type=GeoType.MESH, - shape_geo=mesh, - shape_scale=(1.0, 1.0, 1.0), - shape_margin=0.0, + sdf_data, sparse_volume, coarse_volume = _build_nanovdb_data( + mesh, + resolution=32, narrow_band_distance=(-0.1, 0.1), margin=0.05, - max_resolution=32, device=device, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_sdf_texture.py` around lines 884 - 898, The test_texture_sdf_from_volume duplicates NanoVDB setup and local imports; replace the local NanoVDB setup with a call to the existing helper _build_nanovdb_data() and reuse its returned objects instead of re-creating volumes locally, then pass those objects into _compute_sdf_from_shape_impl (keep the import of _compute_sdf_from_shape_impl but remove duplicated NanoVDB construction). Locate test_texture_sdf_from_volume and swap the duplicated NanoVDB creation and lifetime handling for a call to _build_nanovdb_data(), ensuring returned sparse/coarse volume variables are used in the call to _compute_sdf_from_shape_impl so volume-lifetime semantics remain centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 216-218: The is_empty method incorrectly treats any non-None
texture_data as non-empty even when texture_data is an empty texture struct
returned by create_texture_sdf_from_mesh (e.g.,
create_empty_texture_sdf_data()); update is_empty on the class that defines
is_empty() to also consider empty texture structs as empty by checking
texture_data contents (for example, test whether texture_data is falsy or has no
entries/size rather than only is None) so that when create_texture_sdf_from_mesh
returns create_empty_texture_sdf_data(), None, None, [] the SDF reports empty;
modify the condition in is_empty() to include a check like "texture_data is None
or texture_data is empty" while keeping the existing checks on
data.sparse_sdf_ptr and data.coarse_sdf_ptr.
---
Outside diff comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 346-358: The Mesh.build_sdf sign computation isn't configurable
because Mesh.build_sdf doesn't accept a sign_method and therefore can't pass it
to SDF.create_from_mesh; add a sign_method: SignMethod = "auto" parameter to
Mesh.build_sdf's signature and propagate it into the call to
SDF.create_from_mesh (and any internal helpers that build the SDF) so the same
"auto"/"parity"/"winding" choice exposed by SDF.create_from_mesh is available on
the Mesh API and forwarded unchanged to SDF.create_from_mesh.
---
Nitpick comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 426-431: Currently mesh.is_watertight is always accessed which can
be O(mesh); change the logic so you only evaluate mesh.is_watertight when
sign_method == "auto". Concretely, avoid reading mesh.is_watertight at
top-level; instead, inside the branch if sign_method == "auto" compute
is_watertight and set use_parity = is_watertight, and for other branches set
use_parity = (sign_method == "parity") without touching mesh.is_watertight;
refer to the existing symbols sign_method, mesh.is_watertight and use_parity to
locate where to move the check.
In `@newton/tests/test_sdf_texture.py`:
- Around line 884-898: The test_texture_sdf_from_volume duplicates NanoVDB setup
and local imports; replace the local NanoVDB setup with a call to the existing
helper _build_nanovdb_data() and reuse its returned objects instead of
re-creating volumes locally, then pass those objects into
_compute_sdf_from_shape_impl (keep the import of _compute_sdf_from_shape_impl
but remove duplicated NanoVDB construction). Locate test_texture_sdf_from_volume
and swap the duplicated NanoVDB creation and lifetime handling for a call to
_build_nanovdb_data(), ensuring returned sparse/coarse volume variables are used
in the call to _compute_sdf_from_shape_impl so volume-lifetime semantics remain
centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 46284478-41aa-49fe-9d78-211dc81b210c
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/geometry/sdf_texture.pynewton/_src/geometry/sdf_utils.pynewton/tests/test_sdf_texture.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
newton/_src/geometry/sdf_utils.py (2)
376-379:⚠️ Potential issue | 🟡 MinorAlign
shape_margindocs with texture-backed behavior.This argument text still says the SDF values are offset, but the texture path stores raw mesh distance and only stores
shape_marginfor compatibility. Either bake/apply the margin or update this public contract to match the behavior documented inextract_isomesh().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 376 - 379, The docstring for the shape_margin parameter is inaccurate for texture-backed SDFs; update the documentation in sdf_utils.py (the shape_margin parameter description) to match extract_isomesh() behavior by stating that texture-backed paths store raw mesh distances and do not bake/apply shape_margin (shape_margin is retained only for API compatibility), or alternatively implement baking of the margin into the stored texture—refer to shape_margin and extract_isomesh() when making the change so readers know which code path does not apply the margin.
216-218:⚠️ Potential issue | 🟡 MinorDon’t treat an empty texture struct as a populated SDF.
create_texture_sdf_from_mesh()can returncreate_empty_texture_sdf_data(), None, None, []; with this check,is_empty()returnsFalsesolely becausetexture_datais non-None.🛠️ Proposed fix
def is_empty(self) -> bool: """Return True when this SDF has no sparse/coarse or texture payload.""" - return int(self.data.sparse_sdf_ptr) == 0 and int(self.data.coarse_sdf_ptr) == 0 and self.texture_data is None + has_sparse_or_coarse = int(self.data.sparse_sdf_ptr) != 0 or int(self.data.coarse_sdf_ptr) != 0 + has_texture = ( + self.texture_data is not None + and self._coarse_texture is not None + and int(self.texture_data.subgrid_size) > 0 + ) + return not has_sparse_or_coarse and not has_texture🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 216 - 218, The is_empty() check treats any non-None texture_data as populated; update SDF.is_empty to consider empty texture structures returned by create_texture_sdf_from_mesh/create_empty_texture_sdf_data as also empty. Change the return expression in is_empty() to treat texture_data as empty when it's None OR an empty container (e.g., isinstance(texture_data, (list, tuple)) and len(texture_data) == 0) OR when the texture object exposes an emptiness indicator (e.g., hasattr(texture_data, "is_empty") and texture_data.is_empty() or hasattr(texture_data, "__len__") and len(texture_data) == 0); keep the original checks for sparse_sdf_ptr/coarse_sdf_ptr (int(self.data.sparse_sdf_ptr) == 0 and int(self.data.coarse_sdf_ptr) == 0) and combine with the improved texture_data emptiness test in the is_empty() method.
🧹 Nitpick comments (2)
newton/tests/test_sdf_texture.py (1)
884-898: Reuse_build_nanovdb_data()here too.This duplicates the new helper’s
_compute_sdf_from_shape_impl()call and volume-lifetime handling. Reusing the helper keeps the NanoVDB reference path in one place.♻️ Proposed cleanup
def test_texture_sdf_from_volume(test, device): """Build texture SDF from NanoVDB volumes and verify sampling.""" - from newton._src.geometry.sdf_utils import _compute_sdf_from_shape_impl # noqa: PLC0415 - mesh = _create_box_mesh() - sdf_data, sparse_volume, coarse_volume, _ = _compute_sdf_from_shape_impl( - shape_type=GeoType.MESH, - shape_geo=mesh, - shape_scale=(1.0, 1.0, 1.0), - shape_margin=0.0, - narrow_band_distance=(-0.1, 0.1), - margin=0.05, - max_resolution=32, + sdf_data, sparse_volume, coarse_volume = _build_nanovdb_data( + mesh, + resolution=32, + margin=0.05, + narrow_band_range=(-0.1, 0.1), device=device, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_sdf_texture.py` around lines 884 - 898, The test_texture_sdf_from_volume duplicates the call and volume lifetime handling already encapsulated by the helper _build_nanovdb_data(); replace the direct call to _compute_sdf_from_shape_impl(...) and the manual sparse/coarse volume handling with a call to _build_nanovdb_data(device, mesh, ...) (or the helper's expected args) so the test reuses that helper to obtain sdf_data, sparse_volume, coarse_volume and ensures NanoVDB reference lifetime is managed in one place; update references in the test to use the returned values from _build_nanovdb_data() instead of the current manual implementation.newton/_src/geometry/sdf_utils.py (1)
426-431: Avoid computing watertightness when the sign method is forced.
mesh.is_watertightcan be an expensive CPU edge-sharing pass. Only"auto"needs it; forced"parity"/"winding"should skip that work.⚡ Proposed fix
- is_watertight = mesh.is_watertight - if sign_method == "auto": - use_parity = is_watertight + use_parity = mesh.is_watertight else: use_parity = sign_method == "parity"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/geometry/sdf_utils.py` around lines 426 - 431, Right now mesh.is_watertight is always computed even when sign_method is forced; change the logic so you only call mesh.is_watertight when sign_method == "auto" — e.g., if sign_method == "auto": compute is_watertight = mesh.is_watertight and set use_parity = is_watertight; else set use_parity = (sign_method == "parity") without touching mesh.is_watertight. This avoids the expensive is_watertight call for forced "parity" or "winding".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 473-480: The SDF returned uses create_empty_sdf_data() which
leaves data.scale_baked False even when texture_data.scale_baked was set; update
the code that constructs the SDF (the SDF(...) call) to preserve the metadata by
assigning the created kernel/data object's scale_baked to
texture_data.scale_baked (e.g., data = create_empty_sdf_data(); data.scale_baked
= texture_data.scale_baked) before passing it into the SDF constructor so
sdf.to_kernel_data().scale_baked matches texture_data.scale_baked.
---
Duplicate comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 376-379: The docstring for the shape_margin parameter is
inaccurate for texture-backed SDFs; update the documentation in sdf_utils.py
(the shape_margin parameter description) to match extract_isomesh() behavior by
stating that texture-backed paths store raw mesh distances and do not bake/apply
shape_margin (shape_margin is retained only for API compatibility), or
alternatively implement baking of the margin into the stored texture—refer to
shape_margin and extract_isomesh() when making the change so readers know which
code path does not apply the margin.
- Around line 216-218: The is_empty() check treats any non-None texture_data as
populated; update SDF.is_empty to consider empty texture structures returned by
create_texture_sdf_from_mesh/create_empty_texture_sdf_data as also empty. Change
the return expression in is_empty() to treat texture_data as empty when it's
None OR an empty container (e.g., isinstance(texture_data, (list, tuple)) and
len(texture_data) == 0) OR when the texture object exposes an emptiness
indicator (e.g., hasattr(texture_data, "is_empty") and texture_data.is_empty()
or hasattr(texture_data, "__len__") and len(texture_data) == 0); keep the
original checks for sparse_sdf_ptr/coarse_sdf_ptr (int(self.data.sparse_sdf_ptr)
== 0 and int(self.data.coarse_sdf_ptr) == 0) and combine with the improved
texture_data emptiness test in the is_empty() method.
---
Nitpick comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 426-431: Right now mesh.is_watertight is always computed even when
sign_method is forced; change the logic so you only call mesh.is_watertight when
sign_method == "auto" — e.g., if sign_method == "auto": compute is_watertight =
mesh.is_watertight and set use_parity = is_watertight; else set use_parity =
(sign_method == "parity") without touching mesh.is_watertight. This avoids the
expensive is_watertight call for forced "parity" or "winding".
In `@newton/tests/test_sdf_texture.py`:
- Around line 884-898: The test_texture_sdf_from_volume duplicates the call and
volume lifetime handling already encapsulated by the helper
_build_nanovdb_data(); replace the direct call to
_compute_sdf_from_shape_impl(...) and the manual sparse/coarse volume handling
with a call to _build_nanovdb_data(device, mesh, ...) (or the helper's expected
args) so the test reuses that helper to obtain sdf_data, sparse_volume,
coarse_volume and ensures NanoVDB reference lifetime is managed in one place;
update references in the test to use the returned values from
_build_nanovdb_data() instead of the current manual implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bd415c64-41bb-4ba6-a94c-6dcf2c0416f3
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/geometry/sdf_texture.pynewton/_src/geometry/sdf_utils.pynewton/tests/test_sdf_texture.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Description
Ideally #2483 is merged before this MR such that the benchmarks allow to track the improvements.
Improve mesh SDF creation performance.
SDF.create_from_mesh; the texture-based sparse SDF is now the sole collision representation on CUDA.wp.mesh_query_point_sign_parity) that skips the dense sign grid and fuses sample population with the linearity check into a single kernel, amortizing one BVH query per voxel.Mesh.is_watertightproperty (cached, edge-sharing test). Non-watertight meshes keep the winding-number path; ause_parity=Trueoverride is available when the caller knows the mesh is closed.Checklist
CHANGELOG.mdhas been updatedTest plan
New + updated tests in
newton/tests/test_sdf_compute.pyandnewton/tests/test_sdf_texture.py:TestMeshIsWatertight— box/sphere closed, 5-face cube open, empty mesh, cache invalidation on vertex/index mutation.TestSDFWatertightFastPath— sign agreement vs. the winding-number path on sphere and box voxel grids (0 mismatches), torus grid (<5% mismatches, all near surface), and max per-voxel distance error bounded by0.5 * voxel_sizeon sphere and box.TestComputeSDF/TestComputeSDFGridSamplingported to the texture sampling path.New feature / API change
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation