fix(PatternGenerator): keep randomWalk within the value range - #1484
Open
Jaybhade wants to merge 1 commit into
Open
fix(PatternGenerator): keep randomWalk within the value range#1484Jaybhade wants to merge 1 commit into
Jaybhade wants to merge 1 commit into
Conversation
With a single value the walk has nowhere to step: index 0 is both the bottom and the top, the bottom check wins and increments unconditionally, and from index 1 onwards neither boundary check matches again, so the walk drifts off the end of the array forever. Tone.Pattern then passes undefined to its callback on every tick. Hold the index still when there is only one value, which matches what every other generator does for that case. The walk is unchanged for two or more values.
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.
The bug
randomWalkis the only pattern generator that doesn't bound its index, and with a single value it walks straight off the end of the array and never comes back:Index 0 is both the bottom and the top of a one-value range. The bottom check comes first, so it wins and steps up to 1. From there neither boundary check can ever match again —
index === 0is false, andindex === numValues - 1is the same test — so every following step is an unguarded random ±1 with nothing above it.Through the public API that means a one-value
Tone.Patternhandsundefinedto its callback on almost every tick, since_tickdoesthis._values[index.value]:A single-value array is legal —
PatternGeneratoronly assertsnumValues >= 1— and it is easy to land on whenvaluescomes from data and is assigned at runtime.The fix
Hold the index still when there is nowhere to step to. Every other generator already pins to 0 for a single value through
clamp(index, 0, numValues - 1), so this just bringsrandomWalkin line with its neighbours. For two or more values the walk is unchanged.Tests
Two tests added next to the existing
randomWalktest:never randomly walks outside of the range of values— asserts the invariant the bug broke, over sizes 1 through 4.randomly walks in place when there is only one value— deterministic, expects ten0s.The existing
randomWalktest can't catch this: it uses five values and only asserts that consecutive indices differ by 1, which stays true while the walk is drifting past the end of the array.Both new tests fail on
devand pass with the change. Before the fix:Verification
npm testfor theeventgroup, which covers the changed file — 6/6 test files, 182 passing, 0 failing (180 before the two new tests).npm run lintandnpm run spellcheckare both clean, and prettier reports no formatting changes. I couldn't get a single-browser run of all 145 test files to finish on this machine — it wedges partway through with unrelatedClock/TickSourcetiming failures that don't reproduce when that group runs on its own — so the remaining groups are covered by CI here rather than locally.