Skip to content

Improve sdf creation performance#2475

Merged
nvtw merged 25 commits into
newton-physics:mainfrom
nvtw:dev/tw3/improve_sdf_creation
Apr 20, 2026
Merged

Improve sdf creation performance#2475
nvtw merged 25 commits into
newton-physics:mainfrom
nvtw:dev/tw3/improve_sdf_creation

Conversation

@nvtw

@nvtw nvtw commented Apr 16, 2026

Copy link
Copy Markdown
Member

Description

Ideally #2483 is merged before this MR such that the benchmarks allow to track the improvements.

Improve mesh SDF creation performance.

  • Drop the NanoVDB sparse/coarse volume build from SDF.create_from_mesh; the texture-based sparse SDF is now the sole collision representation on CUDA.
  • Add a fast parity-based construction path (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.
  • Auto-select the fast path via the new Mesh.is_watertight property (cached, edge-sharing test). Non-watertight meshes keep the winding-number path; a use_parity=True override is available when the caller knows the mesh is closed.
  • Move subgrid-center generation and the linear-slot fix-up from a Python loop to GPU kernels / vectorized NumPy, and batch host readbacks so each build syncs the device only once.

Checklist

  • New or existing tests cover these changes
  • The documentation is up to date with these changes
  • CHANGELOG.md has been updated

Test plan

New + updated tests in newton/tests/test_sdf_compute.py and newton/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 by 0.5 * voxel_size on sphere and box.
  • Existing TestComputeSDF / TestComputeSDFGridSampling ported to the texture sampling path.
uv run --extra dev -m newton.tests -k test_sdf

New feature / API change

import newton

mesh = newton.Mesh(vertices, indices)
assert mesh.is_watertight  # O(|F|) edge-sharing test, cached

sdf = newton.SDF.create_from_mesh(mesh, max_resolution=128)
# Watertight: picks the fast parity path automatically.
# Non-watertight: falls back to the winding-number path.
# Force parity on a mesh you know is closed:
sdf = newton.SDF.create_from_mesh(mesh, max_resolution=128, use_parity=True)

Summary by CodeRabbit

  • New Features

    • Added Mesh.is_watertight and SDF.create_from_mesh sign_method ("auto"/"parity"/"winding").
  • Improvements

    • Mesh→SDF now builds texture-backed SDFs by default with a parity fast-path for watertight meshes; GPU construction, occupancy and linearity checks are optimized for performance and use a texture-only pipeline.
  • Tests

    • Added/updated CUDA tests for watertight detection, parity vs winding behavior, and texture SDF sampling.
  • Documentation

    • Guidance on watertight meshes and parity-based construction added.

@nvtw nvtw marked this pull request as draft April 16, 2026 13:17
@nvtw nvtw self-assigned this Apr 16, 2026
@coderabbitai

coderabbitai Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Adds a parity-based sign-query fast path selectable via sign_method/use_parity, a cached Mesh.is_watertight property, refactors GPU kernels for mesh sign queries and linearity demotion, centralizes subgrid slot writes, and makes SDF.create_from_mesh produce texture-backed SDFs by default (no NanoVDB outputs).

Changes

Cohort / File(s) Summary
Texture SDF core
newton/_src/geometry/sdf_texture.py
Adds _query_mesh_sdf(...) to dispatch parity vs winding queries; computes subgrid sample positions on-GPU; refactors linearity demotion into _accumulate_subgrid_linearity_error_kernel and _apply_subgrid_linearity_kernel; adds _interp_coarse_sdf(...) and _write_subgrid_slot(...); updates float32/uint16/uint8 texture population kernels to accept use_parity and use new indirection logic.
SDF API & parity helper
newton/_src/geometry/sdf_utils.py
Introduces SignMethod = Literal["auto","parity","winding"] and sign_method arg on SDF.create_from_mesh; enforces CUDA, validates sign_method, maps autoMesh.is_watertight; routes mesh builds to texture-only pipeline (no NanoVDB outputs); adds @wp.func get_distance_to_mesh_parity; updates SDF.is_empty and extract_isomesh semantics.
Mesh type
newton/_src/geometry/types.py
Adds cached Mesh.is_watertight: bool with lazy computation and invalidation on vertex/index updates; computes watertightness by quantizing verts, canonicalizing triangle vertex IDs, building undirected edges, and checking each edge is shared by exactly two triangles.
Host builders / control flow
newton/_src/geometry/sdf_texture.py
build_sparse_sdf_from_mesh(...) and create_texture_sdf_from_mesh(...) gain use_parity: bool=False; parity flag propagated into kernels; subgrid occupancy centers computed on-GPU and subgrid_required snapshot cloned on-GPU before demotion with a single host readback after texture allocation; replaced Python loop for SLOT_LINEAR with NumPy vectorized writes; removed unused apply_subgrid_start(...).
Tests — compute & texture
newton/tests/test_sdf_compute.py, newton/tests/test_sdf_texture.py
Retargets CUDA mesh SDF tests to texture-only path and adds texture sampling helpers; adds watertight/fast-path parity tests comparing parity vs winding; introduces helpers to build NanoVDB reference volumes only when needed; updates assertions to expect texture_data present and NanoVDB ptrs absent for texture builds.
Docs & changelog
CHANGELOG.md, docs/concepts/collisions.rst
Documents cached Mesh.is_watertight, sign_method option and parity fast-path; notes watertightness guidance for terrain meshes and the new default texture-backed SDF behavior.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • adenzler-nvidia
  • Kenny-Vilella
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Improve sdf creation performance' directly reflects the core objective of the PR, which optimizes SDF creation through faster parity-based construction, GPU kernel fusion, and reduced host-device synchronization.
Docstring Coverage ✅ Passed Docstring coverage is 91.86% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Don't drop shape_margin on the texture-backed mesh path.

create_texture_sdf_from_mesh() never receives shape_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 | 🟠 Major

Invalidate is_watertight after in-place mesh edits.

This cache is only cleared in the property setters, but Mesh.vertices and Mesh.indices still expose the backing writable ndarrays. Once mesh.is_watertight has been computed, an in-place edit like mesh.vertices[0] = ... leaves the cached classification stale and can route build_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 | 🟠 Major

Expose texture SDF API in public module and document parameter changes in CHANGELOG.

The create_texture_sdf_from_mesh function is in newton/_src/ (internal module) and is not exposed in the public newton/geometry.py API. 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:

  1. Exposed in newton/geometry.py by adding it to __all__
  2. Documented in CHANGELOG.md under [Unreleased] with details on the new parameters (watertight and use_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 conflicting watertight and use_parity options.

The watertight parameter enables a completely separate fast-path that uses scanline ray-march for sign classification. The use_parity parameter is only meaningful when watertight=False (standard path). However, there's no validation to prevent users from passing watertight=True, use_parity=True, which would silently ignore use_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 = False then mesh._is_watertight = None to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 779b126 and 4bd08e6.

📒 Files selected for processing (6)
  • newton/_src/geometry/sdf_jfa.py
  • newton/_src/geometry/sdf_texture.py
  • newton/_src/geometry/sdf_utils.py
  • newton/_src/geometry/types.py
  • newton/tests/test_sdf_compute.py
  • newton/tests/test_sdf_texture.py

Comment thread newton/_src/geometry/sdf_jfa.py Outdated
Comment thread newton/_src/geometry/sdf_utils.py
@codecov

codecov Bot commented Apr 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.05263% with 3 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
newton/_src/geometry/sdf_utils.py 89.65% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
newton/_src/geometry/sdf_texture.py (2)

538-549: Consider consolidating duplicated parity distance function.

_parity_signed_distance is identical to get_distance_to_mesh_parity imported from sdf_utils.py (line 27). If this duplication is intentional (e.g., to avoid cross-module wp.func indirection overhead), consider adding a comment explaining why. Otherwise, the watertight kernels could call get_distance_to_mesh_parity directly.

🤖 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_kernel demotes 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4bd08e6 and 75dcda8.

📒 Files selected for processing (1)
  • newton/_src/geometry/sdf_texture.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 75dcda8 and ac08fff.

📒 Files selected for processing (1)
  • newton/_src/geometry/sdf_texture.py

Comment thread newton/_src/geometry/sdf_texture.py Outdated
Comment thread newton/_src/geometry/sdf_texture.py Outdated
nvtw added 2 commits April 16, 2026 17:45
# Conflicts:
#	newton/_src/geometry/sdf_texture.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (4)
newton/_src/geometry/sdf_utils.py (2)

394-400: ⚠️ Potential issue | 🟠 Major

Reject use_parity=True for 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 a ValueError (or fall back to winding) when use_parity is set but is_watertight is False.

🤖 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 | 🟠 Major

Breaking change in mesh SDF path: sparse_volume/coarse_volume/block_coords are now always empty.

Callers that route mesh shapes through Mesh.build_sdf(), to_kernel_data(), or compute_sdf_from_shape(GeoType.MESH, ...) and then sample via NanoVDB (or iterate block_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 | 🟡 Minor

Redundant sync/stall in watertight fused path.

In the watertight branches the .numpy() readback on subgrid_texture_gpu at lines 1587 / 1632 / 1677 forces a full device sync before _apply_linearity_kernel is launched at 1684, and the explicit wp.synchronize() at 1697 immediately precedes subgrid_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() or wp.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 | 🟠 Major

Parity occupancy check samples a vertex for odd subgrid_size.

half = cells_per_subgrid // 2 floors the midpoint. For odd subgrid_size (e.g. 7) this samples a subgrid corner rather than its center, making the subgrid_radius threshold 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 mention use_parity=True as a case where winding-number support is not required.

The same note should be added to the create_texture_sdf_from_mesh docstring 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

📥 Commits

Reviewing files that changed from the base of the PR and between e87a559 and 770b078.

📒 Files selected for processing (2)
  • newton/_src/geometry/sdf_texture.py
  • newton/_src/geometry/sdf_utils.py

@nvtw nvtw requested a review from mzamoramora-nvidia April 17, 2026 07:28
@nvtw nvtw marked this pull request as ready for review April 17, 2026 07:28
@nvtw nvtw requested a review from adenzler-nvidia April 17, 2026 07:30

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Breaking change to SDF.create_from_mesh payload — 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 inspected sdf.sparse_volume / sdf.coarse_volume, sampled via to_kernel_data() (e.g., sample_sdf_extrapolated), or routed through compute_sdf_from_shape(GeoType.MESH, …) (lines 967-977 returns the empty data and None volumes) will silently get an empty SDF.

The CHANGELOG Changed entry on line 20 notes the removal but does not include the migration guidance required by the coding guideline for Changed entries (e.g., point users to Model.texture_sdf_data / SDF.to_texture_kernel_data() and explain that sparse_volume/coarse_volume are now always None for mesh SDFs).

As per coding guidelines: "For Deprecated, Changed, and Removed CHANGELOG 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 | 🟠 Major

Reject use_parity=True for 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 of is_watertight. Notice also that on line 411 watertight=is_watertight is forwarded to create_texture_sdf_from_mesh independently of use_parity, so a forced use_parity=True on an open mesh would build with watertight=False while 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_threshold is unconditionally reassigned on line 775, so the = 0.5 on line 772 is dead. You can drop it (or, mirroring create_from_mesh, gate the wp.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

📥 Commits

Reviewing files that changed from the base of the PR and between e87a559 and 80bb3ee.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • docs/concepts/collisions.rst
  • newton/_src/geometry/sdf_texture.py
  • newton/_src/geometry/sdf_utils.py
✅ Files skipped from review due to trivial changes (2)
  • docs/concepts/collisions.rst
  • newton/_src/geometry/sdf_texture.py

Comment thread CHANGELOG.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Thread shape_margin into the texture build.

shape_margin is documented as being baked into the SDF values, but this path never passes it into create_texture_sdf_from_mesh(), and TextureSDFData has no other way to carry it. The result is that texture sampling/collision uses the raw mesh distance while shape_margin only 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 | 🟡 Minor

Restore 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

📥 Commits

Reviewing files that changed from the base of the PR and between 80bb3ee and 102e277.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • newton/_src/geometry/sdf_texture.py
  • newton/_src/geometry/sdf_utils.py
  • newton/_src/geometry/types.py
  • newton/tests/test_sdf_compute.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • newton/_src/geometry/types.py
  • CHANGELOG.md

Comment thread newton/_src/geometry/sdf_utils.py
Comment thread newton/tests/test_sdf_compute.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
newton/tests/test_sdf_compute.py (2)

1517-1733: Consider a helper for the _is_watertight cache poking.

The pattern mesh._is_watertight = False / True / None is 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 calls SDF.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-level get_cuda_test_devices() pattern.

_sample_texture_sdf_at_points hard-codes device="cuda:0" while the rest of the module uses get_cuda_test_devices() / add_function_test for 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.device or 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

📥 Commits

Reviewing files that changed from the base of the PR and between 102e277 and 1bd1da0.

📒 Files selected for processing (2)
  • newton/_src/geometry/sdf_utils.py
  • newton/tests/test_sdf_compute.py

Comment thread newton/_src/geometry/sdf_utils.py

@adenzler-nvidia adenzler-nvidia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_watertight uses 100 nm quantized float32 tolerance — false negatives fall through to the winding path, which is the right disposition, but a tolerance: float = 1e-7 knob for power users who weld at sub-μm would be nice. Similarly, a one-line filter degenerate-triangle step 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_sdf doesn't forward sign_method to SDF.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_watertight cache has the same stale-after-in-place-edit gap as the existing _cached_hash/_edges caches — 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 neighbors comment on the block_coords_from_subgrid_required call in sdf_texture.py (~line 1451, preexisting) no longer matches the implementation (which now uses subgrid_occupied directly). 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.

Comment thread newton/_src/geometry/sdf_utils.py
Comment thread newton/_src/geometry/sdf_utils.py Outdated
Comment thread CHANGELOG.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1bd1da0 and bab0fea.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • newton/_src/geometry/sdf_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • newton/_src/geometry/sdf_utils.py

Comment thread CHANGELOG.md Outdated

@mzamoramora-nvidia mzamoramora-nvidia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. #2464 fixes target_voxel_size being dropped in the same sdf_utils.py:423-467 block — worth coordinating landing order.
  2. 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-251 covers it, but a Changed entry would help external callers.

# Conflicts:
#	newton/_src/geometry/sdf_utils.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Thread sign_method through Mesh.build_sdf().

SDF.create_from_mesh() exposes the sign_method parameter (selecting between "auto", "parity", and "winding" for mesh-to-SDF conversion), but Mesh.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_watertight can 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

📥 Commits

Reviewing files that changed from the base of the PR and between 896051f and cc4d974.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • newton/_src/geometry/sdf_texture.py
  • newton/_src/geometry/sdf_utils.py
  • newton/tests/test_sdf_texture.py
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md

Comment thread newton/_src/geometry/sdf_utils.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
newton/_src/geometry/sdf_utils.py (2)

376-379: ⚠️ Potential issue | 🟡 Minor

Align shape_margin docs 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_margin for compatibility. Either bake/apply the margin or update this public contract to match the behavior documented in extract_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 | 🟡 Minor

Don’t treat an empty texture struct as a populated SDF.

create_texture_sdf_from_mesh() can return create_empty_texture_sdf_data(), None, None, []; with this check, is_empty() returns False solely because texture_data is 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_watertight can 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

📥 Commits

Reviewing files that changed from the base of the PR and between 896051f and cc4d974.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • newton/_src/geometry/sdf_texture.py
  • newton/_src/geometry/sdf_utils.py
  • newton/tests/test_sdf_texture.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

Comment thread newton/_src/geometry/sdf_utils.py
@nvtw nvtw enabled auto-merge April 20, 2026 07:42
@nvtw nvtw added this pull request to the merge queue Apr 20, 2026
Merged via the queue into newton-physics:main with commit 89d07fb Apr 20, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants