Fixes for single Drumline (#787)

* fix(single_drumline) Position based on original position
Track number of note 'positions'
refactor to position notes after all are tracked
Add necessary comparator functions, etc.

* fix(single_drumline) Fixes from code review
refactor some methods to better spots, more specific names, remove staffIndex param

* fix(single_drumline) Use boolean array instead of clef array

* fix(single_drumline) Remove unecessary verbose ternary operator

Co-authored-by: Justin Litten <justin@jlitten-laptop.WORKGROUP>

src/Common/DataObjects/Pitch.ts

@@ -363,6 +363,25 @@ export class Pitch {
         return !(p1 === p2);
     }
 
+    //These don't take into account accidentals! which isn't needed for our current purpose
+    public OperatorFundamentalGreaterThan(p2: Pitch): boolean {
+        const p1: Pitch = this;
+        if (p1.Octave === p2.Octave) {
+            return p1.FundamentalNote > p2.FundamentalNote;
+        } else {
+            return p1.Octave > p2.Octave;
+        }
+    }
+
+    public OperatorFundamentalLessThan(p2: Pitch): boolean {
+        const p1: Pitch = this;
+        if (p1.Octave === p2.Octave) {
+            return p1.FundamentalNote < p2.FundamentalNote;
+        } else {
+            return p1.Octave < p2.Octave;
+        }
+    }
+
     // This method returns a new Pitch factor-Halftones higher than the current Pitch
     private getHigherPitchByTransposeFactor(factor: number): Pitch {
         const noteEnumIndex: number = Pitch.pitchEnumValues.indexOf(this.fundamentalNote);

src/MusicalScore/Graphical/EngravingRules.ts

@@ -294,8 +294,8 @@ export class EngravingRules {
         // Beam Sizing Variables
         this.clefLeftMargin = 0.5;
         this.clefRightMargin = 0.75;
-        this.percussionOneLineCutoff = 4;
-        this.percussionForceVoicesOneLineCutoff = 3;
+        this.percussionOneLineCutoff = 3;
+        this.percussionForceVoicesOneLineCutoff = 1;
         this.betweenKeySymbolsDistance = 0.2;
         this.keyRightMargin = 0.75;
         this.rhythmRightMargin = 1.25;

src/MusicalScore/Graphical/MusicSheetCalculator.ts

@@ -166,7 +166,11 @@ export abstract class MusicSheetCalculator {
             );
             measureList.push(graphicalMeasures);
         }
-        this.handleStaffEntries();
+
+        const staffIsPercussionArray: Array<boolean> =
+                        activeClefs.map(clef => (clef.ClefType === ClefEnum.percussion));
+
+        this.handleStaffEntries(staffIsPercussionArray);
         this.calculateVerticalContainersList();
         this.setIndicesToVerticalGraphicalContainers();
     }
@@ -1515,7 +1519,8 @@ export abstract class MusicSheetCalculator {
                                accidentalCalculator: AccidentalCalculator, openLyricWords: LyricWord[],
                                activeClef: ClefInstruction,
                                openTuplets: Tuplet[], openBeams: Beam[],
-                               octaveShiftValue: OctaveEnum, linkedNotes: Note[] = undefined,
+                               octaveShiftValue: OctaveEnum, staffIndex: number,
+                               linkedNotes: Note[] = undefined,
                                sourceStaffEntry: SourceStaffEntry = undefined): OctaveEnum {
         if (voiceEntry.StemDirectionXml !== StemDirectionType.Undefined &&
             this.rules.SetWantedStemDirectionByXml &&
@@ -1547,8 +1552,7 @@ export abstract class MusicSheetCalculator {
                 graphicalNote = MusicSheetCalculator.symbolFactory.createGraceNote(note, gve, activeClef, octaveShiftValue);
             } else {
                 graphicalNote = MusicSheetCalculator.symbolFactory.createNote(note, gve, activeClef, octaveShiftValue, undefined);
-                const staffLineCount: number = voiceEntry.ParentSourceStaffEntry.ParentStaff.StafflineCount;
-                graphicalNote = MusicSheetCalculator.stafflineNoteCalculator.positionNote(graphicalNote, activeClef, staffLineCount);
+                MusicSheetCalculator.stafflineNoteCalculator.trackNote(graphicalNote);
             }
             if (note.Pitch) {
                 this.checkNoteForAccidental(graphicalNote, accidentalCalculator, activeClef, octaveShiftValue);
@@ -1604,7 +1608,7 @@ export abstract class MusicSheetCalculator {
         }
     }
 
-    protected layoutVoiceEntries(graphicalStaffEntry: GraphicalStaffEntry): void {
+    protected layoutVoiceEntries(graphicalStaffEntry: GraphicalStaffEntry, staffIndex: number): void {
         graphicalStaffEntry.PositionAndShape.RelativePosition = new PointF2D(0.0, 0.0);
         if (!this.leadSheet) {
             for (const gve of graphicalStaffEntry.graphicalVoiceEntries) {
@@ -1976,7 +1980,6 @@ export abstract class MusicSheetCalculator {
                 sourceMeasure, openTuplets, openBeams,
                 accidentalCalculators[staffIndex], activeClefs, openOctaveShifts, openLyricWords, staffIndex, staffEntryLinks
             );
-            this.graphicalMeasureCreatedCalculations(measure);
             verticalMeasureList.push(measure);
         }
         sourceMeasure.VerticalMeasureList = verticalMeasureList; // much easier way to link sourceMeasure to graphicalMeasures than Dictionary
@@ -1992,17 +1995,6 @@ export abstract class MusicSheetCalculator {
                                    staffEntryLinks: StaffEntryLink[]): GraphicalMeasure {
         const staff: Staff = this.graphicalMusicSheet.ParentMusicSheet.getStaffFromIndex(staffIndex);
         let measure: GraphicalMeasure = undefined;
-        //This property is active...
-        if (this.rules.PercussionOneLineCutoff !== undefined && this.rules.PercussionOneLineCutoff !== 0) {
-            //We have a percussion clef, check to see if this property applies...
-            if (activeClefs[staffIndex].ClefType === ClefEnum.percussion) {
-                //-1 means always trigger, or we are under the cutoff number specified
-                if (this.rules.PercussionOneLineCutoff === -1 ||
-                    staff.ParentInstrument.SubInstruments.length < this.rules.PercussionOneLineCutoff) {
-                    staff.StafflineCount = 1;
-                }
-            }
-        }
         if (activeClefs[staffIndex].ClefType === ClefEnum.TAB) {
             staff.isTab = true;
             measure = MusicSheetCalculator.symbolFactory.createTabStaffMeasure(sourceMeasure, staff);
@@ -2089,8 +2081,8 @@ export abstract class MusicSheetCalculator {
                         voiceEntry, graphicalStaffEntry,
                         accidentalCalculator, openLyricWords,
                         activeClefs[staffIndex], openTuplets,
-                        openBeams, octaveShiftValue, linkedNotes,
-                        sourceStaffEntry
+                        openBeams, octaveShiftValue, staffIndex,
+                        linkedNotes, sourceStaffEntry
                     );
                 }
                 // SourceStaffEntry has inStaff ClefInstruction -> create graphical clef
@@ -2152,12 +2144,11 @@ export abstract class MusicSheetCalculator {
                 graphicalStaffEntry.relInMeasureTimestamp = voiceEntry.Timestamp;
                 const gve: GraphicalVoiceEntry = MusicSheetCalculator.symbolFactory.createVoiceEntry(voiceEntry, graphicalStaffEntry);
                 graphicalStaffEntry.graphicalVoiceEntries.push(gve);
-                let graphicalNote: GraphicalNote = MusicSheetCalculator.symbolFactory.createNote(note,
-                                                                                                 gve,
-                                                                                                 new ClefInstruction(),
-                                                                                                 OctaveEnum.NONE, undefined);
-                const staffLineCount: number = voiceEntry.ParentSourceStaffEntry.ParentStaff.StafflineCount;
-                graphicalNote = MusicSheetCalculator.stafflineNoteCalculator.positionNote(graphicalNote, activeClefs[staffIndex], staffLineCount);
+                const graphicalNote: GraphicalNote = MusicSheetCalculator.symbolFactory.createNote(note,
+                                                                                                   gve,
+                                                                                                   new ClefInstruction(),
+                                                                                                   OctaveEnum.NONE, undefined);
+                MusicSheetCalculator.stafflineNoteCalculator.trackNote(graphicalNote);
                 gve.notes.push(graphicalNote);
             }
         }
@@ -2190,19 +2181,31 @@ export abstract class MusicSheetCalculator {
     //     return graphicalStaffEntry;
     // }
 
-    private handleStaffEntries(): void {
+    private handleStaffEntries(staffIsPercussionArray: Array<boolean>): void {
         for (let idx: number = 0, len: number = this.graphicalMusicSheet.MeasureList.length; idx < len; ++idx) {
             const measures: GraphicalMeasure[] = this.graphicalMusicSheet.MeasureList[idx];
             for (let idx2: number = 0, len2: number = measures.length; idx2 < len2; ++idx2) {
                 const measure: GraphicalMeasure = measures[idx2];
+                //This property is active...
+                if (this.rules.PercussionOneLineCutoff !== undefined && this.rules.PercussionOneLineCutoff !== 0) {
+                    //We have a percussion clef, check to see if this property applies...
+                    if (staffIsPercussionArray[idx2]) {
+                        //-1 means always trigger, or we are under the cutoff number specified
+                        if (this.rules.PercussionOneLineCutoff === -1 ||
+                            MusicSheetCalculator.stafflineNoteCalculator.getStafflineUniquePositionCount(idx2) < this.rules.PercussionOneLineCutoff) {
+                            measure.ParentStaff.StafflineCount = 1;
+                        }
+                    }
+                }
                 for (const graphicalStaffEntry of measure.staffEntries) {
                     if (graphicalStaffEntry.parentMeasure !== undefined
                         && graphicalStaffEntry.graphicalVoiceEntries.length > 0
                         && graphicalStaffEntry.graphicalVoiceEntries[0].notes.length > 0) {
-                        this.layoutVoiceEntries(graphicalStaffEntry);
+                        this.layoutVoiceEntries(graphicalStaffEntry, idx2);
                         this.layoutStaffEntry(graphicalStaffEntry);
                     }
                 }
+                this.graphicalMeasureCreatedCalculations(measure);
             }
         }
     }

src/MusicalScore/Graphical/VexFlow/VexFlowMusicSheetCalculator.ts

@@ -398,7 +398,9 @@ export class VexFlowMusicSheetCalculator extends MusicSheetCalculator {
    */
   protected layoutVoiceEntry(voiceEntry: VoiceEntry, graphicalNotes: GraphicalNote[], graphicalStaffEntry: GraphicalStaffEntry,
                              hasPitchedNote: boolean): void {
-    return;
+      for (let i: number = 0; i < graphicalNotes.length; i++) {
+        graphicalNotes[i] = MusicSheetCalculator.stafflineNoteCalculator.positionNote(graphicalNotes[i]);
+      }
   }
 
   /**

src/MusicalScore/Graphical/VexFlow/VexflowStafflineNoteCalculator.ts

@@ -1,89 +1,175 @@
 import { IStafflineNoteCalculator } from "../../Interfaces/IStafflineNoteCalculator";
 import { GraphicalNote } from "../GraphicalNote";
-import { ClefInstruction, ClefEnum } from "../../VoiceData";
-import { Pitch, NoteEnum, AccidentalEnum } from "../../../Common";
+import { ClefEnum, StemDirectionType, VoiceEntry } from "../../VoiceData";
+import { Pitch, NoteEnum } from "../../../Common";
 import { VexFlowGraphicalNote } from "./VexFlowGraphicalNote";
 import { Dictionary } from "typescript-collections";
 import { EngravingRules } from "../EngravingRules";
 
 export class VexflowStafflineNoteCalculator implements IStafflineNoteCalculator {
-    private instrumentVoiceMapping: Dictionary<string, Dictionary<number, {note: NoteEnum, octave: number}>> =
-                                                new Dictionary<string, Dictionary<number, {note: NoteEnum, octave: number}>>();
     private rules: EngravingRules;
-    private voiceIdx: number = 0;
+    private staffPitchListMapping: Dictionary<number, Array<Pitch>> = new Dictionary<number, Array<Pitch>>();
+    //These render on the single line by default
+    private baseLineNote: NoteEnum = NoteEnum.B;
+    private baseLineOctave: number = 1;
 
     constructor(rules: EngravingRules) {
         this.rules = rules;
     }
-  /**
-   * This method is called for each note, and should make any necessary position changes based on the number of stafflines, clef, etc.
-   * Right now this just directly maps a voice number to a position above or below a staffline
-   * @param graphicalNote The note to be checked/positioned
-   * @param currentClef The clef that is active for this note
-   * @param stafflineCount The number of stafflines we are rendering on
-   * @returns the minimum required x width of the source measure (=list of staff measures)
-   */
-    public positionNote(graphicalNote: GraphicalNote, currentClef: ClefInstruction, stafflineCount: number): GraphicalNote {
-        if (!(graphicalNote instanceof VexFlowGraphicalNote) || currentClef.ClefType !== ClefEnum.percussion ||
-        graphicalNote.sourceNote.isRest() || stafflineCount > 1 || this.rules.PercussionOneLineCutoff === 0 ) {
-            return graphicalNote;
+    /**
+     * This method is called for each note during the calc phase. We want to track all possible positions to make decisions
+     * during layout about where notes should be positioned.
+     * This directly notes that share a line to the same position, regardless of voice
+     * @param graphicalNote The note to be checked/positioned
+     * @param staffIndex The staffline the note is on
+     */
+    public trackNote(graphicalNote: GraphicalNote): void {
+        if (!(graphicalNote instanceof VexFlowGraphicalNote) || graphicalNote.Clef().ClefType !== ClefEnum.percussion ||
+        graphicalNote.sourceNote.isRest() || this.rules.PercussionOneLineCutoff === 0 ||
+        this.rules.PercussionForceVoicesOneLineCutoff === -1) {
+            return;
         }
+        const staffIndex: number =
+                graphicalNote.parentVoiceEntry.parentStaffEntry.sourceStaffEntry.ParentStaff.idInMusicSheet;
 
-        const forceOneLineCutoff: number = this.rules.PercussionForceVoicesOneLineCutoff;
-        const forceOneLine: boolean = (forceOneLineCutoff !== undefined && forceOneLineCutoff !== 0) &&
-                                       (forceOneLineCutoff === -1 ||
-                                        graphicalNote.sourceNote.ParentStaff.ParentInstrument.SubInstruments.length < forceOneLineCutoff);
+        let currentPitchList: Array<Pitch> = undefined;
+        if (!this.staffPitchListMapping.containsKey(staffIndex)) {
+            this.staffPitchListMapping.setValue(staffIndex, new Array<Pitch>());
+        }
+        currentPitchList = this.staffPitchListMapping.getValue(staffIndex);
+        const pitch: Pitch = graphicalNote.sourceNote.Pitch;
+        VexflowStafflineNoteCalculator.findOrInsert(currentPitchList, pitch);
+    }
 
-        const instrumentId: string = graphicalNote.sourceNote.PlaybackInstrumentId;
-        const voiceNumber: number = graphicalNote.parentVoiceEntry.parentVoiceEntry.ParentVoice.VoiceId;
-        let currentInstrumentMapping: Dictionary<number, {note: NoteEnum, octave: number}> = undefined;
+    private static PitchIndexOf(array: Array<Pitch>, pitch: Pitch, start: number = 0): number {
+        if (start > array.length - 1) {
+            return -1;
+        }
 
-        if (!this.instrumentVoiceMapping.containsKey(instrumentId)) {
-            currentInstrumentMapping = new Dictionary<number, {note: NoteEnum, octave: number}>();
-            this.instrumentVoiceMapping.setValue(instrumentId, currentInstrumentMapping);
-        } else {
-            currentInstrumentMapping = this.instrumentVoiceMapping.getValue(instrumentId);
+        for (let i: number = start; i < array.length; i++) {
+            const p2: Pitch = array[i];
+            if (pitch.OperatorEquals(p2)) {
+                return i;
+            }
+        }
+        return -1;
+    }
+
+    private static findOrInsert(array: Array<Pitch>, pitch: Pitch): number {
+        for (let i: number = 0; i < array.length; i++) {
+            const p2: Pitch = array[i];
+            if (pitch.OperatorEquals(p2)) {
+                return i;
+            } else {
+                if (pitch.OperatorFundamentalLessThan(p2)) {
+                    array.splice(i, 0, pitch);
+                    return i;
+                }
+            }
         }
+        //If we reach here, we've reached the end of the array.
+        //Means its the greatest pitch
+        array.push(pitch);
+        return array.length - 1;
+    }
+
+    /**
+     * This method is called for each note, and should make any necessary position changes based on the number of stafflines, clef, etc.
+     * @param graphicalNote The note to be checked/positioned
+     * @param staffIndex The staffline that this note exists on
+     * @returns the newly positioned note
+     */
+    public positionNote(graphicalNote: GraphicalNote): GraphicalNote {
+        const staffIndex: number =
+                graphicalNote.parentVoiceEntry.parentStaffEntry.sourceStaffEntry.ParentStaff.idInMusicSheet;
 
-        let fundamental: NoteEnum = NoteEnum.B;
-        let octave: number = 1;
+        if (!(graphicalNote instanceof VexFlowGraphicalNote) || graphicalNote.sourceNote.isRest()
+            || !this.staffPitchListMapping.containsKey(staffIndex)) {
+            return graphicalNote;
+        }
+        const currentPitchList: Array<Pitch> = this.staffPitchListMapping.getValue(staffIndex);
+        //Don't need to position notes. We aren't under the cutoff
+        if (currentPitchList.length > this.rules.PercussionOneLineCutoff) {
+            return graphicalNote;
+        }
         const vfGraphicalNote: VexFlowGraphicalNote = graphicalNote as VexFlowGraphicalNote;
+        const notePitch: Pitch = graphicalNote.sourceNote.Pitch;
 
-        //if we are forcing to one line, just set to B
-        if (!forceOneLine) {
-            if (!currentInstrumentMapping.containsKey(voiceNumber)) {
-                //Direct mapping for more than one voice, position voices
-                switch (this.voiceIdx % 5) {
-                    case 1:
-                        fundamental = NoteEnum.A;
-                        break;
-                    case 2:
-                        fundamental = NoteEnum.F;
-                        break;
-                    case 3:
-                        fundamental = NoteEnum.D;
-                        break;
-                    case 4:
-                        fundamental = NoteEnum.B;
-                        octave = 0;
-                        break;
-                    default:
-                        fundamental = NoteEnum.C;
-                        octave = 2;
-                        break;
+        //If we only need to render on one line
+        if (currentPitchList.length <= this.rules.PercussionForceVoicesOneLineCutoff) {
+            vfGraphicalNote.setAccidental(new Pitch(this.baseLineNote, this.baseLineOctave, notePitch.Accidental));
+        } else {
+            const pitchIndex: number = VexflowStafflineNoteCalculator.PitchIndexOf(currentPitchList, notePitch);
+            if (pitchIndex > -1) {
+                let fundamental: NoteEnum = this.baseLineNote;
+                let octave: number = this.baseLineOctave;
+                const half: number = Math.ceil(currentPitchList.length / 2);
+                //position above
+                if (pitchIndex >= half) {
+                    octave = 2;
+                    switch ((pitchIndex - half) % 5) {
+                        case 1:
+                            fundamental = NoteEnum.E;
+                            break;
+                        case 2:
+                            fundamental = NoteEnum.G;
+                            break;
+                        case 3:
+                            fundamental = NoteEnum.B;
+                            break;
+                        case 4:
+                            fundamental = NoteEnum.D;
+                            octave = 3;
+                            break;
+                        default:
+                            fundamental = NoteEnum.C;
+                            break;
+                    }
+                } else { //position below
+                    switch (pitchIndex % 5) {
+                        case 1:
+                            fundamental = NoteEnum.F;
+                            break;
+                        case 2:
+                            fundamental = NoteEnum.D;
+                            break;
+                        case 3:
+                            fundamental = NoteEnum.B;
+                            octave = 0;
+                            break;
+                        case 4:
+                            fundamental = NoteEnum.G;
+                            octave = 0;
+                            break;
+                        default:
+                            fundamental = NoteEnum.A;
+                            break;
+                    }
+                }
+                const mappedPitch: Pitch = new Pitch(fundamental, octave, notePitch.Accidental);
+                //Map the pitch, set stems properly
+                vfGraphicalNote.setAccidental(mappedPitch);
+                const parentVoiceEntry: VoiceEntry = vfGraphicalNote.parentVoiceEntry.parentVoiceEntry;
+                if (parentVoiceEntry.Notes.length < 2) { // Only switch stems if we aren't sharing stems with another note
+                    if (mappedPitch.Octave > this.baseLineOctave ||
+                        (mappedPitch.FundamentalNote === this.baseLineNote && mappedPitch.Octave === this.baseLineOctave)) {
+                        vfGraphicalNote.parentVoiceEntry.parentVoiceEntry.WantedStemDirection = StemDirectionType.Up;
+                    } else {
+                        vfGraphicalNote.parentVoiceEntry.parentVoiceEntry.WantedStemDirection = StemDirectionType.Down;
+                    }
                 }
-                //For every new instrument/voice for a instrument, render on diff line
-                this.voiceIdx++;
-                currentInstrumentMapping.setValue(voiceNumber, {note: fundamental, octave: octave});
-            } else {
-                const storageObj: {note: NoteEnum, octave: number} = currentInstrumentMapping.getValue(voiceNumber);
-                fundamental = storageObj.note;
-                octave = storageObj.octave;
             }
         }
-
-        //TODO: Check for playback side effects
-        vfGraphicalNote.setAccidental(new Pitch(fundamental, octave, AccidentalEnum.NONE));
-        return graphicalNote;
+        return vfGraphicalNote;
+    }
+    /**
+     * Get the number of unique "voices" or note positions
+     * @param staffIndex The Staffline to get the count of
+     */
+    public getStafflineUniquePositionCount(staffIndex: number): number {
+        if (this.staffPitchListMapping.containsKey(staffIndex)) {
+            return this.staffPitchListMapping.getValue(staffIndex).length;
+        }
+        return 0;
     }
 }

src/MusicalScore/Interfaces/IStafflineNoteCalculator.ts

@@ -1,6 +1,7 @@
 import { GraphicalNote } from "../Graphical/GraphicalNote";
-import { ClefInstruction } from "../VoiceData";
 
 export interface IStafflineNoteCalculator {
-    positionNote(graphicalNote: GraphicalNote, currentClef: ClefInstruction, stafflineCount: number): GraphicalNote;
+    trackNote(graphicalNote: GraphicalNote): void;
+    positionNote(graphicalNote: GraphicalNote): GraphicalNote;
+    getStafflineUniquePositionCount(staffIndex: number): number;
 }