perf: index label route clearance candidates - #210
Conversation
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed at exact head f7dc11958a220d41c0a2cd0a7ff36685483688d2. The spatial broad-phase is a valuable optimization, and I verified that ordinary finite-coordinate behavior remains identical in a 2,000-case differential check. However, two merge blockers remain:
- Unsafe grid coordinates can make index construction or lookup loop forever.
cellCountmay still be a safe integer even whenx1/x2ory1/y2is not; at that magnitude,x += 1ory += 1no longer changes the value. Route indexing should put such routes in the global set, while label queries should fall back to the full route list. archify.zipwas not regenerated. The sourcegeometry.mjsand the packaged copy have different SHA-256 hashes, so source users receive the optimization while ZIP installs keep the old implementation. Please rebuild the canonical archive with Node 22 after the code fix.
Please also commit regression coverage for unsafe finite coordinates and the unselective-threshold fallback. The PR currently has no GitHub checks and is behind main; after updating it, the exact new head and package freshness need to pass CI.
f7dc119 to
bca9add
Compare
|
Addressed review blockers in latest head |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 SummaryReviewed base Walkthrough
Estimated code review effort: 3 (Moderate) | ~25 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tt-a1i
left a comment
There was a problem hiding this comment.
Re-reviewed current head 86ebff2 on dev2d87fec. The unsafe-coordinate loop finding is fixed and the package is current. The private spatial index retains the exact clearance calculation and deterministic ordering; its fallbacks bound index construction without changing authored geometry.
Verified 2,000 seeded differential cases, 201 focused geometry/layout/checker tests, current-head 66 gate/geometry tests, package freshness and unchanged golden output. The sparse 3,000-route benchmark shows a substantial stage-level improvement; no end-to-end generation speedup is inferred. Final-head CI35134509724 passed all Node, shared-browser/WebM, package and Windows gates. No blocking finding remains; ready for dev integration.
Problem and value
Large diagrams currently compare every relationship label against every route to measure clearance. A conservative grid index removes distant candidates before the existing exact segment calculation. It preserves diagnostic ordering, distances and segment identities, with full-scan fallbacks for large routes, unselective queries and unsafe grid coordinates.
The prior non-terminating-loop review is addressed: all four grid coordinates must be safe integers before either nested loop. Unsafe routes enter the global candidate set; unsafe label queries use the full route list. No dependency, authored geometry, schema, validation threshold or version changes.
Integration and cost
Original
bca9add69604be0f38292eb07472bdf5fcdff509history is preserved. Current head86ebff230f6ab11f1ccfc6be863e7a9456ae64b7integrates dev2d87fec2e555f1dd266c4fbb2a866bc744034343, including #449, #416 and the shared browser gate #440. The merge retains dev's large-receipt minimum-clearance regression alongside the indexing tests.The index is private to one shared clearance function, with bounded per-route indexing and deterministic ordering. Its benefit is strongest for sparse, larger diagrams; dense/unselective inputs can still require the original full comparison. This is not a claim that overall diagram authoring is hundreds of times faster.
Verification
5440dbb, geometry/layout/render-output suites: 201 passed, 0 failed, 0 skipped.128a8aaproduces identical complete results, including hit order, distances and segment indexes. Cases include negative and unsafe coordinates, duplicate identities, diagonal/multisegment routes, varied rectangles, zero/negative/nonfinite/unselective thresholds. The clearance implementation is unchanged by the subsequent dev sync/package commit, so this evidence is reused with that scope.bc26a22geometry + package gates (unchanged production payload after fix(ci): share required browser regressions with releases #440 sync): 82 passed, 0 failed, 2 explicit platform/version skips. Canonical ZIP freshness is checked. Golden renderer/source/package examples remain byte-identical; no new visual acceptance is claimed because the generated diagrams are unchanged.Generated artifacts
Rebuilt only
archify.zipwith official Node 22.23.2 from tracked combined source. Renderer examples remain byte-identical, so Gallery/proof/Viewer outputs need no regeneration.mainremains unchanged; this PR targets dev integration only.