Repository navigation
fix(Tuplets): preserve written dots on every chord member - #1842
Merged
sschmidTU merged 3 commits intoOct 23, 2026
Merged
Conversation
sschmidTU
requested changes
Oct 23, 2026
sschmidTU
left a comment
sschmidTU
left a comment
Contributor
There was a problem hiding this comment.
Thanks a lot for this PR, and for the detailed description and your checks with the file from the issue! I checked it on its base, develop 5b2158f:
- With the files from the issue, I get the same as you: the 16 extra dots are gone, the dotted half notes keep theirs, and the control has none.
- As you wrote, none of the sample images changes. Neither do the 125 scores of the LilyPond test suite, nor 242 more scores I use for testing.
- The suite passes in Firefox and Chrome (765 tests), and lint and tsc are clean. The four tests you mentioned fail on develop, in both browsers.
The change itself looks good to me: it extends the fix for #1082 (3899031), which took the written dots only for the lowest note of the chord, to all its notes, and your comment in the code explains well why that is needed. I'd just like to ask for a smaller test:
- VexFlow internals. The test checks VexFlow's modifiers, duration and ticks (
getModifiers(),getCategory(),getDuration(),getTicks(),Vex.Flow.RESOLUTION). These can change with a VexFlow update, although nothing visible changes. For example, VexFlow 5 (see #1673) calls the dot categoryDotinstead ofdots: the filter would then find no dots, so the undotted cases would pass whatever is drawn, and the dotted ones would fail. OSMD's ownGraphicalNote.numberOfDotsof each note is enough, asStaveNote()draws as many dots as the chord member with the most. The tick check tests the existing tick correction inVexFlowMeasure, which sets the ticks of tuplet notes fromgraphicalNoteLength, rather than this change. - The second render.
StaveNote()runs when the score is loaded, not inrender(), so a second render can't change the dots. - The number of cases. Seven cases with two renders each seem a lot to me for an encoding this rare. Since your change sets the dots of every chord member, the descending pitch order and the double dots don't test more than the undotted and the dotted case. The three controls pass on develop too; of them, I'd keep the one without
<type>, as it checks that the written dots are only used where a type is written.
Also, a small sample in test/data would make the case visible in the visual regression tests (npm run test:visual), where no image changes with this PR so far. The diff below adds one, test_tuplet_nested_chord_dots.musicxml, with the nested tuplets of the issue's file (an outer 6:9 tuplet with an inner 2:3 tuplet of one note, numbers and brackets hidden, as MuseScore exported them): two undotted quarter-note dyads, an undotted and a dotted one, and two dyads without <type>, whose dots come from their duration. The test checks numberOfDots of every note in it:
- On develop, it fails with the dots from the issue: measure 1 has
[[0, 3], [0, 1]]instead of[[0, 0], [0, 0]], so its dyads get 3 dots and 1 dot. - It also fails if the written dots are used without a
<type>(measure 3), or if the chord members get no dots instead of the written ones (measure 2). - With your change, the suite passes in Firefox and Chrome (759 tests), and lint and tsc are clean.
The image shows the sample on develop (above) and with your change (below):
Diff (WrittenDots_Test.ts and the new sample), applies to your branch
diff --git a/test/MusicalScore/Graphical/VexFlow/WrittenDots_Test.ts b/test/MusicalScore/Graphical/VexFlow/WrittenDots_Test.ts
index c6638d02..4cbab7ac 100644
--- a/test/MusicalScore/Graphical/VexFlow/WrittenDots_Test.ts
+++ b/test/MusicalScore/Graphical/VexFlow/WrittenDots_Test.ts
@@ -1,8 +1,8 @@
import { expect } from "chai";
import { TestUtils } from "../../../Util/TestUtils";
import { OpenSheetMusicDisplay } from "../../../../src/OpenSheetMusicDisplay/OpenSheetMusicDisplay";
-import { VexFlowVoiceEntry } from "../../../../src/MusicalScore/Graphical/VexFlow/VexFlowVoiceEntry";
-import Vex from "vexflow";
+import { GraphicalMeasure } from "../../../../src/MusicalScore/Graphical/GraphicalMeasure";
+import { GraphicalVoiceEntry } from "../../../../src/MusicalScore/Graphical/GraphicalVoiceEntry";
describe("Written augmentation dots", () => {
let container: HTMLElement;
@@ -19,76 +19,20 @@ describe("Written augmentation dots", () => {
container.remove();
});
- function score(dots: number, nested: boolean, chord: boolean, descending: boolean = false, writtenType: boolean = true): string {
- const factor: number = 2 - 1 / Math.pow(2, dots);
- const pitches: string[] = chord ? (descending ? ["E", "C"] : ["C", "E"]) : ["C"];
- const notes: string = Array.from({length: 2}, (_, index: number) => {
- const actual: number = nested ? (index === 0 ? 12 : 6) : 1;
- const normal: number = nested ? (index === 0 ? 27 : 9) : 1;
- let tuplets: string = "";
- if (nested) {
- if (index === 0) {
- tuplets = '<tuplet type="start" number="2" show-number="none"><tuplet-actual><tuplet-number>2</tuplet-number>' +
- "</tuplet-actual><tuplet-normal><tuplet-number>3</tuplet-number></tuplet-normal></tuplet>" +
- '<tuplet type="stop" number="2"/>';
- } else {
- tuplets = '<tuplet type="stop" number="1"/>';
- }
- }
- return pitches.map((step: string, pitchIndex: number) => `<note>${pitchIndex > 0 ? "<chord/>" : ""}
- <pitch><step>${step}</step><octave>4</octave></pitch>
- <duration>${480 * normal / actual * factor}</duration><voice>1</voice>
- ${writtenType ? "<type>quarter</type>" + "<dot/>".repeat(dots) : ""}
- ${nested ? `<time-modification><actual-notes>${actual}</actual-notes><normal-notes>${normal}</normal-notes>
- <normal-type>eighth</normal-type></time-modification>` : ""}
- ${pitchIndex === 0 && tuplets ? `<notations>${tuplets}</notations>` : ""}
- </note>`).join("");
- }).join("");
- return `<?xml version="1.0" encoding="UTF-8"?><score-partwise version="4.0">
- <part-list><score-part id="P1"><part-name>Dots</part-name></score-part></part-list>
- <part id="P1"><measure number="1"><attributes><divisions>480</divisions>
- <time><beats>${nested ? 12 + 60 * factor : 32 * factor}</beats><beat-type>64</beat-type></time>
- <clef><sign>G</sign><line>2</line></clef></attributes>
- ${nested ? `<note><rest/><duration>360</duration><voice>1</voice><type>eighth</type>
- <time-modification><actual-notes>6</actual-notes><normal-notes>9</normal-notes></time-modification>
- <notations><tuplet type="start" number="1" show-number="none"/></notations></note>` : ""}
- ${notes}</measure></part></score-partwise>`;
- }
+ /** The dots of the sample's dyads, per measure (see the comment in the sample): m.1 two undotted dyads in nested tuplets,
+ * m.2 an undotted and a dotted one, m.3 two dyads without <type>, dotted quarter notes by their duration. */
+ const expectedDots: number[][] = [[0, 0], [0, 1], [1, 1]];
- for (const [description, dots, nested, chord, descending, writtenType] of [
- ["undotted chords in nested and enclosing tuplets", 0, true, true, false, true],
- ["undotted tuplet chords whose XML pitches descend", 0, true, true, true, true],
- ["dotted tuplet chords", 1, true, true, false, true],
- ["double-dotted tuplet chords", 2, true, true, false, true],
- ["dotted single notes in tuplets", 1, true, false, false, true],
- ["ordinary dotted chords", 1, false, true, false, true],
- ["dots inferred from duration when the written type is absent", 1, false, true, false, false],
- ] as [string, number, boolean, boolean, boolean, boolean][]) {
- it(`Renders ${description}`, async () => {
- await osmd.load(score(dots, nested, chord, descending, writtenType));
- for (let render: number = 0; render < 2; render++) {
- osmd.render();
- const entries: VexFlowVoiceEntry[] = osmd.GraphicSheet.MeasureList[0][0].staffEntries
- .flatMap(entry => entry.graphicalVoiceEntries)
- .filter(entry => !entry.notes[0].sourceNote.isRest()) as VexFlowVoiceEntry[];
- expect(entries.length).to.equal(2);
- if (nested) {
- expect(entries[0].notes.some(note => note.sourceNote.NoteTuplets.length === 2), "nested tuplet membership").to.equal(true);
- }
- for (const entry of entries) {
- expect(entry.notes.length).to.equal(chord ? 2 : 1);
- expect(entry.vfStaveNote.getDuration(), "written quarter-note shape").to.equal("q");
- const modifiers: any[] = (entry.vfStaveNote as any).getModifiers();
- expect(modifiers.filter(modifier => modifier.getCategory() === "dots").length,
- "augmentation dots on every pitch of the rendered chord").to.equal(dots * entry.notes.length);
- expect(entry.vfStaveNote.getTicks().value(), "sounding duration stays independent of the written dots")
- .to.be.closeTo(entry.notes[0].graphicalNoteLength.RealValue * Vex.Flow.RESOLUTION, 1e-8);
- for (const note of entry.notes) {
- expect(note.numberOfDots, "each chord member's graphical dot count").to.equal(dots);
- }
- }
- expect(container.querySelectorAll("svg .vf-notehead").length, "visible noteheads").to.be.greaterThan(0);
- }
+ it("Renders every chord member with the written dots, or with the dots of its duration when the type is absent (#1840)", async () => {
+ await osmd.load(TestUtils.getScore("test_tuplet_nested_chord_dots.musicxml"));
+ osmd.render();
+ osmd.GraphicSheet.MeasureList.forEach((staves: GraphicalMeasure[], measureIndex: number) => {
+ const dyads: GraphicalVoiceEntry[] = staves[0].staffEntries
+ .flatMap(entry => entry.graphicalVoiceEntries)
+ .filter(entry => !entry.notes[0].sourceNote.isRest());
+ // VexFlowConverter.StaveNote() draws as many dots as the chord member with the most
+ expect(dyads.map(dyad => dyad.notes.map(note => note.numberOfDots)), `dots of the dyads' notes in measure ${measureIndex + 1}`)
+ .to.deep.equal(expectedDots[measureIndex].map(dots => [dots, dots]));
});
- }
+ });
});
diff --git a/test/data/test_tuplet_nested_chord_dots.musicxml b/test/data/test_tuplet_nested_chord_dots.musicxml
new file mode 100644
index 00000000..bad374d9
--- /dev/null
+++ b/test/data/test_tuplet_nested_chord_dots.musicxml
@@ -0,0 +1,83 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE score-partwise PUBLIC "-//Recordare//DTD MusicXML 4.0 Partwise//EN" "http://www.musicxml.org/dtds/partwise.dtd">
+<score-partwise version="4.0">
+ <!--
+ Regression sample for #1840: chords are drawn with their written dots (<dot>), not with dots inferred from their duration.
+ Measures 1 and 2 (9/8): an outer 6:9 tuplet with an inner 2:3 tuplet of one note, numbers and brackets hidden,
+ as MuseScore 3.6.1 exported the measure of the issue.
+ A dyad in the inner tuplet lasts 9/4 of its written quarter note, a dyad only in the outer one 3/2 of it,
+ so their durations are not those of their written note values.
+ Measure 1: an eighth rest, then two undotted quarter-note dyads.
+ Measure 2: an undotted and a dotted quarter-note dyad.
+ Measure 3 (6/8): two dyads without <type>, whose dots come from their duration (dotted quarter notes).
+ -->
+ <work>
+ <work-title>Written dots of chords in nested tuplets (#1840)</work-title>
+ </work>
+ <identification>
+ <encoding>
+ <software>hand-written OSMD test sample</software>
+ </encoding>
+ </identification>
+ <part-list>
+ <score-part id="P1"><part-name>Piano</part-name></score-part>
+ </part-list>
+ <part id="P1">
+ <measure number="1">
+ <attributes>
+ <divisions>8</divisions>
+ <key><fifths>0</fifths></key>
+ <time><beats>9</beats><beat-type>8</beat-type></time>
+ <clef><sign>G</sign><line>2</line></clef>
+ </attributes>
+ <note><rest/><duration>6</duration><voice>1</voice><type>eighth</type>
+ <time-modification><actual-notes>6</actual-notes><normal-notes>9</normal-notes></time-modification>
+ <notations><tuplet type="start" number="1" bracket="no" show-number="none"/></notations></note>
+ <note><pitch><step>C</step><octave>5</octave></pitch><duration>18</duration><voice>1</voice><type>quarter</type>
+ <time-modification><actual-notes>12</actual-notes><normal-notes>27</normal-notes><normal-type>eighth</normal-type></time-modification>
+ <notations>
+ <tuplet type="start" number="2" bracket="no" show-number="none">
+ <tuplet-actual><tuplet-number>2</tuplet-number></tuplet-actual>
+ <tuplet-normal><tuplet-number>3</tuplet-number></tuplet-normal>
+ </tuplet>
+ <tuplet type="stop" number="2"/>
+ </notations></note>
+ <note><chord/><pitch><step>E</step><octave>5</octave></pitch><duration>18</duration><voice>1</voice><type>quarter</type>
+ <time-modification><actual-notes>12</actual-notes><normal-notes>27</normal-notes><normal-type>eighth</normal-type></time-modification></note>
+ <note><pitch><step>C</step><octave>5</octave></pitch><duration>12</duration><voice>1</voice><type>quarter</type>
+ <time-modification><actual-notes>6</actual-notes><normal-notes>9</normal-notes><normal-type>eighth</normal-type></time-modification>
+ <notations><tuplet type="stop" number="1"/></notations></note>
+ <note><chord/><pitch><step>E</step><octave>5</octave></pitch><duration>12</duration><voice>1</voice><type>quarter</type>
+ <time-modification><actual-notes>6</actual-notes><normal-notes>9</normal-notes><normal-type>eighth</normal-type></time-modification></note>
+ </measure>
+ <measure number="2">
+ <note><pitch><step>D</step><octave>5</octave></pitch><duration>18</duration><voice>1</voice><type>quarter</type>
+ <time-modification><actual-notes>12</actual-notes><normal-notes>27</normal-notes><normal-type>eighth</normal-type></time-modification>
+ <notations>
+ <tuplet type="start" number="1" bracket="no" show-number="none"/>
+ <tuplet type="start" number="2" bracket="no" show-number="none">
+ <tuplet-actual><tuplet-number>2</tuplet-number></tuplet-actual>
+ <tuplet-normal><tuplet-number>3</tuplet-number></tuplet-normal>
+ </tuplet>
+ <tuplet type="stop" number="2"/>
+ </notations></note>
+ <note><chord/><pitch><step>F</step><octave>5</octave></pitch><duration>18</duration><voice>1</voice><type>quarter</type>
+ <time-modification><actual-notes>12</actual-notes><normal-notes>27</normal-notes><normal-type>eighth</normal-type></time-modification></note>
+ <note><pitch><step>C</step><octave>5</octave></pitch><duration>18</duration><voice>1</voice><type>quarter</type><dot/>
+ <time-modification><actual-notes>6</actual-notes><normal-notes>9</normal-notes><normal-type>eighth</normal-type></time-modification>
+ <notations><tuplet type="stop" number="1"/></notations></note>
+ <note><chord/><pitch><step>E</step><octave>5</octave></pitch><duration>18</duration><voice>1</voice><type>quarter</type><dot/>
+ <time-modification><actual-notes>6</actual-notes><normal-notes>9</normal-notes><normal-type>eighth</normal-type></time-modification></note>
+ </measure>
+ <measure number="3">
+ <attributes>
+ <time><beats>6</beats><beat-type>8</beat-type></time>
+ </attributes>
+ <note><pitch><step>C</step><octave>5</octave></pitch><duration>12</duration><voice>1</voice></note>
+ <note><chord/><pitch><step>E</step><octave>5</octave></pitch><duration>12</duration><voice>1</voice></note>
+ <note><pitch><step>D</step><octave>5</octave></pitch><duration>12</duration><voice>1</voice></note>
+ <note><chord/><pitch><step>F</step><octave>5</octave></pitch><duration>12</duration><voice>1</voice></note>
+ <barline location="right"><bar-style>light-heavy</bar-style></barline>
+ </measure>
+ </part>
+</score-partwise>
+```
</details>
Contributor
Author
|
Thanks for the detailed review and the proposed sample. In After merging develop |
sschmidTU
approved these changes
Oct 23, 2026
sschmidTU
left a comment
sschmidTU
left a comment
Contributor
There was a problem hiding this comment.
Thanks a lot for adopting it so quickly, and for updating the description!
Merging.
Your branch now has exactly the version I tested, merged with the current develop (2eb52a3), so I checked it again there:
- None of the 448 sample images changes, and the new sample is the only new image. None of the 125 scores of the LilyPond test suite and the 242 other scores changes either.
- The suite passes in Firefox and Chrome (763 tests each), and lint and tsc are clean. The new test fails on develop in both browsers, with the extra dots from the issue.
Looks good to me, thanks again for the PR!
Contributor
|
I'm working on an EngravingRule (disabled by default) to render these hidden notes (also greyed out by default, but there will be an option for the color of these notes). edit: correction: the sample has no hidden notes, and measure 1's rhythm is correct, its tuplets are just hidden ( This is how MuseScore renders the sample: OSMD with PR: |
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.
Fixes #1840.
In its existing written-type branch,
StaveNote()already takes the first note's dot count from MusicXML. The later maximum over the chord can reintroduce dots inferred from another member's tuplet duration. Apply the existing written-dot correction to every member of that chord.The regression uses
test/data/test_tuplet_nested_chord_dots.musicxml: undotted and dotted dyads in nested tuplets, plus dyads without<type>whose dots come from their duration. One load/render test checks OSMD'sGraphicalNote.numberOfDotsfor every chord member, without inspecting VexFlow internals. The sample is also included in the visual regression tests.Validation against develop
2eb52a34d4c4fb4b959f22bfb336dcc7f9ce3f67(Linux, Node 24):npm run buildandnpx tsc --noEmit: passed, including lint.npm test -- --browsers ChromeHeadlessNoSandbox,FirefoxHeadless: 763 passed in each browser (1526 total). With the same tests on unmodified develop production code, the new regression fails in both browsers on measure 1's unexpected dots.npm run generate:blessedwith unmodified develop production code, followed bynpm run generate:currentandnpm run test:visualwith this change: 448 existing sample PNGs are pixel-identical. The new sample is the only changed image: 4851 pixels (1.0462%), removing the unwanted dots while retaining the written and duration-inferred dots. Both arms include the new sample and use isolated source copies; the audited base above identifies the blessed production sources.The converter also appears in #1839 and the existing VexFlow migration branches (#1139/#1673); their changed hunks do not implement this dot correction. This change retains the existing duration-selection condition and chord-wide maximum.