letsfindaway / OpenBoard

I'm using this fork to contribute features and fixes to the upstream project. In order to create good pull requests, I'm rebasing my feature branches, squashing and reordering commits, etc. If you fork this repository be aware that my development branches may rewrite history without prior notice.
http://openboard.ch/
GNU General Public License v3.0
9 stars 0 forks source link

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

Closed kaamui closed 10 months ago

kaamui commented 11 months ago

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 commented 11 months ago

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: https://github.com/letsfindaway/OpenBoard/blob/59dda2058ee65b974cb015829cc0fc4f5f3508ed/src/gui/UBThumbnailWidget.cpp#L1191-L1209 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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

I further tried to find out which statement finally makes the difference. It is https://github.com/letsfindaway/OpenBoard/blob/28c3d3040c3fb1106de748940e44f704b1c3d9d0/src/gui/UBThumbnailWidget.cpp#L1218

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 commented 11 months ago

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 commented 11 months ago

It seems to me as if

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 commented 10 months ago

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 commented 10 months ago

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 !