Nested tuplets #92

strangarnenad · 2024-03-26T19:12:04Z

Objective: This draft PR introduces a test implementation aimed at integrating nested tuplets into this music notation software. The primary goal is to evaluate the most effective underlying architecture to support this feature.

Implementation Details: The current iteration successfully implements the insertion of any number of nested tuplets into the notation.

Current State: At this stage, only the basic functionality for creating nested tuplets has been developed. Key features such as import/export capabilities to SMO, XML, and MIDI formats are not yet implemented. This is an initial draft to explore the feasibility and adequacy of the proposed approach.

Request for Feedback: I am seeking feedback on the current implementation strategy and its alignment with the project's architectural standards and long-term vision.

Future Work: Upon reaching a consensus on the proposed approach, I am committed to fully implementing and refining this feature, including the development of import/export functionality and any necessary adjustments based on feedback.

fyi @AaronDavidNewman

AaronDavidNewman · 2024-03-26T21:26:49Z

Hi Nenad, thanks for taking this on! This is a lot to look over, I'll try to find some time in the next couple of days. This would be a welcome contribution.

How do you get the tuplets into the music, currently - is it something you do programatically? Because I don't think there is a UI for it.

Is the commented-out code just parts of the implementation you haven't gotten to yet?

strangarnenad · 2024-03-26T21:41:43Z

@AaronDavidNewman It uses the existing UI. First, create a tuplet; then, select any note inside it and create another one. Note: deletion does not work yet.
Regarding the commented-out code, yes, it needs to be commented out in order for the app to build successfully. Most of this code needs to be reimplemented, adjusted, or modified in some way. ;)

AaronDavidNewman · 2024-03-27T13:34:58Z

I've been thinking about this a little bit. I think it would be best to keep a single SmoTuplet object, and create an array for the attributes that we need for each nested level. Something like:

export interface SmoTupletData {
  durationMap: number[],
  ratioed: boolean,
  bracketed: boolean,
  startIndex: number
}

class SmoTuplet {
data: tupletData[]
...
   }

We can add some logic in the SmoTuplet.deserialize to handle the backward-compatible case - if the tuplet data is not in an array, we put it at the top level of a new array.

I think this simplifies the other serialization cases, and also things like copy and paste. You mostly care about the nesting levels when rendering, other times, you can ignore it.

Do you think that will work? I think everything we need to know, we can glean from that information. But I don't know much about nested tuplets - in almost 50 years of playing music, I've never come across one in the wild. Not even Bernstein or Stravinsky.

strangarnenad · 2024-03-27T19:49:50Z

I think your proposed solution could work, but only for one additional level. This one additional level is more than enough. However, I guess the question is what we want to support here. Is it just one nested level, or many? In theory, it's possible to go deep with nesting until the smallest supported tick value. Currently, it works like this.
If you think backward compatibility should be considered as well, then your idea makes even more sense.

AaronDavidNewman · 2024-03-28T21:24:55Z

I only meant for serialization. Whatever structure you are using to represent the nested tuplets, you should be able to flatten them to an array for serialization, and then explode them on deserialization. Since you can't really serialize a recursive structure.

strangarnenad · 2024-03-29T00:20:00Z

It is a tree, it does not have a circular reference problem. I've enabled import/export to SMO in my last commit.
Was recursive serialisation your only concern? I'm still not entirely familiar with the entire system, so I might be overlooking some potential downsides of this approach.

AaronDavidNewman · 2024-03-30T14:49:55Z

Was recursive serialisation your only concern?

I don't have strong feelings about it, I just like to separate the concerns of container and thing contained.

I made some time to look at the tuplet stuff today. I have lots of changes on a different branch, and I will try to bring them all in, if I can get it working.

While we're addressing tuplets, it would be nice to expose to the UI the different parameters, and also allow people to create their custom tuplet values. In the 'note' menu option, it would be good to create a new dialog box for tuplets.

AaronDavidNewman · 2024-03-30T16:06:27Z

I'm impressed that you were able to do this. I know this is not the easiest part of the code to understand. The whole actor/iterator model didn't work as cleanly as I'd hoped. But maybe I can explain it, and we can figure out how to incorporate this feature in the best possible way. It would be good to stay compatible with the current method because copy/paste depends on it, and we would use it for cases like if the time signature of a measure changes, we want to copy as many notes as possible to the new measure.

Whenever you change the duration of a note, you are transforming all the notes in that measure for the affected voice . You will go through all the notes in that voice, and put them into the new voice. Either 1) creating more notes with smaller durations, 2) increasing duration of current note and deleting future notes, 3) changing the duration of the current note and future notes. 4) skipping over the current note because the operation affects future or past notes.

So the idea of iterateOverTick is that it will return 1) if null, use the old note (no change) 2) if a note, then the note is pushed into the new voice, 3) if an array of notes the array is added to the new voice, 4) an empty array, then no tick is added. You would return this if the measure were already 'full', based on its duration from time signature.

So in the case of creating a new tuplet, each iteration would return null until it reaches the start of the tuplet. Then, I think you would figure out the duration of all the notes for the tuplet tree, and return them in an array.

So my suggestion of how to get the feature in a backwards-compatible way is:

create a new class called SmoTupletTree that contains tuplets: SmoTuplet. SmoMeasure will contain an array of SmoTupletTree[] instead of tuplet directly. SmoTuplet and SmoTupletTree can live in the same tuplet.ts module.
Try to get as much of the tree-traversal logic from measure into the new TupletTree class as you can. SmoMeasure is already a pretty heavy class.
I think it would be convenient if you would keep the full duration of the 'outer' tuplet in the TupletTree class. Then when 'UnmakeTuplet' is called, we can just convert the whole thing back into its non-tuplet note, like the code does today.

To be backwards compatible with serialization and existing scores, you'll have to do something like this in the SmoMeasure.deserialize method:

const tupParams = SmoTupletTree.defaults;
if ((tupJson as any).ctor === 'SmoTuplet') {
        tupTreeParams.childTuplets = [tupJson];
     }

and likewise, you can serialize the SmoTupletTree instance, and create SmoTupletTreeParams class and SmoTupletTreeSer class, following the pattern of other measure modifiers.

This was a long post - hopefully this makes some sense? I really appreciate the effort that you put into this and I want to make sure it is rewarded (at least) with being added to Smoosic proper.

AaronDavidNewman · 2024-03-30T16:19:46Z

I love that we can do this., even though I have no idea when I would ever use it. I think we should allow it nested to any level the user wants. We are really confusing the beamer class, though :) I think the beam should contain an extra line with each nesting.

strangarnenad · 2024-04-01T12:58:57Z

Thanks for the detailed explanation of iterateOverTick and thanks for the suggestion on how to proceed with this. Your idea makes sense to me so I will give it a go and see where this leaves us.

AaronDavidNewman · 2024-08-21T00:21:23Z

I did a quick test on your branch, and it looks really good! I was able to create and remove a tuple, and serialize it and save it, basic cut and paste and undo all work as expected.

I have again made a mess but I will merge your stuff into my branch where I have a lot of UI changes, and then into the main branch, sometime this week.

We'll need to work on the beaming, but since we have no multi-tuplet capability right now, we can work on that after we've merged it.

Thanks again for your contribution and effort, I appreciate it.

strangarnenad · 2024-08-21T07:20:24Z

Hi @AaronDavidNewman this branch still needs some work, e.g xml export is not implemented.
Also I would like to do some cleanup, there are todos all over the place....
I hope I will get to this in next week or so

AaronDavidNewman · 2024-08-21T12:49:32Z

OK, I'll hold off looking it over until you finish. I can see you're making progress, though.

strangarnenad · 2024-09-05T22:23:45Z

Hi @AaronDavidNewman I think this is ready for you to take a closer look.

AaronDavidNewman · 2024-09-07T20:08:21Z

OK, great news. I'll look it over in the next couple of days.

AaronDavidNewman

I think this is enough comments for now, for sure fix the deserialization issues for existing scores, and indicate what you want to do about the commented-out bits. and we can discuss the remaining issues. Since there were some issues in the original code base (e.g. with beaming), we won't need to fix everything for this PR but at least have a strategy. Thanks again for doing all this.

AaronDavidNewman · 2024-09-08T19:06:16Z

src/smo/data/measure.ts

    }
-    return rv;
+
+    for (j = 0; j < jsonObj.tupletTrees.length; ++j) {


A legacy score won't have any 'tupletTrees'. You should try to load the existing scores (Library menu option) and make sure they all load. I think this is just a question of coalescing tuplet trees into an empty array.

Good catch, I'll fix it.

AaronDavidNewman · 2024-09-08T19:10:02Z

src/common/vex.ts

-      params.isTuplet ?
-        params.closestTicks :
-        params.exactTicks;
+    // var duration =


commented out code. If you don't need this maybe remove it, but I did have a question about closestTicks vs. exactTicks in a different comment

We do not need this, I'll remove it.

AaronDavidNewman · 2024-09-08T19:10:19Z

src/render/vex/toVex.ts

-      SmoMusic.closestVexDuration(smoNote.tickCount) :
-      SmoMusic.ticksToDuration[smoNote.tickCount];
+  // const duration =
+  //   smoNote.isTuplet ?


more commented out code

Same, will remove it.

AaronDavidNewman · 2024-09-08T19:12:08Z

src/render/vex/toVex.ts

-      strs.push(`const ${tp.id} = new VF.Tuplet(${narString}, JSON.parse('${tpParamString}'));`);
-    }
-  });
+  // smoMeasure.voices.forEach((voice, voiceIx) => {


commented code. I'm guessing you want to replace this with something. toVex creates a vexflow script out of a score, we use it to compare different versions of vexflow.

Oh, this is the part I totally forgot about :)

AaronDavidNewman · 2024-09-08T19:28:34Z

src/smo/data/note.ts

@@ -91,7 +96,7 @@ export interface SmoNoteParams {
  /**
   * if this note is part of a tuplet
   */
-  tupletId: string | null,
+  tuplet: TupletInfo | undefined,


Maybe do a pull merge, it seems like something that has changed. TupletParamSer now has a string id attribute instead of a structure that just contains a string. I don't think we need to create a new class/interface just for a string.

AaronDavidNewman · 2024-09-08T19:50:59Z

src/smo/midi/midiToSmo.ts

-          defs.startIndex = voiceLen - 3;
-          measure.tuplets.push(new SmoTuplet(defs));
-        }
+        //todo: needs to be check for nested tuplets


It may not be possible to do this with nested triplets because of quantization, anyway. I wouldn't worry too much about getting that right. It seems like there are a large number of values 'possible triplet' could take on when considering nested triplets. Unless you can think of some way around this, maybe we'll just leave the legacy logic and have this algorithm quantize as best it can.

This is another one I forgot about...

AaronDavidNewman · 2024-09-08T20:00:52Z

src/smo/data/note.ts

+  /**
+   * visible duration
+   */
+  stemTicks: number,


I think I like what you've done here. The main issue will be keeping this up-to-date with actual ticks, as durations change. The original idea was to have measure, which knows about triplets, decide if the ticks here represented the stem length or not.

I think I covered all the cases of updating actual ticks in tickDuration.ts and copyPaste.ts. Also I adjusted Note.clone, so it accepts both 'ticks' and 'stemTicks' and by default both parameters are always set to the same value.
I went with this approach as in my head it makes more sense to build logic around stemTicks and then based on the note position/environment simply calculate actual duration. It also eliminates the need for tickmap, at least on tuplets

AaronDavidNewman · 2024-09-08T20:10:46Z

src/smo/xform/beamers.ts

-      const tupletIndex = tuplet.getIndexOfNote(note);
-      const ult = tuplet.notes[tuplet.notes.length - 1];
-      const first = tuplet.notes[0];
+    // if (note.isTuplet) {


Did we want to do something here? There is no way currently to manage different beam levels in the Smoosic UI, e.g. It works OK if it happens 'naturally', like eight + 2 16th, but there is no way to control where the split happens. It would be ideal if a triplet with a nested triplet had a single beam across the duration (I think, I'll actually have to check what is the best practice).

I think each level has its boundaries, and beam is never crossing them. I might be wrong though.
Regarding the beamer implementation, I changed it just enough so that it does not throw errors anymore.
Commented-out code is here for convenience when comparing the old version to the new one.
As you said, this part needs some work...

AaronDavidNewman · 2024-09-08T20:14:50Z

src/smo/data/note.ts

-      return 0;
-    }
-    const vexDuration = SmoMusic.closestSmoDurationFromTicks(this.tickCount);
+    const vexDuration = SmoMusic.closestSmoDurationFromTicks(this.stemTicks);


I see now that dots in tuplets aren't handled correctly in the main branch. This kind of gets back to the note not knowing enough about the tuplet it's in to make this determination, and why I originally had the 'stemTicks' calculated outside of SmoNote. We'll need to think about this.

I think having stemTicks on the note makes it much easier to calculate everything we need. We know the position of the note, we know if it is part of the tuplet, if it is nested and at what level...

AaronDavidNewman · 2024-09-08T20:19:59Z

src/smo/data/tuplet.ts

  numNotes: number,
+  notesOccupied: number,


More of a question: is notesOccupied something that can't be calculated by tuplet.ts? I didn't really like the way tuplets are represented in vexflow - I'd prefer a numNotes and an overall duration. I think this still makes sense - isn't notesOccupied always just this.totalTicks / this.stemTicks? Because I'd prefer not to have value parameters when a value can be calculated from other value parameters.

I need to double-check this. I think I wanted to have a similar or the same interface as VexFlow. When I was looking at how this is handled in XML, VexFlow and now Smoosic each had a different approach to naming....

strangarnenad

@AaronDavidNewman thanks for looking into it. I think I answered on all of the comments. I will clean up what need cleaning and try to implement 'toVex' functionality

strangarnenad · 2024-09-09T20:16:02Z

src/common/vex.ts

-      params.isTuplet ?
-        params.closestTicks :
-        params.exactTicks;
+    // var duration =


We do not need this, I'll remove it.

strangarnenad · 2024-09-09T20:16:26Z

src/render/vex/toVex.ts

-      SmoMusic.closestVexDuration(smoNote.tickCount) :
-      SmoMusic.ticksToDuration[smoNote.tickCount];
+  // const duration =
+  //   smoNote.isTuplet ?


Same, will remove it.

strangarnenad · 2024-09-09T20:17:02Z

src/render/vex/toVex.ts

-      strs.push(`const ${tp.id} = new VF.Tuplet(${narString}, JSON.parse('${tpParamString}'));`);
-    }
-  });
+  // smoMeasure.voices.forEach((voice, voiceIx) => {


Oh, this is the part I totally forgot about :)

strangarnenad · 2024-09-09T20:17:43Z

src/smo/data/measure.ts

    }
-    return rv;
+
+    for (j = 0; j < jsonObj.tupletTrees.length; ++j) {


Good catch, I'll fix it.

strangarnenad · 2024-09-09T20:19:08Z

src/smo/data/note.ts

@@ -91,7 +96,7 @@ export interface SmoNoteParams {
  /**
   * if this note is part of a tuplet
   */
-  tupletId: string | null,
+  tuplet: TupletInfo | undefined,


strangarnenad · 2024-09-09T20:29:15Z

src/smo/data/note.ts

+  /**
+   * visible duration
+   */
+  stemTicks: number,


I think I covered all the cases of updating actual ticks in tickDuration.ts and copyPaste.ts. Also I adjusted Note.clone, so it accepts both 'ticks' and 'stemTicks' and by default both parameters are always set to the same value.
I went with this approach as in my head it makes more sense to build logic around stemTicks and then based on the note position/environment simply calculate actual duration. It also eliminates the need for tickmap, at least on tuplets

strangarnenad · 2024-09-09T20:46:58Z

src/smo/data/note.ts

-      return 0;
-    }
-    const vexDuration = SmoMusic.closestSmoDurationFromTicks(this.tickCount);
+    const vexDuration = SmoMusic.closestSmoDurationFromTicks(this.stemTicks);


I think having stemTicks on the note makes it much easier to calculate everything we need. We know the position of the note, we know if it is part of the tuplet, if it is nested and at what level...

strangarnenad · 2024-09-09T20:47:46Z

src/smo/midi/midiToSmo.ts

-          defs.startIndex = voiceLen - 3;
-          measure.tuplets.push(new SmoTuplet(defs));
-        }
+        //todo: needs to be check for nested tuplets


This is another one I forgot about...

strangarnenad · 2024-09-09T21:00:41Z

src/smo/xform/beamers.ts

-      const tupletIndex = tuplet.getIndexOfNote(note);
-      const ult = tuplet.notes[tuplet.notes.length - 1];
-      const first = tuplet.notes[0];
+    // if (note.isTuplet) {


I think each level has its boundaries, and beam is never crossing them. I might be wrong though.
Regarding the beamer implementation, I changed it just enough so that it does not throw errors anymore.
Commented-out code is here for convenience when comparing the old version to the new one.
As you said, this part needs some work...

AaronDavidNewman · 2024-09-10T03:05:44Z

For the issues of beaming and dots, which are pre-existing in Smoosic, I think the approach should be to just merge those changes as-is, and then fix dots and beams afterwards.

For numNotes, I'm fine with using the same naming as vexflow. I wasn't objecting to the naming so much as I think you can always calculate numNotes from the duration of the tuplet, or visa-versa. So only one of those 2 things would be an attribute of the tuplet, the other would be calculated. And since we already know the tuplet length based on the thing it's replacing (based on the selection), it made sense to have numNotes be an accessor (get), and tupletLength be a duration that is serialized with the tuplet. Does that make sense?

And I'm fine with stemTicks, I think we can make it work. There are some things that don't work quite the same with your duration code, e.g. when you are adding dots to until you can't make the notes any shorter, the existing code always keeps the length of the measure the same, but your code ends up adding an extra 64th note. I thought this might have to do with not using closestVexDuration anymore, but I'm not sure. Also, the existing code in this case also doesn't work perfectly so I'm not too worried about it. For instance, I noticed we never shrink a note to 64th notes, even though vexflow supports it. I think this is because you can't 'dot' a 64th note (that would make 128th note, which I don't think is supported). But we should still be able to make an undotted 64th note.

strangarnenad · 2024-09-10T07:07:06Z

About 64th note, I just realised that the new implementation of tickDuration.ts does not really check for min duration.
I'll see how to implement this.
Thanks

strangarnenad · 2024-09-12T17:28:26Z

@AaronDavidNewman I think I fixed dotted duration. You can check it in my last commit

AaronDavidNewman · 2024-09-14T20:18:48Z

src/smo/data/music.ts

@@ -1666,6 +1666,24 @@ export class SmoMusic {
    stemTicks = stemTicks * 2;
    return SmoMusic.ticksToDuration[stemTicks];
  }
+  //todo: figure out a better name
+  // ## closestSmoDuration


How about closestBeamDuration. Note duration can be indicated by beam or notehead or flag or stem, but beam seems like a good label, whether the actual note is beamed or not. I use vex length elsewhere to mean a duration that can be rendered as a beam duration + dots (that could also have a better name)

AaronDavidNewman

Is this ready to merge? I don't have any major comments that haven't been addressed.

strangarnenad · 2024-09-15T17:33:15Z

I think it is ready now.
I've adjusted toVex and toMidi export.
Regarding midi, it only supports one level of tuplets...

AaronDavidNewman · 2024-09-15T21:41:57Z

Hi, I merged this, and there are a couple of issues loading scores. They all appear to be tuplet-related.

Do you think you could take a look at these? I'd like to keep existing version of tuple backwards-compatible if possible.

Loading some scores from library:
Under 'jazz transcription library', Yama.
Under 'Soprano art songs', Solovey
Under XML samples: An Cloe.

strangarnenad · 2024-09-15T21:54:39Z

Hi, ok, I'll look into it

strangarnenad and others added 2 commits March 18, 2024 19:07

removed DS_Store

fc665fc

Initial implementation

6561326

enable serialization

c262a55

nenadstrangar added 3 commits August 19, 2024 23:10

implementation with iterateOverTick

38cc010

update

16b7c16

merge conflict

c77b74c

nenadstrangar added 5 commits August 30, 2024 18:39

initial implementation of xml import

e22ce0f

initial implementation of xml Export

65f7734

handle remainder on note ticks

367859b

handle remainder on note ticks

d8ae9cf

adjusted implementation of alignNotesWithTimeSignature() function.

3d0aaff

AaronDavidNewman requested changes Sep 8, 2024

View reviewed changes

strangarnenad commented Sep 9, 2024

View reviewed changes

fix on dot duration

a80a4cf

fix on legacy note and tuplet deserialization

350bc0a

AaronDavidNewman reviewed Sep 14, 2024

View reviewed changes

AaronDavidNewman approved these changes Sep 14, 2024

View reviewed changes

nenadstrangar and others added 4 commits September 15, 2024 01:14

implemented toVex export

3ee8db7

adjusted midiToSmo export

563e53c

renamed closestSmoDuration to closestBeamDuration

51ad7ef

Merge branch 'master' into nested_tuplets

625642e

strangarnenad marked this pull request as ready for review September 15, 2024 19:40

AaronDavidNewman merged commit 6ca9e64 into AaronDavidNewman:master Sep 15, 2024
1 check passed

Nested tuplets #92

Nested tuplets #92

Conversation

strangarnenad commented Mar 26, 2024 • edited Loading

AaronDavidNewman commented Mar 26, 2024

strangarnenad commented Mar 26, 2024

AaronDavidNewman commented Mar 27, 2024

strangarnenad commented Mar 27, 2024

AaronDavidNewman commented Mar 28, 2024

strangarnenad commented Mar 29, 2024

AaronDavidNewman commented Mar 30, 2024

AaronDavidNewman commented Mar 30, 2024

AaronDavidNewman commented Mar 30, 2024

strangarnenad commented Apr 1, 2024

AaronDavidNewman commented Aug 21, 2024

strangarnenad commented Aug 21, 2024

AaronDavidNewman commented Aug 21, 2024

strangarnenad commented Sep 5, 2024

AaronDavidNewman commented Sep 7, 2024

AaronDavidNewman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strangarnenad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronDavidNewman commented Sep 10, 2024

strangarnenad commented Sep 10, 2024

strangarnenad commented Sep 12, 2024

Choose a reason for hiding this comment

AaronDavidNewman left a comment

Choose a reason for hiding this comment

strangarnenad commented Sep 15, 2024 • edited Loading

AaronDavidNewman commented Sep 15, 2024

strangarnenad commented Sep 15, 2024

strangarnenad commented Mar 26, 2024 •

edited

Loading

strangarnenad commented Sep 15, 2024 •

edited

Loading