AaronDavidNewman / Smoosic

A music notation editor written in javascript
Other
95 stars 14 forks source link

Nested tuplets #92

Open strangarnenad opened 4 months ago

strangarnenad commented 4 months ago

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.

Screenshot 2024-03-25 at 18 15 55

fyi @AaronDavidNewman

AaronDavidNewman commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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:

  1. 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.
  2. 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.
  3. 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 commented 4 months ago

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.

image

strangarnenad commented 3 months ago

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.