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 #306750, Fix#306751 - Chord alignment #6219

Merged
merged 1 commit into from
Jun 21, 2020

Conversation

njvdberg
Copy link
Contributor

Fix #306750 - Chord symbol alignment incorrectly forces alignment between above & below chords and across staves
Fix #306751 - Chord symbol alignment changes default position without reason

Resolves https://musescore.org/en/node/306750
Resolves https://musescore.org/en/node/306751

Extended the algorithm so it aligns chord symbols, fret diagrams or RNA symbols per staff by collecting the symbols per staff and run the alignment for every staff. When looking for the reference for the alignment, the algorithm now handles placement above and below the staff as two different cases. This solves #306750.

Removed an extra offset which was not required anymore but was a left-over of an earlier
version of the algorithm. This solves #306751.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@MarcSabatella
Copy link
Contributor

Works advertised, code looks good to me! vtest failure looks like slight diff not sure why, but perhaps a result of the change to the default?

@njvdberg njvdberg force-pushed the issue-306750-above-below branch 2 times, most recently from 0e0291b to 3c18116 Compare June 16, 2020 07:33
@njvdberg
Copy link
Contributor Author

The vtest failure is expected. It shows a difference in harmony-15 which is a test which I include in PR #6060 to test the chord alignment. This difference is the expected result of this PR in order to solve #306751.
A new reference is added.

@anatoly-os
Copy link
Contributor

@njvdberg rebase, please.

libmscore/layout.cpp Outdated Show resolved Hide resolved
…ween above & below chords and across staves

Fix #306751 - Chord symbol alignment changes default position without reason

Resolves https://musescore.org/en/node/306750
Resolves https://musescore.org/en/node/306751

Extended the algorithm so it aligns chord symbols, fret diagrams or RNA symbols per staff
by collecting the symbols per staff and run the alignment for every staff. When looking
for the reference for the alignment, the algorithm now handles placement above and below
the staff as two different cases. This solves #306750.

Removed an extra offset which was not required anymore but was a left-over of an earlier
version of the algorithm. This solves #306751.
@njvdberg njvdberg force-pushed the issue-306750-above-below branch from 3c18116 to 9f9d404 Compare June 20, 2020 18:50
@njvdberg
Copy link
Contributor Author

@anatoly-os Rebase done.

@anatoly-os anatoly-os merged commit 440e241 into musescore:3.x Jun 21, 2020
@MarcSabatella
Copy link
Contributor

Is there any progress on the issue discussed on Telgram, where disabling autoplace does not exclude chord symbols from the alignment? This still feels like a necessary change.

@njvdberg njvdberg deleted the issue-306750-above-below branch July 1, 2020 12:41
anatoly-os added a commit that referenced this pull request Aug 11, 2020
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.

4 participants