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

longer loading times on widgets since 5cbda07 #152

Open kaamui opened 10 months ago

kaamui commented 10 months ago

Hi Martin,

I just found that the fix you introduced to prevent glitches (5cbda07) on board has a bad impact on html widgets loading time.

I looked a bit at the code and it seems related to the use of setUpdatesEnabled. With these instructions uncommented, the colorPicker for example, takes a loooong time to finish a complete first loading, the CPU (on my own PC with a good CPU) goes to ~30% of use, and everything seems very slow (even trying to launch web inspector takes a long time in terms of rendering).

I'm going to revert it, but if you've got an idea to how resolve it (I don't), let me know here. Maybe we'll have the time to reintroduce it for 1.7.0.

letsfindaway commented 10 months ago

Good you found that. Yes, reverting for 1.7.0 is a good idea. Let's plan it for 1.7.1.

I can imagine the reason: Widgets, especially while loading cause a high number of paint event. And each of them goes through this event processing code. Stopping and restarting the update seems to introduce additional overhead.

We should address this wen we have a little bit more time. I have ideas for solutions:

But again: we should have some more time for testing after the next try.

Edit: My current (6 months old) computer seems to be so powerful that I can hardly see the effect. I can see that it uses up the CPU time of one core, but still opening the web inspector is nearly instantaneous. So we need a not-so-powerful computer for testing.

letsfindaway commented 10 months ago

How the thumbnail update currently works:

The UBBoardView receives a paintEvent. If the event area is not completely under the left palette, we emit a painted signal. This check should ignore paint events caused by updates of the thumbnails. Double-check whether this works. Edit: It does. after each emit and therefore after each update of the pixmap we have another paint event which is completely under the left palette. This event is caused by updating the pixmap and does not again emit the painted signal. https://github.com/letsfindaway/OpenBoard/blob/1420d5f36cb9188081b6b9bccaeedcb1f0e97f8e/src/board/UBBoardView.cpp#L1784-L1795

This event is handled in UBBoardThumbnailView and forwarded to the thumbnail representing the current scene. The check of the index could be improved to guarantee that at() causes no out-of-bounds error. https://github.com/letsfindaway/OpenBoard/blob/1420d5f36cb9188081b6b9bccaeedcb1f0e97f8e/src/gui/UBBoardThumbnailsView.cpp#L280-L287

The handling continues in the UBDraggableLivePixmapItem::updatePixmap() function. Here the update region is calculated and the pixmap is updated. https://github.com/letsfindaway/OpenBoard/blob/1420d5f36cb9188081b6b9bccaeedcb1f0e97f8e/src/gui/UBThumbnailWidget.cpp#L1184-L1221

Line 1218 causes the glitches we wanted to avoid by temporarily disabling updates, but this seems to be an expensive task, which should not be performed frequently.

The ideas in the previous comment include a timer for rate limiting and an indication whether we move an object.

The indication whether we're currently moving an item could be created at the UBBoardView, which is handling the mouse events. So I propose to add this indication to the painted event and to forward it further. Edit: It is not only moving, it is also resizing. Note that moving or resizing an item is also the only way to affect the scene outside of the visible area. So this check does not have to be repeated in UBDraggableLivePixmapItem::updatePixmap(). The flag does finally indicate whether the current paint event may additionally affect any region of the scene outside of the current board view. affectsWholeScene may be a reasonable name for it.

The rate limiting timer is better at the UBDraggableLivePixmapItem which knows best what rate is still ok. Here we can also compute the combined region of all accumulated events.

letsfindaway commented 10 months ago

Note that we cannot as easily detect moving or resizing of an item at the delegate frame. This behavior is not (and should not be) known to the UBBoardView. So currently I don't have a clear indication whether a paint event is caused by some action which might affect the region outside of the visible area. So the flag affectsWholeScene must be set whenever the selector or play tool is active.