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 #307623: Y offsets change when stave space is changed #6315

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

mattmcclinch
Copy link
Contributor

Resolves: https://musescore.org/node/307623.

This reverts #5699 and calls Score::spatiumChanged() from within ChangeStyleVal::flip(). Not only does it now do the right thing on undo, but it solves the rest of the problem that was introduced recently.

@MarcSabatella
Copy link
Contributor

Looking forward to testing this, thanks for following up! I see how this address the undo issue nicely, but right now I’m not understanding how this actually fixes the problem at hand, does it not still result in calls to Element::spatiumChanged just as before?

@MarcSabatella
Copy link
Contributor

Oh wait, I think I know - the issue was only with offsets of styled elements, probably we were updating the offset before the style.

@dmitrio95
Copy link
Contributor

I guess other usages of undoChangeStyleVal(Sid::spatium, ...) should also be revisited to remove explicit calls to spatiumChanged(). The only other case of changing spatium I was able to find is this line introduced in #6065, it would probably be better to remove spatiumChanged() there as a part of this PR as well.

@mattmcclinch
Copy link
Contributor Author

mattmcclinch commented Jul 9, 2020

This PR has been updated to address @dmitrio95's review. Deleting that one line allowed me to remove the entire switch statement around it. It is not necessary to guard against newSpatium being the same as oldSpatium, because if they are the same value, ChangeStyleVal::flip() will not do anything. This does remove the guard against changing the spatium to a negative value, but it seems out of place to leave that in, since no other similar checks are in place for other Sids.

@mattmcclinch
Copy link
Contributor Author

mattmcclinch commented Jul 9, 2020

My latest push adds back a comment that I had removed by mistake when removing the switch statement. Or rather, it leaves the comment untouched.

@MarcSabatella
Copy link
Contributor

Just tested, seems to work as advertised, nice job! In my investigation yesterday, though, I found a few things we still aren't scaling properly. Line thickness, for instance (whether styled or not). Also offsets on slurs & ties. I'm assuming this means a few element-specific spatiumChanged() functions are missing some stuff. And like I said, a few are missing corresponding locaSpatiumChanged() functions (or have them but they don't do the job properly.

I'd recommend merging this as is but then maybe addressing those other more specific issues in a separate PR (which I'm playing around with now).

MarcSabatella added a commit to MarcSabatella/MuseScore that referenced this pull request Jul 9, 2020
Fixes a few cases where scaling still is not performed properly
because an individual element is not scaling itself properly
or its parent is scaling it multiple times (ties).
In the case of pedal, the scaling problem happens on add,
because the line width was never listed as a styled property.
@MarcSabatella
Copy link
Contributor

See #6318. As mentioned, that PR can either be merged after this one, or the commit can be incorporated into this PR, it doesn't matter to me).

I didn't test every single possibility to see what else might be missing, but nothing else I tried had problems. Except, as mentioned, with respect to staff as opposed to score scaling. Several other elements are not handling localSpatiumChanged properly (or at all). I will let someone else worry about that, though.

@anatoly-os anatoly-os merged commit 1068a35 into musescore:3.x Jul 9, 2020
anatoly-os added a commit that referenced this pull request Jul 9, 2020
Fix #307623: Y offsets change when stave space is changed
anatoly-os added a commit that referenced this pull request Jul 9, 2020
anatoly-os added a commit that referenced this pull request Jul 9, 2020
anatoly-os added a commit that referenced this pull request Aug 7, 2020
@Eism Eism mentioned this pull request Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants