Repository navigation
Group the bar so odd and compound meters sound like themselves - #596
Merged
Merged
Conversation
thcp
changed the base branch from
feat/587-count-in-length-and-custom-meter
to
main
September 8, 2026 12:15
…selves The click marked exactly one beat per bar, on the same rule in both implementations, with only two voices. Correct for 2/4, 3/4 and 4/4. Wrong for every meter that carries internal stresses: 7/8 came out as seven even clicks, which gives a player nothing to lock onto, and 6/8 rendered as six straight eighths when it is felt in two dotted-quarter groups. Adds a grouping to the bar. 7 is played 3+2+2, 6 and 9 and 12 in threes, 5 as 3+2, with an accent at each group start. A meter with no conventional reading, 11 for instance, stays flat rather than being guessed at. Grouping needs a third voice. With only an accent and a plain beat, a 3+2+2 bar has three identical accents and the downbeat stops being findable, so a group start now sounds between the two: 1225 Hz at 0.85, the geometric middle of the existing 1000 and 1500 because the ear hears pitch ratios. Both renderers gained it and both count-ins use it, so counting a player into 7/8 gives them the pulse they are about to play. 2, 3 and 4 are deliberately unchanged. 4/4 does carry a real secondary stress on beat 3, but turning that on by default would change the commonest meter in the app for every existing user. That is their choice to make, and the grouping box makes it available. 6/8 does change, and that is the point of the issue: it was shipping a reading of the meter that no player would count. The user can override the grouping for an explicit meter, "3+2+2" or "2+2+3". One that does not sum to the bar is refused wholesale rather than repaired, on both sides, because a half-understood grouping puts accents on beats nobody asked for. The export mirrors that rather than returning 422: a file is better served with the default grouping than refused. Under Auto no single grouping can apply, since the bar length can change from bar to bar, so each detected bar is grouped by its own length. That is what makes a detected 6/8 passage group in threes with nothing configured, and it needed a bar-position lookup on the live side, where only a downbeat yes/no had existed. The grouping enters the render cache key only when one is in force, so every export made before this keeps hitting the entry it already had. Playback and export are held to the same numbers in tests/js/count-in.test.mjs and tests/test_click_render.py, the existing parity discipline for this pair. Closes #595
Two problems, one root cause: the grouping was visible but never explained. The note under the waveform was wrong. With an explicit meter it said "accenting every 5 beats", which was true before grouping and is not true now: a 5 grouped 3+2 also stresses beat 4. The panel was describing a click it was no longer playing. It now names the grouping and the beats it stresses -- "counting 5 in 3+2 (stress on 1, 4)" -- which also makes the note the place that explains what the grouping box means. A meter with a single group keeps the old wording, so 4/4 and an ungroupable 11 read exactly as they did. The box itself was an unlabelled text field containing something like "3+2". Next to a number input showing 5, that is not decodable: it could be beats, bars, a ratio. It now carries a visible "Groups" label, shown and hidden with the box. Found by looking at the panel rather than the tests, which is why the tests did not catch it -- they asserted the grouping was applied, not that anyone could tell what it meant. The e2e test now pins both the label and the note's wording, and updating it surfaced that the 5-beat case had silently changed its note text. Refs #595
The Groups box is editable on purpose, and anything that adds up to the bar length is accepted: on a 5 that is 2+3, 4+1, 2+2+1, 1+1+1+1+1 or a plain 5. Anything that does not add up is refused, because a grouping the bar cannot hold would put accents on beats nobody asked for. The refusal was silent. The typed value simply reverted to the default with no message, which reads as the field being broken rather than as the input being rejected -- there was no way to learn the rule from the UI. A refused grouping now marks the box and puts the reason in the note that already explains the click: "A grouping has to add up to 5." Both clear after a moment, so the note goes back to describing what is playing. An empty box is a deliberate "use the default", not a mistake, so it is accepted quietly. Only a non-empty grouping that does not fit is worth complaining about. Reuses the existing .warn note style rather than adding one. Refs #595
Parity between the live click and the rendered one was enforced by writing the same expected numbers into tests/js/count-in.test.mjs and tests/test_click_render.py by hand. That catches a mistake in one file. It cannot catch the same wrong reasoning applied to both, which is the failure mode that actually matters here: a symmetric error ships a click that monitors one way and exports another, with two green suites. Both sides are now pinned to tests/fixtures/click_levels.json, generated from Python by tests/_click_parity.py. test_click_parity.py asserts the committed fixture is still what Python produces; tests/js/parity.test.mjs asserts the browser produces the same. Neither language can drift without a visible diff to that file, and regenerating it is a deliberate act in review rather than an edit buried in a test body. The table covers 14 meters, 18 level runs over two full bars each (so a wrong modulo shows on the second), and 24 count-ins across 1 and 2 bars -- including groupings that do not fit their bar and must be refused, and Auto, where each detected bar is grouped by its own length. A shared fixture rather than one side shelling out to the other, because neither CI job can run both languages: pytest runs in a uv container with no node, js-syntax in a node container with no python. A cross-invocation test would have skipped in CI and only ever run locally, which is the shape of a gate that quietly stops gating. Verified the gate fails as intended rather than assuming: grouping 7 as 2+2+3 on the JS side alone fails 13 checks and exits 1, and the mirror-image change on the Python side fails test_click_parity and exits 1. Both pass clean when restored. Refs #595
thcp
force-pushed
the
feat/595-meter-grouping
branch
from
September 8, 2026 12:20
f01675e to
7d1c138
Compare
thcp
marked this pull request as ready for review
September 8, 2026 12:36
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.
Stacked on #594. Review that one first -- this targets its branch, not
main, so the diff here is only the grouping work. Retarget tomainonce #594 lands.Closes #595
The problem
The click marked exactly one beat per bar, by the same rule in both implementations, with two voices. Fine for 2/4, 3/4 and 4/4. Wrong for anything with internal stresses.
7/8 was seven even clicks. No pulse to lock onto.
6/8 was six straight eighths with one accent, when it is felt in two dotted-quarter groups. That one is already shipping.
This is not the same gap as #593. That one let you pick 7. The click still played those seven beats flat.
What changed
Grouping. 7 becomes 3+2+2, 6/9/12 group in threes, 5 is 3+2, with an accent at each group start. A meter with no conventional reading (11) stays flat rather than being guessed at.
A third voice, which grouping requires. With only accent and plain, a 3+2+2 bar has three identical accents and the downbeat stops being findable. Group starts now sound between the two: 1225 Hz at 0.85. Geometric middle of the existing 1000 and 1500, because the ear hears pitch ratios rather than differences.
The count-in is grouped too, so counting a player into 7/8 gives them the pulse they are about to play.
A grouping box for explicit meters:
3+2+2,2+2+3. Shown only when the meter has more than one sensible reading, and prefilled with what is actually being played so you can see 7 is 3+2+2 before deciding to change it.Two judgement calls worth your review
2, 3 and 4 are deliberately unchanged. 4/4 has a real secondary stress on beat 3, but defaulting it on would change the commonest meter in the app for every existing user. The grouping box makes it available to anyone who wants it.
6/8 does change. It was shipping a reading no player would count, and fixing it is the point of the issue, but it means an existing preset sounds different after this.
Design notes
A grouping that does not sum to the bar is refused wholesale rather than repaired, on both sides: a half-understood grouping puts accents on beats nobody asked for. The export mirrors that instead of 422-ing, since a file is better served with the default grouping than refused.
Under Auto no single grouping can apply, because the bar length can change bar to bar, so each detected bar is grouped by its own length. That is what makes a detected 6/8 passage group in threes with nothing configured. It needed a bar-position lookup on the live side, where only a downbeat yes/no existed.
The grouping enters the render cache key only when one is in force, so every export made before this keeps hitting the entry it already had.
Verification
All local, on macOS arm64:
ruff check/format --checkpytest tests/ -qnode --check static/js/*.jstests/js/*.test.mjsnpx playwright testbandit -r app/ -llThe 2 failures are the ogg pair that fails on unmodified
mainon this machine (local ffmpeg has nolibvorbis).Playback and export are held to the same expected numbers in
tests/js/count-in.test.mjsandtests/test_click_render.py, which is the existing parity discipline for this pair. Both were extended together.The e2e test earned its place: it caught that the grouping box did not appear when the meter select changed, because the render hook was only on the panel-load path.