Add per-key wear and highlight variation to the keyboard stage

[malpern]

Summary

• Per-key wear (keycode-seeded): high-traffic caps get polished crowns, calmer micro-grain, and stronger highlight response; the number row keeps factory texture
• Per-key highlight jitter: each cap's crown tilts fractionally so highlights land apart, breaking the instanced-geometry uniformity
• Cool lower-left fill light with two-temperature speculars, kept to a whisper during the dark hold
• All behaviors exposed as KeyboardStageTuning knobs (zero restores the prior look); Metal goldens re-recorded

Review gate

remote review gate selected — GitHub claude-review is the enforced reviewer.

Testing

• Metal golden-image suite re-recorded and re-compared clean
• Keyboard-stage scene/entrance/tuning suites pass
• Accessibility identifier check passes
• GPU frame times measured: 2.8–6.9 ms full pipeline at 1800×1040 (worst at mid-reveal)

🤖 Generated with Claude Code

[claude]

Reviewed the diff. Implementation looks solid overall — the keyCode → wear-tier mapping in wearLevel(for:) correctly matches macOS virtual keycodes (home row, modifiers, number row, etc. all check out), and the new GPU struct fields (material / tuningE) are appended consistently on both the Metal and Swift sides with matching layout.

One actionable gap:

• KeyboardStageTuningTests.testTuningVectorsMatchFields only asserts gpuVectorE.x == fillLightStrength and gpuVectorE.z == wearStrength, but not .y (fillSpecularStrength) or .w (highlightJitter). Since gpuVectorE packs 4 independent tuning values, a future field-order swap (e.g. fillSpecularStrength and highlightJitter transposed) would silently pass this test and only show up as a subtle rendering diff. Worth adding the missing two assertions for full coverage, matching the pattern used for tuningA–tuningD.

Minor/non-blocking:

• wear is scaled by saturate(uniforms.tuningE.z) (clamped to [0,1]), but tuningE.x/tuningE.y/tuningE.w (fill light, fill specular, highlight jitter) are used unclamped. If these are meant to be tunable multipliers during art iteration, this inconsistency could surprise someone dialing wearStrength above 1.0 expecting a stronger effect.
• wearLevel(for:) / materialSeed(for:) are only exercised indirectly through the regenerated snapshot PNGs — no unit test pins the expected wear tier for a given keyCode, so a future keycode typo wouldn't fail until a snapshot diff is manually eyeballed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9114083068

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Found one actionable correctness issue in the shader tuning:

wearStrength tuning knob is silently capped at 1.0, but its own default (1.5) already exceeds that cap.

In KeyboardStage.metal:

float wear = isKey
    ? saturate(input.material.x) * saturate(uniforms.tuningE.z)
    : 0.0;

uniforms.tuningE.z is wearStrength, whose default in KeyboardStageTuning.swift is 1.5. Wrapping it in saturate(...) clamps it to [0,1], so any value ≥ 1.0 (including the shipped default) behaves identically to 1.0 — the field can never actually scale wear beyond full strength, and the "1.5" default is misleading since it has no effect over 1.0. This is inconsistent with the sibling fields in the same tuningE vector (fillLightStrength, fillSpecularStrength, highlightJitter), none of which are saturated before use. Either drop the saturate(...) around uniforms.tuningE.z (if it's meant to be a >1 multiplier like the others) or lower the default to ≤1.0 to match the actual clamp — as-is the tuning surface for wear is silently narrower than it appears.

Minor: KeyboardStageTuningTests.testGpuVectorsMatchFields only asserts gpuVectorE.x/.z, and there's no assertion for vignetteVector — worth covering .y/.w (fillSpecularStrength/highlightJitter) and vignetteVector too, since that's exactly the kind of field-ordering mismatch that's easy to introduce silently in these packed SIMD4 vectors.

[claude]

Correctness: wearStrength tuning default is clamped to a no-op range by the shader.

KeyboardStageTuning.wearStrength defaults to 1.5 (Sources/KeyPathAppKit/UI/KeyboardStage/Metal/KeyboardStageTuning.swift), but the shader consumes it through saturate:

float wear = isKey
    ? saturate(input.material.x) * saturate(uniforms.tuningE.z)
    : 0.0;

saturate(uniforms.tuningE.z) clamps any value ≥ 1.0 down to 1.0, so the shipped default of 1.5 behaves identically to 1.0 — the extra headroom the tuning value implies is silently discarded. This is easy to miss: someone tuning via KEYPATH_STAGE_TUNING_FILE with wearStrength between 1.0 and, say, 3.0 would see no visual change at all above 1.0, which is surprising given the field is a plain Float with no documented [0,1] contract (unlike highlightJitter/fillLightStrength/vignetteStrength, which are not saturated and do scale linearly past 1.0).

Suggest either dropping the saturate() around tuningE.z in the shader (if values > 1 should meaningfully increase wear) or clamping/documenting wearStrength as a [0,1] knob on the Swift side so the default doesn't imply unused headroom.

No other correctness issues found; the struct layout changes (new material/tuningE/vignette fields) are consistently ordered between the Metal and Swift sides, and the entrance-duration test/doc updates match the holdDuration change.

[claude]

Review findings

1. Possible bug — wearStrength tuning knob is silently clamped to a no-op above 1.0. In KeyboardStage.metal:

   float wear = isKey
       ? saturate(input.material.x) * saturate(uniforms.tuningE.z)
       : 0.0;
   

   uniforms.tuningE.z is wearStrength, whose Swift default (KeyboardStageTuning.swift) is 1.5. Wrapping it in saturate(...) clamps it to 1.0, so the default 1.5 behaves identically to 1.0, and any tuning-file override above 1.0 has zero effect. The sibling lanes in the same tuningE vector (fillLightStrength, fillSpecularStrength, highlightJitter) are all used unclamped elsewhere in the same function, so this looks like an accidental saturate() rather than intentional clamping. Worth double-checking against the intended tuning range — if amplification above 1.0 is supposed to be usable (as the 1.5 default implies), drop the saturate around tuningE.z.

2. Test coverage gap for new tuning surface. KeyboardStageTuningTests.swift only asserts gpuVectorE.x and .z; .y (fillSpecularStrength) and .w (highlightJitter) aren't checked, and vignetteVector isn't exercised at all. There's also no decode test confirming the six new Codable keys (fillLightStrength, fillSpecularStrength, wearStrength, highlightJitter, vignetteStrength, vignetteInnerRadius) round-trip correctly from a tuning-override JSON file, which is exactly the mechanism this PR's new KeyboardStageTuningTests/preview harness depends on.

No other correctness/reliability issues found; the GPU struct layout changes (added material/tuningE/vignette fields) are consistently ordered between the Metal and Swift sides, and the entrance-duration test updates are mechanical follow-through from the holdDuration change.