search

lenmus / lomse

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

MusicXML: fix adding spurious staves on reading mid-score <attributes> with <staves> tag #303

Closed dmitrio95 closed 3 years ago

dmitrio95 commented 3 years ago

When Lomse reads a mid-score <attributes> tags which contains some value for <staves> larger than 1, it may attempt to add extra staves to the existing instrument, which may lead to adding spurious staves or even to a crash. This may occur with the scores exported from MuseScore if they contain section breaks, for example:

The cause of the issue is that the MusicXML importer assumes that the instrument has only one staff prior to reading <attributes> tag and adds staves on top of that regardless of whether they are needed. This PR corrects this behavior so no unnecessary staves are added. I am not sure whether something more sensible needs to be done if number of staves actually changes but if it doesn't this solution prevents the issues related to that.

cecilios commented 3 years ago

Thank you by this PR. I will review it as soon as possible!

cecilios commented 3 years ago

I am not sure whether something more sensible needs to be done if number of staves actually changes but if it doesn't this solution prevents the issues related to that.

Currently the staves model is very simple: the number of staves for each instrument is defined in ImoInstrument but it is assumed that it never changes. Staves are modeled as a list of ImoStaffInfo objects, each one containing the information (spacing, number of lines, etc.) for one staff.

I have never tried to address adding support for scores in which the number of staves could be dynamically changed, increasing or decreasing it. For this, probably, the solution would require to remove staves definition from ImoInstrument and move this to an AuxObj attached to an ImoDirection or other ImoStaffObj, something similar to MusicXML <attributes> element.

I don't know how Lomse will currently behave if the number of staves is changed in the middle. If the number of staves is increased I don't foresee any crashes: it will behave as if the instrument had the maximum number of staves defined since beginning, and will be rendered with that maximum number since beginning but with empty staves in parts having less staves. But if the number is decreased, probably there will be crashes due to loops using the maximum defined number of staves instead of the current part number of staves. I never tested.

As to your PR, I think it is correct and nothing else should be done until support for dynamically changing the number of staves is added.

Perhaps this is one of the next important changes to introduce in Lomse, as scores in which the number of staves changes dynamically are common, e.g. ossia staves, although this can be a special case with a different treatment.

Thanks again for this PR.

dmitrio95 commented 3 years ago

Thanks for your comment! Then it is indeed better to postpone further changes until support for variable staves number is added.