feat(highways): per-splitscreen-panel Camera Director cameras - #823
Merged
Merged
Conversation
Make the three 3D highways read the Camera Director bridge per panel so each splitscreen panel renders its own camera (independent orbit/height/zoom/tilt/pan), instead of all panels sharing the focused camera. - Add a shared `_freeCamFor(canvas)` resolver to each highway: prefer this panel's entry in `window.__h3dCamCtlPanels[panelIndexFor(canvas)]`, fall back to the global `window.__h3dCamCtl`, else null (100% stock). Defensive on the splitscreen global name (feedBackSplitscreen || slopsmithSplitscreen), NaN-safe, allocation-free. - highway_3d (guitar): source `_freeCam` from the resolver (was global-only). - keys_highway_3d: adopt the bridge for the first time — layer dolly/height/orbit + pan/pitch offsets onto the pan/zoom follow rig at the camera write. - drum_highway_3d: adopt the bridge — new per-frame `applyCamera()` folds the static base pose + kick-pulse dip + free-cam offsets. - In a follower (popped-out) window there is one panel, so the resolver yields whatever camera the plugin set in that window; no highway change needed for pop-out. Camera Director absent → resolver returns null → renderers behave exactly as before. Bump each plugin patch version. Existing plugin tests pass (drum 15, keys 30); the keys "default look unchanged" test confirms the stock path is byte-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Kris Anderson <topkoa@gmail.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThree highway plugins now resolve Camera Director bridges per panel, apply the resolved camera control data during rendering, add bridge-resolution tests, and bump their manifest versions. ChangesCamera Director free-cam integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds per-splitscreen-panel Camera Director support to the 3D highway renderers so each panel can render with its own independent camera (with safe fallback to the prior “stock” camera when Camera Director is absent/disabled).
Changes:
- Introduces a per-plugin
_freeCamFor(canvas)resolver that preferswindow.__h3dCamCtlPanels[panelIndex]and falls back towindow.__h3dCamCtl. - Updates keys_highway_3d and drum_highway_3d to apply Camera Director free-cam offsets on top of their existing camera rigs.
- Updates highway_3d to resolve the Camera Director bridge per panel (including for the “wide pane” gating logic), and bumps plugin versions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugins/keys_highway_3d/screen.js | Adds per-panel bridge resolver and layers free-cam offsets onto the existing pan/zoom follow rig. |
| plugins/keys_highway_3d/plugin.json | Bumps plugin version to 0.2.1. |
| plugins/highway_3d/screen.js | Adds per-panel resolver and uses it to drive Camera Director enablement + transforms. |
| plugins/highway_3d/plugin.json | Bumps plugin version to 3.31.4. |
| plugins/drum_highway_3d/screen.js | Adds per-panel resolver and a per-frame applyCamera() that combines base pose + kick dip + free-cam offsets. |
| plugins/drum_highway_3d/plugin.json | Bumps plugin version to 0.3.2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address a review note on the free-camera block: the comments described the bridge as "per-panel-aware" without naming the actual globals. Spell out that _freeCam comes from _freeCamFor(highwayCanvas) — window.__h3dCamCtlPanels[ panelIndexFor(canvas)] when split, else the global window.__h3dCamCtl, else null — and update the nearby comment that mentioned only __h3dCamCtl. Comment- only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kris Anderson <topkoa@gmail.com>
…r tests Three review findings on the per-panel camera work: - highway_3d: _bgPanelKey() resolved splitscreen via window.feedBackSplitscreen only, while _freeCamFor() uses the feedBackSplitscreen||slopsmithSplitscreen alias it claims to "mirror". If the rename lands, per-panel background settings would silently stop being per-panel while the camera stayed per-panel. Resolve the alias the same way in _bgPanelKey. - drum_highway_3d: applyCamera()'s "before first positionCamera()" guard tested `_camBaseH == null`, but _camBaseH/_camBaseD were initialized to 0, so the guard never fired (and could apply a base-0 pose for a frame). Initialize to null. - keys + drum: the PR claimed the Camera Director resolver was unit-checked, but nothing exercised it. Extract the resolver into pure, exported helpers (_resolveFreeCam + _ssApi), delegate the per-instance _freeCamFor to them, and add tests/camera_bridge.test.js covering per-panel select, global fallback, null-when-absent, throw-safety, and the slopsmith-alias resolution. Drum 15→21, keys 50→56, all pass; behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kris Anderson <topkoa@gmail.com>
Follow-up to the _bgPanelKey alias fix: _freeCamFor already treats panelIndexFor as potentially throwy and catches to keep framing stable, but _bgPanelKey called it bare. A throwing splitscreen build would take down background-settings resolution (and the render path) even though the camera path falls back safely. Wrap the call in try/catch, falling back to 'main'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kris Anderson <topkoa@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/drum_highway_3d/tests/camera_bridge.test.js (1)
24-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add coverage for
ss.panelIndexFornot a function.Current tests cover missing panel map, missing panel entry, absent globals, and throw-safety, but not the case where
ssis truthy whilepanelIndexForisn't a function — this should also fall through to the global camera per thetypeof ss.panelIndexFor === 'function'guard.➕ Suggested additional test case
test('_resolveFreeCam: falls back to global when panelIndexFor is missing/not a function', () => { const { __test } = load(); const g = { id: 'global' }; assert.equal(__test._resolveFreeCam({}, {}, { 0: {} }, g), g); });
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9ba2dda-3d51-4b29-be37-ed7d9448711b
📒 Files selected for processing (5)
plugins/drum_highway_3d/screen.jsplugins/drum_highway_3d/tests/camera_bridge.test.jsplugins/highway_3d/screen.jsplugins/keys_highway_3d/screen.jsplugins/keys_highway_3d/tests/camera_bridge.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/highway_3d/screen.js
- plugins/drum_highway_3d/screen.js
…ridge fns - _bgPanelKey() treated any non-null panelIndexFor() return as a valid panel id, so a NaN/non-finite index minted a bogus "panelNaN" localStorage key instead of falling back to "main". Gate on Number.isInteger(idx) && idx >= 0. (The camera path is already NaN-safe — panelsMap[NaN] misses and falls through.) - Add a NaN/negative-index case to the resolver tests (drum 22, keys 57, pass). - Convert the camera-bridge helpers' comments to JSDoc (_bgPanelKey, _freeCamFor, _resolveFreeCam, _ssApi across the three plugins) to lift docstring coverage on the changed surface. Comment/robustness only; no behavior change beyond the NaN guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kris Anderson <topkoa@gmail.com>
Finish the docstring pass for the CamDir bridge functions changed in this PR: convert the two per-panel _freeCamFor delegating wrappers to JSDoc, matching the pure _resolveFreeCam / _ssApi helpers. Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kris Anderson <topkoa@gmail.com>
_resolveFreeCam() (keys/drum) and _freeCamFor() (highway_3d) guarded the panel map lookup with only `i != null`, so a non-integer / negative / string index from panelIndexFor() could resolve an unintended or inherited property (e.g. map['toString']) instead of cleanly falling back to the global camera. Gate the index on `Number.isInteger(i) && i >= 0` before `map[i]`, matching the hardening already applied in _bgPanelKey(). Extend the resolver tests with float/string (prototype-key) cases. Behavior change only for malformed indices. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kris Anderson <topkoa@gmail.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
Rebased onto main; #823 already shipped 3.31.4 (per-panel camera), so this Butterchurn buffer-sizing fix advances to 3.31.5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
…ghway (#820) * fix(highway_3d): size Butterchurn output canvas buffer to fill the highway The 3D-highway Butterchurn background set only the output canvas CSS size and called setRendererSize(), but never sized the canvas DRAWING BUFFER (canvas.width/height). Butterchurn does not size the output canvas itself (renderToScreen viewports to the reported size into the default framebuffer), so the buffer stayed at the browser default 300x150 while the viewport was the full highway. Only the bottom-left ~300x150 of the pattern was drawn, then CSS-stretched across the whole highway -- zoomed, soft, and aspect-wrong, worse the larger the panel. Add _bcApplySize(cssW, cssH): set the drawing buffer to the device-pixel render size (round(css * min(DPR, 1.5))), confine every layer (canvas, backdrop, scrim, tint) to the highway rect, and report the same device px to setRendererSize so buffer == on-screen viewport. Seed the buffer at create and switch createVisualizer to pixelRatio:1, textureRatio:1 (DPR is now folded into the reported size, so buffer == viewport == internal texsize, no double-counting). render() and resize() both route through it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> * chore(highway_3d): bump to 3.31.5 (3.31.4 taken by #823 on main) Rebased onto main; #823 already shipped 3.31.4 (per-panel camera), so this Butterchurn buffer-sizing fix advances to 3.31.5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Byron Gamatos <xasiklas@gmail.com>
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase 1 of making Camera Director splitscreen-capable. The three 3D highways now read the Camera Director camera bridge per panel, so each splitscreen panel renders its own camera (independent orbit / height / zoom / tilt / pan) instead of every panel sharing the focused one. This is the renderer-side foundation; camera_director already publishes the per-panel map (
window.__h3dCamCtlPanels), so tiled splitscreen works against it today.How
A shared
_freeCamFor(canvas)resolver in each highway:Defensive on the splitscreen global name (
feedBackSplitscreen || slopsmithSplitscreen, rename in flight), NaN-safe, allocation-free — mirrors the existing_bgPanelKeyper-panel pattern.applyCamera()folds static base pose + kick-pulse dip + free-cam offsets (its camera was previously static).Safety / compatibility
null→ renderers behave byte-for-byte as before. The keys "default look unchanged" unit test confirms the stock path.Number.isFinite-coerced so a malformed bridge can never feed NaN intoposition/lookAt.Testing
plugins/{keys,drum}_highway_3d/tests/camera_bridge.test.js.node --testper file: drum 21/21, keys 56/56 pass (+6 each: the new camera_bridge suite).node --checkclean on all three.Part of the larger "named, shareable camera profiles for splitscreen" effort (camera_director + splitscreen companion PRs to follow).
🤖 Generated with Claude Code
Summary by CodeRabbit