fix(venue/highway): flyover replay on arrangement switch, venue on Virtuoso, and the paused throttle starving the venue#968
Merged
Conversation
…he venue off other screens Two bugs from a live career session. 1. CHANGING ARRANGEMENT REPLAYED THE ARRIVAL FLYOVER. changeArrangement() reloads the song through the normal load path, so highway.js re-emits `song:loaded` — same filename, new arrangement. The venue could not tell that from a fresh arrival, so it reset the machine and flew the camera in from the back of the room again, mid-set, every time the player switched lead -> rhythm. The player is already on stage. onSongLoaded now compares the filename. A repeat of the song already on stage keeps the video pipeline running and only re-syncs the mood: the performance restarts, so the loop follows the reset machine with a quiet crossfade, never the intro. A genuinely different song still gets the full teardown + flyover. 2. THE VENUE SHOWED UP ON THE VIRTUOSO HIGHWAY. The venue was gated purely on `isVenueViz()` — the selected visualization, which is a GLOBAL preference and says nothing about what is on screen. Virtuoso borrows the same highway_3d renderer for its practice charts, so with Venue selected it inherited the backdrop: the crowd and the stage behind a chromatic exercise. Selecting Venue is a preference for the PLAYER; it is not a licence to paint the venue over whatever else happens to be using the renderer. The venue is now gated on viz AND screen (`shouldBeActive`), and follows `screen:changed` — it tears down on leaving the player and rebuilds on return. Nothing else changes: stop() already unbinds the videos from the renderer, so deactivating is enough to clear the backdrop. Tests: both decisions exposed as pure predicates and pinned — arrangement switch vs new song (including the first load, and a malformed payload that must not suppress the flyover forever), and the venue's screen scope. The existing syncViz test encoded the OLD contract (activate regardless of screen), so it now states the new one and additionally asserts the venue does NOT activate on virtuoso. Includes a guard test: with Venue selected AND on the player, the venue IS active — without it, every "not active" assertion could pass vacuously. All 8 new/updated assertions fail against the pre-fix source. eslint clean; JS 1199/1199; pytest 2597 passed.
Contributor
📝 WalkthroughWalkthroughVenue crowd loading distinguishes arrangement switches from new songs. Venue 3D rendering is restricted to the player screen, with runtime screen-change handling. Paused highway rendering now remains continuous when active venue crowd videos require animation. Tests cover these behaviors and defensive failure paths. ChangesVenue behavior scoping
Continuous venue rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HighwayDraw
participant Renderer
participant Highway3D
participant CrowdVideo
HighwayDraw->>Renderer: needs continuous frames?
Renderer->>Highway3D: needsContinuousFrames()
Highway3D->>CrowdVideo: inspect playback state
CrowdVideo-->>Highway3D: active or inactive video
Highway3D-->>Renderer: true or false
Renderer-->>HighwayDraw: bypass or apply paused throttle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Pausing the song dropped the venue, the crowd and the stage to ~10 fps — "everything around the highway drops fps by a lot". draw() caps paused frames to one per _PAUSED_FRAME_INTERVAL_MS (100ms), on an assumption stated plainly in highway-constants.js: a heavy WebGL renderer "does a full render every frame even while paused. That is pure waste." That was true when a paused chart was a still picture. The venue broke the assumption. Its video backdrop keeps playing and its crowd reacts on a clock of their own, and BOTH are drawn into the same canvas as the notes — so a throttle aimed at static notes throttled the entire room. The scene only got a texture upload 10 times a second while the transport sat paused. Renderers can now declare that their picture is not static while the chart clock is stopped: an optional needsContinuousFrames(). The throttle is skipped only when it returns exactly true, and the probe fails closed — a renderer that doesn't implement it, or one that throws, keeps the throttle unchanged. So the GPU saving that motivated #654 survives everywhere it was actually valid. highway_3d implements it and claims continuous frames ONLY while a crowd video is genuinely rolling (bound, unpaused, not ended, readyState >= 2). With no venue pack — the common case — the paused scene really is static, so it keeps the throttle and the GPU still idles. Tests extend tests/js/highway_pause_throttle.test.js, which guards this code path source-level (the draw loop owns the rAF + WebGL lifecycle and is deliberately not reproduced in a vm — see the file header). The new guards pin that the capability GATES the early return rather than merely being called near it, that the probe fails closed on absent/non-function/throwing/truthy-but-not- true, and that the 3D renderer keys off the real video elements and can still return false. All 3 fail against the pre-fix source. eslint 0 errors; JS 1202/1202; pytest 2597 passed.
byrongamatos
changed the title
byrongamatos
added a commit
that referenced
this pull request
Jul 15, 2026
Tester, mid-gig: "the second song in the gig started when the first one ended. But it showed the flyover intro again." The flyover is arriving at the venue, and you arrive once. #968 stopped it replaying on an arrangement SWITCH (same filename), but a gig's song 2 is a genuinely different file, so it took the full-teardown path and played the arrival flyover again — the camera flew in from the back of the room before every track of the set. The play queue now answers isContinuation(): false for the first song of a set (or a standalone play — an arrival), true for song 2..N. onSongLoaded carries the room over to the new song's loop on a continuation, and only a real arrival plays the intro. Verified on the built AppImage: isContinuation goes false (song 1) -> true (song 2) across an advance, and song 2 no longer flies in. Also confirmed NOT a bug, same session: "didn't show the author for the second song." The credits card shows on a queue advance whenever the song carries authors — reproduced with a song that has them as the advanced-to track. The tester's song 2 simply had no `authors:` metadata (most auto-converted feedpaks don't). No code change. Tests: isContinuation across start/advance/clear, and that onSongLoaded gates the flyover on the continuation check. Both fail on pre-fix source. JS 1214/1214.
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
1 participant
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.
Three bugs from a live career session.
1. Changing arrangement replayed the venue arrival flyover
changeArrangement()reloads the song through the normal load path, sohighway.jsre-emitssong:loaded— same filename, new arrangement. The venue could not tell that from a fresh arrival, so it flew the camera in from the back of the room again, mid-set, every time the player switched lead → rhythm.onSongLoadednow compares the filename. Same song → keep the video pipeline running and only re-sync the mood (quiet crossfade, never the intro). Different song → full teardown + flyover, as before.2. The venue showed up on the Virtuoso highway
The venue was gated purely on
isVenueViz()— the selected visualization, a global preference that says nothing about what is on screen. Virtuoso borrows the samehighway_3drenderer for its practice charts, so with Venue selected it inherited the backdrop: crowd and stage behind a chromatic exercise.Selecting Venue is a preference for the player, not a licence to paint the venue over anything else using the renderer. Now gated on viz AND screen, following
screen:changed. Nothing else was needed —stop()already unbinds the videos from the renderer.3. Pausing the song throttled the whole venue to ~10 fps
draw()caps paused frames to one per_PAUSED_FRAME_INTERVAL_MS(100 ms). The rationale is stated plainly inhighway-constants.js: a heavy WebGL renderer "does a full render every frame even while paused. That is pure waste."That was true when a paused chart was a still picture. The venue broke the assumption — its video backdrop keeps playing and its crowd reacts on a clock of their own, and both are drawn into the same canvas as the notes. So a throttle aimed at static notes throttled the entire room: the scene only got a texture upload 10× a second while the transport sat paused.
Renderers can now declare their picture is not static while the chart clock is stopped — an optional
needsContinuousFrames(). The throttle is skipped only when it returns exactlytrue, and the probe fails closed: a renderer that does not implement it, or one that throws, keeps the throttle unchanged. The GPU saving that motivated #654 survives everywhere it was actually valid.highway_3dimplements it and claims continuous frames only while a crowd video is genuinely rolling (bound, unpaused, not ended,readyState >= 2). With no venue pack — the common case — the paused scene really is static, so it keeps the throttle and the GPU still idles.Tests
Every decision is pinned, and all new assertions fail against the pre-fix source:
documentfails closedtrue; and that the 3D renderer keys off the real video elements and can still returnfalseTwo things worth calling out:
syncViztest encoded the old contract (activate regardless of screen) — which is bug 2. It now states the new contract and additionally asserts the venue does not activate on virtuoso.The throttle guards are source-level, matching
highway_pause_throttle.test.js's existing approach — the draw loop owns the rAF + WebGL lifecycle and is deliberately not reproduced in a vm (see that file's header).eslint 0 errors; JS 1202/1202; pytest 2597 passed.
Not verified
None of this is driven on-device — they are runtime/visual behaviours and I have no career gig running here. The logic is pinned; the flyover continuity, the Virtuoso screen, and the paused framerate all want an eyeball on the next build.
Summary by CodeRabbit
Bug Fixes
Tests