fix #3325: layout of multi-voice seconds and unisons #742
Conversation
|
Failure in 7604QDEBUG : TestMxmlIO::voiceMapper3() importMusicXml(0x35d2430, /home/travis/build/musescore/MuseScore/mtest/musicxml/io/testVoiceMapper3.xml) Related to your changes? |
|
This test crashes for me locally too, so I will be able to investigate. |
|
I still figure it's a fluke, but I did make another optimization with that last commit, and now: 16/33 Test #16: tst_benchmark .................... Passed 9.87 sec |
|
A fluke or not? you have been lucky twice then. Here are the result of tst_benchmark for the last 20 builds on Travis (just wrote an ugly python script). Yours is exceptionally low.. but ok other are quite high with no reason... |
|
If nothing else, I think we don't have to worry I made things worse. So, I want to look at dot position relative to the offset notes. Right now, if the notes on the left are dotted, the dotted appear after the notes on the right. This unfortunately not right - see BB p. 56-8. With any luck, it will be easy to adjust the code to offset the notes further to make room for dots for the simple cases at least, and I'd like to give it a shot before we merge this, since I'll have to alter some of my code to handle the dots. The remaining issues with accidentals (like the one you showed me) are less likely to require any changes to my code, so I think I'll look at those separately later. |
|
My luck ran out on the benchmark, but dots work well. |
|
Not sure you want to discuss this here or expose it on the issue tracker to get more feedback.
|
|
You're right about the inconsistent dot spacing, and that I should post to issue tracker for further feedback. I'll do that after fix this. Actually, in hindsight, I know exactly why it's too close for the single dot; I just don't understand why it's different for the double dot. Regarding measure 41, that's deliberate. It's not obvious, but these are four voices, one note per voice. I was trying to make sure I got the position right in a case where some upstem notes have dots but others don't. There are compromises to make; I'll discuss those in the thread too. |
|
I've looked at the failed test and understand why it fails, but I'm not sure what to do and could use help. The accidental position fix I made in that last commit is basically correct - the calculation of the accidental position needs to take the chord offset into account, not just the note offset. That code basically works. The test that is failing is the minWidth test in libmscore/measure. This test loads a score, does a layout, notes the minWidth of a couple of measures, then does a layout again to see if anything changed. Well, things do sometimes change on the first layout after load if the score contains manual adjustments. Not sure why, but I know this was true before my changes too. My change just made this a little worse. I'm about to commit another change with some optimizations and also some instrumentation I added while investigating, in case anyone else finds it useful. The output shows information used to calculate the position of accidentals attached to offset chords. Start up MuseScore with Promenade, look at stdout, then do a Select All to force another layout, then look at stdout again. You'll see the chord offsets change from the initial load to the Select All. This changes the accidental position, and I'm pretty sure that is what causes measure minWidth to change. |
|
I've figured this out. As mentioned, some amount of this same effect - some chords moving slightly on first layout after load of score - happens with or without my changes. I don't know all the causes, but I've found the cause in my particular case. I was using stem()->width() rather than stem->lineWidth() in my chord offset calculations, and the former is apparently not yet meaningful (I guess stems have not been laid out yet). Changing my calls to use lineWidth() instead fixes this, and the test now passes for me. So my next commit should result in passing tests and bring us closer to where I want to be. There is one last thing I'll still want to add to this PR before calling it ready to merge, and that's another fix to accidental placement. Accidentals can be placed too far from a chord if seconds exist on a downstem (true in 1.3 also). When I have that fixed, I'll post again to issue thread for comments, and then I'll consider basic chord (note/dot/accidental) layout as good as it needs to be (although there is still the stem attachment issue I mentioned in the issue thread; I guess I'll submit that separately if it doesn't already exist). |
|
Closing this to re-open a new one after rebasing (haven't figured out a way around that). |
Do not merge yet, but I am submitting the PR so we can see how the tests (and benchmark) go.