Skip to content

fix(router): key obstacle cache by effective exclusions - #5105

Open
cangen185-sudo wants to merge 3 commits into
antvis:masterfrom
cangen185-sudo:codex/fix-manhattan-obstacle-cache
Open

cangen185-sudo wants to merge 3 commits into
antvis:masterfrom
cangen185-sudo:codex/fix-manhattan-obstacle-cache

Conversation

@cangen185-sudo

Copy link
Copy Markdown
Contributor

📝 Description

Keep Manhattan obstacle-map reuse consistent with the actual cells excluded for each edge. Two edges can have identical router options but different excluded terminals or terminal ancestors. Previously the second edge reused the first edge's map, allowing a route through a node that should be an obstacle, or treating its own enclosing group as an obstacle.

Compute the terminal/ancestor exclusion set with the same helper used by ObstacleMap.build, include its sorted IDs in a structured cache key, and retain reuse when exclusions are equivalent. Serialize option arrays as JSON to distinguish IDs such as a|b from the two IDs a and b. Invalidate cached geometry when node visibility or shape changes.

🖼️ Screenshot

The regression calls the production Manhattan router with real Model, Node, and Edge instances and a geometry-only EdgeView fixture. After warming the cache with an edge that excludes its source, another edge must take an orthogonal detour around that node. Every resulting segment is checked against the padded obstacle; fallback routing throws in this test.

💡 Motivation and Context

ObstacleMap.build(model, edge) depends on the current edge's terminals and ancestors, while the previous cache key included only router option values. Visibility and shape also affect obstacle membership but did not invalidate the cache.

The cache retains one entry per model. Switching between different exclusion sets rebuilds it; equivalent exclusions still share the map. This is a correctness fix and does not claim a performance improvement on grouped or terminal-excluding graphs.

🧩 Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Test Case (changes that add missing tests or correct existing tests)

🔍 Self Check before Merge

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed. The focused suites below passed; the full repository suite was not run.

Validation on Windows, against master at 306eddb03fd5313d75d9a3ce084722f44475da01:

  • Before the fix: 6 of the first 8 new cache tests failed; the equivalent-exclusion and position/size controls passed.
  • Final router suite plus Cell and Edge tests: 15 files, 159 tests passed, including 9 new real-model regressions and the unmocked routing case.
  • tsc --noEmit, Biome on both changed files, and git diff --check passed.
  • New tests cover different terminals, different enclosing groups, reparenting, hide/show, shape changes, delimiter-containing IDs, map reuse, geometry invalidation, and actual routing.

AI assistance: implementation and test drafting assisted by OpenAI Codex; the patch was reviewed and locally tested.

@codecov-commenter

codecov-commenter commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.81%. Comparing base (306eddb) to head (6052075).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/registry/router/manhattan/obstacle-map.ts 97.50% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5105      +/-   ##
==========================================
+ Coverage   83.78%   83.81%   +0.03%     
==========================================
  Files         338      338              
  Lines       39416    39407       -9     
  Branches     8164     8179      +15     
==========================================
+ Hits        33023    33028       +5     
+ Misses       6369     6355      -14     
  Partials       24       24              
Flag Coverage Δ
x6 83.81% <97.50%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/registry/router/manhattan/obstacle-map.ts 98.51% <97.50%> (+0.60%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cangen185-sudo

Copy link
Copy Markdown
Contributor Author

Follow-up to the coverage report: commit 60520752a72d0810cb60f325b545101787429bc0 adds a real-model regression for excludeNodes specified as a Node instance. It checks that an indexed Node is excluded, a foreign Node with the same ID is not excluded, an ineffective foreign-node exclusion shares the ordinary map, and the string-ID form still excludes the indexed node.

This covers the three missing identity-selection lines. The remaining changed uncovered line is the defensive missing-paddingBox branch in the cache-key helper. ResolvedOptions requires this geometry and ObstacleMap.build() also consumes it, so I did not add an invalid-options test just to raise coverage.

Local validation on this head: 160 router/Cell/Edge tests passed, including the production Manhattan routing case; targeted V8 coverage, TypeScript, Biome on the changed test, and diff checks passed. Production code is unchanged in this follow-up. Hosted CI for this new head is tracked separately from these local results.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants