fix #280422: align hairpin and dynamics #4507
Conversation
|
What happens if there is a long chain of dynamic->hairpin->dynamic->hairpin? I guess I should just test this myself... |
|
so for case of dyanmic->hairpin->dynamic, works nicely if move the start and end dynamic. However, it I drag the hairpin up, it causes everything to move down, which I think is unexpected behavior:
I would expect the behavior here to be the same as trying to move the dynamics up...which is nothing happens (as autoplacement deliberately and smartly doesn't put the dynamics overlapping the staff). So why does dragging the hairpin behave differently? |
|
Also to answer my original question about what happens for extended chains of dynamic->hairpin->dynamic->hairpin->dynamic, it seems that it doesn't work ok when moving this final dynamic:
Moving the prior dynamics behave as I expect this PR would want it to behave, but not when moving the final dynamic. |
|
Similar behavior which I think isn't optimal when moving the final hairpin of the chain:
For comparison, I like how moving the earlier dynamics and hairpins does affect everything in the chain:
|
|
I realize that this PR is intended to deal with doing scores without any manual placement....so here let me illustrate that the chain problem still manifests itself without doing manual positioning, for instance if I move a note later in the chain down, it only pushes the stuff later in the chain down but I think should also cause stuff earlier in the chain to be moved down so is at same level:
Also even more undesirably, if I then move a note earlier in the chain lower, then the undesired unevenness will maintain itself, even though it would be possible to have completely horizontal chain:
|
libmscore/hairpin.cpp
Outdated
| if (sd && sd->autoplace() && sd->placement() == hairpin()->placement()) | ||
| sd->rypos() = rypos() + ddiff + offset().y() - sd->offset().y(); | ||
| if (ed && ed->autoplace() && ed->placement() == hairpin()->placement()) | ||
| ed->rypos() = rypos() + ddiff + offset().y() - ed->offset().y(); |
There was a problem hiding this comment.
Just a minor note:
ed->rypos() = rypos() + ...
can be written as
ed->rypos() += ...
which is a bit shorter and (maybe) is a bit better to read. At least it is done so in other places in the code.
There was a problem hiding this comment.
not the same, though - ed->rypos() is not rypos() (ed is end dynamic, "this" is the hairpin)
There was a problem hiding this comment.
Ah, didn't notice that, sorry.
|
There is also another possible issue with this approach which is probably the reason why the similar solution was removed previously from the code (see 8bdd1af). The currently proposed solution does not update skyline on moving dynamic markings. In order to do that properly these dynamic markings were not added previously to skyline at all until that alignment (in some cases which adds a bit of complexity), otherwise it was not clear how to adjust it always correctly. Perhaps this problem does not apply fully to this solution as these markings are probably moved only further from the staff (am I right with that? What happens if dynamics has user-defined Y offset?) and are not moved horizontally so updating skylines can be done more easily. Still this question should probably be considered. |
anatoly-os
added
work in progress
|
@ericfont - good catch on the behavior with manual adjustment. I tested a few cases but not all. Similar quirks exist with voltas and other elements that are aligned with each other vertically, so I don't see this as a deal breaker, but the issue where adjusting the hairpin up has the opposite effect is a bug for sure. Really, I might prefer for manual adjustment of individual markings in the sequence to be possible without affecting the others, so you can force them to not align with the need to disable autoplace. This works for pedal segments. But it's problematic here because the elements are of different types with different default offset. So when getting a good starting height for the hairpin, it is easier to work from the manually adjusted position of the dynamics than to get the "default" position. But not impossible, and maybe that's the way to go, would solve these problems. I'll look at that. As for the skyline, good point @dmitrio95 . I did a similar thing in re-adjusting articulations after adding slurs, no problem to do the same here. And correct, we are only adjusting "outwards", so updating the skyline is not a problem. One other thing I may need to do is tie this to a style option, because if the user had created a score in the initial 3.0 release and then fixed it manually, my changes here might result in his work moving the markings too far away. Most cases actually still work correctly, but some don't (last measure in the example). That might change if I stop including the manual offsets in the equation, though. |
MarcSabatella
force-pushed
the
280422-align-dynamics
branch
from
e5b794f
to
1020194
Compare
|
I pushed an updated version. This one is improved in several areas:
It's still the case that a manual adjustment to one of the dynamic-hairpin-dynamic trio adjusts them all. Making it otherwise is trickier than I want to do deal with (because we don't store "user offset" separate from "offset" as per style setting). You can always disable autoplace. It's also still the case that a chain of dynamic-hairpin-dynamic-hairpin-dynamic won't necessarily adjust all together - things are processed left to right. Doing it in multiple passes is possible but not as straightforward as it is for just aligning lines with other lines. Frankly, I'm not sure what the right behavior even is. In some such cases at least you might be better served by manually adjusting one hairpin to angle to meet the other, and that works quite nicely. In any case, getting the basic case right is far more important to me, which is why I suggest my current code might be good enough. |
MarcSabatella
force-pushed
the
280422-align-dynamics
branch
from
1020194
to
25a6691
Compare
|
Another update, this one disables the vertical alignment if you turn on the "allow diagonal" option. This allows you to keep the other advantages of autoplace but lets you control vertical alignment, whether actually making the hairpin diagonal or not. I think it completes the picture nicely. Here is the default now in a "chain" case:
If I turn on "allow diagonal" for the last hairpin, I get this:
I can either take that as it is, or actually make the hairpin diagonal by dragging the handle:
Or I can simply adjust the individual elements individually:
|
MarcSabatella
removed
the
work in progress
|
I removed the WIP label - this is as far as I intend to take it, and I do think it's more than good enough. |
See https://musescore.org/en/node/280422
Assuming people are OK with my analysis of the desired result (autoplace aligns hairpins vertically with start and end dynamic if present), this PR should do the job. Here are the results: