Label declutter: screen-space overlap culling, on by default - #9
Open
aaltshuler wants to merge 1 commit into
Open
Label declutter: screen-space overlap culling, on by default#9aaltshuler wants to merge 1 commit into
aaltshuler wants to merge 1 commit into
Conversation
Dense clusters stacked their top-ranked labels into an unreadable pile — the selector ranked and viewport-culled but never checked where labels land on screen. Ranked selection now runs a greedy occupancy pass in rank order over estimated label boxes (fixed per-character width, cell-grid pruned): a colliding candidate passes its slot to the next-ranked one. showFor ids always render and claim their space first. New LabelConfig fields: overlap 'hide' (default) | 'allow', overlapPadding (px, default 2). Overlap-blind when the viewport cannot project. Seven new selector tests; scheduling-focused suites (and the react security fixture, whose subject is escaping) opt out explicitly with overlap: 'allow'. Verified live in the catalog's new Labels/Declutter story: the same 1,500-node fixture goes from stacked text smears to fully legible labels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SeFxK217ZrHSERqK6kjcb
| // intersection tests; without a projectable viewport there are no boxes | ||
| // and selection stays overlap-blind. | ||
| const declutter = config.overlap !== 'allow' && project !== undefined; | ||
| const pad = Math.max(0, config.overlapPadding ?? 2); |
There was a problem hiding this comment.
Infinite padding hangs selection
When a host supplies overlapPadding: Infinity, the clamp preserves infinity and the derived label box gives the occupancy grid infinite loop bounds. The loop counter starts at negative infinity and never advances, hanging label recomputation on the main thread.
Suggested change
| const pad = Math.max(0, config.overlapPadding ?? 2); | |
| const configuredPad = config.overlapPadding ?? 2; | |
| const pad = Number.isFinite(configuredPad) ? Math.max(0, configuredPad) : 2; |
Knowledge Base Used: Visual presentation and export
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Density had no spatial control: the label selector ranks and viewport-culls, but never checks where labels land on screen — so in dense clusters the top-ranked winners stack into an unreadable pile (they are all in the same blob by construction).
Fix
Greedy screen-space occupancy in rank order, the standard map-renderer declutter pass, inside
selectLabelCandidates:showForids always render and claim their space first.LabelConfigfields:overlap: 'hide' (default) | 'allow',overlapPadding(px, default 2). A cell grid prunes intersection tests; the pass runs only on the existing throttled re-rank triggers — never per frame.Default is on — same philosophy as the settle camera and fit clamp: the out-of-box render should be legible. FakeEngine projects identity coordinates, so decluttering engages in headless tests too; suites that pin label sets over tightly-packed fixtures opt out with
overlap: 'allow'(this repo's scheduling suites and the react security fixture now do, each with a comment saying why).Evidence
New
Labels/Declutterstory (in this PR): the same 1,500-node fixture withoverlap: 'allow'shows text smears in every cluster core; with the default, every rendered label is legible and spaced.Tests
Seven new selector tests: stacked-winner culling, slot promotion, showFor exemption + space claiming, 'allow' restores stacking, unprojectable-viewport fallback, padding widening, determinism. Full check green: 1,807 tests.
🤖 Generated with Claude Code
https://claude.ai/code/session_018SeFxK217ZrHSERqK6kjcb
Greptile Summary
The PR adds default-on, greedy screen-space overlap culling to ranked node-label selection, with opt-out and padding controls.
Confidence Score: 4/5
The PR needs a finite-value guard for overlap padding before merging because an accepted configuration can hang label recomputation on the main thread.
The new occupancy pass derives incrementing grid-loop bounds directly from
overlapPadding; positive infinity survives the current clamp and creates a loop whose negative-infinity counter can never advance.Files Needing Attention: packages/core/src/labels.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Visible candidates] --> B[Forced labels] B --> C[Claim forced label boxes] C --> D[Rank remaining candidates] D --> E{Projectable box?} E -- No --> F[Keep candidate] E -- Yes --> G{Intersects claimed box?} G -- Yes --> H[Skip and try next rank] G -- No --> I[Keep and claim box] F --> J{Capacity reached?} I --> J H --> J J -- No --> D J -- Yes --> K[Emit placements]Reviews (1): Last reviewed commit: "Label declutter: screen-space overlap cu..." | Re-trigger Greptile
Context used: