Skip to content

Fix measure number layout order #23743

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

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Conversation

miiizen
Copy link
Contributor

@miiizen miiizen commented Jul 24, 2024

Resolves: #12955
Resolves: #23760

This PR adds measure numbers to the skyline after other elements, to avoid other elements moving to fit around them. It also improves the positioning of numbers in the first bar of a system.
Screenshot 2024-07-24 at 09 38 56

@miiizen miiizen requested a review from its-not-nice July 24, 2024 08:38
@miiizen miiizen marked this pull request as draft July 24, 2024 09:59
@miiizen miiizen force-pushed the 12955-measure-nums branch from 2e1b071 to 7c1c9eb Compare July 24, 2024 10:45
@miiizen miiizen marked this pull request as ready for review July 24, 2024 10:51
@miiizen miiizen force-pushed the 12955-measure-nums branch from 7c1c9eb to 53e2ca5 Compare July 25, 2024 12:56
@its-not-nice its-not-nice added the vtests This PR produces approved changes to vtest results label Jul 25, 2024
@its-not-nice its-not-nice requested a review from mike-spa July 25, 2024 13:47
}
}

std::pair<double, double> MeasureLayout::getMeasureStartEndPos(const Measure* measure, const Segment* firstCrSeg,
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple of things:

  • I'm not a fan of using pairs for returning two values (a pair to me is almost exclusively associated with maps, whose elements are key-value pairs). I normally either create a struct to hold them, or make the function void and pass them in as references.
  • It seems to me that this function is only really used in one place, where we then take the mid position of x1 and x2. At that point, I think it would be more elegant to call this function getMeasureVisualCenter or something like that and return the center point directly, rather than returning the two values

Copy link
Contributor Author

@miiizen miiizen Jul 26, 2024

Choose a reason for hiding this comment

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

The reason I opted to return two values instead of the centre of the bar is it's use in MeasureLayout::layoutMeasureElements where the values aren't always used to calculate a centre point (MMRests, measure repeats, measure rests). I'll try to get the same points just from the centre position - I agree it'll tidy things up a lot.
If I'm unsuccessful I agree on pairs, I'll use a struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

No no, if there is a use for x1 and x2 that's different from just finding the center, then by all means let's have the function return x1 and x2. It just seemed to me that it was only used for the center but I might have just missed it

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, if the only use for x1 and x2 is to do center = 0.5 * (x1 + x2) then it makes more sense for this function to just return the center directly. But if x1 and x2 are used for other stuff, then it's fine as it is (but without using the pair for return)

@miiizen miiizen force-pushed the 12955-measure-nums branch from 53e2ca5 to 2d08541 Compare July 26, 2024 09:37
@mike-spa mike-spa merged commit 73cc000 into musescore:master Jul 26, 2024
10 of 11 checks passed
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't commit compressed files into the repository. I know other people have done it, but it's bad because:

  1. We can't see what the file really is. It could be an executable for all we know.
  2. If the file is changed later, we won't see what changed. Git will just say "binary file has changes".
  3. It's annoying when parsing these files with programs besides MuseScore.
     # Find *uncompressed* scores created in MuseScore 3:
     grep -Flr '<programVersion>3' .
    
     # Find *compressed* scores created in MuseScore 3:
     find . -type f -iname '*.mscz' | xargs -I% sh -c "unzip -p % | grep -q '<programVersion>3' && echo %"
  4. In the long run, compressed files take up more room than uncompressed files.
    • To save disk space and network bandwidth, Git stores deltas between changed files rather than the file itself. Deltas are calculated based on data rather than file paths, so even brand new files in the repository can be stored as a delta from existing data. And then Git compresses the deltas to save even more room.
    • However, if a file is already compressed then all redundancy is removed, so the file will have nothing in common with previous versions of itself or other files in the repository. In this case the delta is essentially the entire file, and Git's delta compression will be unable to compress it further.

Copy link
Contributor Author

@miiizen miiizen Jul 29, 2024

Choose a reason for hiding this comment

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

I agree with not being able to see the changes. All the vtests are compressed files as we need to test style settings too, so we'd need to make sure they work with uncompressed folders. We would also have to fix saving as an uncompressed folder, as this never works for me and saves a .mscz anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

this never works for me and saves a .mscz anyway

What if you manually delete the .mscz file extension in the Save As dialog?

image

@henkdegroot
Copy link

Not sure if I will become of big fan of this change. I do appreciate the effort, but now the measenumbers are even placed above/outside the volta's. Looks really odd, specially when this happen at the tail end of a volta line.

image

@MarcSabatella
Copy link
Contributor

Agreed, I don't think it makes sense for measure numbers to appear outside voltas. Most text also seems to make sense to appear above the measure number rather than vice versa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Measure numbers collide with other elements [MU4 Issue] Articulations will jump above measure numbers
6 participants