Fix 21 logic errors in dice geometry, sizing, and category mapping

.Rbuildignore

@@ -13,3 +13,4 @@
 ^examples
 ^README\.html$
 ^data-raw$
+^PR\.md$

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: ggdiceplot
 Title: DicePlot Visualization for 'ggplot2'
-Version: 1.2.0
+Version: 1.3.0
 Authors@R: 
     person(
         given = "Matthias", 

NEWS.md

@@ -1,3 +1,98 @@
+# ggdiceplot 1.3.0
+
+## Bug fixes (correctness)
+
+* **Category-to-pip mapping is now a single source of truth.** Previously the
+  plotted pip layout was driven by `length(unique(data$dots))` (computed on the
+  *scaled* dots values) while the legend was driven by `ndots`, and pip slots
+  were assigned per panel in row/appearance order. These could silently
+  disagree. Now every category occupies a fixed pip position determined by its
+  order in the `dots` factor levels, consistent across tiles, facet panels, and
+  the legend. This fixes:
+    - character `dots` decoding to the wrong position depending on row order;
+    - facet panels with different category subsets encoding different data
+      under one legend;
+    - a crash (`replacement has N rows`) when a `dots` palette mapped two
+      categories to the same colour.
+
+* **`n = 6` pip layout now matches the legend.** `make_offsets()` placed the six
+  pips as two horizontal rows of three while the legend drew three rows of two,
+  so four of six categories were mislabelled. The `n = 6` layout is corrected
+  (`dice_map[["6"]]`), and a regression test asserts `make_offsets(n)` and
+  `create_dice_positions(n)` agree for all `n = 1..6`.
+
+* **Pip lookup no longer collides.** Pip-to-data matching used
+  `paste0(category, x, y)` with no separator, so e.g. `(x = 1, y = 23)` and
+  `(x = 12, y = 3)`, or digit-suffixed labels such as `chr1`/`chr12`, collided —
+  producing phantom pips on the wrong tiles and cross-contaminated fill/size.
+  Each data row is now placed directly by its category's slot index, with no
+  string-concatenation join.
+
+* **Pip diameters are now physically correct.** Pips were sized by feeding a
+  millimetre diameter to `GeomPoint`'s `size` aesthetic (a point *font* size),
+  making them ~24% too small on typical plots and able to overflow tile borders
+  in dense grids. Pips are now drawn as true `grid::circleGrob()` circles with
+  millimetre radii, so a pip at `pip_scale = 1` exactly fills the available die
+  space and `pip_scale` behaves as documented.
+
+* **Mapped `size` respects the trained scale globally.** Variable pip sizes were
+  re-normalised per facet panel, so identical values rendered at different sizes
+  across panels and `scale_size(limits =)` had no effect. Normalisation now uses
+  the global size range recorded at build time.
+
+* **`geom_dice()` no longer crashes with the default `ndots`.** Calling
+  `geom_dice()` without `ndots` raised an opaque "argument is of length zero".
+  `ndots` is now validated up front (a single integer 1–6, required) with an
+  actionable message.
+
+* **Mapping per-row tile aesthetics no longer crashes.** Mapping `alpha`,
+  `linewidth`, `width`, or `height` triggered a `unique()`-length mismatch when
+  building the tile grob (or silently mis-assigned values). Tile aesthetics are
+  now aligned per tile.
+
+* **`...` is forwarded to `layer()`.** Documented pass-through parameters (e.g.
+  a constant `width`/`height`/`alpha`) now take effect, and unknown arguments
+  again trigger ggplot2's "unknown parameters" warning.
+
+* **NA / invalid pip sizes follow the ggplot2 convention.** Missing or
+  non-positive sizes are dropped with a warning when `na.rm = FALSE` (silently
+  when `TRUE`); they are no longer silently dropped in some paths and resurrected
+  at full size in others, and an all-invalid layer no longer aborts rendering.
+
+* **More than 6 dot categories** now raises a clear error at build time instead
+  of a cryptic internal failure at draw time.
+
+* **Non-square tiles** use a per-axis pip-packing calculation, so pips are not
+  over-shrunk on elongated tiles, and `make_offsets()` padding scales with the
+  tile size (fixed absolute padding collapsed the grid on small tiles).
+
+* **Alternative coordinate systems** (e.g. `coord_flip()`) degrade gracefully:
+  the data-to-millimetre scale is measured on both axes and falls back to the
+  raw `size` aesthetic with a warning when the coordinate transform is
+  degenerate.
+
+## Validation
+
+* `pip_scale` is validated to lie in `(0, 1]` (or be `NULL`).
+* Duplicate `(dots, x, y)` rows with conflicting fill/size/alpha now warn and
+  collapse to a single pip.
+
+## Breaking changes
+
+* **`ndots` is required** and must be at least the number of `dots` categories.
+* **Visual output changes.** Six-category plots, all pip sizes (~24% larger),
+  and mapped-`size` plots render differently from 1.2.0 — the previous output
+  was incorrect. Add `pip_scale = NULL` to opt out of auto-scaling.
+* **Sample datasets regenerated.** `sample_dice_data1` / `sample_dice_data2` are
+  now produced by deterministic `data-raw/` scripts matching their documented
+  8 × 4 × 5 = 160-row structure; the two datasets are no longer identical. The
+  documented format is unchanged, but exact values differ.
+
+## Documentation
+
+* `geom_dice()` `@examples` now uses long-format data (one row per
+  tile-category) instead of comma-joined strings that were never split.
+
 # ggdiceplot 1.2.0
 
 ## Bug fixes

PR.md

@@ -0,0 +1,159 @@
+# Fix 21 logic errors in the dice geometry, sizing, and category mapping
+
+**Branch:** `fix/logic-review` → `main`
+**Version:** 1.2.0 → 1.3.0
+**Status:** `R CMD check --as-cran` clean (0 errors, 0 warnings, 0 relevant notes); full `testthat` suite green.
+
+---
+
+## Overview
+
+This PR fixes 21 distinct logic errors across `geom_dice()`, its `GeomDice`
+ggproto, the `DiceGrob` draw-time code, and `make_offsets()`. Several of them
+silently produced **scientifically wrong plots** — pips drawn at the wrong
+position, phantom pips on tiles where a category is absent, per-panel-inconsistent
+pip sizes, and a plot/legend mismatch at six categories.
+
+The issues were surfaced by an exhaustive review and each was **empirically
+reproduced** (and each fix re-verified) in a pinned environment: R 4.5.2,
+ggplot2 4.0.2, legendry 0.2.4.
+
+## Root cause
+
+Most of the severe bugs share one root cause: **there was no single source of
+truth for "which category occupies which pip slot."** Four computations decided
+it independently and could disagree:
+
+- `ndots` — drove the legend design;
+- `length(unique(data$dots))` — drove the panel's pip-slot count, computed on the
+  *scaled* dots values (colour strings);
+- `present_levels` — assigned slots per panel, in factor/first-appearance order;
+- `make_offsets()` vs `create_dice_positions()` — two hand-written layouts that
+  disagreed at `n = 6`.
+
+The fix collapses these into one model: **every data row is one pip, placed at
+`slot = <index of its category in the dots factor levels>`**, with a single count
+(`ndots`) shared between the plotted layout and the legend. This eliminated the
+fragile `paste0()` string-join entirely and fixed seven bugs at once.
+
+---
+
+## Fixes by theme
+
+### 1. Category → pip-slot mapping (single source of truth)
+
+| Issue | Symptom | Fix |
+|------|---------|-----|
+| Character `dots` decoded to the wrong position | Reordering identical rows moved a category between opposite corners; plot disagreed with the (alphabetical) legend | `setup_data()` always stores `dots_original` as a factor with global levels; `draw_panel()` places each row at `as.integer(dots_original)` |
+| `n = 6` plot ≠ legend | Plot drew two rows of three; legend drew three rows of two → 4/6 categories mislabelled | `dice_map[["6"]] <- c(1, 7, 2, 8, 3, 9)` in `make_offsets()`; regression test asserts `make_offsets(n)` matches `create_dice_positions(n)` for all `n = 1..6` |
+| Panel count vs legend count desync | Absent levels / `drop = FALSE` re-packed the plot but not the legend | Slot index derived from the global level set, not from present/scaled uniques |
+| Faceting re-packed pips per panel | Panels `{A,B}` and `{B,C}` rendered pixel-identical under one legend | Global factor levels survive per-panel subsetting; slot is level-index based |
+| Non-injective `dots` palette crashed | `values = c(A="red", B="red", …)` aborted with `replacement has N rows` | Slot count no longer computed from scaled colour values |
+| > 6 categories crashed cryptically at draw time | `n must be an integer between 1 and 6`, naming an internal param | Actionable error raised in `setup_data()` at build time |
+
+### 2. Pip ↔ data matching
+
+- **`paste0(cat, x, y)` id collisions** (no separator) produced phantom pips on
+  the wrong tiles and cross-contaminated fill/size — triggered by dense grids
+  (both axes ≥ 10), digit-suffixed labels (`chr1`/`chr12`), or continuous
+  coordinates. Replaced by direct per-row slot placement; **no string join
+  remains**.
+- **Duplicate `(dots, x, y)` rows** with conflicting fill/size/alpha now **warn**
+  and collapse to a single pip instead of silently drawing the first row's
+  aesthetics.
+
+### 3. Draw-time pip sizing (`drawDetails.DiceGrob`)
+
+- **Physical size was wrong.** A millimetre diameter was fed to `GeomPoint`'s
+  `size` (a *font* size), making pips ~24% too small on typical plots and able to
+  overflow tile borders in dense grids. Pips are now drawn as true
+  `grid::circleGrob()` circles with millimetre radii — a pip at `pip_scale = 1`
+  exactly fills the available die space, and `pip_scale` behaves as documented.
+  Verified by pixel measurement: `pip_scale ∈ {0.5, 0.75}` → diameter ratios
+  0.500 / 0.748, pips perfectly round even on non-square panels.
+- **Mapped `size` respected the trained scale.** Sizes were re-normalised per
+  facet panel; identical values rendered differently across panels and
+  `scale_size(limits=)` had no effect. Normalisation now uses the **global** size
+  range recorded at build time, and mapped-ness is detected structurally
+  (`"size" %in% names(data)` before defaults apply), not by a value heuristic.
+- **`min_fill` no longer inverts the encoding.** `min_fill <- 0.25 * pip_scale`
+  (was a constant `0.25`, which made the largest value draw the smallest pip for
+  `pip_scale < 0.25`).
+- **NA / invalid sizes** follow the ggplot2 convention: dropped with a warning
+  when `na.rm = FALSE`, silently when `TRUE`; no longer dropped in one path and
+  resurrected at full size in another. An all-invalid layer no longer aborts.
+
+### 4. Geometry hardening
+
+- **Per-axis packing** replaces a Chebyshev-vs-`min(w,h)/2` conflation, so pips
+  are not over-shrunk on elongated tiles. A 750-case analytic sweep
+  (`n = 1..6` × tile sizes × `pip_scale`) confirms **zero** border-clip and
+  **zero** overlap violations (exact tangency at `pip_scale = 1`).
+- **Proportional padding** (`pad = 0.2 * min(width, height)`) — a fixed absolute
+  pad collapsed / mirrored the dot grid on small tiles; `make_offsets()` also
+  errors clearly if padding leaves no room.
+- **Per-tile `tile_df` aesthetics** — mapping `alpha`/`linewidth`/`width`/`height`
+  per row used to crash via a `unique()`-length mismatch (or silently mis-assign).
+- **Coordinate systems** — the data-to-mm scale is measured on both axes; a
+  degenerate transform (e.g. after `coord_flip()`) falls back to the raw `size`
+  aesthetic with a warning instead of producing zero-size or overflowing pips.
+
+### 5. API surface & validation
+
+- **`ndots` is validated up front** (single integer 1–6, required). A bare
+  `geom_dice()` used to fail with base R's opaque *"argument is of length zero"*.
+- **`...` is forwarded to `layer()`** — documented pass-through params (constant
+  `width`/`height`/`alpha`, …) now take effect, and unknown args again trigger
+  ggplot2's *"unknown parameters"* warning.
+- **`pip_scale` is validated** to lie in `(0, 1]` (or be `NULL`); `offset_scale`
+  is clamped so out-of-range values can never mirror/overlap the layout.
+
+### 6. Docs & data
+
+- **`geom_dice()` `@examples`** now uses long-format data (one row per
+  tile-category) instead of comma-joined strings that were never split.
+- **Sample datasets** — `data-raw/sample_dice_data1.R` / `…data2.R` produced
+  48-row data with the wrong columns and could not regenerate the shipped `.rda`.
+  Both generators now deterministically reproduce the documented 8 × 4 × 5 = 160-row
+  structure; the two datasets are no longer byte-identical. Documented `@format`
+  is unchanged; exact values differ.
+
+---
+
+## Breaking changes
+
+- **`ndots` is required** and must be at least the number of `dots` categories.
+- **Visual output changes** (all correctness fixes — previous output was wrong):
+  - six-category plots use the corrected layout;
+  - all pip sizes are ~24% larger (now physically correct);
+  - mapped-`size` plots normalise against the global range.
+  Add `pip_scale = NULL` to restore the legacy raw-`size` behaviour.
+- **Sample `.rda` regenerated** — exact values differ (structure/docs unchanged).
+
+## Verification
+
+- `R CMD check --as-cran`: 0 errors, 0 warnings, 0 relevant notes.
+- New `tests/testthat/test-logic-fixes.R`: slot/legend consistency for all
+  `n = 1..6`, character-dots order-independence, faceting slot stability, the
+  12×12 digit-suffixed collision grid, mapped-alpha rendering, > 6-category error,
+  `pip_scale` validation, duplicate-cell warning, and `...` forwarding.
+- Analytic no-clip/no-overlap sweep (750 cases) and headless pixel measurement
+  (`ragg`) of the pip-size law and roundness.
+
+## Files
+
+- `R/geom-dice-ggprotto.R` — `setup_data`, `draw_panel`, `dice_grob`,
+  `drawDetails.DiceGrob` rewrite; new `dice_data_unit_mm()` helper.
+- `R/geom-dice.R` — `ndots`/`pip_scale` validation, `...` forwarding, `@examples`.
+- `R/utils.R` — `make_offsets()` layout/pad/validation, `theme_dice()` defaults.
+- `data-raw/sample_dice_data{1,2}.R` + regenerated `data/*.rda`.
+- `tests/testthat/test-logic-fixes.R` (new) + `helper-data.R` shared helpers.
+- `NEWS.md`, `DESCRIPTION`, regenerated `man/*.Rd`.
+
+## Checklist
+
+- [x] `R CMD check --as-cran` clean
+- [x] Tests added for every fixed issue and passing
+- [x] `NEWS.md` updated with fixes + breaking changes
+- [x] Docs / man pages regenerated (roxygen)
+- [ ] Push and open PR (SSH passphrase handled locally)