Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stretch on mmrests #4862

Merged
merged 1 commit into from Apr 18, 2019

Conversation

@MarcSabatella
Copy link
Contributor

commented Apr 2, 2019

Main issue fixed here is https://musescore.org/en/node/281651. The problem is that we do not account for user stretch on mmrests in setStretchedWidth(). I added code to do so, but I note that this will have an impact on the default widths of mmrests. Since this was going to be the case anyhow, I also took this opportunity to tweak things to make mmrests widths more consistent and less "greedy" overall, both of which have been requested multiple times. The difference is subtle but better, I think.

EDIT: I removed a second commit I originally added for https://musescore.org/en/node/287120, I think it needs more discussion

@MarcSabatella MarcSabatella changed the title Mmrest stretch Fix stretch on mmrests Apr 2, 2019
@MarcSabatella MarcSabatella force-pushed the MarcSabatella:mmrest-stretch branch from 33875f8 to 8a404db Apr 2, 2019
@@ -3405,6 +3405,7 @@ System* Score::collectSystem(LayoutContext& lc)
getNextMeasure(lc);

minWidth += ww;
#if 1

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 10, 2019

Contributor

Could we perhaps remove this #if 1 as the code anyway remains enabled?

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Apr 10, 2019

Author Contributor

Certainly, sorry, debugging artifact.

}
int weight = ticks().ticks();
// reduce weight of mmrests
// so the nominal width is not directly proportional to duration

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 10, 2019

Contributor

Actually weight >> 2 should be equivalent to weight / 4 so the MM rest width still remains proportional to its duration, although with a smaller factor. Couldn't it cause problems for some really long MM rests (for example, for parts for which most of the movement is filled with rests)?

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Apr 10, 2019

Author Contributor

Maybe. I didn't want to change the current behavior too much because after discussions on Telegram and checking out previous forum discussions as well as Gould, we're actually in pretty good shape overall. But for sure I'll test with some really long rests.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Apr 10, 2019

Author Contributor

Great call on the long rests, this change resulted in an mmrest taking up a whole system at less than 60 measures, whereas before you could have hundreds.

I'm going to cap the scaling, then, so you get different widths for different sizes up to the cap but then they are all the same. This is one of those cases where we might want the base width to be the same for 100 measure versus 200, but we could let the 200 have greater weight during stretch, so in practice you would normally still see a difference in width. I'm playing with solutions like this now.

// reduce weight of mmrests
// so the nominal width is not directly proportional to duration
if (isMMRest())
weight = (weight >> 2) + 1;

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 10, 2019

Contributor

Also I see that the weight calculation logic is commonly duplicated within this PR. Maybe it can be extracted to a separate Measure class member function, something like

int Measure::layoutWeight() const
      {
      int weight = ticks().ticks();
      if (isMMRest())
            weight = (weight >> 2) + 1;
      return weight;
      }

Also, are there any reasons to write exactly weight >> 2 instead of weight / 4 or something similar?

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Apr 10, 2019

Author Contributor

First: I have this notion from way back when that >> 2 is faster, probably most compilers are smart enough for this to not be true though. I see the >> 2 used in track / voice calculations elsewhere, probably @wschweer comes from the same mindset :-)

As for the refactor, I did consider that. One thing that I didn't make clear: while the two calculations in layout.cpp need to be in sync, the one in measure.cpp could actually be different and it would still work. The measure.cpp calculation is about calculating a default width for the measure, the layout.cpp calculation is about determining what proportion of the "rest" of the system should be allocated to the measure during system fill/stretch. But I didn't see any reason not to use the same calculation.

So, I'll go ahead and do the refactor but add a comment.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:mmrest-stretch branch 2 times, most recently from 4446c1d to 7f64ea7 Apr 10, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Updated as per discussion above - refactored, changed ">> 2" to "/ 4", removed the debug artifact, and added a cap on the scaling of mmrest width.

The cap on the nominal width of mmrests is set at 32 bars (defined as a constant in the code with a TODO to perhaps add a style setting). What this means is, mmrests of longer than 32 bars will all start with the same nominal width. Thus you can have arbitrarily long mmrests on a system and still fit other measures. But, the cap is not used during the stretch process, so in most real world cases, even if you have a system with two mmrests of, say, 100 and 200 bars, the longer one will still be wider - just more subtly so than currently. It's a win on all counts, I think.

I've included a vtest.

@dmitrio95

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

The solution looks completely good to me, but it makes MM rests be wider in some cases. For example, the example score from the issue description which would fit to one system before the patch now spans for two systems:
mmrest-stretch-1

I wouldn't say it is bad but in case user wants to correct this changing layout stretch doesn't really help until the full relayout. #4866 probably solves exactly that issue so maybe it makes sense to merge both pull requests to master together.

In case this widening is not a desired change maybe it makes sense to lower maxMMRestLength parameter or just apply the length-based layout weight for MM rests only for system stretching but not for calculating the minimal MM rest size. The actual fix shouldn't stop working then since basicStretch() containing the user-defined stretch value still will be applied.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Hmm, it shouldn't have made that case wider, I tested it specifically. But I'll check again. The other issue where the system isn't laid out correctly after a change certainly complicated testing though (which is why I had an offer to remove those lines at first).

Anyhow, I'll look into it. I'd also like to be able to reduce stretch further.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

It is indeed true that the line no longer fits in that example as initially laoded, but that's actually a good thing. The mmrest in the example file had already been stretched up to a factor of 2, which was having virtually no effect originally but is now behaving as expected, making it wider as one would expect. Return the rest to default stretch and you'll see it's virtually identical.

It is worth pointing out, though, that while in most cases my change means the default the width of an mmrest will be narrower, there are likely some cases where this isn't true. The main reason mmrests were as wide as they were in 3.0 is that they were taking up too much of the available space when stretching to fill a system to the margin. My change improves this significantly. However, the minimum width of an mmrest was previously completely independent of duration, and with my change, the duration does affect it.

So, whether the default width of an mmrest is narrower or wider with my change depends on which effect is more significant - the increase in nominal width or the decrease in stretch weight. In many (most?) cases it is the latter, meaning mmrests will appear narrower by default. But on a system that is already pretty full, so there isn't much available space for the system fill to distribute, you'll get a wider mmrest by default.

I'm not 100% convinced this is bad either, I think the following in 3.0 (default layout, no user stretch adjustments) looks weird:

image

On the other hand, in my current code, the mmrest gets big enough to push one of those measures of sixteenths to the next system by the time you get to 7 measures of rest:

image

versus

image

It's pretty much that division by four that defines the threshold, although it's a bit more complicated than that because there is also the minimum width style setting and also an influence of the time signature. But, dividing by a bigger number makes the tradeoff point go up.

So I guess the question is, at what point - if any - should the nominal width of that mmrest be enough to push the measure of sixteenths to the next line?

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

BTW, the reason to still have the nominal (unstretched) width have some influence from duration is to avoid situations where on relatively full systems, you don't get a 100 measure rest having the same width as a 4 measure rest. It's mostly a matter of tweaking how big an effect you want it to have.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:mmrest-stretch branch from 7f64ea7 to 7f42317 Apr 12, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I've pushed a modified version of the layout weight calculation for mmrests. This one is, I think, more "fair", and it produces results I find pleasing. I start by assuming the weight should be at least that of a single measure in the time signature, then to that I add a small fraction (1/32) of the remainder of the duration of the rest. I retain the idea of a cap I apply only for calculating the minimum width.

So, in 4/4 time, a single ordinary measure has weight 1920, a two-bar mmrest has weight just a little more than that (1980), then it keeps scaling up linearly until you reach a 32-bar rest, which caps out at just under twice the single bar (3780). After that, the minimum width never increases as you add measures, but its weight when it comes to filling the system continues to increase, so in practice a 100 measure rest is still wider than a 50 bar rest unless space is so tight on the system there just isn't any extra to distribute.

@dmitrio95

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

The mmrest in the example file had already been stretched up to a factor of 2

I didn't notice that, then this is indeed an expected change.

Anyway, the new weight calculation algorithm looks good to me. Could you please squash the commits here? Both of them alter MM rests layout weight so there doesn't seem to be necessary to keep them separate.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

OK, will do.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:mmrest-stretch branch from 7f42317 to ade9f8c Apr 17, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Done.

@dmitrio95 dmitrio95 merged commit d8890ad into musescore:master Apr 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.