feat(highway_3d): fret wires flash on a confirmed hit - #969
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changelog’s unreleased section is substantially expanded with feature, refactor, UI, fix, and security notes. The classic v2 shell removal is documented, historical notes remain, and the 3D highway plugin version is updated to 3.32.0. ChangesRelease documentation and plugin metadata
Estimated code review effort: 1 (Trivial) | ~4 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The fret wires were static scenery: gold inside the anchor lane, grey outside, and nothing tied them to what the player was actually doing. Give them a job. Widen the lane/neck contrast so the wires around the active lane read as a focus cue, and flash the wires bracketing a note when a scorer confirms it. A fretted note lights the wire behind it and the wire it is pressed against; a chord lights only the outermost wires of its shape, so it reads as one bracketed block rather than a picket fence; an open string has no fret of its own and its gem is drawn as a slab spanning the lane, so it lights the lane's edge wires instead. Gated on the provider verdict, never the proximity heuristic -- the latter only means "near the strike line", so it would flash on every passing note whether or not it was played. With no scorer attached the neck behaves exactly as before. Emissive (and emissiveIntensity) carry the flash, not albedo: these are MeshStandard materials in a scene with no envMap, so raising albedo alone barely brightens them. Every value is a named constant -- see FRET_WIRE_* -- because the look is a taste call that wants tuning by eye, not a derivation. Signed-off-by: Kris Anderson <topkoa@gmail.com>
f55c021 to
981b779
Compare
Bring the branch up to date with main (includes #994, which also touched highway_3d/screen.js — the lane hit-line fix — in a different region).
Fast passages overlap their decay tails: consecutive notes on nearby frets left three, four, five wires glowing at once — the picket fence the chord rule was written to avoid, arriving through time instead of through a shape. The apply pass now decays every wire's glow state as before, but flashes only the outermost pair of the lit span (or the single wire when only one is above threshold). Interior wires keep decaying invisibly — the base tier loop re-seeds their materials each frame — so the bracket tightens naturally as the outer tails expire, and a hit inside the current span widens nothing. Net effect: at most two wires are ever lit, and everything currently glowing reads as one bracket, exactly like a chord. Signed-off-by: topkoa <topkoa@gmail.com>
The lit lane strip spans the anchor's width (minimum ~4 frets), which can run a fret past the chord's outermost fret. The chord flash bracketed the shape (wire behind its lowest fret, wire at its highest), so on those anchors the bracket sat one wire INSIDE the lit lane — reading as misaligned rather than as a frame around what's lit. Chord hits now light the anchor lane's edge wires: the exact wires the lane strip itself spans, and the same pair open strings already use, so every hit shape inside a lane produces the same bracket. The shape's own outer pair survives only as the fallback for charts with no anchors. Fretted and open intensities merge into one entry (they light the same two wires now), and an all-open chord on an anchor-less chart still degrades to no flash rather than a bad index. Signed-off-by: topkoa <topkoa@gmail.com>
On a confirmed hit the gem's outline now flashes in the STRING'S OWN colour with the same intensity treatment as the fret wires — the FRET_WIRE_HIT_INTENSITY emissive ramp, faded by the provider's alpha — instead of the fixed spring-green mHitBright rim. Just the rims: the lateral face fill keeps its existing green, and the sustain trail is untouched. Mechanics mirror the wires' pattern. mRimFlash[s] is one material per string (created with the other per-string materials, palette-retint aware, fog-exempt, disposed in teardown); drawNote() assigns it as the outline on a good verdict and records the verdict alpha into a per-frame per-string max (_rimFlashIn); the flash pass applies the intensity ramp once per string. Shared-per-string is the same compromise mGlow already makes — two same-string gems flashing in different phases share the brighter alpha. No decay tail of our own, deliberately: the material is only assigned while the provider confirms the note, and the provider's alpha already fades. When it goes silent the outline reverts, so idle intensity never shows. Signed-off-by: topkoa <topkoa@gmail.com>
The flash was instant-on with a 0.32 s exponential tail, and a held sustain kept re-feeding it — wires stayed lit for the whole note. The requested feel is a shock: light hits the frets, they jolt, it's over. The flash is now a one-shot pulse triggered on the input's rising edge: a near-instant crack up (RISE 25 ms), a fast fall (FALL 160 ms) shaped (1-u)^2 so it drops hard then eases out, with a 26 Hz flicker biting into the fall (the electric shudder — the crack itself stays clean), then hard zero. A held 'active' verdict keeps the input high continuously, which by construction triggers nothing new: one strike per hit, and the wires go dark while the note rings on. A re-strike after the provider goes silent re-triggers cleanly. Seeking backward or a long stall clears all pulse state, and a pulse whose strike time lands ahead of the playhead after a seek is discarded. The outer-pair bracket rule is unchanged — it now selects across pulses instead of decay tails. Knobs: FRET_WIRE_HIT_RISE / _FALL / _FLICKER_HZ / _FLICKER_DEPTH (replacing FRET_WIRE_HIT_DECAY). Signed-off-by: topkoa <topkoa@gmail.com>
The strike trigger was a rising edge on each WIRE's input, which merged distinct hits: two consecutive correct notes on the same fret kept that wire's input continuously high, so the second note produced no strike at all. The wires must respond to what the player did — one strike per judged hit-zone event. The trigger is now per event identity, using the same seen-map pattern as _sparkSeen: the first frame a note gets a good verdict its key (string|fret|time — or the chord key for a strum, which strikes once as a unit) lands in _fwStruck and requests a strike on its wires; the event never fires again however long its verdict stays live. Because every producer is gated, any nonzero input in the apply pass IS a fresh strike, so it restarts a pulse already in flight — a rapid re-hit on the same wire re-cracks instead of being swallowed. Seeks clear the map (replayed notes strike again); it is size-bounded like _sparkSeen. Envelope, flicker, and the outer-pair rule unchanged. Signed-off-by: topkoa <topkoa@gmail.com>
Reverts d003532 and e05d90e. The wire flash returns to its original behaviour: instant-on at the provider's alpha with a smooth exponential fade (FRET_WIRE_HIT_DECAY 0.32 s), held sustains keep their wires lit while the note rings, and no flicker. The outer-pair bracket, lane-framed chords, and string-coloured gem rims are untouched. Signed-off-by: topkoa <topkoa@gmail.com>
There was a problem hiding this comment.
Pull request overview
Updates the bundled highway_3d visualization to make fret wires visually respond to confirmed note hits, improving gameplay feedback and readability on the 3D highway.
Changes:
- Add a hit-confirmed “flash” effect to the fret wires bracketing the played note (and appropriate behavior for chords/open strings).
- Adjust base fret-wire visuals (lane/neck contrast and wire thickness/geometry) to better emphasize the active anchor lane.
- Bump the
highway_3dplugin version to3.32.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugins/highway_3d/plugin.json | Bumps plugin version to reflect the new rendering/feedback behavior. |
| plugins/highway_3d/screen.js | Implements fret-wire hit flash + related visual tuning and gem rim flash integration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
CHANGELOG.md (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate repeated changelog headings.
These repeated
Added,Changed,Fixed, andRemovedheadings trigger the supplied MD024 warnings. Merge each category into one section under[Unreleased], or explicitly configure the linter if segmented sections are intentional.Also applies to: 260-260, 295-295, 319-319, 340-340, 344-344, 352-352, 355-355, 365-365, 418-418, 446-446, 454-454, 457-457, 503-503, 513-513
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 117, Consolidate the repeated Added, Changed, Fixed, and Removed headings in the [Unreleased] section of CHANGELOG.md into one heading per category, preserving all existing entries and their order; only configure the linter instead if the segmented headings are intentionally required.Source: Linters/SAST tools
plugins/highway_3d/screen.js (2)
12734-12746: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFret-wire and gem-rim hit-flash intensity ignores the
glowMulslider.
_m.emissiveIntensity = 1 + (FRET_WIRE_HIT_INTENSITY - 1) * _g;(wires) and the matching gem-rim loop just below both applyFRET_WIRE_HIT_INTENSITYunscaled. Every other emissive material in this file (mHitBright,mStrHitOutline,mAccentOutline,mTapChevron,mBarre,mSusOutline, …) is driven through_applyGlow()or multiplied inline byglowMulso the "Glow" setting controls overall brightness. This new flash is the one place that stays full-intensity regardless of the user's glow preference.💡 Proposed fix
- _m.emissiveIntensity = 1 + (FRET_WIRE_HIT_INTENSITY - 1) * _g; + _m.emissiveIntensity = 1 + (FRET_WIRE_HIT_INTENSITY - 1) * _g * glowMul; ... - if (_m) _m.emissiveIntensity = 1 + (FRET_WIRE_HIT_INTENSITY - 1) * _rimFlashIn[_s]; + if (_m) _m.emissiveIntensity = 1 + (FRET_WIRE_HIT_INTENSITY - 1) * _rimFlashIn[_s] * glowMul;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/highway_3d/screen.js` around lines 12734 - 12746, Update the fret-wire hit-flash calculation and the gem-rim loop using _m.emissiveIntensity so FRET_WIRE_HIT_INTENSITY is scaled by the existing glowMul setting, consistent with the other emissive materials and preserving the current intensity ramp behavior.
13970-13985: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
_fwChordAcckey collision on repeated same-template chords within one frame.The accumulator is keyed by
chordId(falling back to`t${n.t}`only when nullish).ch.idis the chord-template id and is reused across every occurrence of the same shape (repeats/gallops, which this file explicitly handles elsewhere, e.g. the Frantic gallop comments). If two occurrences of the same chord are both confirmed-hit in the same frame, they'd collide into one entry, and_fwE.t(fixed at first-touch, never updated) could resolve the wrong anchor-lane wires for the later occurrence.🔧 Proposed fix
- const _fwK = (chordId !== undefined && chordId !== null) ? chordId : `t${n.t}`; + const _fwK = (chordId !== undefined && chordId !== null) ? `${chordId}|${n.t}` : `t${n.t}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/highway_3d/screen.js` around lines 13970 - 13985, Make the _fwChordAcc key uniquely identify each chord occurrence within the frame, not just the reusable chord-template chordId. Incorporate the occurrence’s timing/anchor identity into the key while preserving the existing fallback for missing chord IDs, and ensure _fwE.t remains associated with that specific occurrence so repeated same-template chords cannot share accumulator entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CHANGELOG.md`:
- Line 117: Consolidate the repeated Added, Changed, Fixed, and Removed headings
in the [Unreleased] section of CHANGELOG.md into one heading per category,
preserving all existing entries and their order; only configure the linter
instead if the segmented headings are intentionally required.
In `@plugins/highway_3d/screen.js`:
- Around line 12734-12746: Update the fret-wire hit-flash calculation and the
gem-rim loop using _m.emissiveIntensity so FRET_WIRE_HIT_INTENSITY is scaled by
the existing glowMul setting, consistent with the other emissive materials and
preserving the current intensity ramp behavior.
- Around line 13970-13985: Make the _fwChordAcc key uniquely identify each chord
occurrence within the frame, not just the reusable chord-template chordId.
Incorporate the occurrence’s timing/anchor identity into the key while
preserving the existing fallback for missing chord IDs, and ensure _fwE.t
remains associated with that specific occurrence so repeated same-template
chords cannot share accumulator entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1d49ac1-5125-4563-bd8d-78a747edf7e8
📒 Files selected for processing (3)
CHANGELOG.mdplugins/highway_3d/plugin.jsonplugins/highway_3d/screen.js
The two render-order tests pinned the old literal hexes (idle 0x666688). The tiers moved to named constants with a retuned idle (FRET_WIRE_IDLE_HEX 0x4A4A60); the tests now assert the code uses the constants AND pin the constants' values, so a future retune is a deliberate two-line change here rather than a silent one. Signed-off-by: topkoa <topkoa@gmail.com>
The wire-flash path clamps the note-state provider's alpha to 0..1; the rim-flash accumulation used it raw, so a provider returning >1 would over-drive emissiveIntensity. Clamped to match. Signed-off-by: topkoa <topkoa@gmail.com>
…view) Signed-off-by: topkoa <topkoa@gmail.com>
Two review findings on the render-order test file, both correct: The header claimed ALL 3D-highway materials use depthTest:false, making renderOrder "the only" draw-order control — but the accent halo materials set depthTest:true. Now says "nearly all", names the exception, and calls renderOrder the primary control. A header someone trusts mid-debug must not overclaim. The fret-wire depthTest/depthWrite assertions matched anywhere in screen.js, which is full of other depthTest:false materials — the test would keep passing if the wire material dropped the flags. Both are now anchored to the wire material literal via FRET_WIRE_IDLE_HEX (unique to it), as two separate anchored matches so property order inside the literal still isn't pinned. Signed-off-by: topkoa <topkoa@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/js/highway_3d_render_order.test.js:216
- The depth-flag assertions for the fret-wire material are still vulnerable to false positives because they aren’t anchored to the
new T.MeshStandardMaterial({ ... })literal itself, and they also implicitly requirecolor:to appear before thedepth*properties. You can make this both order-independent and scoped to the fret-wire material by matching the MeshStandardMaterial literal and using lookaheads within a single{...}block.
// Both depth flags anchored to the fret-wire material literal (via its
// FRET_WIRE_IDLE_HEX color, unique to it) — an unscoped match would pass
// off any other depthTest:false material in the file. Asserted as two
// separate anchored matches so property order inside the literal still
// isn't pinned.
assert.match(
s,
/color\s*:\s*FRET_WIRE_IDLE_HEX[\s\S]{0,400}?depthTest\s*:\s*false/,
'the fret wire material itself must set depthTest: false',
);
assert.match(
s,
/color\s*:\s*FRET_WIRE_IDLE_HEX[\s\S]{0,400}?depthWrite\s*:\s*false/,
'the fret wire material itself must set depthWrite: false (no z-buffer pollution)',
What
The 3D highway's fret wires were static scenery — gold inside the anchor lane, grey outside, and nothing tied them to what the player was doing. This gives them a job:
Which wires flash depends on the note:
ff-1andf— the wire behind it, and the wire it's pressed againstVerified on screen
Verified by the maintainer against a scripted note-state provider (a console-driven "perfect player" that confirms every note as it crosses the hit zone — the dev machine has no guitar attached). The additions below came out of that on-screen iteration.
Additions since the first draft
The levers — all named constants, all in one block
All in
plugins/highway_3d/screen.js, in the fret-wire constants block next toFRET_METALNESS/FRET_BOW_DZ. Nothing here is derived from anything else, so each can be moved independently.Hit flash
FRET_WIRE_HIT_INTENSITY4.2emissiveIntensityat full flash (baseline1). This is what makes a wire read as a light source rather than a brightly-lit object. Went3.0 → 6.0 → 4.2; last feedback on6.0was "slightly too bright", so4.2is an untested guess between.FRET_WIRE_HIT_DECAY0.320.22before dropping the intensity.FRET_WIRE_HIT_EMISSIVE0xFFE9B0FRET_WIRE_HIT_HEX0xFFFFFFFRET_WIRE_HIT_OP1.0Why emissive and not colour: these are
MeshStandardMaterialin a scene with no envMap, so raising albedo alone barely brightens them — it saturates toward white and stops.emissiveIntensityis the lever with real range. Corollary: there's no bloom/post-processing pass in this scene, so past ~8 you're just painting white pixels for longer and can't get glow bleed into the surrounding area. If it needs to be brighter than the material can go, the next step is an additive halo mesh around the flashing wire — a bigger change, not a constant.Base look (the two idle tiers)
FRET_WIRE_ACTIVE_HEX/_OP0xD8A636/0.90xD8A636/0.8FRET_WIRE_IDLE_HEX/_OP0x4A4A60/0.280x666688/0.4FRET_TUBE_RADIUSSTR_THICK * 0.75* 0.55FRET_TUBE_RADIAL86Things to actually look at when picking this up
0.32s tail, consecutive notes on nearby frets overlap their flashes, so neighbouring wires may stay lit more or less continuously. That could look great (a glowing band tracking your hand) or muddy (nothing ever goes dark). This is the single most likely thing to need tuning.alphatracking live input level for a held note, which jitters frame to frame. The decay tail is what smooths it — check it's actually smooth and not pulsing.0.28vs0.4); confirm distant wires haven't been swallowed entirely.anchorLaneBoundsAtreturns null, so open-string flashes silently do nothing (fretted notes are unaffected). Rare — GP imports synthesise anchors — but it's the one case that fails quietly rather than visibly.Implementation notes for a reviewer
_ndGood), never the proximityhitheuristic. The latter only means "near the strike line" — using it would flash on every passing note whether or not it was played. So with no scorer attached, the neck behaves exactly as it does today.update(), long before any note is drawn. The flash is applied in a second pass after the note and chord draw loops, so it sees this frame's verdicts rather than the previous frame's.emissive/emissiveIntensityare re-seeded to baseline in the tier loop each frame — without that, a flash would never fade back out.drawNote()is the chord loop's per-note call and can't see the chord's span, so chord hits collect into a smallMap(keyed by chord id, falling back to chord time —ch.idcan be absent, a documented pitfall) and the flash pass resolves min/max fret into the outer wire pair. This avoided restructuring the chord loop with begin/end hooks.drawNote()is a sibling ofupdate(), not nested in it, so the anchors are snapshotted into a closure var (_drawAnchors) — following the pattern already established there for_drawChordTemplates.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores