Split tools/StrokeHandler into View-Controller

[bhennion]

Follows #4137.

This PR splits tools/StrokeHandler into a View and a Controller. It also optimizes the rendering costs and protects the data with a mutex (see #3579).

• Merge Split geometric shape handlers into Controller-View #4137 and rebase

Edit: the CI failing is due to GCC 8 not handling std::transform_reduce. Will be fixed with #4160 done

[bhennion]

Now that #4137 has been merged, this is up for review.

[bhennion]

Rebased and resolved conflicts

[bhennion]

Resolved conflicts.

[Technius]

I only read the code, but largely LGTM. Will test it out tomorrow.

[rolandlo]

Sorry for the late reply, but I was pretty busy the last couple of weeks.

Codewise I can't see a flaw. I have tested it without issues on Ubuntu 22.10 (both Wayland and X11). However testing it now on my MacBook (with Catalina) I got an assertion failure when drawing filled highlighter strokes rapidly. For filled highlighter strokes it is quite a bit slower than Linux, but my MacBook is also quite a bit older than the Lenovo ThinkPad I use on Linux.

Assertion failed: (isInitialized()), function wipe, file /Users/admin/xournalpp/src/core/view/Mask.cpp, line 85.

** (xournalpp:2820): WARNING **: 20:56:17.257: [Crash Handler] Crashed with signal 6

** (xournalpp:2820): WARNING **: 20:56:17.258: [Crash Handler] Wrote crash log to: /Users/admin/.cache/xournalpp/errorlogs/errorlog.20230209-205617.log

Here's the error log:

Try to get a better stracktrace...
[bt] #1 1   xournalpp                           0x000000010e8b24f2 _ZL12crashHandleri + 1874
[bt] #2 2   libsystem_platform.dylib            0x00007fff6aa075fd _sigtramp + 29
[bt] #3 3   ???                                 0x0000000300000000 0x0 + 12884901888
[bt] #4 4   libsystem_c.dylib                   0x00007fff6a8dd808 abort + 120
[bt] #5 5   libsystem_c.dylib                   0x00007fff6a8dcac6 err + 0
[bt] #6 6   xournalpp                           0x000000010ec37aea _ZN3xoj4view4Mask4wipeEv + 74
[bt] #7 7   xournalpp                           0x000000010ec5594b _ZNK3xoj4view31StrokeToolFilledHighlighterView4drawEP6_cairo + 251
[bt] #8 8   xournalpp                           0x000000010ea59d48 _ZN11XojPageView9paintPageEP6_cairoP20_cairo_rectangle_int + 984
[bt] #9 9   xournalpp                           0x000000010eb4ba4c _ZL16gtk_xournal_drawP10_GtkWidgetP6_cairo + 1004
[bt] #10 10  libgtk-3.0.dylib                    0x000000011080abb5 gtk_widget_draw_internal + 421
[bt] #11 11  libgtk-3.0.dylib                    0x0000000110591181 gtk_container_propagate_draw + 625
[bt] #12 12  libgtk-3.0.dylib                    0x00000001105918e3 gtk_container_draw + 227
[bt] #13 13  libgtk-3.0.dylib                    0x00000001106f55c6 _gtk_pixel_cache_draw + 1430
[bt] #14 14  libgtk-3.0.dylib                    0x0000000110802988 gtk_viewport_render + 232
[bt] #15 15  libgtk-3.0.dylib                    0x00000001105998b2 gtk_css_custom_gadget_draw + 146
[bt] #16 16  libgtk-3.0.dylib                    0x000000011059edc7 gtk_css_gadget_draw + 1447
[bt] #17 17  libgtk-3.0.dylib                    0x0000000110801e56 gtk_viewport_draw + 134
[bt] #18 18  libgtk-3.0.dylib                    0x000000011080abb5 gtk_widget_draw_internal + 421
[bt] #19 19  libgtk-3.0.dylib                    0x0000000110591181 gtk_container_propagate_draw + 625
[bt] #20 20  libgtk-3.0.dylib                    0x00000001105918e3 gtk_container_draw + 227
[bt] #21 21  libgtk-3.0.dylib                    0x000000011072ee3c gtk_scrolled_window_render + 492
[bt] #22 22  libgtk-3.0.dylib                    0x00000001105998b2 gtk_css_custom_gadget_draw + 146
[bt] #23 23  libgtk-3.0.dylib                    0x000000011059edc7 gtk_css_gadget_draw + 1447
[bt] #24 24  libgtk-3.0.dylib                    0x000000011072aa3c gtk_scrolled_window_draw + 92
[bt] #25 25  libgtk-3.0.dylib                    0x000000011080abb5 gtk_widget_draw_internal + 421
[bt] #26 26  libgtk-3.0.dylib                    0x0000000110591181 gtk_container_propagate_draw + 625
[bt] #27 27  libgtk-3.0.dylib                    0x00000001105918e3 gtk_container_draw + 227
[bt] #28 28  libgtk-3.0.dylib                    0x00000001105389fe gtk_box_draw_contents + 62
[bt] #29 29  libgtk-3.0.dylib                    0x00000001105998b2 gtk_css_custom_gadget_draw + 146
[bt] #30 30  libgtk-3.0.dylib                    0x000000011059edc7 gtk_css_gadget_draw + 1447
[bt] #31 31  libgtk-3.0.dylib                    0x00000001105376cc gtk_box_draw + 92

[bhennion]

The last commit should fix it. I'm surprised you're encountering this when simply drawing rapidly though.
It probably means that XojPageView::getVisiblePart() returns an empty range quite often. I don't understand why that would happen...

[rolandlo]

The last commit should fix it.

Looking at the code I'm convinced it does. I haven't managed to reproduce the failing assertion even without the commit. Seems I have a talent to produce crashes when trying out things the first time, which are hard to reproduce afterwards. ;-)

There are still issues with filled highlighter strokes though, not only on MacOS.

On Linux sometimes when I draw "dots" (with the mouse, just one click without moving), the dots are not shown until I grab them with a (rectangle) selection. In the terminal I get messages like:

(xournalpp:17308): Gtk-CRITICAL **: 08:20:13.257: gtk_widget_queue_draw_area: assertion 'height >= 0' failed

(xournalpp:17308): Gtk-CRITICAL **: 08:20:13.898: gtk_widget_queue_draw_area: assertion 'height >= 0' failed

On MacOS there is a clear performance regression for filled highlighter strokes (comparing this PR with the latest nightly build). I will try once more with the PR rebased on master in the evening.

[rolandlo]

Actually the "dots" can also disappear when selecting them. Moreover the selection can extend to include the left upper page corner. It also happens with the pen and without filling.

[bhennion]

On MacOS there is a clear performance regression for filled highlighter strokes (comparing this PR with the latest nightly build). I will try once more with the PR rebased on master in the evening.

That's bad! Do you also observe a performance change with the other tools (pen, filled/not filled, dashed or not)?
I'll try to find out what specific change made this regression.

The dots issue is most likely a missing padding somewhere. I'll figure it out.

[bhennion]

Also, would you mind testing this branch on MacOs configured with -DDEBUG_MASKS=on? It should improve performances by using Quartz surfaces instead of image surfaces

[bhennion]

f448202 might improve performances

[hrdl-github]

The dots issue is most likely a missing padding somewhere. I'll figure it out.

I haven't check up on this, so this should taken wiht a grain of salt: uninitialised / some empty ranges could also cause this, as they translate into rectangles with negative widths or heights. I'm not sure if the same applies to empty but valid ranges where minY == maxY.

[rolandlo]

f448202 might improve performances

It looks to me, that this is indeed the case. It will require some more testing.

[rolandlo]

It looks like the dots issue is related to some strange behavior in Points::addPoint when less than 2 points have already been added. I don't understand it yet, but the result are wrong snappedBounds.

The issue is simple to reproduce (on Linux):

1. Start Xournal++ (PR version)
2. Add a "dot" using the pen or highlighter tool (with or without fill) with one click of the mouse
3. Select the "dot" and observe that the selection rectangle is too large.

[rolandlo]

Looks all good to me know. Performance on MacOS is good as well.
Maybe the last fixup commits could be squashed before merging.

[bhennion]

Rebased on master. I also

• Reduced further the mask wipeout area for filled highlighter strokes, to improve performances.
• Fixed a bug where the bounding boxes of single dots were not updated when those dots grew fatter.

[rolandlo]

@bhennion I have tested it once more and haven't found any issues. From my side you can merge at any moment.

[bhennion]

Thanks a lot! I'll merge in 72 hours unless a(nother) problem is raised.