fix(accidental): extra space in 2nds w/ stems down

Author: gristow

src/accidental.ts

@@ -15,10 +15,18 @@ import { Category, isAccidental, isGraceNote, isGraceNoteGroup, isStaveNote } fr
 import { defined, log } from './util';
 import { Voice } from './voice';
 
-export type Line = {
+type StaveLineAccidentalLayoutMetrics = {
   column: number;
   line: number;
+  /**
+   * A flat line needs more clearance above than below. This is
+   * set to true if the accidental is either a flat or double flat.
+   */
   flatLine: boolean;
+  /**
+   * Double sharps need less clearance above and below than other
+   * accidentals.
+   */
   dblSharpLine: boolean;
   numAcc: number;
   width: number;
@@ -74,95 +82,147 @@ export class Accidental extends Modifier {
     const additionalPadding = musicFont.lookupMetric('accidental.leftPadding'); // padding to the left of all accidentals
 
     // A type used just in this formatting function.
-    type AccidentalListItem = {
+    type AccidentalLinePositionsAndXSpaceNeeds = {
       y?: number;
       line: number;
-      shift: number;
+      /**
+       * The amount by which the accidental requests notes be shifted to the right
+       * to accomodate its presence.
+       */
+      extraXSpaceNeeded: number;
       acc: Accidental;
-      lineSpace?: number;
+      spacingBetweenStaveLines?: number;
     };
 
-    const accList: AccidentalListItem[] = [];
+    const accidentalLinePositionsAndSpaceNeeds: AccidentalLinePositionsAndXSpaceNeeds[] = [];
     let prevNote = undefined;
-    let shiftL = 0;
+    let extraXSpaceNeededForLeftDisplacedNotehead = 0;
 
     // First determine the accidentals' Y positions from the note.keys
     for (let i = 0; i < accidentals.length; ++i) {
       const acc: Accidental = accidentals[i];
 
       const note = acc.getNote();
+      const stemDirection = note.getStemDirection();
       const stave = note.getStave();
       const index = acc.checkIndex();
       const props = note.getKeyProps()[index];
+
+      let headDisplacementXShift = 0;
       if (note !== prevNote) {
         // Iterate through all notes to get the displaced pixels
         for (let n = 0; n < note.keys.length; ++n) {
-          shiftL = Math.max(note.getLeftDisplacedHeadPx() - note.getXShift(), shiftL);
+          const priorShiftL = extraXSpaceNeededForLeftDisplacedNotehead;
+          // The extra space we need for the accidental to the left is the amount
+          // that the left-displaced notehead has been shifted left, less any
+          // extra space that has already been added via the note's xShift.
+          const leftDisplacedNoteheadShift = note.getLeftDisplacedHeadPx() - note.getXShift();
+          // If the current extra left-space needed (shiftL) isn't as big as that,
+          // then we need to set it. But, this will give the accidental ownership of
+          // that x-space, and later when formatting the note, its x_begin would have
+          // twice the getLeftDisplacedHeadPx included in it. So: we need to remove
+          // it from the note's x shift.
+          // if (leftDisplacedNoteheadShift > extraXSpaceNeededForLeftDisplacedNotehead) {
+          //   extraXSpaceNeededForLeftDisplacedNotehead = leftDisplacedNoteheadShift;
+          //   note.setLeftDisplacedHeadPx(-note.getXShift());
+          // }
+          extraXSpaceNeededForLeftDisplacedNotehead = Math.max(
+            note.getLeftDisplacedHeadPx() - note.getXShift(),
+            extraXSpaceNeededForLeftDisplacedNotehead
+          );
+          headDisplacementXShift = note.getLeftDisplacedHeadPx();
+
+          // console.log({
+          //   getLeftDisplacedHeadPx: note.getLeftDisplacedHeadPx(),
+          //   getXShift: note.getXShift(),
+          //   getLeftDisplacedHeadPx_minus_getXShift: note.getLeftDisplacedHeadPx() - note.getXShift(),
+          //   shiftL: extraXSpaceNeededForLeftDisplacedNotehead,
+          //   priorShiftL,
+          // });
         }
         prevNote = note;
       }
       if (stave) {
         const lineSpace = stave.getSpacingBetweenLines();
         const y = stave.getYForLine(props.line);
         const accLine = Math.round((y / lineSpace) * 2) / 2;
-        accList.push({ y, line: accLine, shift: shiftL, acc, lineSpace });
+        accidentalLinePositionsAndSpaceNeeds.push({
+          y,
+          line: accLine,
+          extraXSpaceNeeded: extraXSpaceNeededForLeftDisplacedNotehead,
+          acc,
+          spacingBetweenStaveLines: lineSpace,
+        });
       } else {
-        accList.push({ line: props.line, shift: shiftL, acc });
+        accidentalLinePositionsAndSpaceNeeds.push({
+          line: props.line,
+          extraXSpaceNeeded: extraXSpaceNeededForLeftDisplacedNotehead,
+          acc,
+        });
       }
     }
 
     // Sort accidentals by line number.
-    accList.sort((a, b) => b.line - a.line);
+    accidentalLinePositionsAndSpaceNeeds.sort((a, b) => b.line - a.line);
 
     // FIXME: Confusing name. Each object in this array has a property called `line`.
     // So if this is a list of lines, you end up with: `line.line` which is very awkward.
-    const lineList: Line[] = [];
+    const staveLineAccidentalLayoutMetrics: StaveLineAccidentalLayoutMetrics[] = [];
 
     // amount by which all accidentals must be shifted right or left for
     // stem flipping, notehead shifting concerns.
-    let accShift = 0;
-    let previousLine = undefined;
+    let maxExtraXSpaceNeeded = 0;
 
-    // Create an array of unique line numbers (lineList) from accList
-    for (let i = 0; i < accList.length; i++) {
-      const acc = accList[i];
+    // Create an array of unique line numbers (staveLineAccidentalLayoutMetrics)
+    // from accidentalLinePositionsAndSpaceNeeds
+    for (let i = 0; i < accidentalLinePositionsAndSpaceNeeds.length; i++) {
+      const accidentalLinePositionAndSpaceNeeds = accidentalLinePositionsAndSpaceNeeds[i];
 
-      // if this is the first line, or a new line, add a lineList
-      if (previousLine === undefined || previousLine !== acc.line) {
-        lineList.push({
-          line: acc.line,
+      const priorLineMetric = staveLineAccidentalLayoutMetrics[staveLineAccidentalLayoutMetrics.length - 1];
+      let currentLineMetric: StaveLineAccidentalLayoutMetrics;
+
+      // if this is the first line, or a new line, add a staveLineAccidentalLayoutMetric
+      if (!priorLineMetric || priorLineMetric?.line !== accidentalLinePositionAndSpaceNeeds.line) {
+        currentLineMetric = {
+          line: accidentalLinePositionAndSpaceNeeds.line,
           flatLine: true,
           dblSharpLine: true,
           numAcc: 0,
           width: 0,
           column: 0,
-        });
+        };
+        staveLineAccidentalLayoutMetrics.push(currentLineMetric);
+      } else {
+        currentLineMetric = priorLineMetric;
       }
+
       // if this accidental is not a flat, the accidental needs 3.0 lines lower
       // clearance instead of 2.5 lines for b or bb.
       // FIXME: Naming could use work. acc.acc is very awkward
-      if (acc.acc.type !== 'b' && acc.acc.type !== 'bb') {
-        lineList[lineList.length - 1].flatLine = false;
+      if (
+        accidentalLinePositionAndSpaceNeeds.acc.type !== 'b' &&
+        accidentalLinePositionAndSpaceNeeds.acc.type !== 'bb'
+      ) {
+        currentLineMetric.flatLine = false;
       }
 
       // if this accidental is not a double sharp, the accidental needs 3.0 lines above
-      if (acc.acc.type !== '##') {
-        lineList[lineList.length - 1].dblSharpLine = false;
+      if (accidentalLinePositionAndSpaceNeeds.acc.type !== '##') {
+        currentLineMetric.dblSharpLine = false;
       }
 
       // Track how many accidentals are on this line:
-      lineList[lineList.length - 1].numAcc++;
+      currentLineMetric.numAcc++;
 
       // Track the total x_offset needed for this line which will be needed
       // for formatting lines w/ multiple accidentals:
 
       // width = accidental width + universal spacing between accidentals
-      lineList[lineList.length - 1].width += acc.acc.getWidth() + accidentalSpacing;
-
-      // if this accShift is larger, use it to keep first column accidentals in the same line
-      accShift = acc.shift > accShift ? acc.shift : accShift;
+      currentLineMetric.width += accidentalLinePositionAndSpaceNeeds.acc.getWidth() + accidentalSpacing;
 
-      previousLine = acc.line;
+      // if this extraXSpaceNeeded is the largest so far, use it as the starting point for
+      // all accidental columns.
+      maxExtraXSpaceNeeded = Math.max(accidentalLinePositionAndSpaceNeeds.extraXSpaceNeeded, maxExtraXSpaceNeeded);
     }
 
     // ### Place Accidentals in Columns
@@ -186,14 +246,19 @@ export class Accidental extends Modifier {
     let totalColumns = 0;
 
     // establish the boundaries for a group of notes with clashing accidentals:
-    for (let i = 0; i < lineList.length; i++) {
+    for (let i = 0; i < staveLineAccidentalLayoutMetrics.length; i++) {
       let noFurtherConflicts = false;
       const groupStart = i;
       let groupEnd = i;
 
-      while (groupEnd + 1 < lineList.length && !noFurtherConflicts) {
+      while (groupEnd + 1 < staveLineAccidentalLayoutMetrics.length && !noFurtherConflicts) {
         // if this note conflicts with the next:
-        if (this.checkCollision(lineList[groupEnd], lineList[groupEnd + 1])) {
+        if (
+          this.checkCollision(
+            staveLineAccidentalLayoutMetrics[groupEnd],
+            staveLineAccidentalLayoutMetrics[groupEnd + 1]
+          )
+        ) {
           // include the next note in the group:
           groupEnd++;
         } else {
@@ -202,7 +267,7 @@ export class Accidental extends Modifier {
       }
 
       // Gets an a line from the `lineList`, relative to the current group
-      const getGroupLine = (index: number) => lineList[groupStart + index];
+      const getGroupLine = (index: number) => staveLineAccidentalLayoutMetrics[groupStart + index];
       const getGroupLines = (indexes: number[]) => indexes.map(getGroupLine);
       const lineDifference = (indexA: number, indexB: number) => {
         const [a, b] = getGroupLines([indexA, indexB]).map((item) => item.line);
@@ -216,7 +281,12 @@ export class Accidental extends Modifier {
       const groupLength = groupEnd - groupStart + 1;
 
       // Set the accidental column for each line of the group
-      let endCase = this.checkCollision(lineList[groupStart], lineList[groupEnd]) ? 'a' : 'b';
+      let endCase = this.checkCollision(
+        staveLineAccidentalLayoutMetrics[groupStart],
+        staveLineAccidentalLayoutMetrics[groupEnd]
+      )
+        ? 'a'
+        : 'b';
 
       switch (groupLength) {
         case 3:
@@ -259,8 +329,13 @@ export class Accidental extends Modifier {
         let collisionDetected = true;
         while (collisionDetected === true) {
           collisionDetected = false;
-          for (let line = 0; line + patternLength < lineList.length; line++) {
-            if (this.checkCollision(lineList[line], lineList[line + patternLength])) {
+          for (let line = 0; line + patternLength < staveLineAccidentalLayoutMetrics.length; line++) {
+            if (
+              this.checkCollision(
+                staveLineAccidentalLayoutMetrics[line],
+                staveLineAccidentalLayoutMetrics[line + patternLength]
+              )
+            ) {
               collisionDetected = true;
               patternLength++;
               break;
@@ -270,15 +345,15 @@ export class Accidental extends Modifier {
         // Then, assign a column to each line of accidentals
         for (groupMember = i; groupMember <= groupEnd; groupMember++) {
           column = ((groupMember - i) % patternLength) + 1;
-          lineList[groupMember].column = column;
+          staveLineAccidentalLayoutMetrics[groupMember].column = column;
           totalColumns = totalColumns > column ? totalColumns : column;
         }
       } else {
         // If the group contains fewer than seven members, use the layouts from
         // the Tables.accidentalColumnsTable (See: tables.ts).
         for (groupMember = i; groupMember <= groupEnd; groupMember++) {
           column = Tables.accidentalColumnsTable[groupLength][endCase][groupMember - i];
-          lineList[groupMember].column = column;
+          staveLineAccidentalLayoutMetrics[groupMember].column = column;
           totalColumns = totalColumns > column ? totalColumns : column;
         }
       }
@@ -308,12 +383,14 @@ export class Accidental extends Modifier {
       columnXOffsets[i] = 0;
     }
 
-    columnWidths[0] = accShift + leftShift;
-    columnXOffsets[0] = accShift + leftShift;
+    // columnWidths[0] = maxExtraXSpaceNeeded + leftShift;
+    // columnXOffsets[0] = maxExtraXSpaceNeeded + leftShift;
+    columnWidths[0] = leftShift + maxExtraXSpaceNeeded;
+    columnXOffsets[0] = leftShift;
 
     // Fill columnWidths with widest needed x-space;
     // this is what keeps the columns parallel.
-    lineList.forEach((line) => {
+    staveLineAccidentalLayoutMetrics.forEach((line) => {
       if (line.width > columnWidths[line.column]) columnWidths[line.column] = line.width;
     });
 
@@ -325,26 +402,27 @@ export class Accidental extends Modifier {
     const totalShift = columnXOffsets[columnXOffsets.length - 1];
     // Set the xShift for each accidental according to column offsets:
     let accCount = 0;
-    lineList.forEach((line) => {
+    staveLineAccidentalLayoutMetrics.forEach((line) => {
       let lineWidth = 0;
       const lastAccOnLine = accCount + line.numAcc;
       // handle all of the accidentals on a given line:
       for (accCount; accCount < lastAccOnLine; accCount++) {
-        const xShift = columnXOffsets[line.column - 1] + lineWidth;
-        accList[accCount].acc.setXShift(xShift);
+        const xShift = columnXOffsets[line.column - 1] + lineWidth + maxExtraXSpaceNeeded;
+        accidentalLinePositionsAndSpaceNeeds[accCount].acc.setXShift(xShift);
         // keep track of the width of accidentals we've added so far, so that when
         // we loop, we add space for them.
-        lineWidth += accList[accCount].acc.getWidth() + accidentalSpacing;
+        lineWidth += accidentalLinePositionsAndSpaceNeeds[accCount].acc.getWidth() + accidentalSpacing;
+        // console.log('Line, accCount, shift: ', line.line, accCount, xShift);
         L('Line, accCount, shift: ', line.line, accCount, xShift);
       }
     });
-
+    // console.log({ totalShift, columnXOffsets, additionalPadding });
     // update the overall layout with the full width of the accidental shapes:
-    state.left_shift += totalShift + additionalPadding;
+    state.left_shift = totalShift + additionalPadding;
   }
 
   /** Helper function to determine whether two lines of accidentals collide vertically */
-  static checkCollision(line1: Line, line2: Line): boolean {
+  static checkCollision(line1: StaveLineAccidentalLayoutMetrics, line2: StaveLineAccidentalLayoutMetrics): boolean {
     let clearance = line2.line - line1.line;
     let clearanceRequired = 3;
     // But less clearance is required for certain accidentals: b, bb and ##.

test.html

@@ -0,0 +1,64 @@
+<!DOCTYPE >
+<!-- 
+  This example is available at:
+  https://jsfiddle.net/wjm3t5su/ -->
+<html>
+  <style>
+    body {
+      font-family: Arial, 'sans-serif';
+    }
+  </style>
+  <body>
+    <div id="output"></div>
+    <div style="height: 1000px"></div>
+    <script src="./build/cjs/vexflow-debug.js"></script>
+    <script>
+      const { Factory, EasyScore, System } = Vex.Flow;
+
+      [
+        '(A4 B#4)',
+        '(B4 C#5)',
+
+        // 'C5 D#5',
+        // 'D5 E#5',
+        // 'E5 F#5',
+        // 'E#4 F4',
+        // 'F#4 G4',
+        // 'G#4 A4',
+        // 'A#4 B4',
+        // 'B#4 C5',
+        // 'C#5 D5',
+        // 'D#5 E5',
+        // 'E#5 F5',
+        // 'E#4 F#4',
+        // 'F#4 G#4',
+        // 'G#4 A#4',
+        // 'A#4 B#4',
+        // 'B#4 C#5',
+        // 'C#5 D#5',
+        // 'D#5 E#5',
+        // 'E#5 F#5',
+      ].forEach((noteNames) => {
+        console.log(`%cNotes: ${noteNames}`, 'color: #990000; background-color: #AAAAAA; padding: 1em 1em;');
+        const id = crypto.randomUUID();
+        const div = document.createElement('div');
+        div.setAttribute('id', id);
+        document.querySelector('#output').appendChild(div);
+        const vf = new Factory({
+          renderer: { elementId: id, width: 500, height: 200 },
+        });
+
+        const score = vf.EasyScore().set({ time: '2/4' });
+        const system = vf.System();
+
+        system
+          .addStave({
+            voices: [score.voice(score.notes(`${noteNames}/h`))],
+          })
+          .addClef('treble')
+          .addTimeSignature('2/4');
+        vf.draw();
+      });
+    </script>
+  </body>
+</html>