Merge pull request #1580 from gristow/two-note-chord-spacing

fix: extra space before seconds with modifiers

Author: rvilarl

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,17 +82,21 @@ 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) {
@@ -94,75 +106,97 @@ export class Accidental extends Modifier {
       const stave = note.getStave();
       const index = acc.checkIndex();
       const props = note.getKeyProps()[index];
+
       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);
+          // If the current extra left-space needed isn't as big as this note's,
+          // then we need to use this note's.
+          extraXSpaceNeededForLeftDisplacedNotehead = Math.max(
+            note.getLeftDisplacedHeadPx() - note.getXShift(),
+            extraXSpaceNeededForLeftDisplacedNotehead
+          );
         }
         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 +220,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 +241,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 +255,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 +303,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 +319,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 +357,12 @@ export class Accidental extends Modifier {
       columnXOffsets[i] = 0;
     }
 
-    columnWidths[0] = accShift + leftShift;
-    columnXOffsets[0] = accShift + 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 +374,25 @@ 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;
         L('Line, accCount, shift: ', line.line, accCount, xShift);
       }
     });
-
     // 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 ##.

src/stringnumber.ts

@@ -48,15 +48,21 @@ export class StringNumber extends Modifier {
   // ## Static Methods
   // Arrange string numbers inside a `ModifierContext`
   static format(nums: StringNumber[], state: ModifierContextState): boolean {
+    /**
+     * The modifier context's left_shift state.
+     */
     const left_shift = state.left_shift;
+    /**
+     * The modifier context's right_shift state.
+     */
     const right_shift = state.right_shift;
     const num_spacing = 1;
 
     if (!nums || nums.length === 0) return false;
 
     const nums_list = [];
     let prev_note = null;
-    let shift_left = 0;
+    let extraXSpaceForDisplacedNotehead = 0;
     let shift_right = 0;
     const modLines = 0;
 
@@ -84,8 +90,8 @@ export class StringNumber extends Modifier {
 
       if (note !== prev_note) {
         for (let n = 0; n < note.keys.length; ++n) {
-          if (left_shift === 0) {
-            shift_left = Math.max(note.getLeftDisplacedHeadPx(), shift_left);
+          if (pos === Modifier.Position.LEFT) {
+            extraXSpaceForDisplacedNotehead = Math.max(note.getLeftDisplacedHeadPx(), extraXSpaceForDisplacedNotehead);
           }
           if (right_shift === 0) {
             shift_right = Math.max(note.getRightDisplacedHeadPx(), shift_right);
@@ -101,7 +107,7 @@ export class StringNumber extends Modifier {
         note,
         num,
         line: glyphLine,
-        shiftL: shift_left,
+        shiftL: extraXSpaceForDisplacedNotehead,
         shiftR: shift_right,
       });
     }
@@ -115,7 +121,6 @@ export class StringNumber extends Modifier {
     let last_line = null;
     let last_note = null;
     for (let i = 0; i < nums_list.length; ++i) {
-      let num_shift = 0;
       const note = nums_list[i].note;
       const pos = nums_list[i].pos;
       const num = nums_list[i].num;
@@ -127,14 +132,15 @@ export class StringNumber extends Modifier {
       }
 
       const num_width = num.getWidth() + num_spacing;
+      let num_x_shift = 0;
       if (pos === Modifier.Position.LEFT) {
-        num.setXShift(left_shift);
-        num_shift = shift_left + num_width; // spacing
-        x_widthL = num_shift > x_widthL ? num_shift : x_widthL;
+        num.setXShift(left_shift + extraXSpaceForDisplacedNotehead);
+        num_x_shift = num_width; // spacing
+        x_widthL = Math.max(num_x_shift, x_widthL);
       } else if (pos === Modifier.Position.RIGHT) {
         num.setXShift(num_shiftR);
-        num_shift += num_width; // spacing
-        x_widthR = num_shift > x_widthR ? num_shift : x_widthR;
+        num_x_shift += num_width; // spacing
+        x_widthR = num_x_shift > x_widthR ? num_x_shift : x_widthR;
       }
       last_line = line;
       last_note = note;