Added a QComboBox to adjust preview granularity while doing transforms
ClosedPublic

Authored by hellozee on Dec 26 2018, 3:39 AM.

Details

Summary

The patch adds a QComboBox to the transform tool options to adjust the preview granularity while doing transforms.
BUG: 344210

Test Plan

Draw something, switch to cage transform in move tool, draw at least 3 points and change the granularity values.

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
hellozee requested review of this revision.Dec 26 2018, 3:39 AM
hellozee created this revision.
shubham edited the summary of this revision. (Show Details)Dec 26 2018, 4:35 AM
dkazakov requested changes to this revision.Dec 28 2018, 11:27 AM

Hi, @hellozee!

It looks like your option gets uninitialized somehow, so it tries to pass random numbers to the processing code. Therefore causing it to crash :(

This debugging patch generates the following output:

Patch:

1diff --git a/plugins/tools/tool_transform2/kis_cage_transform_strategy.cpp b/plugins/tools/tool_transform2/kis_cage_transform_strategy.cpp
2index 765c24d..8c63788 100644
3--- a/plugins/tools/tool_transform2/kis_cage_transform_strategy.cpp
4+++ b/plugins/tools/tool_transform2/kis_cage_transform_strategy.cpp
5@@ -89,6 +89,8 @@ QImage KisCageTransformStrategy::calculateTransformedImage(ToolTransformArgs &cu
6 QPointF *dstOffset)
7 {
8
9+ ENTER_FUNCTION() << ppVar(currentArgs.previewPixelPrecision());
10+
11 KisCageTransformWorker worker(srcImage,
12 srcOffset,
13 origPoints,

Output:

Entering "KisCageTransformStrategy::calculateTransformedImage()" currentArgs.previewPixelPrecision() = 32622

Video of the bug:

This revision now requires changes to proceed.Dec 28 2018, 11:27 AM

Ahh sorry, missed the other constructors.

hellozee updated this revision to Diff 48279.Dec 28 2018, 11:57 AM

Updated the other ToolTransformArgs constructor to initialize m_pixelPrecision, m_previewPixelPrecision

dkazakov requested changes to this revision.Dec 29 2018, 11:01 AM

Hi, @hellozee!

I still have an assert when trying to change the values :(

Here is the video:

Here is the assert backtrace:

Entering "KisCageTransformStrategy::calculateTransformedImage()" currentArgs.previewPixelPrecision() = 18
Entering "KisCageTransformStrategy::calculateTransformedImage()" currentArgs.previewPixelPrecision() = 18
Entering "KisCageTransformStrategy::calculateTransformedImage()" currentArgs.previewPixelPrecision() = 18
Entering "KisCageTransformStrategy::calculateTransformedImage()" currentArgs.previewPixelPrecision() = 18
Entering "KisCageTransformStrategy::calculateTransformedImage()" currentArgs.previewPixelPrecision() = 18
[Thread 0x7fff9a4d4700 (LWP 8021) exited]
[Thread 0x7fff96c66700 (LWP 8018) exited]
[Thread 0x7fff9bae2700 (LWP 8020) exited]
[Thread 0x7fff98274700 (LWP 8022) exited]
[Thread 0x7fff9c2e3700 (LWP 8017) exited]
[Thread 0x7fff8cad8700 (LWP 8023) exited]
[Thread 0x7fff9776d700 (LWP 8019) exited]
[Thread 0x7fff9acd5700 (LWP 8016) exited]
ASSERT (krita): "numPoints == m_d->gridSize.width() * m_d->gridSize.height()" in file /home/appimage/persistent/krita/libs/image/kis_cage_transform_worker.cpp, line 176

Program received signal SIGABRT, Aborted.
0x00007fffe9e42c37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007fffe9e42c37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007fffe9e46028 in __GI_abort () at abort.c:89
#2  0x00007fffea78bb4e in QMessageLogger::fatal(char const*, ...) const () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5
#3  0x00007fffebb0d556 in kis_assert_common (assertion=<optimized out>, file=<optimized out>, line=<optimized out>, throwException=<optimized out>, isIgnorable=<optimized out>) at /home/appimage/persistent/krita/libs/global/kis_assert.cpp:90
#4  0x00007fffe7823360 in KisCageTransformWorker::prepareTransform (this=this@entry=0x7fffffffa8e0) at /home/appimage/persistent/krita/libs/image/kis_cage_transform_worker.cpp:176
#5  0x00007fffc019a0f3 in KisCageTransformStrategy::calculateTransformedImage (this=<optimized out>, currentArgs=..., srcImage=..., origPoints=..., transfPoints=..., srcOffset=..., dstOffset=0x602a00147958) at /home/appimage/persistent/krita/plugins/tools/tool_transform2/kis_cage_transform_strategy.cpp:99
#6  0x00007fffc018f6fd in KisWarpTransformStrategy::Private::recalculateTransformations (this=0x602a001478e0) at /home/appimage/persistent/krita/plugins/tools/tool_transform2/kis_warp_transform_strategy.cpp:612
#7  0x00007fffc0192fd3 in KisWarpTransformStrategy::continuePrimaryAction (this=<optimized out>, pt=..., shiftModifierActve=<optimized out>, altModifierActive=<optimized out>) at /home/appimage/persistent/krita/plugins/tools/tool_transform2/kis_warp_transform_strategy.cpp:548
#8  0x00007fffc019b58d in KisSimplifiedActionPolicyStrategy::continuePrimaryAction (this=0x600803ee7bd0, event=<optimized out>) at /home/appimage/persistent/krita/plugins/tools/tool_transform2/kis_simplified_action_policy_strategy.cpp:111
#9  0x00007fffc0144169 in continueActionImpl (action=KisTool::NONE, usePrimaryAction=true, event=<optimized out>, this=0x603a0012bd80) at /home/appimage/persistent/krita/plugins/tools/tool_transform2/kis_tool_transform.cc:307
#10 KisToolTransform::continuePrimaryAction (this=0x603a0012bd80, event=0x7fffffffc0b0) at /home/appimage/persistent/krita/plugins/tools/tool_transform2/kis_tool_transform.cc:380
#11 0x00007fffee49f2ac in KisToolProxy::forwardToTool (this=this@entry=0x6030002776a0, state=state@entry=KisToolProxy::CONTINUE, action=action@entry=KisTool::Primary, event=event@entry=0x60160059bbb0, docPoint=...) at /home/appimage/persistent/krita/libs/ui/canvas/kis_tool_proxy.cpp:179
#12 0x00007fffee49f8aa in KisToolProxy::forwardEvent (this=0x6030002776a0, state=state@entry=KisToolProxy::CONTINUE, action=action@entry=KisTool::Primary, event=event@entry=0x60160059bbb0, originalEvent=originalEvent@entry=0x60160059bbb0) at /home/appimage/persistent/krita/libs/ui/canvas/kis_tool_proxy.cpp:138
#13 0x00007fffeef7debe in KisToolInvocationAction::inputEvent (this=<optimized out>, event=0x60160059bbb0) at /home/appimage/persistent/krita/libs/ui/input/kis_tool_invocation_action.cpp:167
#14 0x00007fffeefab0e6 in KisShortcutMatcher::pointerMoved (this=this@entry=0x6022003209b0, event=event@entry=0x60160059bbb0) at /home/appimage/persistent/krita/libs/ui/input/kis_shortcut_matcher.cpp:264
#15 0x00007fffeef5c3e6 in KisInputManager::Private::handleCompressedTabletEvent (this=0x602200320980, event=0x60160059bbb0) at /home/appimage/persistent/krita/libs/ui/input/kis_input_manager_p.cpp:572
#16 0x00007fffeef46aab in KisInputManager::slotCompressedMoveEvent (this=0x604800240550) at /home/appimage/persistent/krita/libs/ui/input/kis_input_manager.cpp:615
#17 0x00007fffea9951df in QMetaObject::activate(QObject*, int, int, void**) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5
#18 0x00007fffebb583e8 in KisSignalCompressor::start (this=0x6022003209c8) at /home/appimage/persistent/krita/libs/global/kis_signal_compressor.cpp:85
#19 0x00007fffeef535bb in KisInputManager::compressMoveEventCommon<QMouseEvent> (this=this@entry=0x604800240550, event=event@entry=0x7fffffffd010) at /home/appimage/persistent/krita/libs/ui/input/kis_input_manager.cpp:228
#20 0x00007fffeef4df63 in KisInputManager::eventFilterImpl (this=0x604800240550, event=0x7fffffffd010) at /home/appimage/persistent/krita/libs/ui/input/kis_input_manager.cpp:331
#21 0x00007fffea96acd1 in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5
#22 0x00007fffeb3b2255 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Widgets.so.5
#23 0x00007fffeb3b963c in QApplication::notify(QObject*, QEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Widgets.so.5
#24 0x00007fffef0adb6c in KisApplication::notify (this=<optimized out>, receiver=0x600e005324f0, event=0x7fffffffd010) at /home/appimage/persistent/krita/libs/ui/KisApplication.cpp:608
#25 0x00007fffea96ae05 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5
#26 0x00007fffeb3b84b9 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Widgets.so.5
#27 0x00007fffeb40663f in ?? () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Widgets.so.5
#28 0x00007fffeb408b53 in ?? () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Widgets.so.5
#29 0x00007fffeb3b227c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Widgets.so.5
#30 0x00007fffeb3b8fa0 in QApplication::notify(QObject*, QEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Widgets.so.5
#31 0x00007fffef0adb6c in KisApplication::notify (this=<optimized out>, receiver=0x6010000e4520, event=0x7fffffffd4f0) at /home/appimage/persistent/krita/libs/ui/KisApplication.cpp:608
#32 0x00007fffea96ae05 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5
#33 0x00007fffeadbab2d in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Gui.so.5
#34 0x00007fffeadbc6e5 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Gui.so.5
#35 0x00007fffead9b12b in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Gui.so.5
#36 0x00007fffdbab630b in ?? () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5XcbQpa.so.5
#37 0x00007fffea96939b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5
#38 0x00007fffea971c14 in QCoreApplication::exec() () from /home/appimage/appimage-workspace/deps/usr/lib/libQt5Core.so.5
#39 0x000000000040a916 in main (argc=1, argv=0x7fffffffe498) at /home/appimage/persistent/krita/krita/main.cc:459
This revision now requires changes to proceed.Dec 29 2018, 11:01 AM

Is there any other place where pixelPrecision value could be hardcoded? Cause if any value other than 16 is passed, it causes an assert.

Is there any other place where pixelPrecision value could be hardcoded? Cause if any value other than 16 is passed, it causes an assert.

If you start a cage transformation with some value already set (e.g. to 20), then it'll work. So the problem is not in "value 16", but in the fact that it changes during the already started stroke.

hellozee updated this revision to Diff 49723.Jan 17 2019, 2:49 PM
hellozee retitled this revision from Added a QSpinBox to adjust preview granularity while doing transforms to Added a QComboBox to adjust preview granularity while doing transforms.
hellozee edited the summary of this revision. (Show Details)
hellozee edited the test plan for this revision. (Show Details)

Instead of a QSpinBox it uses a QComboBox now, since values are more specific, another implementation could be using QSpinBox values as the exponent for calculating the final value.

hellozee updated this revision to Diff 49751.Jan 17 2019, 7:51 PM

Added a couple of comments so that others don't waste a month after this trivial thing like I did

rempt accepted this revision.Jan 19 2019, 1:34 PM
rempt added a subscriber: rempt.

Looks like it works fine, but I would suggest putting the comboboxes underneath each other, since the tool option widget gets quite wide now.

hellozee updated this revision to Diff 49931.Jan 20 2019, 1:26 PM

Switched the orientation to vertical as in horizontal layout the comboboxes needed more screen space

hellozee updated this revision to Diff 49932.Jan 20 2019, 1:32 PM

Brace initialization looks better

dkazakov requested changes to this revision.Jan 20 2019, 2:40 PM

When setting precision of the final result to '2', Krita crashes because of trying to allocate 0xffffffffffffffff bytes. I guess this precision level is not supported by the grid algorithm.

This revision now requires changes to proceed.Jan 20 2019, 2:40 PM
hellozee updated this revision to Diff 49937.Jan 20 2019, 2:43 PM

Precision Level 2 is not supported by the algorithm

dkazakov requested changes to this revision.Jan 20 2019, 5:01 PM

Hi, @hellozee!

I still have a crash when trying to use your option. It looks like the config value gets uninitialized. I don't know how it happens, but it is not initialized :(
Here is the output:

qrc:/touchstrip.qml:3:1: module "org.krita.sketch.components" is not installed
Entering "KisToolTransformConfigWidget::slotGranularityChanged()" m_uiSlotsBlocked = 0
Entering "KisToolTransformConfigWidget::slotGranularityChanged()" value = "32" value.toInt() = 32
Entering "KisTransformUtils::transformDevice()" config.pixelPrecision() = -836173824
ASSERT (krita): "numPoints == m_d->gridSize.width() * m_d->gridSize.height()" in file /home/appimage/persistent/krita/libs/image/kis_cage_transform_worker.cpp, line 174
Aborted
This revision now requires changes to proceed.Jan 20 2019, 5:01 PM
hellozee updated this revision to Diff 49948.Jan 20 2019, 5:11 PM
dkazakov accepted this revision.Jan 21 2019, 9:55 AM

Hi, @hellozee!

The patch looks okay now. Though it lacks a necessary part: saving of the options to the config file. If you promise to finish the feature later, we can push it as it is (with the two stylistic changes I noted inline) :)

plugins/tools/tool_transform2/kis_tool_transform_config_widget.cpp
1315

Please add a safe assert before this line, because the conversion might fail one day and the users will have a data loss:

KIS_SAFE_ASSERT_RECOVER_RETURN(value.toInt() > 1);
plugins/tools/tool_transform2/tool_transform_args.h
351

According to KDE's coding style, variable declarations should be on different lines:
See here: https://techbase.kde.org/Policies/Frameworks_Coding_Style

This revision is now accepted and ready to land.Jan 21 2019, 9:55 AM
hellozee updated this revision to Diff 49981.Jan 21 2019, 10:08 AM
hellozee updated this revision to Diff 49982.Jan 21 2019, 10:10 AM
hellozee marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
hellozee marked an inline comment as done.