lenmus / lomse

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

Add support for pedal marks #332

Closed dmitrio95 closed 2 years ago

dmitrio95 commented 2 years ago

This PR partially fixes #218 and implements a basic support for pedal marks and lines, including their engraving and importing from MusicXML. Playback effects from pedals are not implemented in the scope of this pull request.

Unfortunately I haven't found any scores that make large use of all MusicXML's features for pedals. I don't have many scores from editors other than MuseScore, and MuseScore exports all pedals as simple pedal lines. Therefore the examples I could use to test my code are mostly hand-edited or based on the examples from the MusicXML documentation. Still, here are some example scores rendered with this PR:

pedal_examples.musicxml.txt (based on the examples from the MusicXML documentation: lines, symbols) Screenshot_20211119_013221

test_pedals.musicxml.txt (initially exported from MuseScore but manually edited to test various MusicXML pedal features): Screenshot_20211119_013309

Based on the previous discussions, I have implemented the following model for pedals: 1) ImoPedalMark: an auxiliary object representing a pedal mark. This may be a standalone pedal mark or a pedal mark which will be followed by a line. This object is graphically represented with a single glyph ("Ped." or "*" or similar signs). 2) ImoPedalLine: a relation object representing a pedal line. Start and end points of the relation represent start and end of the pedal line, and middle points represent pedal changes (_/\_-like signs). Therefore a graphical representation of a pedal line is more complex and includes lines (with start and end corners if necessary), pedal change glyphs and, for pedals spanning for multiple systems, pedal continuation signs (the "(Ped.)" text at the beginning of subsequent systems).

As pedal marks and pedal lines are handled by different engravers, the mechanism from #317 is used to align pedal marks and pedal lines with respect to each other.

As I don't have real-world scores using most of the MusicXML's pedal features the implementation here or its parameters may need to be adjusted in the future. However it can hopefully serve as a starting point for implementation of pedals in Lomse.

cecilios commented 2 years ago

Great! Thank you very much. I will review this as soon as possible.

cecilios commented 2 years ago

Thank you for this PR. I've reviewed the code and run the regression tests and everything looks correct. So I'd like to merge this.

In compilation, there are a few warnings about unused parameters in pedal engravers. But do not spend time fixing them because I'm currently working on reviewing and refactoring the parameters passed to engravers, as we commented in PR #317, and will send a PR in a few days.

As to tests scores I have the same problem than you. Apart from the tests scores from MusicXML website and the tests scores from Lilypond MusicXML test set I only have some scores created with MuseScore. Therefore I cannot test more complex pedal mark notations. But any issue could be fixed in future if needed.

With the test scores I've noticed an issue with Lilypond score 31a-Directions.xml.txt. There are a couple of pedal marks at the end of fourth system and this is the relevant part of the original image, from lilypond website:

image

But Lomse now renders it as follows:

image

Using lines instead of symbols is correct, but pedal lines overlap the lyrics. I've checked the lyrics and pedal lines bounding boxes, but they are properly computed:

image

I did a quick review of how pedal engravers and lyric engravers manage the VProfile, but didn't observe anything wrong. So this has to be investigated and fixed, but in my opinion it is better to merge this now. VProfile and vertical spacing of all objects has to be reviewed and improved, and there will be time to fix all this.

So, if you agree, I will merge this.

dmitrio95 commented 2 years ago

Thanks for noticing this issue, this is actually my mistake. In the pedal line engraver a vertical profile is used only to detect collisions with the base line of a pedal line, not accounting for start and end corners or pedal changes or anything else that may be above the base line.

A simple way to fix this would be to check the entire bounding box for collisions with the vertical profile, but this may lead to an unnecessarily large Y offset if there is no real collision but there is an overlap of the bounding box with the vertical profile. This happens, for example, with my first example score: Screenshot_20211120_204742

Therefore a better way would probably be to separately detect collisions with parts that lay above the base line and shift the base line if there are some collisions within these parts of the line. This approach can both fix collision issues and avoid unnecessary vertical shifts of a pedal line if there is clearly no collision: Screenshot_20211120_213255 Screenshot_20211120_212208

I have added a commit that implements this approach to fix collisions.

cecilios commented 2 years ago

Great! Thank you very much. It is now fixed. Merging...