Pen Tool Work 2

[not-surt]

Still got more I want to do on this but I figure it's best not to make the pull request too monolithic.

• Basic image-based brush painting
• Simple stylus pressure scaling (with 5.12 borked on Windows won't work there)
• Option to render brush preview onto canvas

[mitchcurtis]

Whoa! Awesome! I'm going to comment bit by bit since there's a lot here. :)

I got an assertion failure when drawing/drawing some lines:

ASSERT: "!isEmpty()" in file /Users/mitch/dev/qt5-slate-fw/qtbase/lib/QtCore.framework/Headers/qvector.h, line 241

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Application Specific Information:
abort() called

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib          0x00007fff599a6b86 __pthread_kill + 10
1   libsystem_pthread.dylib         0x00007fff59a5cc50 pthread_kill + 285
2   libsystem_c.dylib               0x00007fff599101c9 abort + 127
3   org.qt-project.QtCore           0x0000000100136659 qt_message_fatal(QtMsgType, QMessageLogContext const&, QString const&) + 9
4   org.qt-project.QtCore           0x0000000100137da4 QMessageLogger::fatal(char const*, ...) const + 202
5   org.qt-project.QtCore           0x0000000100131c62 qt_assert(char const*, char const*, int) + 77
6   libslate.dylib                  0x0000000102024bee QVector<ImageCanvas::StrokePoint>::last() const + 62 (qvector.h:241)
7   libslate.dylib                  0x000000010204c0f0 ImageCanvas::lineAngle() const + 64 (imagecanvas.cpp:941)
8   libslate.dylib                  0x00000001020a7596 ImageCanvas::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 6614 (moc_imagecanvas.cpp:1050)
9   org.qt-project.QtQml            0x000000010109c560 QV4::QObjectWrapper::getProperty(QV4::ExecutionEngine*, QObject*, QQmlPropertyData*, bool) + 880
10  org.qt-project.QtQml            0x000000010109d88f QV4::QObjectWrapper::getQmlProperty(QQmlContextData*, QV4::String*, QV4::QObjectWrapper::RevisionMode, bool*, bool) const + 863
11  org.qt-project.QtQml            0x00000001010a0330 QV4::QObjectWrapper::virtualGet(QV4::Managed const*, QV4::PropertyKey, QV4::Value const*, bool*) + 176
12  org.qt-project.QtQml            0x000000010114d1b8 QV4::Runtime::method_loadProperty(QV4::ExecutionEngine*, QV4::Value const&, int) + 328
13  ???                             0x00000001169075a4 0 + 4673533348
14  org.qt-project.QtQml            0x00000001010bb09c QV4::Moth::VME::exec(QV4::CppStackFrame*, QV4::ExecutionEngine*) + 188
15  org.qt-project.QtQml            0x000000010105a63f QV4::Function::call(QV4::Value const*, QV4::Value const*, int, QV4::ExecutionContext const*) + 367
16  org.qt-project.QtQml            0x0000000101200753 QQmlJavaScriptExpression::evaluate(QV4::CallData*, bool*) + 643
17  org.qt-project.QtQml            0x0000000101209fd6 QQmlNonbindingBinding::doUpdate(QQmlJavaScriptExpression::DeleteWatcher const&, QFlags<QQmlPropertyData::WriteFlag>, QV4::Scope&) + 310
18  org.qt-project.QtQml            0x0000000101207897 QQmlBinding::update(QFlags<QQmlPropertyData::WriteFlag>) + 327
19  org.qt-project.QtQml            0x00000001011e7061 QQmlNotifier::emitNotify(QQmlNotifierEndpoint*, void**) + 785
20  org.qt-project.QtCore           0x000000010036035d QMetaObject::activate(QObject*, int, int, void**) + 125
21  libslate.dylib                  0x00000001020a8395 ImageCanvas::lineLengthChanged() + 37 (moc_imagecanvas.cpp:1403)
22  libslate.dylib                  0x0000000102049b36 ImageCanvas::setCursorSceneX(int) + 86 (imagecanvas.cpp:441)
23  libslate.dylib                  0x000000010205598c ImageCanvas::updateCursorPos(QPoint const&) + 652 (imagecanvas.cpp:2363)
24  libslate.dylib                  0x0000000102057e7d ImageCanvas::hoverMoveEvent(QHoverEvent*) + 109 (imagecanvas.cpp:2960)
25  org.qt-project.QtQuick          0x00000001015da2df QQuickItem::event(QEvent*) + 463
26  libslate.dylib                  0x0000000102056ad4 ImageCanvas::event(QEvent*) + 564 (imagecanvas.cpp:2671)
27  org.qt-project.QtWidgets        0x00000001019cabd1 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 273
28  org.qt-project.QtWidgets        0x00000001019cc138 QApplication::notify(QObject*, QEvent*) + 600
29  org.qt-project.QtCore           0x000000010032a8d4 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 212
30  org.qt-project.QtQuick          0x00000001015f106c QQuickWindowPrivate::sendHoverEvent(QEvent::Type, QQuickItem*, QPointF const&, QPointF const&, QFlags<Qt::KeyboardModifier>, unsigned long, bool) + 236
31  org.qt-project.QtQuick          0x00000001015ecf50 QQuickWindowPrivate::deliverHoverEvent(QQuickItem*, QPointF const&, QPointF const&, QFlags<Qt::KeyboardModifier>, unsigned long, bool&) + 2256
32  org.qt-project.QtQuick          0x00000001015ec959 QQuickWindowPrivate::deliverHoverEvent(QQuickItem*, QPointF const&, QPointF const&, QFlags<Qt::KeyboardModifier>, unsigned long, bool&) + 729
33  org.qt-project.QtQuick          0x00000001015ec959 QQuickWindowPrivate::deliverHoverEvent(QQuickItem*, QPointF const&, QPointF const&, QFlags<Qt::KeyboardModifier>, unsigned long, bool&) + 729
34  org.qt-project.QtQuick          0x00000001015ec959 QQuickWindowPrivate::deliverHoverEvent(QQuickItem*, QPointF const&, QPointF const&, QFlags<Qt::KeyboardModifier>, unsigned long, bool&) + 729
35  org.qt-project.QtQuick          0x00000001015ec959 QQuickWindowPrivate::deliverHoverEvent(QQuickItem*, QPointF const&, QPointF const&, QFlags<Qt::KeyboardModifier>, unsigned long, bool&) + 729
36  org.qt-project.QtQuick          0x00000001015ec959 QQuickWindowPrivate::deliverHoverEvent(QQuickItem*, QPointF const&, QPointF const&, QFlags<Qt::KeyboardModifier>, unsigned long, bool&) + 729
37  org.qt-project.QtQuick          0x00000001015f59fd QQuickWindowPrivate::handleMouseEvent(QMouseEvent*) + 941
38  org.qt-project.QtGui            0x000000010080849a QWindow::event(QEvent*) + 778
39  org.qt-project.QtQuick          0x00000001015f1406 QQuickWindow::event(QEvent*) + 694
40  org.qt-project.QtWidgets        0x00000001019cabd1 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 273
41  org.qt-project.QtWidgets        0x00000001019cc138 QApplication::notify(QObject*, QEvent*) + 600
42  org.qt-project.QtCore           0x000000010032a8d4 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 212
43  org.qt-project.QtGui            0x00000001007f8628 QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) + 3416
44  org.qt-project.QtGui            0x00000001007de90b QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) + 155
45  libqcocoa.dylib                 0x0000000104e926bd QCocoaEventDispatcherPrivate::processPostedEvents() + 493
46  libqcocoa.dylib                 0x0000000104e92f60 QCocoaEventDispatcherPrivate::postedEventsSourceCallback(void*) + 32
47  com.apple.CoreFoundation        0x00007fff2c6a4155 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
48  com.apple.CoreFoundation        0x00007fff2c6a40fb __CFRunLoopDoSource0 + 108
49  com.apple.CoreFoundation        0x00007fff2c687b95 __CFRunLoopDoSources0 + 195
50  com.apple.CoreFoundation        0x00007fff2c68713e __CFRunLoopRun + 1219
51  com.apple.CoreFoundation        0x00007fff2c686a28 CFRunLoopRunSpecific + 463
52  com.apple.HIToolbox             0x00007fff2b91fb35 RunCurrentEventLoopInMode + 293
53  com.apple.HIToolbox             0x00007fff2b91f774 ReceiveNextEventCommon + 371
54  com.apple.HIToolbox             0x00007fff2b91f5e8 _BlockUntilNextEventMatchingListInModeWithFilter + 64
55  com.apple.AppKit                0x00007fff29bdbeb7 _DPSNextEvent + 997
56  com.apple.AppKit                0x00007fff29bdac56 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1362
57  com.apple.AppKit                0x00007fff29bd4cb9 -[NSApplication run] + 699
58  libqcocoa.dylib                 0x0000000104e91bbd QCocoaEventDispatcher::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) + 3117
59  org.qt-project.QtCore           0x000000010032580e QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 366
60  org.qt-project.QtCore           0x000000010032af82 QCoreApplication::exec() + 130
61  org.example.Slate               0x00000001000490fe Application::run() + 30 (application.cpp:170)
62  org.example.Slate               0x000000010005ca10 main + 112 (main.cpp:6)
63  org.example.Slate               0x000000010003ce04 start + 52

[mitchcurtis]

The increase/decrease tool shortcuts no longer work:

qrc:/qml/ui/Shortcuts.qml:285: Error: Cannot assign to non-existent property "toolSize"

[mitchcurtis]

I love the blend/replace mode options! The icons also fit in really nicely.

With the blend mode, is there a plan to fix the accumulation within a single "stroke"? It seems like that could limit its usefulness.

[mitchcurtis]

It seems the cursor/crosshair disappears completely if the tool size is 1.

[mitchcurtis]

Can you add the changes to the non-native MenuBar (app/qml/ui/MenuBar.qml) to the native one (app/qml/ui/+nativemenubar/MenuBar.qml)? Currently I can't test out some things because of this.

[mitchcurtis]

Sorry about the nitpicks, haha.

[not-surt]

Can you add the changes to the non-native MenuBar (app/qml/ui/MenuBar.qml) to the native one (app/qml/ui/+nativemenubar/MenuBar.qml)? Currently I can't test out some things because of this.

Is there anyway to force the native menubar to display? I made the changes, but can't say if they work or not.

It seems the cursor/crosshair disappears completely if the tool size is 1.

I think this might be because of the brush preview. It's taking the brush preview colour as the colour of the canvas and so selecting a colour to contrast against it. This poses a bit of a problem as if the preview is bigger than the cursor its mostly visible, but if its smaller it can be nearly invisible. Could maybe try difference or XOR composition modes.

EDIT: Actually it looks like the cursors not drawing is due to lazy updating of the brush, so it doesn't actually get created and signal emitted until it is first used after changed.

[mitchcurtis]

Is there anyway to force the native menubar to display? I made the changes, but can't say if they work or not.

Yep, comment out this #if in application.cpp:

#if defined(Q_OS_WIN) || defined(Q_OS_MACOS)

[mitchcurtis]

Oh, and you'll need to be on a platform where Qt has support for native menu bars (Windows, macOS and Ubuntu with the global menu bar, which I think is Unity, but not sure).

[mitchcurtis]

By the way, do you want me to create a branch that you can push to if you still have more to do on it? That would be better than pushing unfinished stuff to master. If so, just let me know what it should be called, and then you or I can retarget this pull request to it.

[not-surt]

Removing the lazy updating of brush looks to have fixed the core issue with the cursors.

When using a RangeSlider for brush size range I'm getting binding loop warnings. Switched to using two regular Sliders without any issue. No idea why it should be a problem as should be logically identical. Any idea? I've no prior experience with Quick so I may be missing something obvious.

By the way, do you want me to create a branch that you can push to if you still have more to do on it? That would be better than pushing unfinished stuff to master. If so, just let me know what it should be called, and then you or I can retarget this pull request to it.

If you want. Doesn't affect me either way.

[mitchcurtis]

When using a RangeSlider for brush size range I'm getting binding loop warnings. Switched to using two regular Sliders without any issue. No idea why it should be a problem as should be logically identical. Any idea? I've no prior experience with Quick so I may be missing something obvious.

I think it is due to the fact that there are two bindings in different places. Commenting out these fixes it for me:

//                first.value: canvas ? canvas.lowerToolSize : 1
//                second.value: canvas ? canvas.upperToolSize : 1

If you want. Doesn't affect me either way.

OK, created pen-tool-work-2.

[mitchcurtis]

I added some more comments that would be good if you could address, but there's no harm in merging this now, since it now has its own branch. Also makes it easier for me to make changes. :)

[mitchcurtis]

It looks like the save shortcut no longer works sometimes, because Project::hasUnsavedChanges() returns false even after drawing on the canvas.

[not-surt]

I think it is due to the fact that there are two bindings in different places. Commenting out these fixes it for me:

//                first.value: canvas ? canvas.lowerToolSize : 1
//                second.value: canvas ? canvas.upperToolSize : 1

But then it's no longer a two-way binding, so the slider doesn't update when the values are changed elsewhere such as with shortcuts.

It looks like the save shortcut no longer works sometimes, because Project::hasUnsavedChanges() returns false even after drawing on the canvas.

canSaveChanged signal was being emitted by beginMacro, which I removed from applyCurrentTool, so I added it to applyCommand.

Think the other things you brought up should be fixed.

[mitchcurtis]

But then it's no longer a two-way binding, so the slider doesn't update when the values are changed elsewhere such as with shortcuts.

Hmm true, looks like I didn't read the code properly.

With the following patch to qtquickcontrols2:

diff --git a/src/quicktemplates2/qquickrangeslider.cpp b/src/quicktemplates2/qquickrangeslider.cpp
index ff488dac..ffbf89e1 100644
--- a/src/quicktemplates2/qquickrangeslider.cpp
+++ b/src/quicktemplates2/qquickrangeslider.cpp
@@ -199,6 +199,8 @@ void QQuickRangeSliderNode::setValue(qreal value)
         return;
     }

+    static int i = 0;
+    qDebug() << Q_FUNC_INFO << this << value << ++i;
     // First, restrict the first value to be within to and from.
     const qreal smaller = qMin(d->slider->to(), d->slider->from());
     const qreal larger = qMax(d->slider->to(), d->slider->from());
@@ -228,8 +230,11 @@ void QQuickRangeSliderNode::setValue(qreal value)
     if (!qFuzzyCompare(d->value, value)) {
         d->value = value;
         d->updatePosition();
+        qDebug() << "about to emit valueChanged";
         emit valueChanged();
+        qDebug() << "emitted valueChanged";
     }
+    --i;
 }

 qreal QQuickRangeSliderNode::position() const

I get this output when clicking once (not dragging on a handle) on the RangeSlider:

void QQuickRangeSliderNode::setValue(qreal) QQuickRangeSliderNode(0x7f9c705c4f40) 95.9561 1 about to emit valueChanged void QQuickRangeSliderNode::setValue(qreal) QQuickRangeSliderNode(0x7f9c705c4f40) 96 2 about to emit valueChanged qrc:/qml/ui/ToolSizePopup.qml:73:17: QML Binding: Binding loop detected for property "value" emitted valueChanged emitted valueChanged void QQuickRangeSliderNode::setValue(qreal) QQuickRangeSliderNode(0x7f9c705c4f40) 95.9561 1 about to emit valueChanged emitted valueChanged

So I think it's something to do with the fact that RangeSlider itself will set a fractional value but ImageCanvas' upperToolSize and lowerToolSize properties are integers. When the handle is moved, the value changes and so valueChanged is emitted. The Binding object then catches this and sets e.g. upperToolSize to 96 (since it's an integer), which the second.value: canvas ? canvas.upperToolSize : 1 binding sees and responds appropriately, resulting in recursion.

I'm not sure if this is the best fix, but setting snapMode: RangeSlider.SnapAlways is one way of solving it. It prevents the RangeSlider from setting fractional values in the first place when interacting with it.