Skip to content

fix: morphology per surface cluster - #3741

Open
aclauer wants to merge 13 commits into
mainfrom
andrew/feat/morphology-mem-fix
Open

fix: morphology per surface cluster#3741
aclauer wants to merge 13 commits into
mainfrom
andrew/feat/morphology-mem-fix

Conversation

@aclauer

@aclauer aclauer commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Contribution path

  • Small, safe change that does not need a tracking issue
  • Linked issue or discussion: DIM-XXX / #XXX / URL

Problem

We use dilation + erosion for closing holes in the surfaces in the mls planner. We used imageproc for this, but that allocates an entire 2d "image" per z level, and if there are stray voxels far away causes massive images to get allocated.

Solution

  • hand write the morphology
  • collect clusters of surfaces that cannot interact with each other from the morphology, then operate on those

Massive benefits in really big spaces, and we can get rid of imageproc and all the deps that come from that.

How to Test

AI assistance

Checklist

  • I have read and approved the CLA.

@aclauer aclauer changed the title Swap imageproc for our own refactor: morphology per surface cluster Aug 27, 2026
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

@@           Coverage Diff           @@
##             main    #3741   +/-   ##
=======================================
  Coverage   77.58%   77.58%           
=======================================
  Files        1315     1315           
  Lines      124697   124697           
  Branches    10867    10867           
=======================================
+ Hits        96748    96750    +2     
+ Misses      24816    24815    -1     
+ Partials     3133     3132    -1     
Flag Coverage Δ
OS-ubuntu-24.04-arm 74.23% <ø> (-0.01%) ⬇️
OS-ubuntu-latest 74.78% <ø> (+<0.01%) ⬆️
Py-3.10 74.78% <ø> (+<0.01%) ⬆️
Py-3.11 74.77% <ø> (-0.01%) ⬇️
Py-3.12 74.77% <ø> (-0.01%) ⬇️
Py-3.13 74.77% <ø> (ø)
Py-3.14 74.78% <ø> (-0.01%) ⬇️
Py-3.14t 74.77% <ø> (ø)
SelfHosted-Large 30.22% <ø> (+<0.01%) ⬆️
SelfHosted-Linux 34.74% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aclauer aclauer linked an issue Aug 28, 2026 that may be closed by this pull request
@aclauer aclauer changed the title refactor: morphology per surface cluster fix: morphology per surface cluster Aug 28, 2026
@aclauer
aclauer marked this pull request as ready for review August 29, 2026 01:09
@aclauer
aclauer marked this pull request as draft August 29, 2026 01:12
@greptile-apps

greptile-apps Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The morphology rewrite replaces the image-processing dependency stack with an in-tree distance transform and splits elevation slices into tile clusters. Two reliability problems remain in surfaces.rs: sparse diagonal tile layouts can still force disproportionately large dense grids, and valid coordinates at integer boundaries can overflow during morphology support checks. These failures should be addressed before merge.

Confidence Score: 1/5

The change is not ready to merge because surface extraction can be made resource-intensive by sparse geometry and can fail on valid boundary coordinates.

Two independently reproduced availability failures remain in the morphology implementation: one permits unbounded dense-grid growth from sparse input, and the other overflows support-neighbor arithmetic at coordinate limits.

Files Needing Attention: dimos/navigation/nav_3d/mls_planner/rust/src/surfaces.rs

Security Review

Map geometry that can be influenced externally can trigger denial of service in two ways: a sparse diagonal layout drives quadratic memory and CPU consumption, while supported morphology cells at valid coordinate limits panic during debug/test execution and wrap during release execution.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a finding-proof for the posted diagonal-chain P1 finding and shared the complete Rust diagonal-chain reproduction harness along with two-tile, 256-tile, and surface utility regression outputs.
  • T-Rex produced a focused Rust harness for i32 boundary morphology closing, along with the manifest used to compile it, captured the full source and manifest, and observed that the current surface unit test suite is passing.
  • T-Rex produced a finding-proof for a posted P1 finding in another area; see the review comment for details.
  • T-Rex performed general contract validation showing how 256 isolated cells form a diagonal chain and that the allocation-based close runs in 1.01s and uses about 33.3 MB for 256 cells, highlighting potential resource exhaustion risk if input can be remotely influenced.
  • T-Rex documented another general-contract-validation-proof examining i64 bounds, i32 overflow, and related debug outputs in surfaces.rs, with full source and outputs attached.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (3)

  1. dimos/navigation/nav_3d/mls_planner/rust/src/surfaces.rs, line 245 (link)

    P1 security Boundary support lookup overflows

    The widened bounds in close_at_z avoid the original padded-grid overflow, but a filled morphology candidate still reaches has_support, which evaluates ix + dx and iy + dy as i32. A real closing run with supported 3×3 rings at both coordinate extremes panics in debug/test builds at this expression; release builds instead wrap and can query columns from the opposite coordinate range. Compute support-neighbor coordinates with checked or widened arithmetic and skip values outside the i32 domain before looking them up.

    Artifacts

    Focused Rust harness source for i32 boundary morphology closing

    • The complete authored harness compiles the current repository surface implementation and drives closing at both coordinate extremes, with a meaningful filled-hole assertion; takeaway: it exercises the reported production path directly.

    Manifest used to compile the focused Rust harness

    • The complete authored Cargo manifest declares the exact ahash and rayon dependencies needed by the isolated real-source harness; takeaway: the harness execution is reproducible.

    Captured full source of the boundary closing harness and manifest

    • A command capture prints the full harness and its manifest from the uploaded artifact directory; takeaway: the exact executed test source is available for review.

    Debug run at the i32 minimum boundary panicking in support lookup

    • The focused harness ran morphology closing on a supported hole with valid cells at the i32 minimum-side boundary and captured an overflow panic at surfaces.rs line 245; takeaway: the minimum-side failure is reproducible.

    Debug run at the i32 maximum boundary panicking in support lookup

    • The focused harness ran morphology closing on a supported hole with valid cells at the i32 maximum-side boundary and captured an overflow panic at surfaces.rs line 245; takeaway: the maximum-side failure is reproducible.

    Release observation for both i32 boundary closing cases

    • The optimized harness execution completed both boundary cases with nine cells and filled centers, demonstrating release wrapping avoids the panic but changes the arithmetic model; takeaway: debug/test availability failure remains while release behavior needs safe bounds handling.

    Current surface unit test suite passing

    • The project surface test suite ran all ten current surface tests successfully, including boundary_coordinates_do_not_overflow; takeaway: existing coverage does not reach the overflowing has_support branch.

    View artifacts

    T-Rex Ran code and verified through T-Rex

  2. General comment

    P1 Diagonal tile connectivity permits quadratic dense-grid allocation from sparse input

    • Bug
      • interaction_clusters treats diagonally adjacent occupied tiles as connected even when their only cells are far apart. A 256-cell diagonal chain was joined into one cluster, and close_at_z allocated/scanned a 4,083×4,083 u16 distance grid (16,670,889 elements / 33,341,778 bytes) although the result contained only the 256 original cells.
    • Cause
      • Tile graph traversal accepts every 8-neighbor tile (dx and dy each range from -1 through 1) without validating the distance between cells across those tiles. The resulting cluster's extrema are passed to close_at_z, which unconditionally materializes vec![INF; w * h] for the entire bounding box.
    • Fix
      • Partition clusters using a geometric interaction criterion bounded by the morphology radius (or use a sparse/tiled distance-transform representation), and impose an explicit maximum grid-area/resource budget before allocating w * h.

    T-Rex Ran code and verified through T-Rex

  3. General comment

    P1 Morphology closing still panics at i32 coordinate boundaries through support-neighbor arithmetic

    • Bug
      • Valid cells adjacent to either i32 coordinate limit cause extract_surfaces with closing enabled to panic in debug/test builds. The focused harness uses a valid supported 3×3 ring at each boundary and confirms both i32-min and i32-max cases panic while morphology closing is evaluating candidate support.
    • Cause
      • Although close_at_z now widens padded coordinate bounds and reconstruction arithmetic to i64 (surfaces.rs:263-283, 300-303), it calls has_support at surfaces.rs:309. has_support performs ix + dx and iy + dy as i32 at line 245 for offsets -3..=3. At boundary candidates those additions overflow. In release mode this is unchecked wrapping, so it may silently probe unrelated opposite-side columns instead.
    • Fix
      • Use widened or checked/saturating coordinate arithmetic for the support lookup. Compute neighbor coordinates in i64 and skip values outside i32 range before converting and querying by_col (or use checked_add for each axis). Add focused min- and max-boundary regression tests that exercise a filled candidate and therefore reach has_support, rather than only diagonal extreme original cells.

    T-Rex Ran code and verified through T-Rex

Reviews (2): Last reviewed commit: "Merge branch 'main' into andrew/feat/mor..." | Re-trigger Greptile

Comment thread dimos/navigation/nav_3d/mls_planner/rust/src/surfaces.rs
Comment thread dimos/navigation/nav_3d/mls_planner/rust/src/surfaces.rs Outdated
@aclauer
aclauer marked this pull request as ready for review August 29, 2026 19:19
Comment thread dimos/navigation/nav_3d/mls_planner/rust/src/surfaces.rs
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Aug 29, 2026
@github-actions github-actions Bot added ready-to-merge Required CI checks have passed on this PR and removed ready-to-merge Required CI checks have passed on this PR labels Aug 31, 2026
@github-actions github-actions Bot added ready-to-merge Required CI checks have passed on this PR and removed ready-to-merge Required CI checks have passed on this PR labels Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Excessive memory in morphological operations in mls planner

1 participant