Fix svg drawing for staff line optimization

[Kovak]

Not certain if this is the best way to fix it, but I noticed on master branch that when saving as SVG the staff lines are only drawn for the first measure if byMeasure is false. This occurs because the layout function for staff lines no longer respects the change in bounding box size instead always using the measure width to determine the sizing.

[Jojo-Schmitz]

@Kovak: could you please sign the CLA?
Maybe @sidewayss what's to have a look at this?

[sidewayss]

Interesting. I don't see context here for understanding the way to reproduce this issue, or what exactly the issue is. The fix to file.cpp seems very reasonable, if there is a size change that it not being accounted for in the existing code. It's one line of code change and simply gets the more up-to-date value. If you want me to look at this more closely, let me know.

[Kovak]

The context is that right now on master branch if you save an svg file and it attempts to use the optimized staff line drawing where it stretches the first measure to be size of the entire set of measures that exists on one line, it does not work. All that happens is that the first measure gets staff lines drawn and every following measure has no lines drawn whatsoever.

This is because the current code only ever respects the measure width: https://github.com/cairn-labs/MuseScore/blob/4dd9b61cb82e655fef2468713519a179bd75dfd4/libmscore/stafflines.cpp#L99

However, the current SVG line draw optimization ONLY sets the bounding box width of the staff lines, not the measure those staff lines belong to. https://github.com/cairn-labs/MuseScore/blob/4dd9b61cb82e655fef2468713519a179bd75dfd4/mscore/file.cpp#L2709

There is absolutely no way for the staff line drawing optimization to take effect at the moment.

In the older musescore 2.2 branch, the drawing code for the staff lines respected this bounding box size change when drawing the lines. In current master branch, it does not.

I have also signed the CLA now.

[sidewayss]

So essentially, MuseScore now has its own optimization for staff line drawing, and adjustments must be made to the SVG way of optimizing. Is this new optimization optional? You say "attempts to use" the new feature.

The primary reason for the SVG optimization is continuous horizontal scores, all one staff. It sounds like the new MuseScore optimization now supersedes that in any score with multiple systems. In a single-system score (imagine one with >100 measures) would this new feature apply? In which case the SVG optimization would no longer be necessary at all.

That would be nice, to eliminate that SVG optimization code altogether. It's not the prettiest, but it gets the job done for the vast majority of cases now. But it sounds like it might just be obsolete.

[Kovak]

Ok look here is the results of installing master branch and exporting the SVG for the default file, I have had to convert it to a PNG because github will not allow me to post an SVG.

Whatever optimization you are mentioning is not at all currently in effect. What you have at the moment is broken code that attempts to perform an optimization but due to the changing in the way Stafflines layout function works, never takes affect.

The SVG optimization code is still necessary, the other optimization you are mentioning does not appear to exist at all.

[sidewayss]

You misunderstand me. The optimization I refer to is the the "the optimized staff line drawing where it stretches the first measure to be size of the entire set of measures that exists on one line" that you mention. Is that not a new feature? I assumed it was, but maybe it has been around. Sorry for any misunderstanding. At some point this optimized staff line drawing you refer to came into existence and now you have fixed the downstream effect on SVG export. I think we can agree on that :-)

[sidewayss]

Separately, if this optimized staff line drawing already draws the first measure of each system until the end of each system, then the SVG optimization code is not necessary anymore. Unless that optimized staff line drawing is a user option, thus not always in effect.

[Kovak]

Now I'm not that familiar with musescore, but diving into the code when I was fixing this what you're saying about there existing optimized staff line drawing that stretches the first measure to a system to the end of that system, is not true. There are currently 2 modes: it either draws each measure independently (in SVG export this is byMeasure = True, this is also how everything is rendered inside the software) or it at the moment draws one measure but fails to stretch it. There is no code that does the stretching across systems outside of the SVG export.

[sidewayss]

Now I'm confused. You were talking about the SVG export optimization the whole time. Well something must have changed. I have used it in various types of scores and it has worked. Something must have changed underneath it. Others have used it and the only complaints I heard were about other features.

[Kovak]

This is because the old implementation of layout for staff lines used the width (which would have been modified by the bounding box modification)

https://github.com/musescore/MuseScore/blob/2.2/libmscore/element.cpp#L908

the new one uses the measure's width (which does not get modified by the bounding box modifcation)

https://github.com/cairn-labs/MuseScore/blob/4dd9b61cb82e655fef2468713519a179bd75dfd4/libmscore/stafflines.cpp#L99

[sidewayss]

Now it make sense to me. Thanks.

[Kovak]

Sorry when I had initially made the PR I had forgotten where the old definition of StaffLines was so I couldn't include that detail. Had to go dig it up. The reason I ended up introducing a whole new function for the purpose is that I didn't want to disrupt any of the other logic that was dependent on measure size. I doubt we can just stretch the size of the measure without side-effects the way we do the stafflines object.

[ericfont]

FYI, I made an issue report a couple days ago, unaware of this PR.

https://musescore.org/en/node/269870

One thing I don't like about your PR is that you have an additional function StaffLines::layoutWithoutMeasureWidth() which I'm guessing is only going to be used for SVG export. But the function is almost exactly the same as layout() except that:

qreal w = measure()->width();

got changed to:

qreal w = bbox().width();

I'd much rather those two functions be combined together as one. The reason is to prevent code diveregence in the future, because maybe someone make a future code improvement to StaffLines::layout() but then doesn't know to make a corresponding change to StaffLines::layoutWithotuMeasureWidth(). So what I would suggest is to maybe only have one function but have a default boolean paramemter called something like useEntireBoundingBox which is false by default unless the function is called with that true, and then have a little if statement when setting the correct value of w.

If you do update this PR, please include "fix #269870" at the start of the commit message, so that the issue is properly updated in the musescore.org issue tracker.

[sidewayss]

As a previous author of the relevant code here, I'm pleased to see others working with it and with SVG export in general. @ericfont's request seems minimally invasive to me, and I have no problem with it. Though it's @Kovak's code, not mine.

[sidewayss]

I have a real fix for this available now. See here:
https://musescore.org/en/node/269870#comment-835681

[anatoly-os]

See c859eae which fixes the issue.