Skip to content

Make wear and jitter tuning knobs honest about their ranges - #1244

Merged
malpern merged 1 commit into
masterfrom
claude/tuning-knob-honesty
Jul 29, 2026
Merged

Make wear and jitter tuning knobs honest about their ranges#1244
malpern merged 1 commit into
masterfrom
claude/tuning-knob-honesty

Conversation

@malpern

@malpern malpern commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to #1243, addressing its review findings (the PR auto-merged on the flaky-test re-run before these landed):

  • Drop the accidental saturate() on wearStrength (the 1.5 default silently behaved as 1.0; default is now an honest 1.0 — the exact effective value the approved goldens rendered)
  • Gate seed-only gloss variation (roughness offset, specular gain) with min(highlightJitter, 1) so zeroing the knobs fully restores the uniform baseline; identical output at shipped defaults
  • Cover all gpuVectorE/vignetteVector lanes in the packing test

Review gate

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

Testing

  • Metal golden-image suite compares unchanged (proves zero visual drift at defaults)
  • Tuning, entrance suites pass (18 tests)

🤖 Generated with Claude Code

Review findings on the tuning surface:

- The shader saturated wearStrength, so the shipped 1.5 default silently
  behaved as 1.0 and overrides above 1.0 were no-ops. Drop the clamp
  (plain multiplier like its siblings) and set the default to 1.0 —
  the same effective value the goldens and installed review approved.
- Seed-only gloss variation (roughness offset, specular gain) ignored
  highlightJitter, so zeroing both knobs could not restore the uniform
  baseline. Gate both terms with min(highlightJitter, 1): identical at
  the shipped default, fully off at zero.
- Cover all gpuVectorE and vignetteVector lanes in the packing test.

Metal goldens compare unchanged, confirming no visual drift at the
shipped defaults.

Co-Authored-By: Claude Fable 5 <[email protected]>
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Reviewed the diff — one clear issue plus one worth double-checking.

Test file has duplicate assertions (KeyboardStageTuningTests.swift)
The added block re-asserts two lines that already exist in the file:

  • XCTAssertEqual(tuning.gpuVectorE.x, tuning.fillLightStrength) appears twice (once as pre-existing context, once newly added).
  • XCTAssertEqual(tuning.gpuVectorE.z, tuning.wearStrength) also appears twice (newly added, then again as the original trailing line).

Looks like a copy/paste slip when appending the new .y/.w/vignette assertions — the duplicates don't add coverage and should be dropped, leaving just the new, distinct checks for fillSpecularStrength, highlightJitter, vignetteStrength, and vignetteInnerRadius.

Unbounded wear now feeds unclamped specular gain (KeyboardStage.metal)
Changing saturate(uniforms.tuningE.z)max(uniforms.tuningE.z, 0.0) intentionally removes the old implicit upper clamp so wearStrength values above 1.0 can actually amplify wear (previously saturate silently capped it at 1.0, which — combined with the old default of 1.5 — meant the multiplier above 1 was a no-op; good catch). However, wear now has no upper bound at all, and it's used unclamped in:

float keySpecularGain = isKey
    ? (1.0 + (materialSeedB - 0.5) * 0.30 * min(uniforms.tuningE.w, 1.0))
        * (1.0 + wear * 0.55)
    : 1.0;

The roughness expression a few lines up is still bounded by the surrounding clamp(..., 0.12, 0.82), but keySpecularGain/specular aren't clamped in the shown diff, so a wearStrength tuned above ~2 could blow out the specular highlight on high-wear keys. If wearStrength is only ever set from a small, developer-controlled range this is fine — but if it's exposed to broader tuning/config, consider clamping wear (e.g. min(wear, someMax)) before it feeds the specular gain.

@malpern
malpern merged commit 4eb30fa into master Jul 29, 2026
3 checks passed
@malpern
malpern deleted the claude/tuning-knob-honesty branch July 29, 2026 16:48
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