lenmus / lomse

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

Fix a crash on engraving a beam including invisible notes #312

Closed dmitrio95 closed 3 years ago

dmitrio95 commented 3 years ago

This PR fixes another case of a crash with invisible notes (see also #311). If a beam includes invisible notes or fully consists of invisible notes then a crash happens because the engraver assumes that a beam always contains at least one visible note. This PR makes the engraver skip invisible staff objects passed to it and explicitly check that there are visible notes which can be used to engrave the beam. m_numNotes counter is reset later by the beam engraving algorithm so it should be fine to reuse it for this purpose.

Also this PR includes changes to properly delete the engraver if the last shape it produces is nullptr. Actually it would probably be even better if the EngraverMap would be responsible for deleting the engravers stored in it but this is probably something that doesn't belong to the scope of this PR.

An example score for this crash: lomse_beam_invisible_note_crash.musicxml.txt

cecilios commented 3 years ago

Thank you very much. I review this as soon as possible.

cecilios commented 3 years ago

I'm having problems to understand the use cases for invisible notes. Until now I thought they were only used for playback, as explained in my comments in PR#311. But the tests scores attached to this PR and to PR#311 does not seem to be oriented to playback effects. For playback, I do not see the need to include beams in invisible notes. Also I do not see the idea behind mixing visible and invisible notes in a beam or in a tuplet. This is not for playback but for something different, perhaps an app displaying the invisible notes as an question mark symbol and use it for aural training exercises in which the student must identify the played note.

Did you find in internet MusicXML scores containing such awkward constructions? Any idea about the scores origin or purpose?

cecilios commented 3 years ago

The PR does its work and no problems detected, so I merge it. But I would like to have clear ideas about invisible notes. I have checked that MuseScore renders them with grey notheads, and for printing, it prints the beam but not the noteheads. It would be nice to check what Lylipond, Finale and others do with this score and with the one in PR#311.

Actually it would probably be even better if the EngraverMap would be responsible for deleting the engravers stored in it

Yes, but there are so many things to do !

dmitrio95 commented 3 years ago

But the tests scores attached to this PR and to PR#311 does not seem to be oriented to playback effects.

Yes, those are just artificial examples I have prepared to get minimal examples of the issues. The original score where I have encountered this crash is Over_the_Top_Constellations.zip (originally downloaded from here). This score contains multiple places which look like this (invisible notes are grayed out): изображение

Here visible notes have playback turned off (which doesn't seem to be reflected in the MusicXML file though) and invisible notes are what is being played here. The intention is apparently to tweak playback of those grace notes to make them sound a bit earlier and take a time from the previous note rather than from the current one. Here beams are also marked invisible, but MusicXML doesn't have print-object attribute for all objects (although here it can be concluded that the beams are invisible since all their notes are also invisible).

There are also examples of ties to invisible notes, also for playback purposes (and also producing a crash). I don't think I have real-world examples of beams connecting both visible and invisible notes (but I have an occasional example of this with the tuplet crash, in some tablature score where that note is tied and should indeed not be displayed). Still this is a possible situation and it is better to be able to deal with it without crashing. That is why I am trying to fix all such cases that I happen to encounter. Perhaps some of these crashes are better to fix at the MusicXML importer level though as there are cases that indeed do not make much sense to support in the engraving code.

cecilios commented 3 years ago

Thank you. Now I understand the use of beams in invisible notes for playback.

But still don't have clear ideas about what Lomse should do with invisible notes. Perhaps a global option to render or not invisible notes should be the most appropriate approach, for supporting different applications. For edition apps. such as MuseScore, probably they would like to see the invisible notes rendered, perhaps in a different color (grey?), so that the user can edit those notes. But for printing or other uses, the invisible notes and their related notations (beams, tuplets, etc.) must not be rendered unless also shared with visible notes. I will continue thinking on this but ideas are always welcome!

cecilios commented 3 years ago

Still this is a possible situation and it is better to be able to deal with it without crashing.

Yes, totally agree.

Perhaps some of these crashes are better to fix at the MusicXML importer level ...

From my previous comment, for now Lomse should take the safest approach. And if a global option is finally implemented, Lomse will need to import those invisible notes and related notation.

dmitrio95 commented 3 years ago

Some applications may need both to display or to hide invisible notes depending on the context, so probably that option should better be changeable at runtime, perhaps on a per-document basis. Editor apps will definitely need both: MuseScore, for example, displays visible notes when editing a score but has a per-document option to hide invisible elements and always hides them when printing a score or exporting it to PDF or any image format. Other than that, I don't think there are many options on what to do with invisible objects: they can either be displayed or not, or perhaps they can be ignored on import if that makes sense for any application. What can make implementation of such option non-trivial is the question of whether invisible elements should affect layout of visible ones or not. For visual editors the most reasonable answer is probably not, but then it somehow needs to be ensured that invisible notes do not affect horizontal spacing of subsequent notes, vertical spacing of object placed above or below staff etc. However perhaps in some other situations a simpler approach could also be useful with laying out both visible and invisible objects in a usual way. So globally there are probably not so many choices here but finer details may depend on what will this feature be targeted for.

cecilios commented 3 years ago

Thank you. I totally agree with your comments. A global option modifiable at run time, per document, looks as the best approach.

The question of how invisible notes, when displayed, should affect horizontal and vertical spacing is really difficult to answer without more information about use case scenarios. The solution of treating them as normal notes is simple: just to generate a note shape instead of an invisible shape, but could produce unexpected results if that is not the idea behind using the invisible notes. But the option of displaying them without affecting spacing looks complex. I will try to gather more information on invisible notes and use case scenarios before deciding.