Skip to content

mesh: refit host solid-angle BVH from the real root, not node 0#1600

Open
EylonKrause wants to merge 1 commit into
NVIDIA:mainfrom
EylonKrause:fix/mesh-solid-angle-refit-root
Open

mesh: refit host solid-angle BVH from the real root, not node 0#1600
EylonKrause wants to merge 1 commit into
NVIDIA:mainfrom
EylonKrause:fix/mesh-solid-angle-refit-root

Conversation

@EylonKrause

@EylonKrause EylonKrause commented Jun 29, 2026

Copy link
Copy Markdown

Description

The host solid-angle BVH refit always starts the recursion at node index 0:

void bvh_refit_with_solid_angle_host(BVH& bvh, Mesh& mesh) { bvh_refit_with_solid_angle_recursive_host(bvh, 0, mesh); }

whereas the sibling non-solid-angle refit correctly starts at the real root — bvh_refit_host does bvh_refit_recursive(bvh, *bvh.root) (bvh.cpp). For a grouped BVH (groups != nullptr), build_with_groups finishes by calling reorder_top_down_bvh, which moves all leaf nodes to the front and sets *bvh.root to an index > 0 for a non-leaf root. Starting the solid-angle refit at node 0 (now a leaf) therefore computes solid_angle_props for only that single leaf and returns; the root and all other internal nodes keep their uninitialized solid_angle_props (allocated via wp_alloc_host, not zeroed). mesh_query_point_sign_winding_number traverses from *bvh.root, so it reads that garbage → wrong sign / winding-number results (and possible NaNs).

This is reachable from Python: wp.Mesh(..., groups=<array>, support_winding_number=True) on a CPU device passes both straight through to wp_mesh_create_host with no guard (only the cuBQL constructor rejects the combination). The CUDA path is unaffected — bvh_refit_with_solid_angle_device does a bottom-up refit over the leaf nodes via node_parents and never uses a literal root index.

Changes

  • warp/native/mesh.cpp: start the host solid-angle refit at *bvh.root, matching bvh_refit_host. For a non-grouped host build the root is already 0, so that path is unchanged; this only fixes the grouped case.

Testing

  • Static: the fix makes bvh_refit_with_solid_angle_host use the same root as its sibling bvh_refit_host (*bvh.root); the 0 was incorrect after reorder_top_down_bvh reassigns the root for grouped builds.
  • I was not able to build the native CPU library on my single-GPU WSL2 box, so I could not run a host repro / ASan locally — flagging that for the reviewer. A runtime check would build a grouped CPU mesh spanning >1 group with support_winding_number=True and compare mesh_query_point_sign_winding_number at known inside/outside points against the non-grouped (or CUDA) result; under ASan it would also surface the uninitialized solid_angle_props read.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed BVH solid-angle refitting to start from the correct root node, improving bound and solid-angle updates when this feature is enabled.

bvh_refit_with_solid_angle_host started the recursion at literal node 0,
while the sibling bvh_refit_host uses *bvh.root. For a grouped BVH,
reorder_top_down_bvh moves leaves to the front and sets *bvh.root > 0, so
starting at 0 computed solid_angle_props for a single leaf only and left
the root and internal nodes uninitialized. mesh_query_point_sign_winding_number
then reads that uninitialized memory, giving wrong winding numbers / NaNs
for a grouped CPU mesh built with support_winding_number=True. Start the
refit at *bvh.root (no-op for non-grouped builds, where root == 0).

Signed-off-by: EylonKrause <eylon1909@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Enterprise

Run ID: 7a43a4d2-d40f-447b-9e22-13af3a10f58e

📥 Commits

Reviewing files that changed from the base of the PR and between b8596fd and a86f492.

📒 Files selected for processing (1)
  • warp/native/mesh.cpp

📝 Walkthrough

Walkthrough

In bvh_refit_with_solid_angle_host, the recursive traversal now starts from *bvh.root instead of the hardcoded index 0.

BVH Root Fix

Layer / File(s) Summary
Use actual BVH root in solid-angle refit
warp/native/mesh.cpp
bvh_refit_with_solid_angle_host passes *bvh.root instead of 0 as the starting node to bvh_refit_with_solid_angle_recursive_host.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main fix: refitting the host solid-angle BVH from the actual root instead of node 0.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown

Greptile Summary

This single-line fix corrects a root-index bug in bvh_refit_with_solid_angle_host on the CPU path: it now starts the recursive refit at *bvh.root instead of the hardcoded literal 0, matching the behaviour of the sibling bvh_refit_host.

  • Root cause fixed: For grouped BVH builds, reorder_top_down_bvh moves all leaf nodes to the front and sets *bvh.root to a non-zero index; the old code started the solid-angle refit at node 0 (a leaf after reordering), leaving internal-node solid_angle_props uninitialized — causing wrong winding-number sign results and potential NaNs.
  • Scope: Only the CPU host path is affected; the CUDA path already uses a bottom-up leaf-based approach and is unchanged. Non-grouped builds keep *bvh.root == 0, so behaviour there is identical to before.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted correction to the CPU solid-angle BVH refit entry point.

The one-line change exactly mirrors the pattern already used by bvh_refit_host and is validated by reading reorder_top_down_bvh: leaf nodes land at the front, the true root is written to *bvh.root, and starting at index 0 was always wrong for grouped builds. Non-grouped builds are unaffected because their root remains 0. The CUDA path is independent and untouched.

No files require special attention. The only changed file, warp/native/mesh.cpp, has a clear, correct, and complete fix.

Important Files Changed

Filename Overview
warp/native/mesh.cpp One-line fix: replaces the hardcoded 0 root index in bvh_refit_with_solid_angle_host with *bvh.root, matching bvh_refit_host. Correct and minimal; the fix also benefits the refit path in wp_mesh_refit_host.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["wp_mesh_create_host / wp_mesh_refit_host"] --> B["bvh_refit_with_solid_angle_host(bvh, mesh)"]
    B -->|"BEFORE (bug): hardcoded index 0"| C["bvh_refit_with_solid_angle_recursive_host(bvh, 0, mesh)"]
    B -->|"AFTER (fix): real root"| D["bvh_refit_with_solid_angle_recursive_host(bvh, *bvh.root, mesh)"]

    C --> E{"Is node 0 a leaf?\n(grouped build: YES)"}
    E -->|"YES — leaf"| F["Compute solid_angle_props for node 0 only\nAll other nodes stay UNINITIALIZED"]
    E -->|"NO — internal"| G["Recurse correctly\n(non-grouped builds only)"]

    D --> H{"node = *bvh.root\ninternal or leaf?"}
    H -->|"internal"| I["Recurse into left + right children\nCombine solid_angle_props bottom-up"]
    H -->|"leaf (single-tri mesh)"| J["Compute solid_angle_props for that leaf"]
    I --> K["All nodes correctly initialized\nmesh_query_point_sign_winding_number reads valid data"]

    style F fill:#ff6b6b,color:#fff
    style K fill:#51cf66,color:#fff
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["wp_mesh_create_host / wp_mesh_refit_host"] --> B["bvh_refit_with_solid_angle_host(bvh, mesh)"]
    B -->|"BEFORE (bug): hardcoded index 0"| C["bvh_refit_with_solid_angle_recursive_host(bvh, 0, mesh)"]
    B -->|"AFTER (fix): real root"| D["bvh_refit_with_solid_angle_recursive_host(bvh, *bvh.root, mesh)"]

    C --> E{"Is node 0 a leaf?\n(grouped build: YES)"}
    E -->|"YES — leaf"| F["Compute solid_angle_props for node 0 only\nAll other nodes stay UNINITIALIZED"]
    E -->|"NO — internal"| G["Recurse correctly\n(non-grouped builds only)"]

    D --> H{"node = *bvh.root\ninternal or leaf?"}
    H -->|"internal"| I["Recurse into left + right children\nCombine solid_angle_props bottom-up"]
    H -->|"leaf (single-tri mesh)"| J["Compute solid_angle_props for that leaf"]
    I --> K["All nodes correctly initialized\nmesh_query_point_sign_winding_number reads valid data"]

    style F fill:#ff6b6b,color:#fff
    style K fill:#51cf66,color:#fff
Loading

Reviews (1): Last reviewed commit: "mesh: refit host solid-angle BVH from th..." | Re-trigger Greptile

@EylonKrause

Copy link
Copy Markdown
Author

Disclosure: this contribution was authored with an AI coding assistant (Claude) and reviewed before submission.

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.

1 participant