BUG: Fix wrong output and invalid input handling in 19 filters

[imikejackson]

Summary

This change fixes defects that a code review found in filters that do not have a V&V report. Each defect makes a filter give wrong output, read or write past the end of an array, crash, or fail with a valid configuration. Each fix has a regression test that fails on develop. There is one commit for each filter or shared utility. No test data archive is added or changed. 70 files changed, +2612 / −318 lines.

Behavior changes

• Copied child objects keep their names. When CropImageGeometry, ResampleImageGeom, PadImageGeometry, RotateSampleRefFrame, RemoveFlaggedVertices or CropEdgeGeometry write to a new geometry (not in place), a child object whose name contains the source geometry name is no longer renamed. For example, Data/Cell Feature Data copied to Cropped now gives Cropped/Cell Feature Data, not Cropped/Cell Feature Cropped. A saved pipeline that refers to the old, incorrect name must be updated.
• New parameter component shapes. The Feature Ids parameter of ComputeFeatureBounds and the Mask Array and Cluster Ids parameters of Silhouette accept only 1-component DataArrays. Other shapes fail parameter validation (−208).
• New preflight requirements. ComputeLargestCrossSections (−3711) and SurfaceNets (−56350 for Cell Feature Ids, −56351 for the selected cell DataArrays) require each cell-level input DataArray to have one tuple for each cell of the Image Geometry. The DataArray can be in any Attribute Matrix, and the Image Geometry does not need an assigned Cell Data Attribute Matrix.
• WriteASCIIData Multiple Files mode rejects two selected DataArrays whose names are equal when case is ignored (−51003), because the macOS and Windows file systems ignore case.
• ReadImage: the parameter "Put Input Origin at the Center of Geometry" is renamed "Center Geometry on (0, 0, 0)". The key and the behavior do not change.
• RegularGridSampleSurfaceMesh: Use Custom Output Type is now directly above Output Type for Feature Ids and is linked to it. The keys do not change.

================================================================================

1. Shared utilities
   ================================================================================

src/simplnx/DataStructure/DataPath.hpp/.cpp
src/simplnx/Utilities/ImageProcessing/ImageGeometryCrop.cpp
src/simplnx/Utilities/ImageProcessing/ImageGeometryResample.cpp

• DataPath::rebase(oldPrefix, newPrefix) replaces the leading components of a path. It compares whole components, so the child Edge Geometry Ids does not start with Edge Geometry. DataPath::attemptRename now uses it. On develop, attemptRename is correct only when the old and new prefixes have the same depth. Pipeline rename tracking uses only prefixes of the same depth, so its behavior does not change.
• Seven places that copy the children of a geometry or a DataGroup built the new path by replacing the source name everywhere in the path string. They now use DataPath::rebase. The places are CropEdgeGeometry, RotateSampleRefFrame, RemoveFlaggedVertices, PadImageGeometry, CopyDataObject, ImageGeometryCrop and ImageGeometryResample. On develop, a parent DataGroup whose name contains the geometry name is renamed too, so the copy fails with −5102.
• A physical crop whose maximum equals the upper bound of the Image Geometry (origin + dimensions × spacing) is accepted. On develop, it fails with −5553, while a slightly larger value is clamped. This fixes ReadImage, CropImageGeometry, ReadZeissTxm, ReadNIfTI and ReadMha.
• src/simplnx/Utilities/ImageIO/ImageStackCropping.hpp: the same fix for the Z axis of an image stack. A physical Z crop maximum equal to origin + number of files × spacing is accepted. On develop, it fails with −23525.

================================================================================
2. Image readers

src/Plugins/ImageProcessing/src/ImageProcessing/Filters/

• ReadImage: a physical crop whose minimum is below the origin is clamped at execute, as preflight already does. On develop, execute fails with −2002. Preflight now returns the warnings of the crop helper, so the −50503 "clamped" warning reaches the user.
• ReadImageStack: preflight and execute calculate the Z slice range of the full stack with one helper. On develop, a physical Z crop that does not start at the first slice fails at execute. A Z crop also moves the Z origin to the first kept slice, as X and Y crops do. Convert to Grayscale on an image with 1 or 2 components fails in preflight (−23507). On develop, it reads neighboring values or reads past the end of the buffer.

================================================================================
3. SimplnxCore filters

• ComputeFeatureBounds: the edge geometry contains a box for every feature. On develop, the last box is lost. Each edge has the Feature Id of its feature, not the position of the box. The internal bounds buffer is sized by the Feature Data Attribute Matrix, so extra tuples are written as NaN instead of read past the end of the buffer.
• ComputeLargestCrossSections: a Feature Ids DataArray whose tuple count is not the number of cells is rejected in preflight (−3711). On develop, a shorter DataArray is read past its end, and the values read are used as an index for a write. A correctly sized DataArray in another Attribute Matrix is still accepted.
• Silhouette: the Direct algorithm maps sparse Cluster Ids such as {3, 7} to dense indices and rejects negative ids (−54081). On develop, both write past the end of a table. With Use Mask on, preflight checks that the mask, the Cluster Ids and the input DataArray have the same number of tuples (−8977). Points in cluster 0 use the mean distance to their own cluster, not the sum.
• RegularGridSampleSurfaceMesh: a custom output type different from the Face Labels type works. On develop, it fails with bad_cast. A Face Labels value that does not fit in the output type is an execute error (−11801). A Face Label below 0 is treated as 0. On develop, the −1 exterior label that QuickSurfaceMesh and SurfaceNets write is copied into the output, so the rest of the row becomes −1.
• QuickSurfaceMesh and SurfaceNets: the Direct algorithm checks that each selected feature DataArray has more tuples than the largest Feature Id (−62073), as the Scanline algorithm already does. On develop, Direct reads past the end of the DataArray and returns success.
• SurfaceNets: Cell Feature Ids (−56350) and each selected cell DataArray (−56351) must have one tuple for each cell. On develop, a shorter DataArray is read past its end. With smoothing on, a negative Max Distance from Voxel Center (−56352) or Relaxation Iterations (−56353) fails in preflight. On develop, the negative distance is undefined behavior in std::clamp.
• WriteASCIIData: on develop, two selected DataArrays with the same name in Multiple Files mode write the same file, and the second one overwrites the first.
• CropEdgeGeometry: in Interpolate mode, a vertex outside the crop bounds that two crossing edges share is duplicated, one copy for each clip point. On develop, both edges use one clip point, so one edge moves off its line. StringArray and NeighborList data in the Vertex and Edge Attribute Matrices are copied with the kept vertices and edges. On develop, they make preflight throw bad_cast.

Test plan

• Each new regression test passes. Each one failed on develop for the reason that its commit message gives.
• The tests of filters with an in-core and an out-of-core algorithm run in each in-memory algorithm scenario.
• The full test suite passes (3123 of 3124 locally on macOS; the one test not run needs Python, which the local build does not include, as on develop).
• The example pipelines that use the changed filters preflight without new errors.
• CI passes on all supported platforms.

[joeykleingers]

PR 1765 Code Review: BUG: Fix wrong output and invalid input handling in 19 filters

Reviewed commit 6b43dc5 (imikejackson, 13 commits, compared against merge base c008bcc on develop). Coverage: all 69 changed files. I read every source, header and doc diff in full and checked each fix against the develop code. For the 17 test files, I listed every added test case and read the CropEdgeGeometry and CopyDataObject tests in full. I did not read the other 15 test files line by line.

Findings

• [Low] ReadImageStack still rejects a physical Z crop whose maximum is exactly the upper bound
  File: src/Plugins/ImageProcessing/src/ImageProcessing/Filters/Algorithms/ReadImageStackCropping.hpp:46 (ComputeCroppedZRange → ComputeCroppedZDimension)
  This PR makes a crop maximum of origin + dims × spacing valid for ReadImage, CropImageGeometry, ReadZeissTxm, ReadNIfTI and ReadMha, and ReadImageStack's X/Y crops get the fix through ReadImage. The Z axis doesn't. With 3 files, origin 0 and spacing 1, zBoundPhysical[1] = 3.0 passes the range check at ImageStackCropping.hpp:82. Then floor(3.0) = 3 >= zDim fails with -23525. So in one filter, X/Y accept the upper bound and Z rejects it. This bug predates the PR, but the PR fixes the same bug everywhere else. Fix: clamp zMaxIndex to zDim - 1 in ComputeCroppedZDimension, the way ImageGeometryCrop now does, and add a Crop_Physical_Z_MaxAtBound test. Any other caller of that shared helper gets the same fix.

• [Low] The new location checks also reject Feature Ids that have the right size but sit in another Attribute Matrix
  Files: src/Plugins/SimplnxCore/src/SimplnxCore/Filters/ComputeLargestCrossSectionsFilter.cpp:122, src/Plugins/SimplnxCore/src/SimplnxCore/Filters/SurfaceNetsFilter.cpp:161, :168
  The bug was reading past the end of a short DataArray. The fix rejects any DataArray whose parent isn't the geometry's Cell Data Attribute Matrix. That also rejects a Feature Ids array with exactly getNumberOfCells() tuples stored in another Attribute Matrix of the same shape, which worked on develop. The PR body states this behavior change, so it's a design question rather than a bug: comparing the tuple count with imageGeom.getNumberOfCells() would prevent the out-of-bounds read without breaking those layouts. Also, -3711 and -56350 are each used for two different conditions ("no Cell Data" and "wrong parent"). Separate codes would make the failure tests more specific.

• [Low] No test covers the CopyDataObject fix when "Use New Parent" is on
  File: src/Plugins/SimplnxCore/src/SimplnxCore/Filters/CopyDataObjectFilter.cpp:132
  On develop, the created paths were made by replacing the target name in the path string. With Use New Parent on, that kept the old parent (Group/A_copy/child), but the object is copied to NewParent/A_copy/child. Using newDataPath in rebase fixes that case too, but the new test only checks the suffix case under the same parent. Add a section with k_UseNewParent_Key = true and a group child, and check that the declared paths are under the new parent.

• [Nit] Anonymous namespace in a header
  File: src/Plugins/ImageProcessing/src/ImageProcessing/Filters/Algorithms/ReadImageStackCropping.hpp:6
ZRange and ComputeCroppedZRange are defined in an unnamed namespace inside a header that two .cpp files include, so each file gets its own copy. This is the CERT DCL59-CPP pattern. Put them in nx::core (the function is already inline). The header also relies on other includes for std::floor and fmt::format. Include <cmath>, <fmt/format.h> and simplnx/Common/Result.hpp directly.

• [Nit] Redundant comparison in the output-range check
  File: src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/RegularGridSampleSurfaceMesh.cpp:419
minLabel > outputMax || maxLabel > outputMax: minLabel <= maxLabel, so the first comparison never decides the result. Use maxLabel > outputMax.

C++ checks

• Memory/lifetime: no issues. The CropEdgeGeometry parallel tasks borrow sourceIndices, and taskRunner.wait() returns before that vector is destroyed. NeighborList::setValue takes a copy of the vector, so duplicated rows don't share storage with the source.
• Thread safety: no issues. In CopyAttributeArrays, StringArray and NeighborList data is copied on the calling thread while worker tasks write other arrays. In RegularGridSampleSurfaceMesh, the converted slice is written while the existing mutex is held.
• Efficiency: no issues worth raising. RegularGridSampleSurfaceMesh allocates one converted buffer per slice, and only for a custom output type.
• Formatting: clang-format 19.1.1 with the root .clang-format, dry run on all 50 changed C/C++ files: pass. CI's clang_format_pr check also passes.

Documentation

No issues found. The 19 doc changes match the code. For example, the -50503 clamp warning that ReadImage now passes through exists in ImageGeometryCrop.cpp:172-180, and the Silhouette text on the cluster-0 mean matches both algorithms.

Missing or failed evidence

• I listed every added test case in 15 of the 17 test files and checked that it targets the stated defect, but I didn't read those test bodies line by line. CI passing confirms they run and pass. I didn't confirm locally that each one fails on develop; that comes from the PR description.
• No local build or test run.

Confirmed correct

Each defect below is confirmed against the develop code:

• DataPath::rebase / attemptRename: on develop, attemptRename overwrote only oldPath.getLength() components. A shorter newPath read past its end, and a longer one was cut short. rebase compares whole path components (Edge Geometry ≠ Edge Geometry Ids) and handles prefixes of any depth. Each call site's .value() can't fail, because child paths always start with the source path.
• Child renaming by substring (7 places): StringUtilities::replace replaced every occurrence in the full path string, so Cell Feature Data became Cell Feature Cropped. All 7 call sites now use rebase.
• Crop at the upper bound: floor((max − origin)/spacing) equals dims when max is exactly the upper bound, which tripped the xMax > numCells − 1 error (-5553). Clamping with std::min fixes it, and a value past the bound still goes to the existing clamp and warning branch.
• ReadImage minimum below the origin: on develop, execute computed a negative voxel index and failed with -2002, while preflight clamped. Execute now clamps to 0 as well.
• ReadImageStack Z range: on develop, execute converted physical Z through the destination geometry, which is already cropped and offset, so any crop not starting at slice 0 mapped to the wrong slice. Preflight and execute now use the same ComputeCroppedZRange. The Z origin shift is replaced by a postprocessed override, which runs as a deferred UpdateImageGeomAction (ReadImageStackFilter.cpp:395-400).
• ComputeFeatureBounds: the currentOffset-- before resizing dropped the last box, and edge Feature Ids held the box index. The bounds buffer is now sized by the Feature Data Attribute Matrix, which the Split and Unified output loops already iterate over.
• Silhouette Direct: the distance table had numClusters + 1 columns but was indexed by raw id, so sparse ids wrote past its end. The dense mapping matches the Scanline path. Empty clusters (ids found only in masked-out tuples) are left out of both the division and the minimum. Cluster 0 is now divided by its count, which matches the Scanline change.
• RegularGridSampleSurfaceMesh: getDataRefAs<DataArray<T>> used the Face Labels type, so it threw bad_cast when the output type was different. The range check runs before workers are scheduled, and negative labels are clamped to 0 before the toggle logic.
• QuickSurfaceMesh/SurfaceNets Direct: the selected feature DataArrays are now checked against the largest Feature Id, matching the Scanline path. The removed preflight variables were unused. A negative Max Distance gave std::clamp a low bound above its high bound, which is undefined behavior; preflight now rejects it.
• WriteASCIIData: PrintDataSetsToMultipleFiles names each file <dir>/<targetName><ext> (OStreamUtilities.cpp:557), so two arrays with the same name wrote the same file.
• CropEdgeGeometry: on develop, there was one clip point per outside vertex, so a second crossing reused the first clip point. The (vertex, clipPoint) key gives each crossing its own vertex, extra copies are added after the originals, and the else branch never reads srcVertices for a duplicate. On develop, StringArray and NeighborList preflight failed at dynamic_cast<const IDataArray&> with bad_cast.
• CI: passes on macOS, both Ubuntu toolchains, Windows, and clang-format.

Verdict

APPROVE: every fix addresses a confirmed defect on develop and has a matching regression test. The findings are a related upper-bound case the PR doesn't cover, one design question, one untested branch, and two nits; none of them blocks merging.