Fix #7819 - adjustment to previous page-clipping fix #22135

wizofaus · 2024-04-01T20:23:27Z

Resolves: #7819

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

I signed the CLA
The title of the PR describes the problem it addresses
Each commit's message describes its purpose and effects, and references the issue it resolves
If changes are extensive, there is a sequence of easily reviewable commits
The code in the PR follows the coding rules
There are no unnecessary changes
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)

src/engraving/rendering/dev/paint.cpp

RomanPudashkin · 2024-04-08T07:27:52Z

src/engraving/rendering/dev/paint.cpp

-
-            if (!painter->hasClipping()) {
-                painter->setClipping(true);
-                painter->setClipRect(pageRect);


With this change, we can no longer specify a custom clipping area... I'm also wondering how it affects performance, since this code was added to reduce the CPU load during playback (only the playback cursor rect is redrawn)

You can still specify a clip rect (namely via opt.frameRect) but with this change it will be intersected with the page rect, to avoid drawing outside page rect.

RomanPudashkin · 2024-04-08T07:30:53Z

src/engraving/rendering/dev/paint.cpp

-            if (!painter->hasClipping()) {
-                painter->setClipping(true);
-                painter->setClipRect(pageRect);
-                disableClipping = true;


It's also causing this regression:

I think I've also seen that in master sometimes

How do you reproduce that? I couldn't, using a build from my own branch.

Hmm ok I can reproduce it (it only seems to happen at particular zoom levels), but it does also happen using the version from master - just not quite as obviously, and only if you zoom in and out while playing back. Will investigate.

Wow, this has me totally baffled, nothing makes sense as to when and why this happens and what to do about it (for me most of the time zooming into 141% then starting playback is enough, but sometimes it doesn't happen at that level and you need to zoom in even further). It's hard not to suspect there's a bug in Qt somewhere, but I agree it's definitely more likely to happen with the changes in this PR than on master, and I'd even accept it's likely to affect more users than the bug that it was trying to fix (serious as that is), so best not to merge until someone's tracked down the issue (I'll keep looking but I've already spent way more time on it than I would have thought reasonable).

Ok, found something that works, have a slightly better (though far from perfect) understanding now of why the existing clipping rectangle is getting set (Qt Quick's dirty rectangles), but it seems that the safest/most reliable/performant fix is to simply treat the case of the drawing rectangle extending below the page as a special case and in this case overriding the clipping rectangle with the page rectangle (intersecting was causing other issues).

wizofaus · 2024-04-08T08:13:51Z

I admit I didn't test during playback, can try, but I'm pretty sure the logical change is sound... Get Outlook for Android<https://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.cpp<#22135 (comment)>:

@@ -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 GitHub<#22135 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI5UAJJ6AMO2BV6W2METJ3Y4JGN5AVCNFSM6AAAAABFSEZ7NOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBVG4ZDSNJQGQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

wizofaus · 2024-05-16T03:21:23Z

src/engraving/rendering/dev/paint.cpp

@@ -122,6 +122,10 @@ void Paint::paintScore(Painter* painter, Score* score, const IScoreRenderer::Pai
                painter->translate(-pageRect.topLeft());
            }

+            if (drawRect.bottom() > pageRect.bottom()) {
+                painter->setClipRect(pageRect);


Makes sense to treat this as a special case, as there's really the only one way elements can end up being drawn "off" the page, and in that case there's no point trying to maintain the QtQuick dirty rectangle optimisations (which are what control the existing clipping rect).

The only time I know of when painter->hasClipping() is false is when exporting to PDF (and maybe printing), and there's no issue in that case.

wizofaus · 2024-05-16T04:03:07Z

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)

cbjeukendrup · 2024-06-06T14:32:48Z

src/engraving/rendering/dev/paint.cpp

@@ -122,6 +122,11 @@ void Paint::paintScore(Painter* painter, Score* score, const IScoreRenderer::Pai
                painter->translate(-pageRect.topLeft());
            }

+            if (painter->hasClipping() && drawRect.bottom() > pageRect.bottom()) {


Actually, I think checking only the bottom is not really enough. Elements can fall off the page to the left or right side just as well as on the bottom side (for example, a too-long text element on the last measure of a system may extend beyond the right margin)

sure, I couldn't find a case of it, but happy to add those checks.

ok, now checks if the drawRect extends outside the pageRect on any side, and if so uses the whole page rectangle as the clip rectangle, which works as expected. In principle it should be possible to do an intersection to work out the minimum clipping rectangle, but it seems to cause too many glitches when rendering the cursor during playback (such glitches do still exist in master if you zoom in/out during playback, which I haven't fixed).
Have tested by having 4 pages, each of which has an object that rendered off a different edge of the page (and currently it's true all such cases CAN cause them to get drawn outside the page area).

wizofaus · 2024-09-01T00:09:55Z

Confirmed fix is still required & rebased.

cbjeukendrup · 2024-09-01T20:04:29Z

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

MuseScore/src/notation/view/abstractnotationpaintview.cpp

Lines 1368 to 1370 in 186c925

    
           //! NOTE: redraw in a slightly larger area than the cursor rect to avoid graphical artifacts 
        
           RectF dirtyRect1 = fromLogical(oldCursorRect).adjusted(-1, -1, 2, 1); 
        
           RectF dirtyRect2 = fromLogical(newCursorRect).adjusted(-1, -1, 2, 1);

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?

wizofaus · 2024-09-01T20:43:08Z

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).

RomanPudashkin · 2024-09-10T07:53:36Z

@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

RomanPudashkin · 2024-09-18T07:43:49Z

@wizofaus please rebase

RomanPudashkin · 2024-09-18T07:54:18Z

src/engraving/rendering/score/paint.cpp

@@ -122,6 +122,12 @@ void Paint::paintScore(Painter* painter, Score* score, const IScoreRenderer::Pai
                painter->translate(-pageRect.topLeft());
            }

+            if (painter->hasClipping() && (drawRect.top() < pageRect.top() || drawRect.bottom() > pageRect.bottom()


Shouldn't it be drawRect.top() > pageRect.top() || drawRect.bottom() < pageRect.bottom()?

Or maybe we can even use something like drawRect.intersected(pageRect) here...

If you merged that other change you mentioned to draw the cursor separately then yes, I'd like to try the intersect method again. But no, this is otherwise correct - if the drawRect extends outside the pageRect, it uses the whole pageRect for clipping.

Yes, I merged it

Something's changed since I last worked on this, I can't get the clipping rectangle to work properly at all now...happy to hand it over as I don't know how much time I'll have. The problem definitely still exists at any rate.

Actually...I did get it working...but the playback cursor still draws off the bottom of the page area when there are too many staves which looks a bit odd...

cbjeukendrup · 2024-09-24T13:20:37Z

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.

MarcSabatella · 2024-09-24T13:49:42Z

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?

wizofaus · 2024-09-24T19:44:58Z

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 · 2024-09-24T19:58:17Z

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:

wizofaus · 2024-09-24T20:01:35Z

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

wizofaus · 2024-09-24T20:09:52Z

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

MarcSabatella · 2024-09-24T22:18:30Z

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.

wizofaus · 2024-09-25T00:44:26Z

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 · 2024-09-25T01:05:04Z

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:

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.

MarcSabatella · 2024-09-25T01:16:29Z

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.

wizofaus · 2024-09-25T01:22:01Z

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).

cbjeukendrup · 2024-09-25T01:26:45Z

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.

wizofaus · 2024-09-25T01:35:43Z

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).

MarcSabatella · 2024-09-25T02:01:11Z

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.

cbjeukendrup · 2024-09-26T16:55:29Z

src/engraving/rendering/score/paint.cpp

+                                                             || drawRect.left() < pageAbsRect.left()
+                                                             || drawRect.right() > pageAbsRect.right())) {


Do we still need these < and > checks now that we're back at the .intersected solution?

Technically no, but it's a minor optimisation that makes debugging easier and intent a bit clearer.

I don't think so... The intent is already pretty clear, and they do nothing for optimization. It would be better to get rid of these checks

zacjansheski · 2024-10-03T20:50:59Z

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

cbjeukendrup reviewed Apr 2, 2024

View reviewed changes

src/engraving/rendering/dev/paint.cpp Outdated Show resolved Hide resolved

wizofaus force-pushed the clip_page_render_refix branch 3 times, most recently from 6c5885e to cbd0a32 Compare April 2, 2024 04:53

cbjeukendrup approved these changes Apr 7, 2024

View reviewed changes

cbjeukendrup assigned DmitryArefiev Apr 7, 2024

RomanPudashkin reviewed Apr 8, 2024

View reviewed changes

RomanPudashkin unassigned DmitryArefiev Apr 8, 2024

wizofaus force-pushed the clip_page_render_refix branch 2 times, most recently from b60c737 to 4ea7e01 Compare May 16, 2024 03:14

wizofaus commented May 16, 2024

View reviewed changes

wizofaus force-pushed the clip_page_render_refix branch 6 times, most recently from a7de10d to 8184057 Compare May 22, 2024 23:17

cbjeukendrup requested a review from RomanPudashkin June 6, 2024 14:27

cbjeukendrup reviewed Jun 6, 2024

View reviewed changes

wizofaus force-pushed the clip_page_render_refix branch 2 times, most recently from beeaaf8 to ebe5331 Compare June 10, 2024 22:53

wizofaus mentioned this pull request Aug 28, 2024

Staves/systems overflowing right margin #23507

Closed

wizofaus force-pushed the clip_page_render_refix branch from 6e737e7 to 1e2c216 Compare September 1, 2024 00:09

RomanPudashkin reviewed Sep 18, 2024

View reviewed changes

wizofaus force-pushed the clip_page_render_refix branch from 1e2c216 to 3e6e49c Compare September 23, 2024 23:50

wizofaus closed this Sep 23, 2024

wizofaus force-pushed the clip_page_render_refix branch from 3e6e49c to 090793d Compare September 23, 2024 23:50

wizofaus reopened this Sep 23, 2024

RomanPudashkin assigned zacjansheski Sep 24, 2024

RomanPudashkin approved these changes Sep 24, 2024

View reviewed changes

wizofaus force-pushed the clip_page_render_refix branch from 16db424 to e5bd50b Compare September 25, 2024 23:07

cbjeukendrup mentioned this pull request Sep 26, 2024

[MU4 Issue] Instruments are drawn outside the score if added a lot of instruments #7819

Closed

cbjeukendrup reviewed Sep 26, 2024

View reviewed changes

Fix musescore#7819 - adjustment to previous page-clipping fix

9afba26

wizofaus force-pushed the clip_page_render_refix branch from e5bd50b to 9afba26 Compare October 9, 2024 21:23

Fix musescore#7189 - remove check as per PR comment

80ecffe

cbjeukendrup merged commit 05934b3 into musescore:master Oct 10, 2024
11 checks passed

		\|\| drawRect.left() < pageAbsRect.left()
		\|\| drawRect.right() > pageAbsRect.right())) {

Fix #7819 - adjustment to previous page-clipping fix #22135

Fix #7819 - adjustment to previous page-clipping fix #22135

Conversation

wizofaus commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wizofaus May 16, 2024 • edited Loading

Choose a reason for hiding this comment

wizofaus May 16, 2024 • edited Loading

Choose a reason for hiding this comment

wizofaus commented Apr 8, 2024 via email

wizofaus May 16, 2024 • edited Loading

Choose a reason for hiding this comment

wizofaus commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wizofaus Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

wizofaus commented Sep 1, 2024

cbjeukendrup commented Sep 1, 2024

wizofaus commented Sep 1, 2024

RomanPudashkin commented Sep 10, 2024

RomanPudashkin commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wizofaus Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

cbjeukendrup commented Sep 24, 2024

MarcSabatella commented Sep 24, 2024

wizofaus commented Sep 24, 2024 • edited Loading

wizofaus commented Sep 24, 2024

wizofaus commented Sep 24, 2024

wizofaus commented Sep 24, 2024

MarcSabatella commented Sep 24, 2024

wizofaus commented Sep 25, 2024 • edited Loading

wizofaus commented Sep 25, 2024 • edited Loading

MarcSabatella commented Sep 25, 2024

wizofaus commented Sep 25, 2024

cbjeukendrup commented Sep 25, 2024

wizofaus commented Sep 25, 2024

MarcSabatella commented Sep 25, 2024

Choose a reason for hiding this comment

wizofaus Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacjansheski commented Oct 3, 2024

wizofaus May 16, 2024 •

edited

Loading

wizofaus May 16, 2024 •

edited

Loading

wizofaus May 16, 2024 •

edited

Loading

wizofaus Jun 10, 2024 •

edited

Loading

wizofaus Sep 23, 2024 •

edited

Loading

wizofaus commented Sep 24, 2024 •

edited

Loading

wizofaus commented Sep 25, 2024 •

edited

Loading

wizofaus commented Sep 25, 2024 •

edited

Loading

wizofaus Sep 26, 2024 •

edited

Loading