lenmus / lomse

A C++ library for rendering, editing and playing back music scores.
MIT License
117 stars 28 forks source link

Add initial support for arpeggio #299

Closed dmitrio95 closed 3 years ago

dmitrio95 commented 3 years ago

This pull request contains a basic implementation of arpeggio in Lomse. This implementation is somewhat limited (which I have briefly described below) but it is still able to import an arpeggio attached to one chord and display it when rendering a score. Therefore it can be useful to handle arpeggio in the most common cases and is hopefully able to serve as a basis for further improvements.

Examples

Here are some examples of arpeggio rendering with the current version of this PR:

Example 1 Example 2 Example 3
arp1 arp2 arp3

Limitations of the current implementation

In this PR arpeggios are implemented in the internal model similarly to chords, as RelObjs connecting several notes. This seems to be natural in the context of the Lomse score model and potentially allows representation of a "true" cross-staff arpeggio involving separate chords on different staves. However reading such arpeggios is not implemented in the MusicXML importer (primarily because it wasn't clear for me how they are supposed to be represented in a MusicXML file), and the importer assumes that arpeggio always spans only for one chord. Similarly, the engraver only checks for arpeggio in one note and renders it only for one chord. This works fine in the most common case of a one-chord arpeggio but in order to support cross-staff arpeggio this will need to be corrected.

Also this PR does not include support of arpeggio in the LDP format (not sure if I need to do something special to add it or it is handled automatically?) and does not include support for MIDI rendering of arpeggio so all chords would sound non-arpeggiated on playback.

cecilios commented 3 years ago

Thank you for this PR. Great work! With this PR you understand the Lomse code almost as well as I do!

I'm currently on vacation, short on time for this and with limited access to internet. So reviewing the PR will take me some time. But I'll be happy to merge this as soon as I review and test the code, hopefully very soon.

After an initial review of the changes, my impression is that your approach for this PR is the one I would have followed. The limitations you mention in importing MusicXML arpeggios and on engraving are totally acceptable. You even took into account that the length of the arpeggio line could change if more space is added between staves later!

The lack of arpeggio support in playback is also something totally acceptable, to be addressed in future.

As for LDP, the short answer is: forget about LDP. The long answer is that the LDP format is not handled automatically but, in any case, LDP is only used in my application and I do not expect any more needs. In fact, the application is very old and I will stop maintaining it very soon. Therefore, LDP support could probably be totally removed in the near future. The LDP format is also used in Lomse as the format for the stack of undo/redo operations for edition commands; therefore, to remove LDP from the library, this issue has to be addressed, as well as to replace the LDP format in many unit tests. As my application does not need score edition, and nobody has ever posted an issue related to this, I give no priority to edition commands and my efforts are on MusicXML, layout and playback, expanding the support for more notation. Forget about LDP.

There is something in your comments that leaves me intrigued: the issue of "true" cross-staff arpeggios. Are you referring to two independent chords, on different staves, joined by the same arpeggio line? Is this a valid notation? My books on music notation are at home and I cannot check this now; I will have to investigate this as well as the possible MusicXML support for this, if it exist. Do you have any sample scores displaying this? In any case, if the result of this research is that it is a valid notation that has to be supported, this could affect a lot to the solution implemented in this PR but, in any case, an improvement for future.

As said, my first impression is that you did an excellent work! Thank you.

dmitrio95 commented 3 years ago

Thanks for your response! It would be totally fine for me to wait for any time so please feel free to make a review any time later.

There is something in your comments that leaves me intrigued: the issue of "true" cross-staff arpeggios. Are you referring to two independent chords, on different staves, joined by the same arpeggio line? Is this a valid notation?

Yes, I don't remember seeing it often in scores I have dealt with, but this notation exists for the case when two hands should arpeggiate chords sequentially, as a one arpeggio. Some examples can be found, for example, in Listzt's (page 9) or Grieg's works: Screenshot_20210803_013515 Screenshot_20210803_013329 MusicXML is also intented to support this, as follows, for example, from this issue but at this moment I don't have any examples of MusicXML files containing cross-staff arpeggio.

In any case, if the result of this research is that it is a valid notation that has to be supported, this could affect a lot to the solution implemented in this PR but, in any case, an improvement for future.

Actually I think it might be possible to implement this without very large changes to the structure introduced here. Possibly it would be enough to make a separate engraver for arpeggio which would get notified by chord engravers that they are ready for adding an arpeggio in the same way as chord engravers get triggered by the corresponding note engravers as these notes are found in a score. Arpeggio would then be engraved when its last chord is ready for that. Expanding cross-staff arpeggio if a staff distance changes is already implemented as a part of this PR so it would likely work as it is.

cecilios commented 3 years ago

No problems found neither in unit tests nor in regression tests. Also, no issues to comment in your code, so the PR is mergeable as it improves Lomse by adding partial support for arpeggios. I will do it.

I also have been thinking on how to model and deal with arpeggios, and also considering arpeggios that go cross staff across sets of chords (let's call them "cross-chords" arpeggios). You said that:

Actually I think it might be possible to implement this without very large changes to the structure introduced here. Possibly it would be enough to make a separate engraver for arpeggio which would get notified by chord engravers that they are ready for adding an arpeggio in the same way as chord engravers get triggered by the corresponding note engravers as these notes are found in a score. Arpeggio would then be engraved when its last chord is ready for that. Expanding cross-staff arpeggio if a staff distance changes is already implemented as a part of this PR so it would likely work as it is.

The approach you followed to implement arpeggio lines is very interesting. And your ideas for expanding the solution to deal with cross-chords arpeggios looks feasible to me. I agree with the idea of creating a separate engraver for the arpeggio line to engrave it at current position, perhaps similar to the barline engraver for dealing with cross-staff spacing increments. This will decouple the engraver from the chord engraver and will facilitate dealing with cross-chords arpeggios. Triggering this engraver when the last chord in the arpeggio is engraved seems the only feasible approach. My only concern is about horizontal spacing. Currently, as the arpeggio line is engraved as part of the chord shape and the line shape is added to a chord note, the spacing algorithm automatically deals with its space, as it is part of the note shape. So the new engraver for the arpeggio line still needs to attach the arpeggio line to a note shape in the chord and recompute its boundings.

If I had implemented arpeggio lines I probably would have done it in a different way. As engraving chords is complex enough and arpeggio lines are just a type of line before the chord, I would not even considered the possibility of engraving the arpeggio lines as part of the chord engraving process. So probably I would have modeled "arpeggiated" as an attribute for chords (just for playback or other needs), as member variables in ImoChord object. And the arpeggio line modeled as an independent ImoStaffObj (ImoArpeggio, a non-timed staffObj) with information about the arpeggio line start and end notes; as they could be both in different chords it automatically will cope with cross-chords arpeggios. This approach will affect to the spacing algorithm (but just to add ImoArpeggio as another type of non-timed staffObj, no changes in computations needed) and will require an independent engraver, simple, just to engrave the line at current position, perhaps similar to the barline engraver for dealing with cross-staff spacing increments.

But your approach is also understandable, simple and valid, and unless complex problems detected when dealing with cross-chords arpeggios I would keep it.

Having reviewed the MusicXML approach for arpeggios and cross-chord arpeggios, I really find it as something requiring improvements, so I have posted my thoughts here: https://github.com/w3c/musicxml/issues/423.

dmitrio95 commented 3 years ago

Thanks for merging! Actually I somehow did not not consider an idea of having arpeggio as a StaffObj. I had an impression that the staff object status is reserved for elements that constitute the main content of a score and can have a meaning more or less independently of whether there are other objects nearby in the score. Objects that are always attached to other object or always connect something in a score seem to fall into other categories in the internal model so I have only considered choosing an object type for arpeggio between them. If the scheme with StaffObjs turns out to work better I certainly can rework it that way, but if having arpeggio as a RelObj is fine then it would indeed be better to consider this again when implementing a cross-staff (and cross-chord) arpeggio support.

cecilios commented 3 years ago

Sorry, I didn't intent to suggest that my approach is better, just to explain my reasoning.

I had an impression that the staff object status is reserved for elements that constitute the main content of a score and can have a meaning more or less independently of whether there are other objects nearby in the score.

More or less I agree. But to me, the main difference is where the notation is attached to (conceptually). StaffObjs are objects attached to the staff and AuxObjs and RelObjs are attached to StaffObjs. As you assumed, StaffObjs normally represent the 'main' content of the music (basically notes, rests, clefs, key signatures and time signatures). All other notations are considered auxiliary objects.

But deciding if an object is attached or not to the staff is an open subjective decision, and sometimes (very very rare cases) an object that initially looks as auxiliary, is better modeled as an StaffObj, for having more flexibility or to greatly simplify algorithms and processes.

When I said that my first choice for modeling the arpeggio line would have been as an StaffObj, it was because I thought that attaching it to the chord would complicate more the already complex chord engraving process. And, as the arpeggio line is just a type of vertical line placed before the chord, perhaps modeling it as an StaffObj located before the first note of the chord would simplify all processes without limiting the model. The arpeggio line is "an auxiliary notation associated to a chord", but observe that "chord" is not an StaffObj: the chord is modeled as a collection of StaffObjs (the notes), thus, why not to add one more StaffObj (the arpeggio line) to the collection of StaffObjs that represents a chord? With the benefit of decoupling its engraving process from chord engraving, but at the cost of adding 'useless' entries to the StaffObjs table used for accessing the score by time position (the ColStaffObjs object).

In any case, I sincerely think your approach is conceptually better, as arpeggio lines are auxiliary notation. So, unless unexpected problems when implementing support for cross-chord arpeggios, I fully support your approach.

But, is ImoArpeggio a RelObj or should it be an AuxObj?

In current implementation, to use a RelObj is unnecessary and an AuxObj attached the base note will be enough; in fact, your code only accesses the arpeggio from the chord base note and only to access the relation attributes (direction, color, etc.), not the participants (the other notes); so an AuxObj is enough. I didn't mention this in the PR review because thinking about the future (cross-chord arpeggios) it is clear that it is also necessary to know the involved chords; therefore, a RelObj linking the base notes of the affected chords is needed and, thus, I decided to postpone the decision.

When starting to code the solution to support cross-chord arpeggios, it will be necessary to decide if both, single-chord arpeggios and cross-chord arpeggios, will use the same model or not. To use the same model the current approach of using a RelObj seems effective, but at the cost of including in the relation all notes in all affected chords, when only base notes are needed. Note that the relation cannot be restricted to the chord base notes as for single-chord arpeggios there is only one note and it is not possible to have a RelObj with only one participant.

The alternative I see is to split the model: in all cases, to use an AuxObj for modeling the arpeggio attributes and, in addition, only for cross-chord arpeggios, to use an additional RelObj containing only the base notes of the chords. Thus the base note of an arpeggiated chord will always have an AuxObj containing the arpeggio attributes and, optionally a RelObj linking all chords (their base notes) when the arpeggio is a cross-chord arpeggio.

Anyway, I think you did a great work and the details for supporting cross-chord arpeggios will have to be decided when working the code for it.

Thanks again for your PR and for contributing to this project.

cecilios commented 3 years ago

Please, a question: I'm adding a couple of scores to the Lomse regression test set (I will commit them soon). The one from Lilypond is already in my set, so I replicated the examples in your post (“Arpeggio of different lengths” and “Arpeggio on cross-staff chords”). I've noticed differences in rendering between your image and what I get. All of them but one are probably due to resolution, scaling factors and document sizes, so they look fine to me. But there is one that I cannot explain: the vertical position of words “Andantino” and “dolce”; also the horizontal position of word “dolce” is different. All other texts are placed at the same positions in both scores but not these two. Can you find an explanation for this?

My image: image

Your image: image

Also, a request: please for future PRs I will appreciate if you could also include some unit tests. For engraving and layout I know that normally it is difficult to code tests; thus, the automated visual regression tests can be used instead for detecting bugs and changes in layout behaviour and I would not expect you to provide unit tests here. But I will appreciate unit tests for other library parts, such as the MusicXML importer, as they should be simple to code and are of great help to detect bugs when refactoring or adding more features. Thank you!

dmitrio95 commented 3 years ago

The difference is probably in default-y attributes of those text elements. I have prepared based on the MusicXML code found here, and it contains default-y="15" attributes for these texts. If I remove the default-y attributes these texts are positioned in the same way as it is shown on your image. Here is the file I have used to prepare my screenshot: musicxml_example_arpeggiated.musicxml.txt

As for unit tests, sure, I will add them in my future pull requests.

cecilios commented 3 years ago

Confirmed. The difference is in default-x and default-y attributes. Thank you!

cecilios commented 3 years ago

@dmitrio95 If you don't have plans in a near future to add support for arpeggiated chords in playback, I have time and can take this throughout this month. Please confirm me. Thank you.

dmitrio95 commented 3 years ago

Yes, I have no plans to work on playback issues in the nearest future. My current focus with Lomse is on improving engraving, adding various notation support and fixing crashes I have encountered with some MusicXML scores. If that helps I can file issues on things I plan to do but anyway I will unlikely get to anything related to playback in the nearest future.

cecilios commented 3 years ago

Ok, thank you. I will take the task of arpeggios playback. I would like to establish a simple procedure so that we are informed of what each one does, to avoid that, by chance, both of us get to work on the same thing. For me it is enough with a private email informing that something is going to be addressed and, in any case, without any commitment to finalize it. Opening an issue and adding a comment "taken by xxxxxxx" is also a good way of doing it, as you prefer. The important thing is to avoid duplicate work. Thanks!

dmitrio95 commented 3 years ago

Well, I am fine with any of the options. Some of the issues I eventually work on are pretty small and are unlikely to overlap with anything but for larger tasks some sort of synchronization makes sense. I can send you a brief overview of the next things I was planning to do or open issues for them, whichever you prefer. Probably the nearest task for me would be finalizing pedal import and renedring — I have pedals half-working for a long time already but I still need to figure out a better way to support both pedal signs and lines and synchronize positions of both these types of pedals.

cecilios commented 3 years ago

Some of the issues I eventually work on are pretty small and are unlikely to overlap with anything but for larger tasks some sort of synchronization makes sense.

Yes, of course, the need is only for larger tasks. Then I will always open an issue (if not already in the list) and assign it to me. As for things that you would like to do, proceed as you prefer at every moment, send me a brief comment by PM or open an issue and self-assign it. I wouldn't like to create administrative burden, just to share information to avoid repeating work. And, as said, in no case the communication will be taken as a commitment to do it.

As to piano pedal marks, is you need help or would like to comment alternatives or ideas, feel free to ask. You can use issue #218 for this or PM, as you prefer.

dmitrio95 commented 3 years ago

Thanks, then I'll keep you informed on the issues I will take.