MuseScore icon indicating copy to clipboard operation
MuseScore copied to clipboard

Harp pedalling diagrams and UI

Open miiizen opened this issue 1 year ago • 1 comments

This pull request adds harp pedalling diagrams to MuseScore for GSoC 2022. (Builds on PR #12269)

Pedal diagram demo

Known issues

  • [x] Opening via shortcut
  • [x] Stacking of diagrams around other elements
  • [ ] Placement of the popup on the score
  • [x] The arrow at the top of the UI
  • [ ] Probably some spacing within the UI

  • [x] I signed CLA
  • [x] I made sure the code in the PR follows the coding rules
  • [x] I made sure the code compiles on my machine
  • [x] I made sure there are no unnecessary changes in the code
  • [x] I made sure the title of the PR reflects the core meaning of the issue you are solving
  • [x] 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?"
  • [x] I created the test (mtest, vtest, script test) to verify the changes I made

miiizen avatar Jul 29 '22 14:07 miiizen

This PR needs a rebase too; this time all conflicts are just about the #includes.

cbjeukendrup avatar Aug 25 '22 00:08 cbjeukendrup

Hi @miiizen! We have started working on MuseScore 4.1, and we would like to include this PR. It would be great if you could rebase it. I've also posted a few review comments that I had written earlier but apparently not yet published.

cbjeukendrup avatar Mar 20 '23 13:03 cbjeukendrup

Hi @cbjeukendrup! That's great news thanks for letting me know. I'll have time to rebase and make these changes around the end of next week.

miiizen avatar Mar 20 '23 14:03 miiizen

Had a poke at the diagram to see how things are going with it. Found a few things and thought I'd share. Sorry if this is a bit premature and you were planning on asking us to test later.

https://user-images.githubusercontent.com/21022311/235263589-30a26e9d-470c-4c01-a67d-56f4bd9343e5.mp4

Tantacrul avatar Apr 28 '23 22:04 Tantacrul

@miiizen A rebase is needed again...

cbjeukendrup avatar May 06 '23 12:05 cbjeukendrup

This PR looks great (and ready to be merged), thank you so much! The only issue that it has is the broken unit tests. If you can't fix them yourself (or don't have enough time), I suggest disabling them (I'll fix them later). Or you can try rebasing this PR one more time, maybe this will help

RomanPudashkin avatar May 10 '23 16:05 RomanPudashkin

The utest failures are all the same:

               <Image>
8:                  <Image>
8: -                  <z>8099</z>
8: +                  <z>8199</z>
...

I guess embedded images got moved into a higher z-level?

And a rebase is needed

Jojo-Schmitz avatar May 10 '23 17:05 Jojo-Schmitz

Thanks for the feedback! Rebasing hasn't fixed them - afraid I won't have time to try to fix the tests til after Tuesday, feel free to have a look yourself.

miiizen avatar May 11 '23 20:05 miiizen

Fortunately, I think the test failures are no mystery at all. The default z index of an element is defined as its ElementType casted to int multiplied by 100. Because the HARP_DIAGRAM type got inserted, the default z-index for images was incremented (because the IMAGE element type comes after HARP_DIAGRAM).

The reason that the z index is written to the file in those specific files where the tests fail, is that it is "customised" in those places. And the reason that it is customised and gets changed while reading/writing the score, is BSymbol::add (and Image is derived from BSymbol). What happens is that when reading the score, the z index of the inner image gets set to the z index of the outer image minus one, regardless of what's written in the file. The z index of the outer image was not customised, so will use the new default value. Therefore the z index of the inner image will be changed too.

So the fix would be to just update the contents of src/engraving/tests/parts_data/part-image.mscx and src/engraving/tests/parts_data/part-image-parts.mscx according to https://github.com/musescore/MuseScore/actions/runs/4952108865/jobs/8857989773?pr=12597#step:9:3323

(Of course, this behaviour of changing z index while reading is very questionable, but that's a separate problem.)

cbjeukendrup avatar May 11 '23 23:05 cbjeukendrup