fix(donut-s2): Direct/Advanced label collision avoidance and ring-overlap truncation - #905
Open
madelineluke wants to merge 3 commits into
Open
fix(donut-s2): Direct/Advanced label collision avoidance and ring-overlap truncation#905madelineluke wants to merge 3 commits into
madelineluke wants to merge 3 commits into
Conversation
…rktree Restores the WIP collision cascade algorithm (donutLabelCollisionUtils.ts) from an earlier stash, resolved against changes landed since (advanced-labels). Also fixes one stale integration test that expected the old fontSize-0 hiding mechanism for thin segments; the WIP now excludes them from the label data source entirely instead. Known issue still to debug: a positioning bug pushes labels too far in Safari/Firefox (see the "slivers" story) - not yet root-caused. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root cause of labels being pushed far below the ring (the known Safari/Firefox-in-slivers-story bug): getLabelCollisionTransforms substituted minGapExpr (a bare additive expression) into "idealY - minGapExpr * rank" without parentheses, so operator precedence silently turned it into "idealY - nameFontSize + valueFontSize + (16 * rank)" instead of "idealY - (nameFontSize + valueFontSize + 16) * rank". The error compounded across the sorted cascade, producing large spurious pushes for later-ranked labels. Fixed by parenthesizing minGapExpr before every multiplication. Also wires the (now-fixed) collision algorithm into AdvancedLabel, which previously had no collision avoidance at all - added getAdvancedLabelData mirroring getSegmentLabelData, switched the swatch/text marks from polar radius+theta positioning to the collision-adjusted x/y fields, and switched the hemisphere check from raw arcTheta to the derived hemisphere field throughout. Also fixes a latent bug in segmentLabelUtils.ts where the baseline signal still used the pre-collision arcTheta instead of the collision-adjusted position, inconsistent with its own dy logic. Note: this does not address the separate "smoosh" issue (a single label's own row-stacking cap compressing its rows when its block is too tall for its reserved space near the ring's pole) - that remains a distinct, still-open problem from label-vs-label collision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n fix Two follow-up fixes to the collision-cascade positioning work, both found via live testing: 1. The horizontal reach cap for left-hemisphere labels was a flat outerRadius*ratio value, derived for the worst case of a label sitting exactly at the ring's equator. Away from the equator (most labels), the ring's real half-width is smaller, leaving genuine unused space between the label and the container's actual edge. Replaced with a per-label dynamic cap (available radius minus that label's own ring half-width at its actual Y), which equals the old flat cap exactly at the equator but grows for every label away from it. 2. When a row's content is narrower than the cap (the common case), the cap equals the row's own exact natural width - a razor's-edge boundary where our own getLabelWidth (canvas) measurement and Vega's internal text-truncation math can round against each other by a fraction of a pixel, truncating a character that didn't need to go. Verified canvas measureText and real SVG rendering agree to within 0.01px for the same text/font, ruling out a font mismatch - the fix is to only apply a real `limit` when content genuinely exceeds the cap, passing Vega's "no limit" value (0) otherwise. Also adds a dedicated DONUT_ADVANCED_LABEL_RING_GAP (currently 20px, same as direct labels - still open for design confirmation) separate from the shared DONUT_LABEL_RING_GAP, since AdvancedLabel's bulkier swatch+multi-row block was found to need a different value than SegmentLabel's simpler two-line block. Adds unit test coverage for all of the above (getDonutOuterRadiusExpr's per-label-type ring gap selection, the dynamic cap formula, and the truncation-limit boundary guard) and fixes the three existing unit tests that asserted the old flat-ratio cap expression string. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Description
Fixes the donut label collision-avoidance algorithm (a parenthesization bug in the running-max cascade formula was pushing labels far below the ring), wires the same collision avoidance into AdvancedLabel (which previously had none), and adds ring-overlap protection via text truncation for both SegmentLabel and AdvancedLabel.
getLabelCollisionTransforms's min-gap expression missing parentheses, which broke the vertical push-down math for colliding direct labels.outerRadius * ratiotruncation cap with a dynamic per-label cap (DONUT_RADIUS - collisionHalfWidth) that uses each label's own actual remaining room, freeing up unused space away from the ring's equator.limit(Vega ellipsis truncation) so labels only truncate when content would genuinely overlap the ring, guarding against a floating-point boundary mismatch that could truncate content that already fit.Related Issue
N/A - direct debugging/hardening of
feat/donut-advanced-labelslabel positioning.Motivation and Context
Advanced/Direct labels could overlap each other or overrun the donut ring at certain sizes and content lengths; this makes both mark types self-consistently avoid collisions and gracefully truncate instead of visually breaking.
How Has This Been Tested?
limitexpressions and collision math.Screenshots (if appropriate):
Types of changes
Checklist: