Add missing commits from release-7.0 branch

[aclark-aquaveo]

Missing commits prevent xmssnap compiling against xmsgrid 9.0.11

[gagelarsen]

Code review of the cherry-picked commits. I verified this is a faithful cherry-pick: the resulting GmMultiPolyIntersectionSorterTerse.cpp is byte-identical to origin/release-7.0 apart from one trailing whitespace, and the master-only fixes to that file (ad16e97, 0c24f36, 6f7de28) are all already ancestors of release-7.0, so nothing is being silently reverted. Nearly all of the GmMultiPolyIntersector.cpp churn is clang-format reflow of test code; the production changes are Sort / RemoveIntersectionsWithoutMatch and the new GmPtSearch::PtsInBoxInRtree API.

9 inline findings below (3 medium in the sorter, 1 medium in GmPtSearch, 5 low).

[gagelarsen]

[Medium] New pass runs after RemoveCornerTouches, which can already orphan half a legitimate pair.

RemoveIntersectionsWithoutMatch() is inserted after RemoveCornerTouches(), which can itself already have deleted one half of a legitimate entry/exit pair. FindPreviousNextNeither only inspects the immediately adjacent t-groups, so a cell entered in t-group i (a group with >1 member, e.g. a shared node) and exited two or more groups later has its entry classified inNeither and erased by the (i > 0 && i + 2 < tChange.size()) branch.

Previously the orphaned exit survived and the cell still appeared in the traversal output; now the new pass deletes the orphan too, so the cell vanishes entirely and the neighbouring cell's span silently absorbs its interval.

Running the new pass before RemoveCornerTouches (or teaching it about m_polys1/m_polys2) would avoid compounding the two removals.

[gagelarsen]

[Medium] The second RemoveCornerTouches() was deleted, not just supplemented.

That call ran after SwapAdjacents() and was the only corner-touch pass to see the post-swap ordering. RemoveIntersectionsWithoutMatch is not a superset of it: a corner touch that yields two intersections for the same cell at nearly equal (but not bitwise equal) t values is treated by the new pass as a valid entry/exit pair and kept, whereas the removed pass would have grouped them via EQ_TOL(..., m_tol) and dropped them.

If the deletion is intentional it deserves a comment explaining why the new pass subsumes it; otherwise the call should stay.

[gagelarsen]

[Low] hasMatch.size() < 2 guard makes the pass's contract size-dependent.

A single unmatched intersection is preserved here (and FixArrays then synthesizes an exit, so the cell is still reported), while two or more all-unmatched intersections wipe m_ixs and yield empty polyIds/tValues/points.

Either the lone orphan should also be dropped, or the guard should be documented; as written it reads like an accidental special case rather than a deliberate one.

[gagelarsen]

[Medium] Greedy first-match pairing mispairs cells with an odd number of surviving intersections.

Concrete case (possible with a concave UGrid cell, or a cell the line grazes at a vertex mid-crossing): cell A has entry@0.1, vertex-graze@0.5, exit@0.9. i=0 pairs 0.1 with the graze at 0.5 and breaks; 0.9 is then unmatched and erased.

The output reports A as 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.

[gagelarsen]

[Low] Exact floating-point <= on t values conflicts with the tolerance-based t-grouping used everywhere else in this 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)).

Suggest !EQ_TOL(m_d->m_ixs[j].m_t, t, m_tol) && m_d->m_ixs[j].m_t > t for consistency.

[gagelarsen]

[Low] Signed/unsigned comparison and an avoidable full-vector copy.

int i / int j are compared against m_d->m_ixs.size() / oldIx.size() at lines 280, 287 and 308, which produces signed/unsigned warnings; the rest of this file consistently uses size_t.

Also, std::vector<xms::ix> oldIx = m_d->m_ixs; (line 308) copies the whole vector just to filter it — a std::remove_if over hasMatch in place would avoid the copy. (Minor: m_ixs is small.)

[gagelarsen]

[Medium] New public box query silently returns nothing for an inverted box.

box aBox(bMin, bMax) is built straight from the caller's arguments with no normalization or validation. If the caller passes the corners in the other order (min > max in any coordinate), boost's covered_by silently matches nothing and the caller gets an empty result with no indication of misuse — commit fb0ca5d even changes the unit-test baseline to enshrine this (see GmPtSearch.cpp:1114).

Since this is a brand-new public API that downstream (xmssnap) will call with computed corners, normalize with std::min/std::max per component, or assert.

[aclark-aquaveo]

The missing documentation on the public interface is a real gap and it's in #200.

One correction to the premise though: this isn't an unnoticed edge case that the baseline change happened to enshrine. 2b01a5f (fb0ca5d here) changed that baseline from {0, 1, 4} to {} and added the comment above it — "The first point must be less than or equal to the second one in both coordinates or the search won't find anything." The inverted-box behavior was observed and deliberately pinned. What's missing is that the contract lives only in the test, not on GmPtSearch.h:55.

That distinction matters for the fix: normalizing with std::min/std::max would change behavior that shipped in 9.0.11, which is the tag xmssnap is being pointed at. Documenting the precondition on the header, or asserting on it, keeps the released contract intact. Tracked in #200 with that tradeoff noted — happy to go the normalization route if you think no caller depends on the empty-result behavior, but I'd want that decided deliberately rather than as a drive-by on a sync PR.

[gagelarsen]

[Low] 2D mode silently discards the caller's z range on the new public API.

The m_2dSearch branch overwrites bMin.z/bMax.z with -1/1, so a z range passed by a 2D-mode caller is ignored. That's fine as an internal detail of PtsWithinDistanceToPtInRtree, but it is undocumented on the new public interface (GmPtSearch.h:55), and the new testPtsInBox case never exercises a 2D call with a meaningful z range, so nothing catches a future regression here.

[gagelarsen]

[Low] Doxygen \param names don't match the signatures.

This block (and the one at line 574 for the public overload) documents \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.

With WARN_IF_UNDOCUMENTED = YES in Doxygen/Doxyfile, this produces warnings for the real (undocumented) parameters plus warnings for the non-existent ones.

[aclark-aquaveo]

The mismatch is real and it's in #200 — both blocks document a_pt/a_distance against an a_min/a_max signature, and the public overload still carries the "a_ptIdx will never be included in this result" line despite having no a_ptIdx parameter.

On the warnings claim: WARN_IF_UNDOCUMENTED = YES is set in Doxygen/Doxyfile, but nothing runs Doxygen in CI — grepping for doxygen across .github/workflows/XmsGrid-CI.yaml and the rest of the build files returns no hits. So this produces no warnings on any automated build today; it's a documentation defect for whoever reads the header or runs Doxygen locally. Doesn't change that it should be fixed, just that it isn't gating anything.

[aclark-aquaveo]

All 9 findings check out against the code — I verified each one rather than taking them on faith, and the mechanisms are real. Two notes where I'd adjust the framing are in the individual threads (inverted box, Doxygen warnings).

I'd like to merge this PR unchanged and take all 9 into #200 instead. Reasoning:

1. Fixing here reintroduces the drift this PR removes. GmMultiPolyIntersectionSorterTerse.cpp is currently byte-identical between this branch and origin/release-7.0 (ignoring line endings), and the four commits map 1:1 onto cb11fdd, 77d619a, ef6a0a1, 2b01a5f. The only prod-code delta against release-7.0 is a pre-existing testActivity2d baseline that master already carried, not something this PR introduces. Any edit made here puts master back out of sync with the release — the exact condition that produced this PR.

2. Four of the findings change shipped behavior with no reproducing case. The RemoveCornerTouches ordering, the greedy pairing, and the exact-<= t comparison all sit inside the shipped fix for 0015785. I traced each and agree the mechanism is reachable by code reading, but none of them came with an input that triggers it, and testBug15785 won't catch a regression introduced by changing that logic. Changing intersection behavior on theory risks re-breaking HydroAS arc snapping in a way we'd find in the field.

3. The inverted-box normalization would change released API behavior. xmssnap already compiles against PtsInBoxInRtree as tagged in 9.0.11.

4. This code is already published. It ships in release-7.0 and is tagged in 9.0.11. Whatever we fix has to land on master and release-7.0 together or we recreate the split — #200 records that as a hard constraint.

The blocker driving the timing: xmssnap cannot compile against the master-based 9.0.11 because these commits are absent. Merging as-is lets us tag and unblock it, then fix properly on both branches.

#200 splits the findings into a tier that's safe cleanup (2, 3, 6, 8, 9 — no behavior change) and a tier that needs a failing test first (1, 4, 5, 7). It also picks up an empty-bodied loop in FixTValueAtDuplicateXy() at GmMultiPolyIntersectionSorterTerse.cpp:85-88 that predates this PR and is on both branches.

If you'd rather see any of the tier-1 items land here before merge, say which and I'll add them — but that does mean master and release-7.0 diverge again from the moment it merges.

[gagelarsen]

works for me