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

center one-line staff #20767

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sammik
Copy link
Contributor

@sammik sammik commented Dec 30, 2023

(For some context see comments to #19086)

One line staff looks better, if it is centered. It can be done manually by setting y - offset.
This PR just makes it happen automagically, whenever user changes number of lines to one, or move it back, if user increase number of lines.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@XiaoMigros
Copy link
Contributor

Shouldnt line 143 be m_staffType->setYoffset(yOffset + Spatium((linesOld - 1) / 2))? That way it will be centered when changing from non-5 line staves too. Also, it's perhaps worth considering to center all staves and not just 1-line ones?

@sammik
Copy link
Contributor Author

sammik commented Jan 22, 2024

Shouldnt line 143 be m_staffType->setYoffset(yOffset + Spatium((linesOld - 1) / 2))? That way it will be centered when changing from non-5 line staves too. Also, it's perhaps worth considering to center all staves and not just 1-line ones?

Yes, that is the question, what is intended behaviour.

Relation between 5-lines and 1-line staff is clear, because of barlines height.

Anything else (5 : 2 : 3 : 1) is questionable (in my opinion).

@igorkorsukov igorkorsukov force-pushed the master branch 6 times, most recently from fa1f8d3 to 525a11a Compare February 14, 2024 09:08
@oktophonie oktophonie mentioned this pull request Feb 26, 2024
@RomanPudashkin RomanPudashkin requested a review from mike-spa April 22, 2024 07:12
src/engraving/dom/stafftypechange.cpp Outdated Show resolved Hide resolved

// if change to one line staff, center by setting y offset 2
if (linesNew == 1 && linesOld != 1) {
m_staffType->setYoffset(yOffset + Spatium(2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Spatium(2) is not the correct variable to use, because it returns the score-wide value of spatium. If this staff is scaled, spatium is different. Better to do something like double yCenteringOffset = 2 * spatium(); (spatium() function always returns the local spatium).
  2. The distance between staff lines can be changed too, so it's not always equal to the spatium, so even better would be double yCenteringOffset = 2 * staff()->lineDist(tick()) * spatium();
  3. You are applying the 2-space offset on top of the already existing offset. Could this lead to strange results if the user has already applied some manual offset? I think it would be safer in this case to override the user offset and just set offset = 2sp for the single line and offset = 0 for all the other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-spa thank You. Couple notes:

  1. Spatium(2) is not the correct variable to use, because it returns the score-wide value of spatium. If this staff is scaled, spatium is different. Better to do something like double yCenteringOffset = 2 * spatium(); (spatium() function always returns the local spatium).
  • I think, here no, as it is in Spatium units, so it is magnitude independent.
  1. The distance between staff lines can be changed too, so it's not always equal to the spatium, so even better would be double yCenteringOffset = 2 * staff()->lineDist(tick()) * spatium();
  • great idea! But if so, also line distance change on one line staff should recenter it, otherways something like this would happen:
LineHeight.mp4

So I fixed that too in last commit.

  1. You are applying the 2-space offset on top of the already existing offset. Could this lead to strange results if the user has already applied some manual offset? I think it would be safer in this case to override the user offset and just set offset = 2sp for the single line and offset = 0 for all the other cases.
  • Yes, I was thinking of this too, but in some situations, results may be even worse, I think.

Lets say, I have staff with offset set and want to create staff change to oneline staff . With Your proposal, offset is lost and staff is not centered. Than If I change back to 5 line staff, still wrongly offsetted. see:

New1.mp4

Oroginally, it behaves, I think better:

Old1.mp4

But Yes, in other situations, your proposal may be bit better (if previous offset is not meant as "general offset, but just like ad-hoc offset)

Changing to oneline in this situation seems to be more logical. But I am not sure, if change back is:

New2.mp4

In original, change to one line is worse, but change back may is really back to previous state:

Old2.mp4

I am not sure, if new one is better, but I aplied Your proposal, so it is done new way.
I can chage it, of course.

P.S. it seems, selection on offset staff needs improvement too :)

Have a nice day!

sammik added 4 commits April 22, 2024 12:31
- change from 1 line staff -> offset 0 Spatium
- take line distance into account, so in fact: (2 * lineHeight) Spatium
@sammik sammik force-pushed the staffTypeChange-issues-center-oneline-staff branch from 06516e4 to ab5f5f0 Compare April 22, 2024 14:20
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Apr 22, 2024
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 29, 2024
@Jojo-Schmitz

This comment was marked as outdated.

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 14, 2025
@mike-spa mike-spa closed this Jan 21, 2025
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 21, 2025
@oktophonie oktophonie reopened this Jan 21, 2025
@musescore musescore deleted a comment from mike-spa Jan 21, 2025
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