Simplify seed layout by dropping seed packing - #9
Merged
Conversation
The boundary seed splits labels into a left and right column by pole x. Previously the split point balanced the two columns by total box height; switch it to balance the number of labels per column instead, which keeps the two stacks even in count regardless of how many labels wrap to multiple lines. Drops the now-unused cumulative-height computation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
The crowded branch of .sideColumn() previously used the Hungarian only to fix a top-to-bottom order and then ran the packLen() dynamic program to compress the mixed-height boxes onto a one-line grid. Replace that with the same uniform tallest-box grid the uncrowded branch already uses: build m slots centred on the pole span and Hungarian-assign the labels directly. One box per slot keeps the assignment overlap-free without the packing DP, and the seed no longer introduces the leader crossings the packing step did. packLen() is now unused from R; the kernel is removed in a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
The seed slot pitch was `tallest box + gap` (gap = 0.25 line heights). Now that the crowded branch lays labels on the same grid as the uncrowded one, use a pitch of exactly the tallest box height: neighbouring boxes end up at most touching rather than separated by an extra gap, so the column packs a little tighter and the seed grid is proportional to the text alone. The downstream sweeps still resolve any padded-extent contact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
With the crowded seed column placed on the Hungarian grid, nothing calls packLen() any more. Delete src/pack.cpp, regenerate the Rcpp exports, and drop the two packLen unit tests. This removes the one-dimensional packing dynamic program entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
The uncrowded and crowded cases of .sideColumn() now do the same thing -- lay a uniform tallest-box grid and Hungarian-assign labels to it -- differing only in the slot count and whether the grid is clamped into the viewport. Fold them into a single path: `nSlot = max(m, min(capacity, ...))` collapses to `m` when crowded (capacity < m), and the viewport clamp is applied only when the grid actually fits. Verified to reproduce the previous per-branch output exactly on both crowded and uncrowded columns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
The seed grid pitch no longer adds `gap` and packLen (its only other user) is gone, so `scene$gap` is dead. Remove it from placementScene(); `pad` remains the sole derived spacing constant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
devtools::document() removes man/packLen.Rd now that the kernel and its Rcpp export are gone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
Review follow-up. Dropping the inter-slot gap means adjacent tallest boxes in a dense pole region can overlap by up to 2*pad on their padded extents, so the seed is no longer strictly conflict-free -- the downstream sweeps resolve those small overlaps. Update the .sideSlots title and the placeLabels pipeline note accordingly, and fix the now-singular "spacing constant" wording (only `pad` remains). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
Review follow-up (test-coverage gap). The only placement fixture stays uncrowded, so the m > capacity path -- the one the packLen removal changed most -- had no coverage. Add a direct .sideColumn test that forces the crowded branch and asserts the grid is m slots at the tallest-box pitch, extends past the viewport, and is centred on the pole span. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
nSlot was `max(m, min(capacity, spanSlots + 2*m))` -- a generous slot target (pole span + slack) capped at the viewport capacity. Since the Hungarian always assigns labels to the pole-nearest slots, the far slack slots never move a label; the slot count only sets the cost-matrix size. Collapse it to `max(m, capacity)`: the uncrowded grid now fills the viewport rather than a pole-sized window, giving byte-identical placements (verified on spread, clustered, and the golden example) while dropping the spanSlots computation. The Hungarian now scales with the viewport's slot capacity, which for normal panels is the same order as the old bound. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
placementScene() carried one clearance constant, pad = 0.05*char_h, that was
folded into the padded box extents and reused as the polish's hard clearance,
while the polish's soft target spacing was a separate 0.6*char_h. Replace pad
with two explicit scene fields:
* hardPad -- the hard box clearance, used everywhere pad was (padded extents,
side-slot line offset, radial candidates, polish hard clearance) AND now
also folded into the seed slot pitch (slotH = max(boxH) + 2*hardPad), so a
column stacks *padded* boxes. This makes the extents and the pitch
consistent: adjacent boxes touch at gap 0 with no padded overlap, restoring
a conflict-free seed for any hardPad >= 0.
* softPad -- an extra target spacing the polish alone aims for, on top of
hardPad (pad_tgt = hardPad + softPad); nothing else uses it.
Both default to 0 at the engine level (placeLabels/placementScene), so the
headless engine no longer pads by default. The golden fixture is regenerated
to the new conflict-free baseline (len 17.99 -> 18.08, still bb/ll/lb = 0), and
the crowded-branch test gains a hardPad-widens-the-pitch assertion.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
Surface the two clearances as user parameters, threaded like label.buffer: fancyMask() and geom_mark_shape() -> the geom params -> the shape_enc grob -> my_make_label() (where the units are converted to mm alongside label.buffer) -> my_place_labels() -> placeLabels(hardPad, softPad). Defaults at the user layer: label.hardpad = unit(0, "pt") (the label margin usually gives enough separation; raise it mainly for con.type = "box", where the drawn outlines would otherwise touch) and label.softpad = unit(6, "pt") (roughly the previous 0.6*char_h polish spacing, now an absolute amount). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uNr8DUhQ6cUXkA4TvTLhX
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.
No description provided.