Skip to content

fixed an indexing bug in MeshToVolume.cuh - #2306

Merged
kmuseth merged 1 commit into
AcademySoftwareFoundation:masterfrom
kmuseth:fix_VolumeToMesh_bug
Aug 31, 2026
Merged

fixed an indexing bug in MeshToVolume.cuh#2306
kmuseth merged 1 commit into
AcademySoftwareFoundation:masterfrom
kmuseth:fix_VolumeToMesh_bug

Conversation

@kmuseth

@kmuseth kmuseth commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Fix index error in MeshToVolume.h's checkNeighbours

Fix out-of-bounds neighbour offset in mesh_to_volume_internal::checkNeighbours

In the (pos, data, mask) overload of checkNeighbours, the i+1,j,k-1 branch
(mask[6]) reused the offset for i+1,j,k instead of applying the -1 z offset,
so it silently checked the wrong voxel's value. This let a negative
neighbour go undetected during the self-intersection checks in mesh-to-volume
conversion (ValidateIntersectingVoxels / RemoveSelfIntersectingSurface),
which could leave incorrect boundary voxel distances or fail to remove
self-intersecting surface voxels near that specific neighbour direction.

Bugfix contributed by hexuejun (replaces this PR)

Added TestMeshToVolume.testCheckNeighboursOffsets, which checks all 26
neighbour offsets used by checkNeighbours against the canonical
util::COORD_OFFSETS table using a synthetic leaf-node buffer, so the same
class of copy-paste offset error is caught for any of the 26 branches, not
just this one. Verified the new test fails at exactly offset 6 ([1,0,-1])
against the pre-fix code and passes clean post-fix, with no regressions in
the existing TestMeshToVolume/TestVolumeToMesh suites.

Signed-off-by: Ken Museth <ken.museth@gmail.com>

@danrbailey danrbailey 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.

Hi Ken, the PR itself looks good. Just a minor comment - the point of the pendingchanges/ folder is that each PR should have its own unique changes file. This means that merging PRs remains independent of each other (no merge conflicts). We shouldn't be adding changes to an existing pendingchanges file or that defeats the point.

I also spotted this new PolySoupToLevelSet tool. I'm not sure I have heard what this is, it appears to have been merged as part of this big vdb_tool PR that went in recently (#2241). I would prefer that changes to the core library have their own PR if possible, especially substantial new developments like this, because the requirements for merging into the core library should be more rigorous IMO. The code comments reference some upcoming publication, but it would be great if we could hear a bit more about this tool in a future TSC meeting if possible? I'm curious to know more.

@kmuseth

kmuseth commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

happy to share more details about the PolySoupToLevelSet tool, but that's unrelated to this PR

@kmuseth
kmuseth merged commit c81d904 into AcademySoftwareFoundation:master Aug 31, 2026
44 checks passed
@kmuseth

kmuseth commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

forgot to address your comments about that change log - let me move it to a separate file

@kmuseth

kmuseth commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Actually, the review comment about change logs belonging to seperate files involves far more than this PR, so let me prepare a separate PR that addresses this broadly

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.

2 participants