Follow-up: 9 review findings from PR #199 cherry-pick (fix on master AND release-7.0)

[aclark-aquaveo]

Follow-up to the code review on #199. That PR cherry-picked four commits from release-7.0 onto master (cb11fdd, 77d619a, ef6a0a1, 2b01a5f) to unblock xmssnap, which cannot compile against the master-based 9.0.11 release. The review raised 9 findings against the cherry-picked code. They were deliberately not fixed in #199 — see "Why not in #199" below.

Hard constraint: fix on both branches

This code is shipped in release-7.0 and tagged in 9.0.11. GmMultiPolyIntersectionSorterTerse.cpp is currently byte-identical between master and origin/release-7.0 (ignoring line endings), which is the state #199 existed to restore.

Any fix here must land on master and release-7.0 together. Fixing only one side recreates exactly the drift #199 was opened to repair.

Tier 1 — safe cleanup (no behavior change)

• Document or restore the deleted second RemoveCornerTouches(). Sort() previously called it after SwapAdjacents(); that call was removed, not supplemented. RemoveIntersectionsWithoutMatch is not a superset of it — the removed pass grouped near-equal t values via EQ_TOL(..., m_tol), while the new pass uses exact <= and keeps them as a valid pair. If the deletion is intentional, add a comment saying why; otherwise restore the call.
  xmsgrid/geometry/GmMultiPolyIntersectionSorterTerse.cpp:74

• Document the hasMatch.size() < 2 early return. A single unmatched intersection is preserved (and FixArrays synthesizes an exit), while two or more all-unmatched intersections clear m_ixs and yield empty polyIds/tValues/points. Either drop the lone orphan too, or document the guard — as written it reads accidental.
  xmsgrid/geometry/GmMultiPolyIntersectionSorterTerse.cpp:275

• Fix signed/unsigned comparisons and drop the vector copy. int i / int j are compared against m_d->m_ixs.size() / oldIx.size(), producing warnings; the rest of the file uses size_t. std::vector<xms::ix> oldIx = m_d->m_ixs; copies the whole vector to filter it — std::remove_if over hasMatch would do it in place.
  xmsgrid/geometry/GmMultiPolyIntersectionSorterTerse.cpp:280,287,308

• Document that 2D mode discards the caller's z range. The m_2dSearch branch overwrites bMin.z/bMax.z with -1/1, so a z range passed by a 2D-mode caller is ignored. Fine as an internal detail of PtsWithinDistanceToPtInRtree, but undocumented on the new public interface. Add a test exercising a 2D call with a meaningful z range so a future regression is caught.
  xmsgrid/geometry/GmPtSearch.cpp:553, xmsgrid/geometry/GmPtSearch.h:55

• Fix Doxygen \param names. Both new blocks document \param a_pt and \param a_distance, but the functions take a_min/a_max. The public overload's block also still says "a_ptIdx will never be included in this result" although it has no a_ptIdx parameter. Note: no CI job runs Doxygen today, so this is a documentation defect only, not a build warning.
  xmsgrid/geometry/GmPtSearch.cpp:532,574

Tier 2 — needs a reproducing case first

These four change the behavior of a shipped bug fix (issue 0015785, HydroAS arc snapping) or a shipped public API. Each mechanism was verified as reachable by code reading, but no reproducing input was produced. testBug15785 will not catch a regression introduced by "fixing" them. Get a failing test first.

• RemoveIntersectionsWithoutMatch runs after RemoveCornerTouches, compounding removals. RemoveCornerTouches can itself orphan half a legitimate entry/exit pair: FindPreviousNextNeither inspects only immediately adjacent t-groups, so a cell entered in a multi-member t-group and exited two or more groups later is classified inNeither and erased. Previously the orphaned exit survived and the cell still appeared in the output; now the new pass deletes the orphan too, so the cell vanishes and the neighbouring cell's span absorbs its interval. Running the new pass before RemoveCornerTouches, or teaching it about m_polys1/m_polys2, would avoid this.
  xmsgrid/geometry/GmMultiPolyIntersectionSorterTerse.cpp:71

• Greedy first-match pairing mispairs cells with an odd number of surviving intersections. For a cell with entry@0.1, vertex-graze@0.5, exit@0.9 (possible with a concave cell, or a line grazing a vertex mid-crossing), i=0 pairs 0.1 with the graze at 0.5 and breaks; 0.9 is then unmatched and erased. Output reports the cell ending at t=0.5 instead of 0.9 — wrong exit point plus a gap in the traversal. Correct behaviour would pair outermost-first, or drop the middle singleton.
  xmsgrid/geometry/GmMultiPolyIntersectionSorterTerse.cpp:280

• Exact floating-point <= on t values conflicts with the tolerance-based grouping used elsewhere in the file. FindWhereTValuesChange uses EQ_TOL(..., m_tol) and SwapAdjacents uses EQ_EPS(..., FLT_EPSILON). The comment says "not part of a future block", but the code's notion of "block" disagrees with the rest of the pipeline for any pair separated by less than m_tol (which TraverseLineSegment sets to min(minCellFraction * 1e-5, 1e-5)). Suggested: !EQ_TOL(m_d->m_ixs[j].m_t, t, m_tol) && m_d->m_ixs[j].m_t > t.
  xmsgrid/geometry/GmMultiPolyIntersectionSorterTerse.cpp:293

• Decide the inverted-box contract for PtsInBoxInRtree, then document it on the public header. box aBox(bMin, bMax) is built from the caller's arguments with no normalization, so corners passed in the other order make boost's covered_by match nothing. This is deliberate and pinned — 2b01a5f changed the baseline from {0, 1, 4} to {} and added a comment saying so — but the contract appears only in the test, not on the public interface. Note that normalizing with std::min/std::max would change behavior already released in 9.0.11 that xmssnap compiles against; documenting or asserting is the lower-risk option.
  xmsgrid/geometry/GmPtSearch.cpp:545, xmsgrid/geometry/GmPtSearch.h:55, test at xmsgrid/geometry/GmPtSearch.cpp:1111

Unrelated pre-existing defect (spotted while reviewing)

• FixTValueAtDuplicateXy() contains a loop with an empty body. Predates Add missing commits from release-7.0 branch #199 and is present on master and release-7.0 alike. Either restore the intended body or delete the loop.
  xmsgrid/geometry/GmMultiPolyIntersectionSorterTerse.cpp:85-88

Why not in #199

1. Add missing commits from release-7.0 branch #199 exists solely to make master match release-7.0 so a master-based tag unblocks xmssnap. Editing the code in that PR reintroduces the divergence it was opened to remove.
2. Tier 2 touches a shipped fix for 0015785 with no reproducing case for any of the hypotheticals. Changing intersection logic on theory risks re-breaking HydroAS arc snapping.
3. The inverted-box change would alter public API behavior that xmssnap already compiles against in 9.0.11.

Split this into per-finding issues if that suits the workflow better — grouped here to keep the both-branches constraint in one place.

[aclark-aquaveo]

This doesn't need to be fixed in the release-7.0 branch. Any changes just need to go into master now.