fixed an indexing bug in MeshToVolume.cuh - #2306
Conversation
Signed-off-by: Ken Museth <ken.museth@gmail.com>
danrbailey
left a comment
There was a problem hiding this comment.
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.
|
happy to share more details about the PolySoupToLevelSet tool, but that's unrelated to this PR |
|
forgot to address your comments about that change log - let me move it to a separate file |
|
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 |
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.