MuseScore icon indicating copy to clipboard operation
MuseScore copied to clipboard

Fix #7819 - adjustment to previous page-clipping fix

Open wizofaus opened this issue 1 year ago • 2 comments
trafficstars

Resolves: #7819

Had previously fixed this but code had been moved around since and additional logic around when to apply clipping added by others.

  • [x] I signed the CLA
  • [x] The title of the PR describes the problem it addresses
  • [x] Each commit's message describes its purpose and effects, and references the issue it resolves
  • [x] If changes are extensive, there is a sequence of easily reviewable commits
  • [x] The code in the PR follows the coding rules
  • [x] There are no unnecessary changes
  • [x] The code compiles and runs on my machine, preferably after each commit individually
  • [ ] I created a unit test or vtest to verify the changes I made (if applicable)

wizofaus avatar Apr 01 '24 20:04 wizofaus

I admit I didn't test during playback, can try, but I'm pretty sure the logical change is sound...

Get Outlook for Androidhttps://aka.ms/AAb9ysg


From: Casper Jeukendrup @.> Sent: Monday, April 8, 2024 6:12:14 PM To: musescore/MuseScore @.> Cc: wizofaus @.>; Author @.> Subject: Re: [musescore/MuseScore] Fix #7819 - adjustment to previous page-clipping fix (PR #22135)

@cbjeukendrup commented on this pull request.


In src/engraving/rendering/dev/paint.cpphttps://github.com/musescore/MuseScore/pull/22135#discussion_r1555401453:

@@ -129,21 +131,14 @@ void Paint::paintScore(draw::Painter* painter, Score* score, const IScoreRendere }

         // Draw page elements
  •        bool disableClipping = false;
    
  •        if (!painter->hasClipping()) {
    
  •            painter->setClipping(true);
    
  •            painter->setClipRect(pageRect);
    
  •            disableClipping = true;
    

I think I've also seen that in master sometimes

— Reply to this email directly, view it on GitHubhttps://github.com/musescore/MuseScore/pull/22135#discussion_r1555401453, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI5UAJJ6AMO2BV6W2METJ3Y4JGN5AVCNFSM6AAAAABFSEZ7NOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBVG4ZDSNJQGQ. You are receiving this because you authored the thread.Message ID: @.***>

wizofaus avatar Apr 08 '24 08:04 wizofaus

The vdiff failure here is real, it looks like anti-aliasing is not working properly. But the same is true for me when running a build from master! (actually I'd noticed that a bit recently, didn't think much of it)

wizofaus avatar May 16 '24 04:05 wizofaus

Confirmed fix is still required & rebased.

wizofaus avatar Sep 01 '24 00:09 wizofaus

I think this looks good now, @RomanPudashkin any objections?

However I'm still slightly disappointed that the initial fix of intersecting the existing clip rect with the page rect introduced a regression, because that was more elegant imo. I was wondering, would that regression be fixed by replacing https://github.com/musescore/MuseScore/blob/186c925cb4255e38b6f609868ad4049eff77c0c0/src/notation/view/abstractnotationpaintview.cpp#L1368-L1370 with

    RectF dirtyRect1 = fromLogical(oldCursorRect).adjusted(-1, -1, 2, 2);
    RectF dirtyRect2 = fromLogical(newCursorRect).adjusted(-1, -1, 2, 2);

or perhaps

    RectF dirtyRect1 = fromLogical(oldCursorRect).adjusted(-2, -2, 2, 2);
    RectF dirtyRect2 = fromLogical(newCursorRect).adjusted(-2, -2, 2, 2);

?

@RomanPudashkin which solution would you prefer, the current one or the original one?

cbjeukendrup avatar Sep 01 '24 20:09 cbjeukendrup

As far as I can determine the original fix only didn't work reliably because of some bug in Qt, or something I couldn't figure out anyway. Artifacts still do appear from time to time even with the current master (but don't really cause a problem). But it really is only to handle a situation that shouldn't really happen under normal circumstances - in fact arguably MuseScore should never try to place elements that don't fit on the page (hence if you have too many staves, it should force introduce a mid-system page-break, or force scale everything down to fit).

wizofaus avatar Sep 01 '24 20:09 wizofaus

@cbjeukendrup @wizofaus I've been playing with the playback cursor recently. My idea now is to draw it outside of the score view. Here's what it looks like: https://github.com/musescore/MuseScore/pull/24576/commits/af07099e87bb7ad31165d6af3be30b33ec33f8d0

This decently reduces CPU/GPU load during playback and we also don't have to increase the redraw area to fix this issue. Also, there will no longer be that problem when the playback cursor erases something during playback

RomanPudashkin avatar Sep 10 '24 07:09 RomanPudashkin

@cbjeukendrup @wizofaus I've been playing with the playback cursor recently. My idea now is to draw it outside of the score view. Here's what it looks like: af07099

This decently reduces CPU/GPU load during playback and we also don't have to increase the redraw area to fix this issue. Also, there will no longer be that problem when the playback cursor erases something during playback

Is there a downside? (other than it using more QML, which I'm not really a fan of...) Is it worth me taking this commit and combining with my original clipping fix in this PR?

wizofaus avatar Sep 11 '24 20:09 wizofaus

You don't have to do anything. Just wait until we merge my PR and rebase yours after that

RomanPudashkin avatar Sep 12 '24 08:09 RomanPudashkin

@wizofaus please rebase

RomanPudashkin avatar Sep 18 '24 07:09 RomanPudashkin

Suddenly I started doubting whether this is desirable. When things that fall off the page are not drawn, you won't ever notice that they are there:

  • if an entire staff falls off the page, you can easily overlook that, and your full score won't have all instruments visible and the conductor will be confused
  • if you have accidentally dragged an element off the page, and you can't see it anymore, you won't know that it is there. This means you're stuck with an obsolete element in your score; and more importantly, if that's an element that influences playback or something like that, you're going to have a hard time figuring out why playback is weird, because you have no way to see the element that's causing the weirdness.

Sorry for this very late realisation. But we'll need to give this some UX thought to this.

Of course, it is ugly, when stuff is drawn beyond the border of the page. But whenever that happens, it's a user error. If anything, we should try to prevent such user errors from happening; but we should not "mask" user errors, because that makes it impossible to correct them.

cbjeukendrup avatar Sep 24 '24 13:09 cbjeukendrup

Definitely agreed - it's better to show the content off the page than not. But better still to figure out something reasonable to do. Merely scaling the page down to force it to fit isn't going to work well, because that will in turn impact how many measures fit on the page, which in turn may affect how much vertical space is required. Maybe a warning could be added during corruption checks?

MarcSabatella avatar Sep 24 '24 13:09 MarcSabatella

It's supposed to be WYSIWYG, i.e. representing what your output will look like when you print it out. I don't believe there's any other such application out there that doesn't crop/clip items that don't fit on the on-screen representation of the page. And there may well be cases that's what the user wants, especially if it's just a design feature for a title page etc. Perhaps there's a case for having a mode where you can see items drawn off the edge of the page but what it does now by default very much looks like a bug - especially in the "publish" view.

I will note that the original bug reported was just "Instruments should not go outside the score if a lot of instruments added to the Score Wizard" - and hence fixing the clipping is not necessarily a sufficient solution, but I would argue it is a necessary one. I did actually previously propose that it should simply try to spread the system over multiple pages if it can't fit it on one (currently it does in fact move the system to the start of the next page if there are any text elements like "title" etc.).

But for other cases where symbols might end up off the edge of the page, then allowing them to be drawn outside the current page area is pretty obviously wrong, as they might even end up drawn over the previous or subsequent page area.

wizofaus avatar Sep 24 '24 19:09 wizofaus

BTW if you do manage to modify an element so it ends up drawn onto another page (e.g. I did by changing the horizontal justification), you can't interact with it at all or determine what it belongs to:

image

wizofaus avatar Sep 24 '24 19:09 wizofaus

Though interestingly with the keyboard you can even enter notes in staves that are off the bottom of the page:

image

wizofaus avatar Sep 24 '24 20:09 wizofaus

Perhaps we could leave items drawn off the page in "Floating" view only, whatever that is... (I've never quite understood).

wizofaus avatar Sep 24 '24 20:09 wizofaus

I can’t think of any graphics editors I have ever used that dont still draw elements when moved off the page. It’s actually a pretty crucial feature in that context. Dragging things off the page is less of a thing in MuseSvore, but it still seems way more natural to still see it than not. I’m more concerned with the case of a system extending past the bottom margin though, and making it as obvious as possible that this is what is happening.

MarcSabatella avatar Sep 24 '24 22:09 MarcSabatella

Heh, I just tried using the "highlighter" Pen in MS Word Draw mode, and you do actually see it initially draw off the edge of the page boundaries, but then it cleans it up shortly after. Seems to be a quirk of that tool (Insert Shape is similar) as nothing else (WordArt etc.) ever draws off the edge of the page, and at any rate it's basically only while the mouse is captured and you're initially placing/sizing the elements. No other applications I tried (including Adobe Acrobat, etc.) allow drawing off the edge of a page at all, and elements that don't fit on the page are always clipped. Which examples were you thinking of? (and what did MS 3 do? Other notation software?)

wizofaus avatar Sep 25 '24 00:09 wizofaus

BTW the current behaviour (in MS 4.3) when entering a line of text that won't fit on the page isn't great, you end up not being able to see what you're typing: image

In fact in general items that extend over the right edge of a page are effectively clipped when the next page background is drawn anyway.

wizofaus avatar Sep 25 '24 01:09 wizofaus

Google Drawings, Inkscape, etc. Drawing apps that allow you to place object on a canvas then move them. It’s a common technique to move objects off the page temporarily while working as a form of temporary storage for elements you aren’t sure you will want to use, like in doing A/B comparisons of different designs. Not that this relates directly to MuseScore, but the point is, it’s a very common pattern and the idea of having the off-canvas elements visible is something that I’d think most users would be quite familiar with. And as noted, it’s quite bad for elements you’ve moved off the page and potentially affecting layout and playback to be impossible to see and select.

MarcSabatella avatar Sep 25 '24 01:09 MarcSabatella

Sure, I accept that there are some downsides to always clipping. But at very least it should definitely do so in Publish mode, and really we need to think a bit more about how to ensure that objects drawn completely outside the page boundaries can still be interacted with (because they can't with the mouse in the current version).

wizofaus avatar Sep 25 '24 01:09 wizofaus

Clipping to page borders in Publish mode only seems a good compromise. And yes, it is indeed problematic that items outside a page (or dragged to the wrong page, even) cannot be clicked. Probably not trivial to solve. But anyway, being able to see the element at least is still better than not being able to see it anymore at all.

cbjeukendrup avatar Sep 25 '24 01:09 cbjeukendrup

That should be relatively easy to change, but it's worth asking then if it really resolves the bug as reported.

Is there any case for having systems split over multiple pages if required? (I have some study scores that do that, though they flip into landscape mode, and it's only ever across two "open" pages, never over a page turn).

wizofaus avatar Sep 25 '24 01:09 wizofaus

Seems pretty clear the person reporting wouldn't be happy with any proposed rearranging of the pixels that end up on the outskirts of the page. They presumably just want the music to actually fit on the page. Which isn't a bad thing to want, but as noted, that a much harder problem.

Anyhow, I agree, not showing the off-page elements in publish mode seems like a no-brainer. Potentially could also disappear if you turn off display of invisible elements, even though that of course isn't really right.

MarcSabatella avatar Sep 25 '24 02:09 MarcSabatella

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved

zacjansheski avatar Oct 03 '24 20:10 zacjansheski