fixes for small staves #4827

MarcSabatella · 2019-03-24T22:08:21Z

IThis PR contains fixes for the following issues:

https://musescore.org/en/node/282021
https://musescore.org/en/node/282026
https://musescore.org/en/node/286645

Nothing major, just a bunch of places we weren't checking the staff scaling where we needed to be. So, I added mag() overrides for a number of element types, and I made sure to factor it in to a few calculations where it wasn't. Also tweaked the definition of Element::spatium() to ignore staff scaling for elements within system elements (basically, volta text) as well as system elements themselves.

There are still two issues tagged "smallstaves" I haven't addressed that I very much want to see solved for 3.1. One is https://musescore.org/en/node/277039 (regarding barline layout) and the other is https://musescore.org/en/node/284440 (position/offsets of elements not scaling). The barline one is probably not that hard, but there are other significant barline issues that make me wonder if we don't need some more fundamental changes there. And as per my comments in the issue regarding position not scaling, there might also be some deeper decisions to make about that. I would very much appreciate if @wschweer and/or @dmitrio95 could take a look at those.

BTW, I have a sense that https://musescore.org/en/node/283312 will probably want to wait until we have a solution for the position/offset issue in general, but that's another one that should be fixed for 3.1 if possible. There is also https://musescore.org/en/node/284783 but that one is likely to be ugly, and it was just as bad in 2.3.2 and hardly anyone noticed.

MarcSabatella · 2019-03-25T16:15:52Z

(it's correct for the volta to remain full size since it is a system element)

MarcSabatella · 2019-03-26T23:04:55Z

I decided to take a look at what would be involved in fixing the offset issue, and indeed, the best solution seems to be at the element level. Specifically, modifying ScoreElement::styleValue() to take staff size into account when returning a value of type POINT_SP or POINT_SP_MM (if sizeIsSpatiumDependent). Works like a charm with respect to basic offsets and anything else that goes through the property mechanism, only a few lines of code. I plan to add a commit for this, I just need to test a bit more.

But, there are a whole bunch of style settings not associated with properties that we don't get covered - things like lyrics top/bottom distance, the various autoplace min distances, etc. Some of these are being scaled where they need to in the code, a lot aren't. Here, I think we are over-relying on styleP() and the pre-computed values it returns - man of these then need to be scaled by staff mag and that just isn't happening consistently.

But, the basic offset fix is important, easy, and seems safe enough to me since it's only going to affect scaled staves they are clearly not working right as it is. So I think once I add that commit, I would consider small staves "fixed enough", and catching all the specific places where we fail to scale some specific style setting will be a task for another day.

MarcSabatella · 2019-03-27T03:04:23Z

Position/offset - along with all other spatium-dependent properties are now handled via the styleValue() change mentioned above.

The one other change needed was to make sure we force the offset to be recalculated when adding a text element to the score (including on read, if there is no explicit offset in the file). Otherwise, text gets added at the default unscaled offset. If you then do Ctrl+R it fixes itself, but this is lost on save/reload. So I added an automatic reset of offset when adding the element (assuming the property is styled)

Most other element types do this same reset automatically on every layout - setting the offset to the style default if the offset property is styled - but text elements don't. This was an explicit change done here:

96af32e#diff-65296810944b21effeabb791791901cfR1338.

I can verify a number of things don't work as well if I uncomment those lines, but it seems to be safe to do this reset on add rather than on layout.

So I'm done working on this, I think things are good enough for now.

MarcSabatella · 2019-03-27T03:06:34Z

Updated vtest:

MarcSabatella · 2019-03-27T04:45:02Z

I had forgotten about https://musescore.org/en/node/283312, that turned out to be a simple fix too :-)

dmitrio95 · 2019-03-27T07:34:09Z

libmscore/volta.cpp

@@ -51,7 +51,7 @@ static const ElementStyle voltaStyle {
 //   VoltaSegment
 //---------------------------------------------------------

-VoltaSegment::VoltaSegment(Spanner* sp, Score* s) : TextLineBaseSegment(sp, s, ElementFlag::MOVABLE | ElementFlag::ON_STAFF)
+VoltaSegment::VoltaSegment(Spanner* sp, Score* s) : TextLineBaseSegment(sp, s, ElementFlag::MOVABLE | ElementFlag::ON_STAFF | ElementFlag::SYSTEM)


Isn't this violating the current meaning of SYSTEM flag a bit? As far as I understand the system flag is assigned to elements that are drawn once for the whole system and do not belong to any particular staff. This doesn't seem to be true for volta segments, though there probably should be a way to mark that the element should use system-wide style settings.

Voltas are special. They are already being added with the system flag set, but only on the Volta itself, not the VoltaSegments, and this caused problems once I started scaling lines - some aspects of the volta would scale, others wouldn't, depending on whether they were handled in Volta or VoltaSegment. So I needed to make these consistent.

A little background:

When added to the top staff, voltas are treated as regular system elements - they appear on the top visible staff in the score and on all staves of the score. So the system flag is absolutely appropriate. However, there were enough requests to be able to also add voltas on lower staves that at some point, this capability was added. Voltas added to staves other than the top staff get added and have the system flag set, but then they are special-cased in excepts.cpp so the volta does not get exported to parts.

Probably it makes more sense to only set the system flag when actually being added to the top staff. And actually, fix it so it gets set when added to the top visible staff - right now that's a bug (see https://musescore.org/en/node/277265).

Anyhow, the way we treat voltas with respect to the SYSTEM flag for voltas is a bit of a hack indeed, but it's not my hack; I'm just cleaning it up a little by making sure it also gets set for the volta segments.

FWIW, I would say, we could change the volta add code to only set the system flag when adding to the top visible staff (and force the track to 0 even if there are invisible staves above). But then, I worry about cases where the volta is added then staves are rearranged, and you end up with a volta on the top staff that is not set as SYSTEM. But in any case, changing the basic behavior of voltas seemed outside the scope of this PR, so I didn't change it.

Thanks for your explanation! I agree, it is indeed better to keep that system flag consistent with the volta itself.

dmitrio95 · 2019-04-09T15:25:38Z

libmscore/line.cpp

@@ -989,7 +989,7 @@ void SLine::writeProperties(XmlWriter& xml) const
      //
      // write user modified layout
      //
-      qreal _spatium = spatium();
+      qreal _spatium = score()->spatium();


This probably fixes the situation for offset but may break it for off2, it uses exactly local spatium() on score reading:

MuseScore/libmscore/line.cpp

Lines 61 to 63 in 3f91190

else if (tag == "off2") {

setUserOff2(e.readPoint() * spatium());

}

Maybe both these properties should be switched to using the same spatium value in case it doesn't spoil the existing files.

Good catch, you are absolutely correct.

Since things are already quite messed up for existing files, I will switch this to using global spatium for off2 as well n read. Will only affect scores that have small staves and diagonal lines, and they are likely to be messed up because of the offset bug that this commit was fixing.

MarcSabatella · 2019-04-10T15:42:17Z

FWIW, I made the off2 change, and since I was thinking about diagonal lines anyhow, I went ahead and also submitted #4887.

dmitrio95 · 2019-04-11T11:22:20Z

libmscore/segment.cpp

@@ -482,6 +482,9 @@ void Segment::add(Element* el)
      Q_ASSERT(track != -1);
      Q_ASSERT(el->score() == score());
      Q_ASSERT(score()->nstaves() * VOICES == int(_elist.size()));
+      // make sure offset is correct for staff
+      if (el->isStyled(Pid::OFFSET))
+            el->setOffset(el->propertyDefault(Pid::OFFSET).toPointF());


If scaling is needed for offset, it should probably be applied not only in Segment::add(). For example, the following scenario still works incorrectly with this patch:

Make a small staff

Add there a staff text.
Its offset gets scaled (y = -1.40sp while the style default is -2sp)

Flip placement (X)

Flip placement again
Now offset equals to -2sp so it is not scaled again (actually scaling gets broken on step 3 already).

By the way, shifting offset a bit and resetting it to style default correctly assigns the scaled value, it looks like flipping placement direction avoids calling styleValue() but takes somehow the value directly from the style.

I think I have it figured out, hopefully have a new commit soon...

Done. The original code should have worked for most non-text elements, because they generally explicitly set offset in this same basic way during layout. But TextBase::layout() has this commented out (with a TODO that it should already be done). That's something I've run into before and never quite understood. But, no problem, I just fixed the code in undoChangeProperty() where we change offset based on placement.

dmitrio95 · 2019-04-11T11:34:38Z

Other than the issue on styled values scaling, the other four commits look good to me. In case it takes more time to correct this, could you please extract that commit to a separate PR? I will merge the other commits then.

MarcSabatella · 2019-04-11T12:38:15Z

I'm looking into it...

MarcSabatella · 2019-04-11T13:56:05Z

All should be good once Travis says so

fixes for small staves

MarcSabatella · 2019-04-11T15:14:46Z

Maybe too good to be true, but - I think something here may have just fixed https://musescore.org/en/node/281019, which I had just started looking at last night. In any case, it now seems better. More testing required before I mark it fixed.

Resolves: https://musescore.org/en/node/298564 We currently write the offsets for slur grips scaled according to the staff, but we read them scaled according to the score. That's because on read, the slur hasn't been added yet, so the track is not set. I fixed the exact same issue for other lines in musescore@812a281 but slurs have their own own read/write and I neglected to make the corresponding change for them. This fixes the issue by explicitly scaling according to score. Note there are other reasons why this makes sense, see other comments in musescore#4827 for more information on the scaling of offsets and other properties.

MarcSabatella force-pushed the small-staves-1 branch from 940eed0 to c560bdc Compare March 25, 2019 16:09

MarcSabatella force-pushed the small-staves-1 branch from c560bdc to 5136e00 Compare March 27, 2019 02:48

MarcSabatella force-pushed the small-staves-1 branch from c6c7f24 to 3f91190 Compare March 27, 2019 04:56

dmitrio95 reviewed Mar 27, 2019

View reviewed changes

dmitrio95 reviewed Apr 9, 2019

View reviewed changes

MarcSabatella force-pushed the small-staves-1 branch from 3f91190 to f1520c9 Compare April 9, 2019 16:46

dmitrio95 reviewed Apr 11, 2019

View reviewed changes

MarcSabatella added 5 commits April 11, 2019 06:36

fix #282021: scale beam thickness

d788c38

fix #282026: scale stem displacement

286a528

fix #286645: scale elements

ec6f167

fix #284440: scale spatium-dependent properties

bddbad2

fix #283312: adjusted hairpins on small staves shift on save/reload

812a281

fix scaling on flip

93a7129

MarcSabatella force-pushed the small-staves-1 branch from f1520c9 to 93a7129 Compare April 11, 2019 13:30

dmitrio95 merged commit 3232ff3 into musescore:master Apr 11, 2019

anatoly-os pushed a commit that referenced this pull request Apr 11, 2019

Merge pull request #4827 from MarcSabatella/small-staves-1

2565a42

fixes for small staves

MarcSabatella mentioned this pull request Dec 16, 2019

fix #298564: shift of manually adjusted slur on small staff #5538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes for small staves #4827

fixes for small staves #4827

MarcSabatella commented Mar 24, 2019

MarcSabatella commented Mar 25, 2019 •

edited

Loading

MarcSabatella commented Mar 26, 2019

MarcSabatella commented Mar 27, 2019

MarcSabatella commented Mar 27, 2019

MarcSabatella commented Mar 27, 2019

dmitrio95 Mar 27, 2019

MarcSabatella Mar 27, 2019

dmitrio95 Mar 27, 2019

dmitrio95 Apr 9, 2019

MarcSabatella Apr 9, 2019

MarcSabatella commented Apr 10, 2019

dmitrio95 Apr 11, 2019 •

edited

Loading

MarcSabatella Apr 11, 2019

MarcSabatella Apr 11, 2019

dmitrio95 commented Apr 11, 2019

MarcSabatella commented Apr 11, 2019

MarcSabatella commented Apr 11, 2019

MarcSabatella commented Apr 11, 2019 •

edited

Loading

	else if (tag == "off2") {
	setUserOff2(e.readPoint() * spatium());
	}

fixes for small staves #4827

fixes for small staves #4827

Conversation

MarcSabatella commented Mar 24, 2019

MarcSabatella commented Mar 25, 2019 • edited Loading

MarcSabatella commented Mar 26, 2019

MarcSabatella commented Mar 27, 2019

MarcSabatella commented Mar 27, 2019

MarcSabatella commented Mar 27, 2019

dmitrio95 Mar 27, 2019

Choose a reason for hiding this comment

MarcSabatella Mar 27, 2019

Choose a reason for hiding this comment

dmitrio95 Mar 27, 2019

Choose a reason for hiding this comment

dmitrio95 Apr 9, 2019

Choose a reason for hiding this comment

MarcSabatella Apr 9, 2019

Choose a reason for hiding this comment

MarcSabatella commented Apr 10, 2019

dmitrio95 Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

MarcSabatella Apr 11, 2019

Choose a reason for hiding this comment

MarcSabatella Apr 11, 2019

Choose a reason for hiding this comment

dmitrio95 commented Apr 11, 2019

MarcSabatella commented Apr 11, 2019

MarcSabatella commented Apr 11, 2019

MarcSabatella commented Apr 11, 2019 • edited Loading

MarcSabatella commented Mar 25, 2019 •

edited

Loading

dmitrio95 Apr 11, 2019 •

edited

Loading

MarcSabatella commented Apr 11, 2019 •

edited

Loading