fix: unify section-difficulty fill formula with drawHud's tiers - #79
Merged
Conversation
The per-section "glass fill" emitted for feedBack-plugin-sectionmap (difficulty:sections-updated) used a different formula than this plugin's own player HUD: drawHud() computed a discrete difficulty tier (idxLevel/max_difficulty), while calculateAndEmitSectionDifficulties() used a continuous mastery-scaled fraction. These disagree materially for a lower-depth phrase/section (e.g. 50% vs. 30% for the same mastery/difficulty pair) -- the two formulas were only never visible side by side because drawHud() already defers to sectionmap's minimap when it's installed. - screen.js: extracted _tierFillFrac(mastery, maxDifficulty) as the single source of truth for the discrete-tier arithmetic; _presentedDifficultyLevel() and drawHud() now delegate to it, and calculateAndEmitSectionDifficulties() now uses it (applied to the section's own aggregated maxSectionDifficulty) instead of its old continuous formula. - INTEGRATION.md: rewritten to describe the real, current contract. section_map was rewritten at some point to consume the difficulty:sections-updated event exclusively -- it no longer reads highway.getPhrases()/getMastery() independently at all -- but both this file and section_map's own CLAUDE.md kept describing the old Host-getters-only architecture as current fact. Also documents the load-order assumption (alphabetical script load puts difficulty_ladder before section_map) and fallback/stale-event behavior, and flags a dead window.feedBackViz_dynamic_difficulty probe left over from the plugin's rename (fixed in the feedBack-plugin-sectionmap repo). - Tests: _tierFillFrac coverage (tier boundaries, no-ladder case, mastery clamping) and a calculateAndEmitSectionDifficulties test demonstrating the exact 50%-vs-30% case the old formula got wrong.
Reviewer's GuideThe PR fixes inconsistent section-glass fills by sharing the HUD’s discrete difficulty-tier formula with emitted section updates, adds regression coverage, and rewrites integration documentation around the current event-driven section-map contract while releasing version 0.9.11. Sequence diagram for unified section difficulty updatessequenceDiagram
participant DifficultyLadder
participant Host as window.highway
participant FeedBack as window.feedBack
participant SectionMap
DifficultyLadder->>Host: getPhrases()
DifficultyLadder->>Host: getMastery()
DifficultyLadder->>DifficultyLadder: _tierFillFrac(mastery, maxSectionDifficulty)
DifficultyLadder->>FeedBack: emit difficulty:sections-updated
FeedBack->>SectionMap: difficulty:sections-updated payload
SectionMap->>SectionMap: _smUpdateDifficultyFills()
SectionMap->>SectionMap: render fillPercentage and glassSize
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="screen.js" line_range="607" />
<code_context>
+ // used a different, continuous mastery-scaled fraction here,
+ // which could disagree materially with drawHud()'s discrete
+ // tiers for the same mastery/difficulty pair.
+ var fillPercentage = _tierFillFrac(mastery, maxSectionDifficulty).fillFrac * 100;
// Determine glass size based on section difficulty
</code_context>
<issue_to_address>
**issue (bug_risk):** A generated empty section can have `max_difficulty: 0` while another phrase in the song has a positive difficulty, as allowed by the generation code. The old section formula emitted a 0% fill for that section, but `_tierFillFrac()` now emits 100%, so an empty/no-difficulty section is rendered as a fully mastered glass.
**Triggers:** When a song contains an empty section phrase alongside at least one positive-depth phrase.
**Suggested fix:** Preserve the section renderer's prior zero-depth behavior by emitting 0% (or omitting the glass) when `maxSectionDifficulty <= 0`, while retaining the fully-filled convention only for the per-phrase HUD if that is required.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: screen.js:607
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ One edge-behavior observation worth confirming — the fix itself is clean and correct.
Reviewed changes
screen.js— extracted_tierFillFrac(mastery, maxDifficulty)as the single source of truth for the discrete-tier fill arithmetic, and migrated all three former sites (_presentedDifficultyLevel,drawHud's per-phrase glass, andcalculateAndEmitSectionDifficulties' section fill) onto it. Section fill now applies the tier formula to the section's ownmaxSectionDifficultyinstead of scaling against the song-wide max.INTEGRATION.md— rewritten to describe the real, current event-based contract (difficulty:sections-updatedis the API), with load-order, fallback, and stale-event behavior documented; the issue-#8 record kept under a superseded marker.plugin.json—0.9.10 → 0.9.11(patch, real-behavior bug fix).CHANGELOG.mdupdated.tests/screen.test.js— 4 new tests covering_tierFillFracboundaries/clamping and the 50%-vs-30% section case; 60/60 pass locally andnode --checkis clean.
I verified the refactor is semantically equivalent at all three migrated sites (the only delta is the deliberate mastery clamp inside _tierFillFrac, which is harmless for in-range inputs), that both INTEGRATION.md globals it names (_ddCapabilities.sectionDifficulty at screen.js:25, __slopsmithSectionMapHooksInstalled at screen.js:976) are grounded in the actual code, and that the new section test genuinely pins the fix (it would still fail with the old continuous formula). No remaining un-migrated tier sites exist in screen.js.
Big Pickle (free) ｜ 𝕏
Sourcery caught a real regression in the previous commit: _tierFillFrac's "no ladder -> fully filled" convention is correct for drawHud's per-phrase case (a single-tier phrase reads as already at its one level), but wrong for the section-level aggregate, where maxSectionDifficulty === 0 means "nothing overlaps this section at all" -- e.g. an empty/silent section next to phrases that do have depth. The old continuous formula always emitted 0% for this case; routing it through _tierFillFrac's shared zero-difficulty branch silently flipped it to 100%. calculateAndEmitSectionDifficulties() now special-cases maxSectionDifficulty <= 0 to 0%, matching prior behavior, while still using the shared discrete-tier math for every section that actually has difficulty content (the real fix this PR is about). Added a regression test reproducing the exact trigger Sourcery described.
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
2 participants
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
The audit in #63 found the documented Section Map integration contract contradicts what's actually implemented — and it's worse than the issue text: verifying against the real code in both repos turned up two independent, distinct problems, not one.
1. The docs describe an architecture that no longer exists.
INTEGRATION.md(andfeedBack-plugin-sectionmap's ownCLAUDE.md) both claim "no direct plugin-to-plugin API — both plugins independently read the same Host getters." That was true at one point, butsection_map's glass rendering was since rewritten to consume this plugin'sdifficulty:sections-updatedevent exclusively — its currentscreen.jshas no code path left that readshighway.getPhrases()/getMastery()for difficulty data at all. Both docs kept asserting the old architecture as current fact.2. The two glass renderers actually use different formulas. This plugin's own player HUD (
drawHud()) computes a discrete difficulty tier:idxLevel = min(max_difficulty, floor(mastery * (max_difficulty + 1))),fillFrac = idxLevel / max_difficulty. The event it emits forsection_mapused a continuous formula instead:mastery * maxSectionDifficulty / globalMaxDifficulty. These disagree materially for a lower-depth phrase/section — e.g. mastery 0.6 against a max_difficulty-2 phrase/section (of a song-wide max of 4) lands on discrete tier 1 of 2 (50%), not the continuous formula's 30%. The two were never visible side-by-side (drawHud()already defers tosection_mapwhen it's installed), which is likely why this drifted unnoticed.Fix
screen.js— extracted_tierFillFrac(mastery, maxDifficulty)as the single source of truth for the discrete-tier arithmetic._presentedDifficultyLevel()anddrawHud()now delegate to it;calculateAndEmitSectionDifficulties()now uses it too (applied to the section's own aggregatedmaxSectionDifficulty— the existing max-of-overlapping-phrases rule is unchanged), replacing its old continuous formula.INTEGRATION.md— rewritten to describe the real, current contract: the event is the API, with the load-order assumption (difficulty_ladder<section_mapalphabetically, so the capability marker is set beforesection_map's one-time availability check) and fallback/stale-event behavior documented. The old issue-Verify integration with feedBack-plugin-sectionmap section difficulty visualization #8-era content is kept below a "superseded" marker as historical record rather than deleted outright.feedBack-plugin-sectionmapcorrects itsCLAUDE.mdand removes a related dead check — see that repo's PR.Fixes #63
Checklist
pytest— 137/137 passingnode tests/screen.test.js— 60/60 passing (4 new:_tierFillFractier boundaries/no-ladder/clamping, and acalculateAndEmitSectionDifficultiestest demonstrating the exact 50%-vs-30% case the old formula got wrong)plugin.jsonversion (0.9.10 → 0.9.11 — bug fix affecting real behavior)CHANGELOG.mdGenerated by Claude Code
Summary by Sourcery
Align section-map difficulty visualization with the player HUD and document the current event-based integration contract.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: