Skip to content

Improved collision management between beams, beamlets and rests #16410

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

Closed
wants to merge 4 commits into from

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented Feb 17, 2023

Resolves: #15301

The general purpose of the PR is to improve the collision management between beams, beamlets and rests. This involves two separate jobs:

  1. Move rests vertically to clear collisions with beams (yes, rests should move, not beams).
  2. Move chords/rests horizontally to give appropriate padding to beamlets-VS-beamlets and beamlets-VS-rests situations.

There is a basic underlying requirement to both taks, that is to improve the Shape of beams. At the moment, Beam::shape() returns the bare bounding box, which is a very rough approximation of the actual shape and isn't good enough for what we need to do. This requires to:
a. Promote BeamSegment to being an actual engraving item.
b. Implement BeamSegment::shape() to return a good approximation of the actual shape. If the beam is horizontal this is just a single rectangle. If not, it is broken into multiple rectangles.
c. Implement Beam::shape() as a collection of BeamSegment::shape().

Here's how the shapes were done before:
image
Here's how they are done now:
image

Having taken care of this prerequisite, moving rests vertically to clear beams is rather straightforward and happens in LayoutBeams::verticalAdjustBeamedRests.

The horizontal spacing is a trickier issue. It requires to
a. Let the Chord know when it has a beamlet.
b. Add the beamlet to the chord shape.
c. Add appropriate paddings.

BEFORE:
image

AFTER:
image

NOTE: this PR does not address all the requirements of multil-voice situations. That will be a separate job, also coming soon.

@its-not-nice its-not-nice added the vtests This PR produces approved changes to vtest results label Feb 17, 2023
@igorkorsukov
Copy link
Contributor

About vtests, please see https://github.com/musescore/MuseScore/wiki/Visual-Tests

@cbjeukendrup
Copy link
Member

@igorkorsukov See also my comment at #16237 (comment); after updating the reference files, you can't see the diff visually anymore, right?

And there is another thing that's regrettable about the new approach: when a new vtest is added, there are no reference files for this test, so you can't see the diff for it. With the old approach this was possible.

@igorkorsukov
Copy link
Contributor

igorkorsukov commented Feb 17, 2023

@igorkorsukov See also my comment at #16237 (comment); after updating the reference files, you can't see the diff visually anymore, right?

And there is another thing that's regrettable about the new approach: when a new vtest is added, there are no reference files for this test, so you can't see the diff for it. With the old approach this was possible.

from #16237

@igorkorsukov I'm afraid I see a problem:
So now you need to update the reference files when the tests are changed (that's annoying, but not the problem I mean).

But when the reference files are updated, the test on CI will pass without generating diff images, because there is no diff anymore then. So, once you have updated the reference files, you can't see anymore what has changed relative to the master branch. That's very inconvenient. Is there a solution for this?

The purpose of the VTests was namely not only automatically detecting changes, but also visually (not automatically) determining whether the changes are correct, by looking not only at the new situation, but also at the diff. It is important that we can do that.

(Also, it's regrettable that the "GIF" diffs are gone now, because those were actually the most useful in some cases. But it looks like those can be added back relatively easily.)

  1. I think it may be the way as in this PR, we saw that the tests failed, i.e. there are visual changes and, accordingly, there is an archive with these changes.
    With the next commit, we update the references. It turns out that we have both an archive with changes and updated references.
    I understand that if we break the order of actions, then everything will get confused.
    Although probably on the CI we can compare with the references from the master to always see the difference. But then there may be problems with updating references in the current branch, they may be incorrectly updated or not updated at all. To solve this problem, we can make two comparisons, with references from the master and in the current branch, so that on the one hand we can always see the difference, on the other hand, make sure that the references are updated.

  2. I can add GIF, it's easy

  3. Yes, there is no reference when adding a new test, it is inconvenient, I agree (of course it can be added if you have a binary with which we will compare, we can somehow simplify it, automate it)

If there are a lot of problems and difficulties with the new approach, I will return the old one when two binaries are on CI, two data set is generated and compared. Let's try, get some experience and then decide whether to return it back or not.

The new approach has its advantages, the main point of the changes was to make more efficient work with the test locally, during development. At the same time, to be able to check what has changed on thousands of files (where they will be taken is a separate topic). This is important because we plan to start making big changes to the engraving engine this year, in order not to break anything and keep it under control, we need to keep its current work (not the implementation, but the result of the work).

@mike-spa
Copy link
Contributor Author

mike-spa commented Mar 9, 2023

Closing as included in the bigger work of #16719

@mike-spa mike-spa closed this Mar 9, 2023
@mike-spa mike-spa deleted the restsClearBeams branch February 17, 2025 10:18
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.

[MU4 Issue] Beams Collide With Rests When Beams Are Over Rests
4 participants