Fix HexTile centerline cell selection

[sanyamsingh001]

HexTile now computes its third cube coordinate symmetrically in both the CPU implementation and the WGSL shader. This avoids deriving it by subtracting separately computed coordinates with floating-point rounding, which can select a different cell at a centerline tie.

The existing stylize regression uses a 70×44 layer with center (35,22), radius 9.3, scale 0.5, and zero rotation and smear. Its effective 35×22 buffer has center (17.5,11) and radius 4.65. The retained shader-only comparison failed at the original tolerance before the correction and passed afterward; the recorded worst pixel was (17,2). Cell-index calculations explain that recorded color, but were not GPU tracing.

This f3f24ef-based successor changes only the CPU and shader expressions. It preserves upstream WireRemoval changes and test-service poison recovery; the historical Mem fixture lint allowance is no longer included. Inputs and tolerance remain unchanged, with no parameter sweep.

Original full repository CI passed on this exact two-file source: formatting, strict release Clippy, 3,671 native tests and 2 doctests passed, with 31 native tests ignored and no failures. The native total includes the 222-test GPU harness and the existing stylize regression. PDF backend policy, layering, 39 attributed assets, and compilation of 32 Wasm crates passed. The Wasm check retained the non-fatal pointer_wrap::State.disabled warning; Mesa diagnostics are preserved.

Validation ran on Linux with Mesa software-rendering settings and EFFECTCRAFT_REQUIRE_GPU=1. Passing harness counts do not prove every optional GPU, FFmpeg or parser branch executed. These results qualify the f3f24ef-based successor, not later main revisions, other hardware/backends/platforms, or browser/Wasm runtime behavior.

AI-generated with Codex; implementation and review used GPT-6.1 Sol and GPT-6 Astra agents.

Summary by CodeRabbit

• Bug Fixes
  • Updated hexagonal tiling calculations in stylized and pixel effects to use coordinates derived directly from the input position. This improves consistency in how hex cells are rounded and centered across the two effects. No new controls or capabilities were added.

[FredApps]

• This mixes two unrelated changes: the HexTile shader fix and a clippy allowance in crates/script/src/tests_services.rs:14-15. Please split the allowance out, or say which clippy invocation fails on current main without it. Main's tests_services is already #[cfg(test)] and clippy.toml allows unwrap in tests.
• The shader change at crates/gpu/src/shaders/fx_pixel2.wgsl:652 has no test that fails before and passes after. The three existing cchextile cases in crates/gpu/src/tests_fx_pixel2.rs:218-220 do not land on a centerline. Please add a case with a pixel on the tie (for example a centre/rotation where u is exactly 0) and show it failed on the old shader.
• The CPU reference hex_cell (crates/effects/src/stylize3.rs:349-366) still uses -cx - cz in f64. After this change the GPU uses a different expression, so the two can pick different cells at a tie. Either change the CPU reference the same way, or explain why the GPU is now the one that is right and show the parity test still holds at the tie.
• The "observed" wrong cell is not shown anywhere. Please give the input (radius, rotate, center, pixel) and the old and new cell indices.
• No hosted CI ran. The author's results are on an older base; main has moved.

[sanyamsingh001]

I checked this against f3f24ef and the retained failure evidence.

The Mem allowance is superseded there: all three lock().unwrap() sites now use PoisonError::into_inner. I am omitting the allowance from the current-main successor. It originally followed three unwrap_used errors at tests_services.rs:16,19,23 from Rust 1.99.1 running cargo clippy --workspace --all-targets --release -- -D warnings on 8f02f52. That source had a compound cfg(all(test, not(windows))) gate despite allow-unwrap-in-tests=true. This is historical evidence, not a claim that current main needs the allowance.

The first existing radius-9.3 case already exercises the tie. The layer is 70×44 with input center (35,22); scale 0.5 gives a 35×22 pattern-7 buffer, effective radius 4.65 and center (17.5,11). Rotation and smearing are zero. Pixel x=17 is sampled at 17.5, so u is exactly zero. effect_case calls effect_direct(false), reaching this direct comparison before the composited cases.

The old shader failed the unchanged comparison: “adj false scale 0.5: 2 of 770 pixels over 1e-3 (max 0.7781502)”, with worst pixel (17,2). After the one-file correction, the unchanged test passed: 1 passed, 0 failed, 0 ignored, 213 filtered, using:

cargo test -p effectcraft-gpu --release --lib tests_fx_pixel2::stylize -- --exact --nocapture --test-threads=1

For that input at (17,2), source-arithmetic cell indices (rx,rz) are old f32 (0,-1), corrected (1,-1), and CPU f64 (1,-1). The old f32 evaluation makes ddx greater than ddy and adjusts rx. The symmetric expression preserves q==ccy at u=0; the existing f64 CPU expression already selects the same cell here. CPU code, inputs and tolerance were unchanged. This explains this case without claiming universal tie parity.

Those indices are calculated inferences, not recorded GPU intermediates. The fixture/sample calculation exactly matches all four retained actual CPU/GPU channels at the worst pixel (17,2).

The f3f24ef shader changes concern Wire Removal and do not incorporate this HexTile correction; the HexTile case and CPU reference are unchanged. Historical focused validation observed Vulkan llvmpipe with REQUIRE_GPU=1, and prior full CI remains local, exact-base evidence. I am preparing a shader-only successor preserving the upstream changes; its original full CI is still pending. No hosted or current-main CI result is claimed here.

[FredApps]

Thanks for the detailed write-up, @sanyamsingh001! It settles most of my review:

• The tie is tested. I re-ran your numbers in an f32 emulation of both shaders (not on a GPU). The first cchextile case (radius 9.3, scale 0.5, centre (17.5, 11)) has u == 0 in column 17. The old shader picks a different cell from the CPU at 2 pixels, the worst at (17, 2): old (0, −1), new and CPU (1, −1). With the new expression there are no mismatches. So the existing case fails before and passes after, and no new test is needed. You're right about that, sorry I missed it.
• The allowance: agreed. Main already uses PoisonError::into_inner in tests_services.rs, so it should come out.

One point is still open: the CPU reference (crates/effects/src/stylize3.rs:349-366). hex_cell still computes cy = -cx - cz, which in f64 isn't always bit-equal to cx on the centreline, so the GPU and CPU can still split a tie. With the default centre at scale 0.5, the same emulation finds 104 of 380 radii between 2 and 39.9 where some u == 0 pixel gets a different cell on the CPU than on the new shader. The old shader matched the CPU on none of those either, so this PR doesn't make anything worse. Computing cy in hex_cell with the same symmetric expression ((-√3/3·x − y/3) / r) brings it down to 17 radii; the rest are ordinary f32/f64 rounding at other edges. I'd suggest making that one-line CPU change in the shader-only successor too, so the reference and the shader agree by construction.

My change request stays in place until the successor without the tests_services.rs allowance is up. Hosted CI should run on it against current main, and then I'll re-review. 🙂

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: eab54fd9-053d-424a-ab02-390d1037665d

📥 Commits

Reviewing files that changed from the base of the PR and between f3f24ef and 9bcf2e2.

📒 Files selected for processing: 2

• crates/effects/src/stylize3.rs
• crates/gpu/src/shaders/fx_pixel2.wgsl

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The hex-cell calculations in hex_cell and fp2_hextile now compute the third cube coordinate directly from their input coordinates. Both calculations use that value in the existing cube-rounding logic.

Changes: Hex Coordinate Calculation

Layer / File(s)	Summary
Direct third-coordinate calculation crates/effects/src/stylize3.rs, crates/gpu/src/shaders/fx_pixel2.wgsl	hex_cell calculates cy directly from x, y, and r. fp2_hextile calculates its third cube coordinate directly from u and v. Both use the calculated value in the existing rounding logic.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: fredapps

Merge Risk: ⚪ Minimal · up to 9bcf2

The CPU and shader now calculate the third coordinate directly. The checked centerline cases showed no output mismatch, so no supported behavior regression prevents merging.

### Architecture Summary

Architecture risk: 🔵 Low · up to 9bcf2

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — crates (service) was modified; 2 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in crates/effects/src/stylize3.rs: hex_cell now computes cy directly from x, y, and r rather than deriving it from cx and cz; this changes the cube coordinate used for rounding and locating the hex cell.
• observed — Modified behavior in crates/gpu/src/shaders/fx_pixel2.wgsl: fp2_hextile replaces the derived third cube coordinate with a symmetric calculation from u and v; the surrounding cube-rounding logic now uses this directly calculated value.

Pre-merge checks: 5

Check name	Status	Explanation
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title accurately and concisely describes the main change: correcting HexTile centerline cell selection in the CPU and shader implementations.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[00xJS]

Tested on AMD hardware: RX 6900 XT, Windows 11, Adrenalin 32.0.21045.5002, EFFECTCRAFT_REQUIRE_GPU=1.

This fixes the cchextile mismatch from #1605 on this card. Before, on main and on the #1571 branch, tests_fx_pixel2::stylize failed on both Vulkan and DX12/FXC with cchextile radius 9.3: 4 of 770 pixels over 1e-3 (max 0.778).

	before	this PR
Vulkan, whole effectcraft-gpu suite	3 failed	stylize passes; 212 passed, 2 failed (the other two #1605 tests, posterize and ccgridwipe, unchanged)
DX12 + FXC (this PR's fx_pixel2.wgsl change applied on #1571 2a20b02a), tests_fx_pixel2::stylize	failed, 4/770 px	passes

---

Generated with Claude Code

[sanyamsingh001]

@FredApps, for the requested hosted validation, I checked the workflow definitions at main ef74d829. None of the configured pull_request path filters include crates/effects/src/stylize3.rs or crates/gpu/src/shaders/fx_pixel2.wgsl. Which maintainer-supported CI route should we use to validate this PR against current main? The current published head is 9bcf2e2.