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

allow autoplaced elements to cross into skyline #4982

Merged
merged 25 commits into from May 13, 2019

Conversation

@MarcSabatella
Copy link
Contributor

MarcSabatella commented May 1, 2019

I'm calling this a work in progress because I definitely need to test more, and actually I think I can simplify this further. But so far I'm excited by the results; this really feels good to me.

The basic idea is that I am allowing elements to be manually moved into the skyline - including into the staff. Without disabling autoplace, so they still get added to the skyline and thus other elements continue to avoid them. I started off thinking I could only do this for fingering, then I saw how to do it for most other elements.

The code as I have it uses a property to control whether elements allow this, mostly because I wasn't seeing how to preserve manual adjustments in existing files. But now that I've refined the algorithm I use to calculate the placement, I'm thinking I may be able to drop that and just do this by default. I will be testing that theory tomorrow. If so, I can eliminate all the code managing the property, but we'll see.

Meanwhile, if anyone wants to try this branch just to get a sense of the behavior, be my guest.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:278999-customize-autoplace branch from 8ef4d23 to 1157dd0 May 2, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 2, 2019

Indeed, I am pleased to confirm no longer need the ABS_OFFSET property - the new behavior is compatible with the old. So I'm down a very small change that I think will have an enormous impact.

One thing I'd like to look at next is applying this to lyrics, or even just allowing manual adjustment to offset to work at all - right now it doesn't. The alignment algorithm always confused me before, but I think I understand enough about how things work now that I have a decent chance of improving this too using a similar approach.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 2, 2019

Hairpins were among the few elements that were not picking up my changes, that is fixed now.

Also I add some code to cmdFlip() that I now realize we really should have had all along. If an element defaults to above the staff, and you manually move it down, element is obviously now closer to the staff than the default. You probably want that same end result - element closer to staff than default - if you then flip the element below. Previously we were just keeping any user offset unchanged which made no sense at all. With the old offset, the only thing keeping the element from colliding with the staff upon flipping was the skyline. So you might have an element you had moved further above the staff than the default, then on flipping it below, it's now pressed right up against the staff.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 2, 2019

Got lyrics working too, which is especially gratifying because currently you can't do manual adjustment of lyrics at all without disabling autoplace. So, with this change you can not only make fine adjustments without losing autoplace, but you can drag lyrics pretty much anywhere you like.

I think I figured out how to get a GIF in here (drag & drop):

UCuRXYDey6

@dmitrio95

This comment has been minimized.

Copy link
Contributor

dmitrio95 commented May 2, 2019

The idea of this change is very interesting, but it is still not fully compatible with the old behavior and has its own drawbacks.

As far as I understand, the idea of the change is the following.

The current element coordinates calculation of the autoplaced element can be represented approximately like
p = autoplace( pbase + poffset )
where by autoplace I called a function which maps element's coordinates before doing the autoplacement to the final coordinates, p = (x, y).

This PR changes the formula to
p = autoplace( pbase + (xoffset, ystyle) ) + (0, yextra)
where yextra is calculated as
yextra = yoffset - ystyle

That yextra becomes the offset not from the base position but from the autoplaced position of this element if it has yoffset = ystyle.

Of course, having an yextra not affected by the autoplacement algorithm should allow doing absolutely anything to the final element's position but the current scheme of calculating yextra as well as the position that is passed to the autoplacement algorithm has some downsides:

1) It changes the meaning of Y offset and leads to different results when it probably shouldn't.

For example, see the following example of different staff texts with different Y offsets assigned:
Current master:
master
This pull request:
pr

What happens is probably that all these texts have an obstacle if they have the default Y offset. This makes them all be shifted by the autoplacement algorithm with this change. The old behavior causes them shift only if their actual "unautoplaced" position causes an element have intersection with a skyline.

In the same way, if the note under "-4sp" text gets moved up this gap between the text element and the note is preserved.

Actually there are probably exactly two cases when the full compatibility remains:

  • An element is not shifted by autoplacement with the default Y position;
  • An element has Y offset value that equals to the style default.

If both these conditions are violated then the behavior will differ. It looks like it shouldn't be a very rare case.

2) It makes the meaning of Y offset less clear.

As I noted above, this change makes Y offset be split into the "styled" and "extra" components which behave entirely differently. As this is not denoted by the interface, and that yextra value cannot actually be seen it may cause a confusion in understanding what exactly an Y offset value means.

Possible solutions

Overall, these changes are really useful, but if the appearance of scores created in older MuseScore versions should not be changed, or the old meaning of offset value turns out to be more understandable then this idea may need some more tweaks. Actually I don't currently see any solution that would be clearly good here but I hope something can be found to make it real. But some options I can see are:

  • Have yextra offset as a separate property
  • Have a flag enabling the new behavior (that is, return ABS_OFFSET property back)
  • Trigger the new behavior only in case the "unautoplaced" position (ybase + yoffset) causes the element to overlap a skyline (this doesn't address the second issue I mentioned but perhaps it is not as bad actually…)
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 2, 2019

Thanks, you are right about the compatibility issue. I did see similar issues at first, which is why I originally said I couldn't manage to preserve manual offsets from 3.0. That was with a slightly different algorithm that failed in more cases than this. Then I hit on the current algorithm which actually does yield the same results in quite a few cases that my first attempt did not.

Returning to use of ABS_OFFSET property is possible for sure, although I'm also intrigued by the third possibility you raised.

Regarding the meaning of Y offset being less clear in my implementation, though, I am not so sure I agree. I don't think it was very clear before either. After all, look at your example, how you explain to the average person why those first two notes - with offset of -2sp and -3sp - have essentially the same actual vertical position in MuseScore 3.0.5? That's why I called the property I added "absolute offset" - it means increasing the offset always moves an element down, decreasing it always moves an element down. None of this business of hitting the spin box in the Inspector and seeing nothing happen until the offset "catches up" to the autoplaced position, which is what would happen if you start increasing the Y offset of that first "2sp (style)" text. Or having the offset start affecting the element but then having it continue to accumulate even though you've hit the "wall" of the skyline. But indeed, it's not clear what default this offset is actually being applied to, and I get that this too is a concern.

Another issue with 3.0.5, though, is that there were actually a couple of different ways that pos & offset were managed. Aside from lyrics, text elements by default always showed the offset defined by the text style - any autoplace adjustment was incorporated into "pos" unbeknownst to the user. But for lyrics, "pos" never changed; autoplace incorporated its adjustments into offset instead.

The bottom line is that there really are three parameters here: the style default, the autoplace adjustment, and user override. Trying to manage this with only "pos" and "offset" is a compromise. But, I will continue to ponder this.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 4, 2019

I have it working now with hopefully full compatibility, going back to the approach of using a property to switch the behavior but also being more careful to handle each of the different cases. It was a lot trickier than I hoped, but I'm happy with what I came up with. Right now it's just implemented for autoplaceSegmentElement(), I want to refactor the relevant code to make it easier to use in the other places, but that will have to wait for tomorrow.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 6, 2019

In the process of refactoring I hit upon a simpler and more natural way to represent the algorithm I am using, so now I'm reworking my code. I have added a MIN_DISTANCE property to each Element that can override the style. So, just as with the style property, a large negative value (eg, -999) allows completely free positioning, but also, a smaller negative value (eg, -2) allows intrusion into the skyline while still "floating" up and down with changes to the skyline. Right now I'm managing the property automatically but it's also possible to expose it in the Inspector.

This approach maintains compatibility but also provides a ton of flexibility and just feels like a natural extension of what we have.

I should have an update soon.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 6, 2019

OK, I see the merge conflict, stand by...

@Jojo-Schmitz

This comment has been minimized.

Copy link
Contributor

Jojo-Schmitz commented May 6, 2019

Probably my #4989's fault. Try to not lose the reset buttons' accessibilty texts

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:278999-customize-autoplace branch from 164743c to fab7b0f May 6, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 6, 2019

It should build now, and I will guess the tests will pass unless there is an explicit test that tries to move an element into the skyline, in which case you'll get see the new minDistance tag.

The version pushed is still just for autoplaceSegmentElement; I have yet to finish refactoring and cleaning up. I pushed this now in part to see if the tests pass, also in case anyone wants to comment on the direction.

In order to get something that worked compatibly, the code is necessarily more complicated, although most of the changes here just managing the new MIN_DISTANCE property. The actual changes to the autoplace algorithm itself are very localized - just some added code to handle moving elements into and within the skyline, which things that simply weren't allowed before.

In this version, the basic autoplace algorithm is unchanged, so it should yield exactly the same results for existing scores. The only time you should see a change is if you explicitly move a segment element (eg, staff text) into the skyline, or if you use the new "Minimum distance" setting in the Inspector.

The minimum distance setting provides an override to the style value in the same way as any other Inspector setting. So, staff text has a default minDistance of 0.5sp as per Format / Style / Staff Text, but if you can increase or decrease it here in the Inspector, and also use the "Set as style" button to update the style - meaning we will now have a UI to update all those new min distance style settings I added earlier. I say "will now have" because again, so far it's only implemented for "segment elements".

Access to the min distance setting in the Inspector makes what is going on more transparent, but I don't expect most users will mess with this. The real magic is as in my previous versions: simply moving an element into the skyline - partially or completely - will "just work". The Inspector now shows you exactly what is going on: both offset and min distance are updated automatically as you move the element. The meaning of these parameters is exactly as before - offset is measured relative to the staff, min distance defines how much separation (positive) or overlap (negative) is allowed with respect to the skyline.

If you move an element into the skyline but still outside the staff, I assume you are just changing the relative position and thus will still "float" it up and down with the skyline (eg, if you just want to move one element below another but still want them to move together). But once you move an element completely onto the staff, I assume you want to leave it there. I call this "magic", but there is nothing going on but updating of offset and min distance as you move things within the skyline, and all of it is in plain sight in the Inspector.

Along the way, I have also fixed the rather annoying glitches that happen when dragging an autoplaced elements - it would often end up jumping to a different location on release, because the offset is measured relative to a different default. And a few other related issues. If/when this gets merged, I'll find any appropriate issues and close them manually.

Again, I still need to refactor this to make it work for things other than segment elements. Lyrics and fingering currently work rather differently from other element types in terms of the interpretation of pos & offset, and that will probably need to continue to be the case.

I'm quite satisfied with how this feels in practice, and how the use of min distance feels like a natural extension of what we already have. That said, I recognize that scores that take advantage of this new facility will look different loaded into earlier versions of MuseScore. To me, it is totally worth it. As I said earlier, I think this will be a "game changer" in terms of turning around people's impressions of automatic placement. But since there will be the compatibility issue in that direction, we do have to think about the timing of it. To paraphrase a Chinese proverb about planting trees, the best time to have implemented this would have been six months ago. The second best time is now :-)

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:278999-customize-autoplace branch from 403814e to b607e91 May 7, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 7, 2019

With this last update, I believe I have everything covered. Well, aside from handling of a couple of line types I can't fix until #4894 is merged, but I do believe I made it so the PR's are independently mergeable.

This is a bigger PR than what I started with (and took a lot more effort to get right!), but there's actually not as much to understand as it may seem at first. Here is a summary of what is going on in the code:

  1. The most important change is that there is now a Pid::MIN_DISTANCE property for autoplaced elements. Setting this to negative values (while leaving the style value at the default) is what allows elements into the skyline. This property is managed using the normal style property mechanism, which requires the ElementStyle data structure for each element type to associate the Pid with the corresponding Sid, and it is exposed as for other properties in the Inspector. This change single-handedly makes everything possible. The rest is just to make the user experience more seamless.

  2. A new _offsetChanged member of Element class (with get/set functions) so we can tell when an element has just been moved. Along with this is the _changedOffset member, which is used to make sure when you drag or move an element, it really ends up where you wanted, and doesn't jump on release like it often does currently. And the rebaseOffset() function that actually does the fixup. Each of the various autoplace functions now uses these to make offset changes more reliable.

  3. A new function rebaseMinDistance() that does the work of figuring out what change to MIN_DISTANCE and/or OFFSET is required in order to achieve the desired position. Each autoplace function now calls this just before actually modifying the position of an element.

  4. Miscellaneous improvements to layout of fingering and textlines, behavior of the flip command, and changes to align with those in PR #4894 to hopefully avoid merge conflicts.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 8, 2019

I've removed the "work in progress" tag - after more testing and one small bug fix, I think it's now ready for review again. I really took @dmitrio95 comments to heart and addressed both the compatibility issue and the change in interpretation of "offset". The price is more complexity in the code, but the added benefit is more transparency and more control for the user, including the ability to override minDistance for any element and a way to set the style defaults for the new minDistance settings I added recently.

Here's an image showing the new rendering of the example that had been used in discussing compatibility:

image

And another GIF showing more of what this PR allows:

5k09UW2ChD

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 8, 2019

Not sure what the test issue is, from the log it seems maybe the script hung, but I'm not sure. Not sure I created the thing correctly. Advice?

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 9, 2019

Never mind, I see the script is trying to init from my home folder :-). Will upload fix soon.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:278999-customize-autoplace branch from a12c5df to dbc8ce7 May 9, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 9, 2019

The test failure is the script I created, but it's failing due to a 0.04sp discrepancy in either offset or minDistance (I tried two different versions of the script, both were off by 0.04sp). It's such an exact and consistent difference, doesn't seem like floating point roundoff or differences in how Windows vs Linux renders fonts or anything like that. It's half the thickness of a staff line, wonder if somehow there is some dependency there in temrs of how skyline is calculated? Anyhow, will investigate more tomorrow.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 9, 2019

My working theory is that somewhere there is a calculation involving the stafflines position/bbox that is under a #ifndef NDEBUG and results in the skyline being different by half the staff line width depending on that flag. This would explain what is going on. Looking...

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 9, 2019

I haven't completely ruled this out, but a new clue (the same 0.04sp discrepancy in pos depending on the specific text I add, even without my code being involved) gives me different insight - it may be something about font rendering after all. Still looking.

SysStaff* ss = m->system()->staff(si);
QRectF r = bbox().translated(m->pos() + pos());
qreal yOff = offset().y() - propertyDefault(Pid::OFFSET).toPointF().y();

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella May 9, 2019

Author Contributor

an oversight, forgot to remove these lines, will submit fix!

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 10, 2019

It was making me uncomfortable to leave out textline, letring, and vibrato pending PR #4894, so I went ahead and incorporated the code I needed from that PR into this one (plenty left in that PR that should be merged too, though!)

I also enhanced rebaseOffset() to do detect when an element is moved to the opposite side of a staff and flip its placement accordingly. It works beautifully with my existing code to rebase offsets and minDistance. To be safe, I do this only if the PLACEMENT property is a styled/unstyled (as opposed to nostyle) property. There are a few element types that honor placement but not via style, but I don't think any of them actually support automatic placement, so this should cover everything. If not, it's easy enough to add the property to the elementstyle.

Finally, a small fix, I had forgotten to remove a vestige of my original implementation (the "Yextra" algorithm) from autoplaceMeasureElement(), which is used for repeat text. My code was still working bit would have suffered the same compatibility issues. That's taken care of now.

@MarcSabatella MarcSabatella force-pushed the MarcSabatella:278999-customize-autoplace branch from 16ea699 to 4298b0a May 10, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 10, 2019

Needless to say, I plan to squash all this if/when we're ready for merge. But at least some of this history seems useful to have around meanwhile.

BTW, I've taken the liberty of making builds of this PR available to cadiz1 and geetar - the ones who really instigated this work by pleading for it for string numbers specifically. Also to Ziya Mete Demircan, who was one of the strong advocates for a "first let autoplace position things, then let me adjust freely" type of approach in the long issue thread on disabling autoplace completely - https://musescore.org/en/node/278999. All are people whose opinions I respect greatly. They have been helpful in pointing out things that needed work (and their feedback is now incorporated), and their enthusiastic response has been gratifying. I may continue sharing builds with selected users while this is under review.

@anatoly-os

This comment has been minimized.

Copy link
Contributor

anatoly-os commented May 13, 2019

I suggest leaving all the commits as they are to keep the history of such groundbreaking changes.

@anatoly-os anatoly-os merged commit 66b30c3 into musescore:master May 13, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Copy link
Contributor

dmitrio95 left a comment

Sorry for the late review but there are still some issues regarding this PR that worth discussing.

The new solution of this problem is indeed seems great considering maintaining a compatibility with old scores and giving a possibility to a user to obtain much more control on the autoplacement procedure results.

However there are still some functional issues and usability concerns with this PR that need to be addressed in current master. I'll try to list them below.

Functional issues

  1. Place a fermata on a note, drag it below the staff. Result: fermata is not flipped, its placement is not changed.
    The same issue goes for some other types of elements, for example slurs or lyrics, for slurs it may be not a big issue though.

  2. Placement of hairpins is not flipped if a hairpin has more than one segment (that is, it spans for more than one system). The same seems to be true for other spanners too (trill lines, for example).

  3. Place an accent to a note, drag it onto a staff or to the other side of the staff. Result: the accent marking gets pushed to the original side of the staff, just like it was before this PR.

Possible usability issues

All the issues below are debatable and may be OK to exist like they are but I would like to point on them and discuss some possible usability concerns here.

  1. Once MIN_DISTANCE is reduced it cannot go larger (except for undoing and manual inspector adjusments).
    This is OK while the element still resides inside a staff, but consider the following scenario:
  • Drag a staff text so that it intersects a staff
  • Drag it to the position near the staff but outside of it (maybe just a bit adjusting its position after the previous dragging, for example)
  • Change some note's pitch in the same segment to make it be placed outside a staff.
    Results of the step 3 will depend on whether we execute the step 1 or not. Step 2 alone and step 1+step 2 have seemingly the same result but they set MIN_DISTANCE property to different values (-999 vs. some value that is close to zero) which results in autoplacement giving completely different results. My concern here is that this may not be an expected behavior.
  1. Element's position after flipping placement may (or may not) be not what was intended by a user.
    As an example, if user drags a fermata to the other side of the staff (say, below the staff), at least the following positioning options are available:
    2.1) Put it just where user released a mouse
    2.2) Put to the style default position for fermatas placed below
    2.3) Put is to the style default position if user released a mouse close enough to it (though it is not clear what is enough here).

This pull request implements the option (2.1) but it may happen that more often the desired effect is (2.2) or (2.3). The default positions are good in that they are properly aligned both with notes positions (x value) and other similar elements in the same staff (y value) so it might be better just to leave the element in this position in case of flipping and let the user to adjust the position later if needed. This is the approach I took in my experimental patch with flipping elements by mouse dragging (dmitrio95@48b26ce).

However I am not 100% sure which option would be better here, maybe some more general approach with snapping to some sensible positions while dragging an element may happen to be even better (though it can make position fine-tuning via a mouse less usable sometimes).

Other notes

I left a few comments within a code pointing to possible issues. The main issue related to the code itself is probably relying on placement property to determine the actual element's placement: as I described in inline comments, it does not currently work in many cases.

Still, other than that, I wasn't able to find any serious issues with this patch. So, overall, while there are still some issues with certain element types, this is a really great improvement to the autoplacement system. Thanks for the great work @MarcSabatella!

p.rx() = 0.0;
bool saveChangedValue = _offsetChanged;

if (staff() && propertyFlags(Pid::PLACEMENT) != PropertyFlags::NOSTYLE) {

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 May 14, 2019

Contributor

That is exactly the reason why I went for reusing Scrore::flip() code in dmitrio95@48b26ce and not trying to deal with Pid::PLACEMENT directly: currently Pid::PLACEMENT is not really the only property responsible for the element's placement above or below a staff. Score::flip() flips placement for some elements, direction for others or even something more exotic like anchor for articulations. Dealing with PLACEMENT directly leads to flipping not working for some elements like articulations or slurs.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella May 14, 2019

Author Contributor

Right, this solution only works for the cases it works for, but it does so easily so I thought it worth doing. If you would like to continue your work on the other branch I can certainly revert this. Or you could use your code only for the cases that mine doesn't handle. Or maybe I can call your code from here.

}
}

if (offsetChanged() > 0) {

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 May 14, 2019

Contributor

It would probably be better to use an enumeration for offsetChanged so the meaning of checks like this one would be easier to understand.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella May 14, 2019

Author Contributor

Agreed, I was playing with so many different solutions here I wasn't sure I was going to use this at all, now that it is settled, an enum makes sense. I'll add one.

if (flipped && !multi) {
off.ry() += above ? -staffHeight : staffHeight;
undoChangeProperty(Pid::OFFSET, off + p);
_offsetChanged = saveChangedValue;

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 May 14, 2019

Contributor

Does _offsetChanged get restored always correctly here? It may have negative values while saveChangedValue is a boolean variable and will convert to int as either 0 or 1.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella May 14, 2019

Author Contributor

Ouch, definitely a mistake! Will fix. I'm sort of thinking it is actually correct for it to always restore positive, since offset adjustment performed here may be more consistent with that. Need to think that through more.

if (offsetChanged()) {
// user moved element within the skyline
// we may need to adjust minDistance, yd, and/or offset
bool inStaff = placeAbove() ? r.bottom() + rebase > 0.0 : r.top() + rebase < staff()->height();

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 May 14, 2019

Contributor

As I said above, placement property (which is what is checked by placeAbove() function) cannot be often used as a reliable source of the information on the element's placement. So either placement property handling or this code (and similar code in other places) probably needs some adjustments in order to make it work always correctly.

Also height of the staff may differ within a score, it looks like querying the relevant SysStaff for the actual staff height should give a better information (although probably not fully reliable either).

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella May 14, 2019

Author Contributor

While the placement property might not be sufficient to determine position relative to the staff, it is how autoplace normally works - we rely on this many places. In particular, it's what we use to determine which skyline to try to avoid, when calculating yd. So I'm not sure this code needs to be adjusted because of that, but maybe you can find a specific case?

Accounting for height correctly is good indeed, I'll do that.

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella May 14, 2019

Author Contributor

Actually, regarding height - the layout code for below placement (for all elements I could find that honor it) also just uses plain staff()->height(), so elements placed below staff are already placed incorrectly if the height changes mid-score. I see there is a TODO in Staff::height() to use tick, at some point someone would go in and deal with that. But I'm inclined to leave my code here alone, then, for consistency, and to make it easier to update all Staff::height() calls together when the time comes.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 14, 2019

Thanks for the feedback! I will respond to points individually as appropriate.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 14, 2019

I'll try to respond point by point:

Functional issues:

  1. I don't see a good way to check if an element honors placement at all (maybe there is and I missed it?) so I settled for checking if it's styled. This does mean we miss fermatas, which I guess need to be flipped anyhow.

  2. Spanners have but one placement property for all segments, so I didn't think it made sense to flip it given only segment is probably being moved. That's why I limited this to spanners of a single segment.

  3. Yes, autoplace of articulations happens differently. I can look to see about adding similar support, but I'm not sure how feasible it will be given all the special casing we do.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 14, 2019

Usability issues:

  1. Not adjusting minDistance when moving away from the staff was a deliberate choice, the idea being to only adjust it when necessary. I figured people can still adjust minDistance manually via the Inspector.
    But geetar made the same observation, and I do have commented-out code in place that enables this behavior. I tested it and it works well, I just wasn't sure it made sense. But at this point, since I'm of two minds, and both you and getter mentioned it, that seems like the vote is 2.5 to 0.5 in favor of changing it :-), so I'll uncomment the lines.

  2. True, and to be honest, it's easier to make it work that way. But given it seems there are other things to yet to be done with respect to to auto-flip behavior, I'm not going to worry about that right now.

@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 14, 2019

Thanks again for the thorough review! I'll submit a PR later today addressing the enum, saveOffsetChanged, staff height, and the rebasing of min distance when moving away from the staff.

The automatic flip behavior I consider more "optional" - it's nice to have, but it can be tweaked later with no compatibility issues. So while to me it would be very important to have the basic min distance support in 3.1 (see below), further improvements to our ability to automatically detect opportunities to flip elements could come at any time. Speaking of which, we have adjustPlacement() in read206 to do this for 2.x scores on read, could be interesting to do something similar for 3.0 scores as well.

See also my PR #5017, which addresses a lyrics compatibility issue I discovered in recent testing.

Additional thoughts:

@anatoly-os mentioned in chat yesterday the possibility of holding off on this until 3.1.1. While I understand why that might seem like a good idea, to me the nature of the change makes it seem inappropriate for a "point" release. So I really do hope to do all I can to resolve concerns and make sure this goes into 3.1.

That said, as I mentioned then, there is a fallback plan to consider, where we basically include this code in 3.1 but simply disable rebaseMinDistance() (or hide it behind a preference), so that people don't "accidentally" take advantage of it before we're ready. The advantage is that at least with the support for Pid::MIN_DISTANCE in place, at least it would be the case that scores created in 3.1.1 would open correctly in 3.1.

But again, if it were up to me, I'd say 3.1 should have this in full. We know the basic approach works, and if there are bugs they can be fixed in 3.1.1.

@MarcSabatella MarcSabatella mentioned this pull request May 14, 2019
@MarcSabatella

This comment has been minimized.

Copy link
Contributor Author

MarcSabatella commented May 14, 2019

See PR #5018, which contains updates for the changes discussed, including the ability to freely drag articulations (which required only a little extra shuffling in the code). No auto-flip (same issues as fermata) but could be added later. Articulation layout is a bit special because it's tied to the chord separately from the skyline, but anyhow, it works.

Also, the change to use tick-dependent staff height in calculations I put off for now, since as mentioned above, plain staff()->height() is how we handle placement anywhere else as well. I also put off any further tweaking of the auto-flip facility.

Regarding the automatic flipping of placement in general - I do still think it generally preferable to have drag be interpreted literally, meaning if you drag an element to a particular location, it should end up there, not snap somewhere else. After all, if you want it snapped to a default location, you could use "X". Of course, one reason people don't do this is they don't know about it - but now we have a perfect place to hook in a tour. Still, again, it's something that's easy enough to revisit later based on user feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.