Skip to content

feat(highway_3d): general per-panel zoom that works in every camera mode - #139

Merged
cchacs merged 6 commits into
mainfrom
ccr-6378c956-yvg2qf
Oct 4, 2026
Merged

cchacs merged 6 commits into
mainfrom
ccr-6378c956-yvg2qf

Conversation

@cchacs

@cchacs cchacs commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

What

Adds a general camera zoom to 3D Highway that works in every camera mode, and shows it first in Split Screen's per-panel Viz ⚙ popover.

Why: the only zoom control was "Locked zoom" (cameraLockZoom). It scales the camera distance only while "Lock camera at frets 1–12" is on and the visible notes are at or below fret 12. In a small split-screen panel the normal auto-framed view sits too far back and nothing in the popover could bring it closer, so the zoom slider looked dead.

Changes (plugins/highway_3d)

  • New cameraZoom setting, 0–1. It multiplies the final camera distance in the camera update, after the fret-row fit guard, so it applies in locked and auto modes alike. The slider maps piecewise: 0 → 0.7×, 0.5 → exactly 1.0×, 1 → 1.45×, so the default leaves the framing unchanged (camViewZoomMul).
  • While the lock is engaged, cameraLockZoom has already scaled the distance and the view zoom multiplies on top, so the view zoom is bounded (camBoundViewZoom) to keep the product within the limits either slider documents. Without the lock, it keeps its full range.
  • The near end is limited to 0.7× because at 0.55× the outer fret numbers (15–21) and string ends were clipped by the panel edge (the fret-row guard only probes the camera's centre column). At 0.7× every fret number through 24 stays on screen.
  • Exposed first in the static panelControls descriptors, which Split Screen reads for its Viz popover. Per-panel values use the existing h3d_bg_panel<N>_cameraZoom keys.
  • cameraLockZoom declares dependsOn: 'cameraLockLow'. Split Screen greys it out until the lock is on (merged as feedBack-plugin-splitscreen#78). Documented in plugins/highway_3d/CLAUDE.md.
  • window.h3dBgSetCameraZoom(v) sets the global value. No slider was added to the global Settings page.
  • Plugin version 3.34.2 → 3.35.0, plus a CHANGELOG.md entry.
  • Tests: updated the two that pinned the old shape (the exact panelControls key list and the curDist lerp expression), and added tests for the neutral default, the monotonic mapping, and the bound against Locked zoom.

Default behaviour is unchanged. At 0.5 the multiplier is exactly 1.0×.

How it was checked

  • node --test tests/js/*.test.js tests/plugins/*/js/*.test.js: 1245 pass, 0 fail.
  • Ran the real host with Split Screen in headless Chromium (software WebGL) and screenshotted a two-panel split on a chart with notes. At the default slider position the framing matches the original view. With the lock off, slider 1 gives a distant view and 0 a close one with every fret number visible. With the lock on and both sliders at their closest, the view matches Locked zoom alone rather than stacking.
  • Soft or pixelated output in some of my screenshots came from core's load-adaptive render scale reducing resolution on the slow software renderer. With it pinned to full resolution the view is sharp at every zoom value. That scaler is unrelated to this change.
  • Not tested on a real GPU, a high-DPI display at native scale, or very narrow panels, where the 0.7× limit may clip earlier.

feedpak surface

  • This PR does not change how the app reads/writes feedpaks (manifest keys, pack files, folder layout)
  • …or it does, and the spec change landed first via the FEP process

Checklist

  • CHANGELOG.md [Unreleased] updated (user-visible changes)
  • Tests added/updated for new behaviour
  • Commits are DCO signed off (git commit -s): not done, the commits carry no Signed-off-by line

🤖 Generated with Claude Code

https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH

claude added 2 commits October 4, 2026 16:23
…mera mode

The only zoom control was "Locked zoom", which applies only while
"Lock camera at frets 1-12" is on and the visible notes are at or below
fret 12. In a small split-screen panel the auto-framed view could not be
brought closer.

Add a cameraZoom setting (0..1, 0.5 = unchanged) that scales the final
camera distance in every mode, expose it first in panelControls so Split
Screen's Viz popover shows it, and add window.h3dBgSetCameraZoom.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH
At the closest slider setting (0.55x camera distance) the outer fret
numbers and strings were clipped by the panel edge, because the fret-row
guard only probes the camera's centre column. Limit the near end to 0.7x,
which keeps every fret number through 24 on screen.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH
@codacy-production

codacy-production Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity · 0 duplication

Metric Results
Complexity 5
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Declare dependsOn: 'cameraLockLow' on the cameraLockZoom panel control so
hosts such as Split Screen can grey it out until the lock is enabled.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH
Comment thread plugins/highway_3d/screen.js Resolved
Comment thread plugins/highway_3d/screen.js Resolved
Comment thread plugins/highway_3d/screen.js Resolved
@kilo-code-bot

kilo-code-bot Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review of eb9ba96 (on top of a1e653b). The push is a cleanup, not a behaviour change: camLockZoomMul() (screen.js:1257) now holds the single CAM_LOCK_ZOOM_MIN + (CAM_LOCK_ZOOM_MAX - CAM_LOCK_ZOOM_MIN) * zoom mapping that the three locked tgtDist sites and the cameraZoom bound previously each computed inline, and the cameraZoom prose moved out of the Splitscreen section into Camera. Verified: the four call sites (screen.js:9877, 11775, 13878, 15728) evaluate to the same expression as before, camLockZoomMul sits in the same closure scope as CAM_LOCK_CENTER_FRET (which those same sites reference), camLockZoomMul(0.5) === 1 exactly in binary64, and the new source-scan assertions hold (one CAM_LOCK_ZOOM_MIN + occurrence, four camLockZoomMul(cameraLockZoom) calls). The documented bound (min(lockMul, 0.7) … max(lockMul, 1.45)) and the per-end clamp behaviour described in CLAUDE.md match camBoundViewZoom.

All previously reported findings are resolved in current code: the neutral default (camViewZoomMul piecewise, screen.js:1237), the compounding bound (camBoundViewZoom, screen.js:1247), dependsOn (documented at plugins/highway_3d/CLAUDE.md:143), the duplicate-mapping site (now folded into camLockZoomMul), the travel-clamp consequence (documented), and the CLAUDE.md placement nit (paragraph moved to Camera).

Files Reviewed (3 files)
  • plugins/highway_3d/screen.js
  • plugins/highway_3d/CLAUDE.md
  • tests/js/highway_3d_panel_controls.test.js
Notes
  • dependsOn's consumer lives in the external Split Screen plugin (not vendored in plugins/), so it could not be re-verified from this repo; the claim rests on the author-supplied split-screen PR reference.
  • Tests were not executed (read-only review mode); assertions were checked by static analysis only.
Previous Review Summaries (3 snapshots, latest commit a1e653b)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit a1e653b)

Status: No Issues Found | Recommendation: Merge

Incremental review of a1e653b (on top of 126769a). The only change since the last review is in tests/js/highway_3d_panel_controls.test.js: the bound-product assertions swap 1e-9 for a named TOLERANCE = 1/1000000. That is pure float slack — the nearest product that could approach a limit (lockMul 0.8, zoom 0 → 0.8 × (0.7/0.8)) still clears it by orders of magnitude, so the test's guarantee is unchanged. Targeted re-verification of the three earlier findings against current code: camViewZoomMul (screen.js:1237) is piecewise so slider 0.5 is exactly 1.0×, camBoundViewZoom (screen.js:1248) keeps the locked product inside both sliders' limits, and dependsOn is documented in plugins/highway_3d/CLAUDE.md. No new issues in changed lines.

Files Reviewed (1 file)
  • tests/js/highway_3d_panel_controls.test.js

Previous review (commit 126769a)

Status: No Issues Found | Recommendation: Merge

Incremental review of 126769a (fix commit on top of 7c55a4a). All three previously reported findings are resolved in current code: camViewZoomMul() is piecewise-linear so the default slider 0.5 is exactly 1.0x; camBoundViewZoom() keeps the cameraZoom x cameraLockZoom product inside both sliders' documented limits; and dependsOn is now documented in the plugin's CLAUDE.md with its host reference. No new issues found in the changed lines.

Files Reviewed (3 files)
  • plugins/highway_3d/screen.js
  • plugins/highway_3d/CLAUDE.md
  • tests/js/highway_3d_panel_controls.test.js

Previous review (commit 7c55a4a)

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 1
WARNING 1
SUGGESTION 1
Issue Details (click to expand)

CRITICAL

File Line Issue
plugins/highway_3d/screen.js 1232 CAM_VIEW_ZOOM_MIN raised 0.55 → 0.7 without re-centering the mapping, so the default cameraZoom = 0.5 yields 1.075× instead of the documented 1.0× — every existing user gets a slightly wider default view, contradicting the PR's "default behaviour unchanged" claim and the code comments.

WARNING

File Line Issue
plugins/highway_3d/screen.js 15703 _viewZoomMul multiplies on top of the locked-mode cameraLockZoom factor, so the combined range is 0.385×–2.1×; the near-end clamp added to stop fret-number clipping is defeated exactly in locked mode (0.55 × 0.7).

SUGGESTION

File Line Issue
plugins/highway_3d/screen.js 16679 dependsOn is an undocumented descriptor field with no consumer in this repo; confirm the Split Screen host implements it and document it, or drop it.
Files Reviewed (5 files)
  • plugins/highway_3d/screen.js - 3 issues
  • plugins/highway_3d/plugin.json - 0 issues (version bump 3.34.2 → 3.35.0 only)
  • CHANGELOG.md - 0 issues
  • tests/js/highway_3d_camera_framing.test.js - 0 issues (regex pins expression shape only; no test asserts the neutral-multiplier invariant)
  • tests/js/highway_3d_panel_controls.test.js - 0 issues

Fix these issues in Kilo Cloud


Reviewed by free · Input: 98.9K · Output: 11.6K · Cached: 553.5K

…Locked zoom

Raising the near limit to 0.7 left a single linear blend whose midpoint
was 1.075x, so the default slider position moved the camera. Map the
slider piecewise (0 -> 0.7, 0.5 -> exactly 1.0, 1 -> 1.45).

While the lock is engaged cameraLockZoom has already scaled the distance
and the view zoom multiplied on top, reaching 0.385x and 2.1x. Bound the
view zoom by the lock multiplier so the product stays within the limits
either slider documents.

Document the panelControls dependsOn field. Add tests for the neutral
default, the monotonic mapping and the product bound.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

The new default is not neutral. CAM_VIEW_ZOOM_MIN = 0.7 paired with CAM_VIEW_ZOOM_MAX = 1.45 gives 0.7 + 0.75 × 0.5 = 1.075 at cameraZoom = 0.5, so every existing user gets a 7.5 % farther camera on upgrade — with no control anywhere to put it back. The constant pair needs one value changed before this merges; everything else is secondary.

Reviewed changes — all three commits at 7c55a4a (5 files, +36/−9), read end-to-end against plugins/highway_3d/screen.js, its two updated source-scan tests, the live Split Screen host, and the camera-distance math traced through camUpdate().

  • New cameraZoom setting — added to BG_DEFAULTS (:2423) and _BG_FLOAT_KEYS (:2843, so _bgCoerce clamps it to 0..1), declared as instance state (:4869), read per panel in _bgLoadSettings() (:8809), and picked up by the settings-change dispatcher (:8504).
  • Applied to the camera distance — camUpdate() multiplies the curDist lerp target by _viewZoomMul after _fretRowFitBoost (:15702), so it reaches locked and auto framing alike. Two float ops on the per-frame path; no new allocation or DOM work, so the plugin's 60 fps budget is unaffected.
  • New per-panel control descriptor — cameraZoom ("Zoom (In ↔ Out)", range 0..1, step 0.05, default from BG_DEFAULTS) heads factory.panelControls (:16646), which is what Split Screen's Viz ⚙ popover renders. window.h3dBgSetCameraZoom (:2965) is load-bearing, not decorative: the host's _vizPanelSet derives h3dBgSet + Cap(key) and returns early when the setter is missing, so without it the per-panel localStorage write would never reach _bgEmitChange → _bgLoadSettings().
  • cameraLockZoom gains dependsOn: 'cameraLockLow' (:16679) — see the inline note; the field is currently inert.
  • Version bump + CHANGELOG — 3.34.2 → 3.35.0 and an [Unreleased] / Added entry. The new CHANGELOG line correctly preserves the file's CRLF endings.
  • Two tests updated — the panelControls key list and the curDist lerp regex.

node --test tests/js/*.test.js tests/plugins/*/js/*.test.js passes locally: 1242 pass, 0 fail, matching the PR's claim.

ℹ️ Nitpicks

  • The PR body's "Not changed" section still reads "Locked zoom is still listed in the popover with no hint that it depends on the lock checkbox" — commit 7c55a4a added that hint, so the body describes the pre-7c55a4a state.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using big-pickle (free) | 𝕏

Comment thread plugins/highway_3d/screen.js Resolved
Comment thread plugins/highway_3d/screen.js Resolved
Comment thread plugins/highway_3d/screen.js Resolved
Codacy flagged the 1e-9 literals in the cameraZoom bound test. Use a named
TOLERANCE constant with no exponent literal; the products under test differ
from their limits by far more than float slack, so behaviour is unchanged.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three findings from my previous review are verified resolved against 126769a:

  • Neutral default. camViewZoomMul is now piecewise with a hard 1.0× pivot at 0.5, and camViewZoomMul(0.5) === 1 is pinned by a real assertion rather than a source-shape regex. Checked by hand: 0 → 0.7, 0.5 → 1.0, 1 → 1.45, monotonic across the sweep, NaN → 1, out-of-range clamped.
  • Locked-mode stacking. camBoundViewZoom clamps the product rather than the view zoom alone. I checked prevLockActive against all three sites that fold lockZoomMul into tgtDist (_applyNoteCamTargets returns lockActive and every caller assigns it; the lookahead path sets it alongside lockActive; the bootstrap-hold path correctly retains the prior value), so _lockMulNow is true exactly when the lock multiplier is already inside tgtDist. The product invariant holds at the boundaries — at lockMul ≤ 0.7 the clamp pins the total to lockMul, at lockMul ∈ [0.7, 1] to MIN, and symmetrically at the far end. The near-end and far-end cases from my comment are both covered.
  • dependsOn. Confirmed real: get-flashbacks/feedBack-plugin-splitscreen#78 is merged, and the host's _wireVizControlDeps implements exactly what the new CLAUDE.md line claims — input.disabled + opacity: 0.4 until the gate is on, with missing / non-toggle / self-referencing / cyclic targets all ignored. The doc no longer overstates anything.

node --test tests/js/*.test.js tests/plugins/*/js/*.test.js: 1245 pass, 0 fail — matches the updated PR body.

Nothing here blocks the merge. Two small follow-ups, both anchored below: a fourth copy of the lock-zoom expression that this file already has an explicit precedent against desyncing, and a user-visible side effect of the new bound that isn't written down anywhere.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using big-pickle (free) | 𝕏

Comment thread plugins/highway_3d/screen.js Resolved
Comment thread plugins/highway_3d/screen.js Resolved
Comment thread plugins/highway_3d/CLAUDE.md Resolved

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a1e653b is cosmetic and correct — a named TOLERANCE in place of four repeated 1e-9 literals, with the loop bounds unchanged. node --test tests/js/highway_3d_panel_controls.test.js: 4 pass, 0 fail. Nothing to action.

One note for the record, no response needed: the constant is 1 / 1000000 (1e-6), so the tolerance loosened by 1000× rather than just being renamed. That is immaterial here — the smallest gap the assertions exist to catch is 0.385 vs 0.55, five orders of magnitude larger than either value, and total(1, 0) / total(1, 1) are still pinned by exact assert.equal — but the commit subject ("instead of 1e-9 literals") reads as though the value is unchanged.

Pullfrog  | Fix it ➔ | View workflow run | Using big-pickle (free) | 𝕏

…raction

Fold the four copies of the cameraLockZoom -> multiplier expression (the
three locked tgtDist sites and the cameraZoom bound) into camLockZoomMul(),
so retuning it cannot desync them. Identical arithmetic; a test pins the
mapping (0 -> 0.55, 0.5 -> 1.0, 1 -> 1.45) and that only the helper
computes it.

Move the cameraZoom paragraph from the Splitscreen controls list to the
Camera section and add that Locked zoom and View zoom interact: the further
Locked zoom is from its default, the less of View zoom's travel does anything.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G99dXhcfEFmVR7R4jLpYRH
@cchacs
cchacs merged commit ce883f7 into main Oct 4, 2026
14 checks passed
@cchacs
cchacs deleted the ccr-6378c956-yvg2qf branch October 4, 2026 18:58
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.

2 participants