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

thumbnails only updating the visible region when user zoomed in the current view #143

Closed
kaamui opened this issue Oct 16, 2023 · 10 comments
Closed
Labels
bug Something isn't working

Comments

@kaamui
Copy link

kaamui commented Oct 16, 2023

Hi Martin,

I just found that the live thumbnails are only updating the part of the region that is visible on the view. If, for example. you move a ruler while your view is zoomed at x9.0, you'll see only the visible part of the ruler move.

I also found, but it is not related, that any element that is partially placed under one of the two palettes will produce some glitches. Not the same bug, but same idea : the region that should be updated is not correct.

Did you make in purpose that the region updated in the live thumbnails is only the visible part ? For performance reasons ? Do you think it is easy to fix ?

Capture d’écran de 2023-10-16 10-10-29

@letsfindaway
Copy link
Owner

Hi Martin,

I just found that the live thumbnails are only updating the part of the region that is visible on the view. If, for example. you move a ruler while your view is zoomed at x9.0, you'll see only the visible part of the ruler move.

Can reproduce.

I also found, but it is not related, that any element that is partially placed under one of the two palettes will produce some glitches. Not the same bug, but same idea : the region that should be updated is not correct.

Did you make in purpose that the region updated in the live thumbnails is only the visible part ? For performance reasons ? Do you think it is easy to fix ?

Yes, it was for performance reason. To fix it, just always update the complete region:

if (region.isNull())
{
// full update
pixmapRect = QRectF(QPoint(0, 0), mSize);
}
else
{
// only render affected region
/*
* To avoid problems with antialiaing we have to make sure that the
* pixmapRect has an integer size and position and covers at least
* the affected region. The following steps assure this.
*/
pixmapRect = mTransform.mapRect(region); // translate to pixmap coordinates
pixmapRect += QMarginsF(1, 1, 1, 1); // add a margin
pixmapRect = pixmapRect.toRect(); // cut to integer
pixmapRect &= QRectF(QPointF(0, 0), mSize); // limit to pixmap area
}

Make sure that the if part is always executed and never the else part.

Please evaluate the performance on a not-so-powerful computer. It might be remarkable with complex scenes.

In fact, we would only have to do a full update when we move or resize an item which extends over the visible range. But I have no idea how to determine this here.

Capture d’écran de 2023-10-16 10-10-29

I can also reproduce this, but have to further analyze this.

@letsfindaway
Copy link
Owner

letsfindaway commented Oct 16, 2023

I can reproduce the problem with the ruler for other tools like compass or triangle, but not for axes and not for strokes. And it is not important whether the tool was partly below the toolbar. I think it is the size calculation of the tools.

Edit: Probably not the size calculation, because sometimes really parts of the ruler are staying on the screen. With size calculation issues I would just expect the border line.

@letsfindaway
Copy link
Owner

letsfindaway commented Oct 16, 2023

For the update of the thumbnail I have a four-line fix which does not affect performance much. Shall I open a PR? Or you may just take the patch. Here is the diff:

diff --git a/src/gui/UBThumbnailWidget.cpp b/src/gui/UBThumbnailWidget.cpp
index 9efcb400..623345da 100644
--- a/src/gui/UBThumbnailWidget.cpp
+++ b/src/gui/UBThumbnailWidget.cpp
@@ -44,7 +44,7 @@
 #include "document/UBDocumentProxy.h"
 #include "document/UBDocumentController.h"
 
-#include "board/UBBoardPaletteManager.h"
+#include "board/UBDrawingController.h"
 
 #include "core/memcheck.h"
 
@@ -1187,10 +1187,12 @@ void UBDraggableLivePixmapItem::updatePixmap(const QRectF &region)
     {
         QPixmap pixmap = this->pixmap();
         QRectF pixmapRect;
+        const auto tool = UBDrawingController::drawingController()->stylusTool();
+        const auto affectsWholeScene = tool == UBStylusTool::Play || tool == UBStylusTool::Selector;
 
-        if (region.isNull())
+        if (region.isNull() || affectsWholeScene)
         {
-            // full update
+            // full update if region unknown or play/selector tool active, which may affect whole scene
             pixmapRect = QRectF(QPoint(0, 0), mSize);
         }
         else

@kaamui
Copy link
Author

kaamui commented Oct 16, 2023

Please make a PR based on the dev branch. I think this one is quite important and should be available in 1.7.0

@kaamui
Copy link
Author

kaamui commented Oct 18, 2023

I did some further testing. I don't know much how is working the update system and how updating a specific region can "freeze" updating of other regions of the different views and other rendering systems, but I did test the issue with 1.6.4 and 1.7.0a-221020 and could not replicate it.

So it seems to be related to the last implementation of the live thumbnails. For me, the issue is only reproducible by moving the ruler or other tools under a live thumbnail.

I'll continue to test things and give you updates if new useful information is available.

edit : issue reproducible with 1.7.0a-230427. Confirmed that issue is not happening if the moved object is not moving under a live thumbnail

edit : only happening when the moved item is moved under the active page thumbnail

@letsfindaway
Copy link
Owner

letsfindaway commented Oct 19, 2023

I further tried to find out which statement finally makes the difference. It is

setPixmap(pixmap);

I tried delaying the setPixmap() using a timer to make it occur outside of any paint handling, but this did not make a difference. I even tried to use a quite long timer (5 seconds delay). Then the issue does not occur for the first five seconds. But as soon as the first timer fires and invokes setPixmap(), the issue occurs. And note, as it is from a timer it is really from the event loop, where such a call should be allowed anyway.

Need more ideas...

Edit: rate limiting the pixmap updates does not help. The problem just occurs not so often.

@letsfindaway
Copy link
Owner

edit : only happening when the moved item is moved under the active page thumbnail

More precisely: the bounding rectangle of the item must be under the active page thumbnail. So if you rotate the ruler, it is not the ruler itself, but its bounding rectangle, which is bigger.

@letsfindaway
Copy link
Owner

letsfindaway commented Oct 19, 2023

It seems to me as if

  • Setting the thumbnail pixmap invalidates the area of the board scene behind the pixmap.
  • This aborts any currently running rendering process of the scene, because this new information has to be taken into account.
  • Due to some problem in Qt (???) however some areas of the previous rendering process are not rendered again, leaving artefacts on the screen.
  • Note that the artefacts are not only a thin line, but quite large. So it is not an issue with the bounding rect of the item, but a previous rendering must completely be aborted and not properly continued.

Looks similar to https://bugreports.qt.io/browse/QTBUG-12932

I can mitigate the problem by triggering a full repaint of the scene, but this again triggers an update of the pixmap and we end in an endless loop of updates.

@letsfindaway
Copy link
Owner

letsfindaway commented Oct 19, 2023

I have a solution! Disabling updates while setting the pixmap solves the problem:

            UBApplication::boardController->controlView()->setUpdatesEnabled(false);
            setPixmap(pixmap);
            UBApplication::boardController->controlView()->setUpdatesEnabled(true);

I will create a PR in a few minutes.

Edit: Unfortunately this leads again to high CPU usage and an endless rendering loop. So further analysis is needed. No PR so far. :-(

Edit 2: Found a solution for breaking the endless update loop! PR follows.

@kaamui
Copy link
Author

kaamui commented Oct 19, 2023

You're just incredible ! Can't imagine the time it would have cost me.

In the mean time we discovered a big issue where, because of the cache mode of our samba client on Kubuntu 22.04, for a newly imported pdf, when calling cleanupDocument, the part checking the number of pages returns 0, and every object,image etc is then deleted.

We had to rolllback the last version of Openboard in our pilot schools. I fixed the issue (commit tomorrow), but we'll have to report a bit the 1.7.0 release.

I'll integrate your PR for this issue and can't thank you enough for the effort you put on this !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants