Skip to content

feat: align hex/square visual styles, rename hex pieces#63

Open
tim4724 wants to merge 9 commits intomainfrom
worktree-moonlit-conjuring-brooks
Open

feat: align hex/square visual styles, rename hex pieces#63
tim4724 wants to merge 9 commits intomainfrom
worktree-moonlit-conjuring-brooks

Conversation

@tim4724
Copy link
Copy Markdown
Owner

@tim4724 tim4724 commented Apr 8, 2026

Summary

  • Style alignment: Match hex stamp proportions (highlights, shadows, borders, strokes) to square using hex drawn height as reference. Remove hex Normal border stroke, delete mini stamp functions, use apothem-based cell gaps, polygon-offset board border, unified ghost colors, height-based grid/ghost strokes, 1-column ghost-style garbage meter for both modes.
  • Hex piece rename: Rename hex pieces to match square counterparts (I4→I, T→J, L→L, S→O, F→S, Tp→T, Fm→Z) so both modes share the same piece names and colors. Rotate J and Z default orientations 60° CCW.
  • Performance: Cache sCell/stampHeight/gridLineWidth in constructors, eliminate per-frame allocations (scratch objects, empty arrays, layout cache), batch garbage effects into compound paths, use integer ghost set keys.
  • Visual tests: Add style-comparison composite test (square vs hex side-by-side) and hex all-pieces+ghosts tests (3 tiers × 8 players).

Test plan

  • 363 unit tests pass
  • 51 Playwright visual snapshot tests pass (37 existing + 14 new)
  • Style comparison screenshot shows matching proportions
  • All 7 piece types visible in both square and hex pieces screenshots
  • Hex pieces use correct colors matching square counterparts

🤖 Generated with Claude Code

…square

Align hex cell rendering with square to ensure consistent visual style:
- Match stamp proportions (highlights, shadows, borders) using hex drawn
  height as reference dimension instead of circumradius
- Remove hex Normal border stroke (square has none)
- Delete mini stamp functions — reuse full-size stamps at smaller scale
- Use apothem-based cell gap for uniform perpendicular spacing
- Board border uses polygon offset for even gap on all sides
- Ghost pieces use same ghostColor() alphas (remove hardcoded 0.4)
- Grid/ghost strokes use height-based formula matching square
- Garbage meter: unified 1-column ghost-style for both modes

Rename hex pieces to match square counterparts (same name, same color):
  I4→I, T→J, L→L, S→O, F→S, Tp→T, Fm→Z
Rotate J and Z default orientations 60° CCW.

Performance: cache sCell/stampHeight/gridLineWidth in constructors,
eliminate per-frame allocations (scratch objects, empty arrays, layout
cache), batch garbage meter/effects into compound paths, use integer
ghost set keys.

Add style-comparison visual test (composite square vs hex screenshot)
and hex all-pieces+ghosts tests (3 tiers × 8 players).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Claude finished @tim4724's task in 4m 17s —— View job


Code Review

  • Gather context and understand PR scope
  • Read key changed files
  • Analyze changes in detail
  • Review performance changes
  • Review test changes

This is a well-engineered PR. The style-alignment work is thorough, performance improvements are thoughtful, and the test coverage is solid. I found a few minor items below but no actual bugs.


Architecture / Design

Hex stamp API change (CanvasUtils.js) — switching from circumradius (hexSize) to drawn height (size) as the public API parameter is the right move for proportional alignment with square mode. The internal computation cr = size / _SQRT3 is clean. Merging _createHexStampCanvas into getHexStamp reduces surface area without losing clarity.

Outline outset algorithm (HexConstants.js:168–197) — the miter-join formula is mathematically correct: the average-normal miter vector scaled by outset / (av · n1) achieves uniform perpendicular distance from both edges. One note on the degenerate guard:

if (Math.abs(dot) < 0.001) dot = 1;

This silently falls back to 1× outset for near-antiparallel edges (180° convex corners). That's a safe clamp, but worth a comment like // degenerate: antiparallel edges — use unscaled offset so future readers understand why the fallback exists.

_GHOST_KEY_STRIDE = 32 (HexBoardRenderer.js:11)HEX_COLS=11 and HEX_VISIBLE_ROWS=21 are both safely below 32, so col * 32 + row is collision-free. The comment documents the invariant but there's no runtime guard. If either constant ever grows past 32 without a corresponding stride update, the ghost preview would silently misfire. A one-line assertion in the constructor would make this invariant hard to violate:

// In constructor, after the assignment:
if (HEX_COLS_N >= _GHOST_KEY_STRIDE || HEX_VIS_ROWS >= _GHOST_KEY_STRIDE) {
    throw new Error('_GHOST_KEY_STRIDE too small for board dimensions');
}

Or update the constant from 32 to something derived: var _GHOST_KEY_STRIDE = Math.max(HEX_VIS_ROWS, HEX_COLS_N) * 2 | 0.


Performance

All the batching changes are correct and well-structured:

  • _EMPTY_EFFECTS / _NO_SHAKE frozen objects — eliminates per-frame GC pressure. [Good pattern.]
  • _cachedPreviewCells.length = 0 instead of = [] — reuses the array allocation. [Correct.]
  • Constructor-cached _sCell, _stampHeight, _gridLineWidth — these are stable post-construction (renderers are recreated on layout change), so the caching is safe.
  • try/finally for globalAlpha cleanup in _drawGarbageEffects — correct pattern; protects against early continue paths (though currently all paths hit the finally).
  • Module-level _getIndicatorColor / _getDefenceColor — avoids allocating new closures on each render call.

The _nextPanelLayout caching in BaseUIRenderer.js keyed only on nextCount is fine — if boardY or cellSize change, the renderer itself is recreated. The cache is cheap and the key is stable during play.


Tests

style-comparison.spec.js:60 — the page.waitForTimeout(200) timing hack:

await waitForGameRender(page);
// Wait until the render loop has actually drawn hex content to the canvas.
await page.waitForTimeout(200);

waitForGameRender is not called a second time after injecting the hex state. Since this test runs infrequently (only with npm run test:visual), flakiness risk is lower, but a slow CI runner could still miss the frame. Consider calling waitForGameRender(page) again here instead of a fixed timeout.

hex-fixtures.js:buildHexAllPiecesGhostState — the extra ghost format stores x: 0, ghostY: 0 with absolute grid coordinates as blocks. This works because DisplayRender.js computes drawCol = ghost.x + gbx = 0 + abs_col. It's slightly unconventional (blocks semantically are offsets, not positions), but it's consistent with the format contract and the bounds check is correctly delegated to HexBoardRenderer.drawGhostBlock.


Minor nits

  • HexPiece.js:J comment: // 3 in a row + 1 on top of center[-1,1],[0,0],[1,-1] is a diagonal in axial coords, not a straight row. Something like // zigzag diagonal + arm or just pointing to the rotation note would be more precise, though this is cosmetic.

  • artwork/builder.js renames hS to hO and reuses hS for the new S-shape. If anyone has cached artwork using the old key names, they'd get a stale render. This is an existing limitation of the artwork key scheme, not introduced here.


Summary

No blocking issues. The outset guard and _GHOST_KEY_STRIDE concerns are both minor — the outset guard is safe as-is, and the stride only matters if board constants change. The waitForTimeout(200) in the style-comparison test is the one item I'd consider addressing before merge if CI test stability is a concern.

…e keys

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hex extra ghosts now include x:0, ghostY:0 so the render loop uses
the same offset logic for both square and hex — no duck-typing needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hex extra ghosts now include x:0, ghostY:0 so the render loop uses
the same offset logic for both modes — no duck-typing needed.
Freeze _EMPTY_EFFECTS and _NO_SHAKE to catch accidental mutation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace per-row beginPath/fill with a single compound path per effect,
matching the hex garbage effect batching pattern.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move bounds validation into drawGhostBlock itself rather than relying
on callers. Both square and hex renderers now guard row and column.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tim4724 tim4724 deployed to preview April 8, 2026 20:06 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant