| Summary: | Safe Assert when painting on selection masks that were vector only. | ||
|---|---|---|---|
| Product: | [Applications] krita | Reporter: | wolthera <griffinvalley> |
| Component: | Tools/Selection | Assignee: | Dmitry Kazakov <dimula73> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | dimula73, halla, lynx.mw+kde |
| Priority: | NOR | ||
| Version: | git master (please specify the git hash!) | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/graphics/krita/commit/6f283ca981fd8ecf3e85ba2fd8859e75e0670b80 | Version Fixed In: | |
| Sentry Crash Report: | |||
|
Description
wolthera
2019-08-28 17:24:23 UTC
This does not crash on Linux, but Dmitry warned it will crash on Windows:
KisSelectionComponent* KisShapeSelection::clone(KisSelection* selection)
{
/**
* TODO: make cloning of vector selections safe! Right now it crashes
* on Windows because of manipulations with timers from non-gui thread.
*/
KIS_SAFE_ASSERT_RECOVER_NOOP(QThread::currentThread() == qApp->thread());
return new KisShapeSelection(*this, selection);
}
Some additional info, for me it also triggers a second SAFE ASSERT when I undo the above steps to reproduce: SAFE ASSERT (krita): "changeList.memento == memento" in file /home/appimage/workspace/Krita_Release_Appimage_Build/krita/libs/image/tiles3/kis_memento_manager.cc, line 278 So it does seem to mess up the undo stack somehow, which sounds a bit worrying to me, even though I didn't really notice anything going wrong so far. The above assert message is from the 4.3.0-beta1 appimage (but master also gives it), so would this be a possible release blocker? Not a release blocker, since it's been this way since 2019, but I'll put it on the list of bugs to discuss during the meeting tomorrow. The first assert seem to be not actual anymore after I refactored vectors (though I'll recheck), but the second one by Lynx3d has caught a real bug: 1) Create a rectangular **pixel** selection 2) Draw a line on it with a brush 3) Create a rectangular **vector** selection at a side of the previous one 4) Draw a line on it with a brush 5) Press Ctrl+Z twice to undo the second selection Result: there is a garbage instead of the selection That second bug seem to be present since 2013 when I first forbidden having both projection and a pixel selection at the same time: https://invent.kde.org/graphics/krita/-/commit/6ab2a6ae8816d4cea29b394b1b50cb1ac57424fd The fix and a testing AppImage is in this MR: https://invent.kde.org/graphics/krita/-/merge_requests/351 Git commit a87dced5f88f26e282dfc86d18a3c32ecf68fa96 by Dmitry Kazakov. Committed on 29/05/2020 at 11:34. Pushed by dkazakov into branch 'master'. Fix undo breakage after converting a pixel selecion into a vector one Since 2013, when a selection is switched into a vector mode, its pixel selection is used as a projection. It is done intentionally, because we need a way to auto-flatten selection when someone opens a transaction on this devices. It lets all our painting tools work on selections smoothly without any conversions. And this switching used to break undo on the projection device. Obviously, the vectors are rendered on the device without any transactions, so we cannot undo them. To overcome this issue, a special mechanism "reincarnation with detached history" for paint devices was introduced (KisPaintDevice::reincarnateWithDetachedHistory). When a device reincarnates, it starts a new undo history from scratch.The old undo history is kept intact inside the returned command, so the device can later be recovered to its original state. As a consequence, there is an important API change in KisSelection, now the seleciton should be converted to vector mode explicitly and undo command for it should be kept in the common image's undo history (that was the reason of the previous huge patch for KoShapeControllerBase). M +15 -0 libs/image/kis_paint_device.cc M +19 -0 libs/image/kis_paint_device.h M +40 -0 libs/image/kis_paint_device_data.h M +152 -28 libs/image/kis_selection.cc M +20 -5 libs/image/kis_selection.h M +38 -19 libs/image/kis_transaction_data.cpp M +2 -0 libs/image/kis_transaction_data.h M +1 -6 libs/image/tests/CMakeLists.txt M +6 -5 libs/image/tests/kis_selection_test.cpp M +2 -2 libs/libkis/Selection.cpp M +2 -6 libs/ui/flake/kis_shape_controller.cpp M +0 -4 libs/ui/flake/kis_shape_selection_model.cpp M +1 -1 libs/ui/kis_selection_decoration.cc M +3 -3 libs/ui/kis_selection_manager.cc M +167 -18 libs/ui/tests/kis_shape_selection_test.cpp M +2 -0 libs/ui/tests/kis_shape_selection_test.h M +1 -1 libs/ui/tests/util.h M +1 -1 plugins/impex/libkra/kis_kra_load_visitor.cpp M +2 -2 plugins/impex/libkra/kis_kra_save_visitor.cpp M +2 -2 plugins/impex/libkra/tests/kis_kra_saver_test.cpp M +4 -4 plugins/impex/libkra/tests/util.h https://invent.kde.org/graphics/krita/commit/a87dced5f88f26e282dfc86d18a3c32ecf68fa96 Git commit 5fcc49fb6126979e03739fa2fd8be063b5c2fc85 by Dmitry Kazakov. Committed on 29/05/2020 at 11:34. Pushed by dkazakov into branch 'master'. Remove an assert when cloning a shape selection The problem seem to have been fixed with my recent shapes' multithreading refactoring that changed all the signal compressors to be thread-safe. M +0 -6 libs/ui/flake/kis_shape_selection.cpp https://invent.kde.org/graphics/krita/commit/5fcc49fb6126979e03739fa2fd8be063b5c2fc85 Git commit 6172584f8512238bba8ceb3cda79fbbc87609dc9 by Dmitry Kazakov. Committed on 29/05/2020 at 11:34. Pushed by dkazakov into branch 'master'. Remove shape tracking from KoShapeControllerBase Long ago, when Flake was flourishing as a cross-application framework, it supported shapes being toplevel shapes (that is, having no parent). It created a lot of troubles, because all the **roots** still had to be stored somewhere and some entity had to track shapes addition and removal from the hierarchy. That was exactly the reason why KoShapeControllerBase was born. Those days it was called KoShapeDocumentBase, which meant "a document that stores all shapes, including the root ones". Legends say that it was also supposed to add/remove shapes from KoShapeManager, but no written proofs of it survived. When Krita introduced vector layers, every vector layer became a flake's "canvas". Every canvas also had a KoShapeManager, that was responsible for painting the shapes. But Krita diverged from the expected way to track the shapes: instead of adding a KoShapeDocumentBase to every vector layer, it added only one "document" for all the layers. This document (now called KisShapeController) dispatched add/remove notifications to the active layer. That was fun, but never worked as expected. Due to complexity of signal delivery and multithreading, it could dispatch add/remove notifications to a wrong layer. It also made managing shapes' lifetime rather cumbersome: when a group shape was destroyed, it had to destroy all the children, but it had *no* link to the controller, so it couldn't notify it! Due to these problems, at some point Krita switched to "no multiple roots" paradigm. Every vector layer started to have exactly one root at the top of the shapes hierarchy (it was the vector layer itself). It solved all the notifications problems: the root could track all its children and manage KoShapeManager properly. It also solved the lifetime problem, since there was a single point where shapes could be added/removed, KoShape::setParent(). After this change KoShapeControllerBase stopped serving its original purpose and was only needed to add lazily create vecotor layers or shape selections, when new shapes were dropped to the canvas. But we still had to pass all shape additions through it. It resulted in a very weird chain of calls, when one wanted to add a new shape to the image: 1) KoShapeController::addShapeDirect() 2) KoShapeCreateCommand::redo() 3) KoShapeControllerBase::addShape() 4) KoCanvasBase::addCommand() # NOTE ^^^: adding a command to the undo stack while another # command is running! This patch breaks this chain. KoShapeControllerBase now doesn't track anything and is not even allowed to track anything. Having multiple toplevel (root) shapes **is now prohibited**. Instead, all the shapes hierarchy is now tracked by the single root shape. To achieve that, special methods were added to SimpleShapeContainerModel to manage KoShapeManager contents (they were moved from ShapeLayerContainerModel). >From now on, the official ways to add a shape to a hierarchy is the following: 1) If you know what parent you want to add the shape to, just create KoShapeCreateCommand() and it will do everything for you (adding and resolving z-index collisions) 2) If you just want to drop the shape to the canvas and want it to be added "somewhere", then use KoShapeController::addShapesDirect(). If will find a suitable layer for the shape (or create a new one). All that will be done in undoable manner. I hope in the next few iterations we will be able to remove KoShapeControllerBase completely from the Krita's codebase. It doesn't do much useful work anymore. It's main reason for exintence now is to provide KisImage size and DPI value. # Conflicts: # libs/flake/KoShapeController.cpp # plugins/flake/textshape/kotext/commands/AddAnnotationCommand.cpp M +1 -0 libs/flake/CMakeLists.txt M +13 -7 libs/flake/KoShapeController.cpp M +5 -5 libs/flake/KoShapeControllerBase.cpp M +9 -31 libs/flake/KoShapeControllerBase.h M +41 -0 libs/flake/SimpleShapeContainerModel.h A +64 -0 libs/flake/commands/KoAddRemoveShapeCommands.cpp [License: GPL (v2+)] A +54 -0 libs/flake/commands/KoAddRemoveShapeCommands.h [License: GPL (v2+)] M +0 -4 libs/flake/commands/KoPathCombineCommand.cpp M +4 -5 libs/flake/commands/KoShapeClipCommand.cpp M +20 -43 libs/flake/commands/KoShapeCreateCommand.cpp M +4 -6 libs/flake/commands/KoShapeDeleteCommand.cpp M +4 -5 libs/flake/commands/KoShapeUnclipCommand.cpp M +30 -30 libs/flake/tests/MockShapes.h M +14 -21 libs/flake/tests/TestPointMergeCommand.cpp M +21 -20 libs/flake/tests/TestPointRemoveCommand.cpp M +9 -11 libs/flake/tests/TestShapePainting.cpp M +5 -3 libs/ui/actions/kis_selection_action_factories.cpp M +42 -46 libs/ui/flake/kis_shape_controller.cpp M +1 -2 libs/ui/flake/kis_shape_controller.h M +6 -11 libs/ui/flake/kis_shape_layer.cc https://invent.kde.org/graphics/krita/commit/6172584f8512238bba8ceb3cda79fbbc87609dc9 Git commit f3d61bf4e99a44dc1eba364f33141a777616afea by Dmitry Kazakov. Committed on 02/06/2020 at 09:49. Pushed by dkazakov into branch 'krita/4.3'. Remove an assert when cloning a shape selection The problem seem to have been fixed with my recent shapes' multithreading refactoring that changed all the signal compressors to be thread-safe. M +0 -6 libs/ui/flake/kis_shape_selection.cpp https://invent.kde.org/graphics/krita/commit/f3d61bf4e99a44dc1eba364f33141a777616afea Git commit 6f283ca981fd8ecf3e85ba2fd8859e75e0670b80 by Dmitry Kazakov. Committed on 02/06/2020 at 09:49. Pushed by dkazakov into branch 'krita/4.3'. Fix undo breakage after converting a pixel selecion into a vector one Since 2013, when a selection is switched into a vector mode, its pixel selection is used as a projection. It is done intentionally, because we need a way to auto-flatten selection when someone opens a transaction on this devices. It lets all our painting tools work on selections smoothly without any conversions. And this switching used to break undo on the projection device. Obviously, the vectors are rendered on the device without any transactions, so we cannot undo them. To overcome this issue, a special mechanism "reincarnation with detached history" for paint devices was introduced (KisPaintDevice::reincarnateWithDetachedHistory). When a device reincarnates, it starts a new undo history from scratch.The old undo history is kept intact inside the returned command, so the device can later be recovered to its original state. As a consequence, there is an important API change in KisSelection, now the seleciton should be converted to vector mode explicitly and undo command for it should be kept in the common image's undo history (that was the reason of the previous huge patch for KoShapeControllerBase). M +15 -0 libs/image/kis_paint_device.cc M +19 -0 libs/image/kis_paint_device.h M +40 -0 libs/image/kis_paint_device_data.h M +152 -28 libs/image/kis_selection.cc M +20 -5 libs/image/kis_selection.h M +38 -19 libs/image/kis_transaction_data.cpp M +2 -0 libs/image/kis_transaction_data.h M +1 -6 libs/image/tests/CMakeLists.txt M +6 -5 libs/image/tests/kis_selection_test.cpp M +2 -2 libs/libkis/Selection.cpp M +2 -6 libs/ui/flake/kis_shape_controller.cpp M +0 -4 libs/ui/flake/kis_shape_selection_model.cpp M +1 -1 libs/ui/kis_selection_decoration.cc M +3 -3 libs/ui/kis_selection_manager.cc M +167 -18 libs/ui/tests/kis_shape_selection_test.cpp M +2 -0 libs/ui/tests/kis_shape_selection_test.h M +1 -1 libs/ui/tests/util.h M +1 -1 plugins/impex/libkra/kis_kra_load_visitor.cpp M +2 -2 plugins/impex/libkra/kis_kra_save_visitor.cpp M +2 -2 plugins/impex/libkra/tests/kis_kra_saver_test.cpp M +4 -4 plugins/impex/libkra/tests/util.h https://invent.kde.org/graphics/krita/commit/6f283ca981fd8ecf3e85ba2fd8859e75e0670b80 Git commit dbbaede31f5b1947957bbfae847c85236818577d by Dmitry Kazakov. Committed on 02/06/2020 at 09:49. Pushed by dkazakov into branch 'krita/4.3'. Remove shape tracking from KoShapeControllerBase Long ago, when Flake was flourishing as a cross-application framework, it supported shapes being toplevel shapes (that is, having no parent). It created a lot of troubles, because all the **roots** still had to be stored somewhere and some entity had to track shapes addition and removal from the hierarchy. That was exactly the reason why KoShapeControllerBase was born. Those days it was called KoShapeDocumentBase, which meant "a document that stores all shapes, including the root ones". Legends say that it was also supposed to add/remove shapes from KoShapeManager, but no written proofs of it survived. When Krita introduced vector layers, every vector layer became a flake's "canvas". Every canvas also had a KoShapeManager, that was responsible for painting the shapes. But Krita diverged from the expected way to track the shapes: instead of adding a KoShapeDocumentBase to every vector layer, it added only one "document" for all the layers. This document (now called KisShapeController) dispatched add/remove notifications to the active layer. That was fun, but never worked as expected. Due to complexity of signal delivery and multithreading, it could dispatch add/remove notifications to a wrong layer. It also made managing shapes' lifetime rather cumbersome: when a group shape was destroyed, it had to destroy all the children, but it had *no* link to the controller, so it couldn't notify it! Due to these problems, at some point Krita switched to "no multiple roots" paradigm. Every vector layer started to have exactly one root at the top of the shapes hierarchy (it was the vector layer itself). It solved all the notifications problems: the root could track all its children and manage KoShapeManager properly. It also solved the lifetime problem, since there was a single point where shapes could be added/removed, KoShape::setParent(). After this change KoShapeControllerBase stopped serving its original purpose and was only needed to add lazily create vecotor layers or shape selections, when new shapes were dropped to the canvas. But we still had to pass all shape additions through it. It resulted in a very weird chain of calls, when one wanted to add a new shape to the image: 1) KoShapeController::addShapeDirect() 2) KoShapeCreateCommand::redo() 3) KoShapeControllerBase::addShape() 4) KoCanvasBase::addCommand() # NOTE ^^^: adding a command to the undo stack while another # command is running! This patch breaks this chain. KoShapeControllerBase now doesn't track anything and is not even allowed to track anything. Having multiple toplevel (root) shapes **is now prohibited**. Instead, all the shapes hierarchy is now tracked by the single root shape. To achieve that, special methods were added to SimpleShapeContainerModel to manage KoShapeManager contents (they were moved from ShapeLayerContainerModel). >From now on, the official ways to add a shape to a hierarchy is the following: 1) If you know what parent you want to add the shape to, just create KoShapeCreateCommand() and it will do everything for you (adding and resolving z-index collisions) 2) If you just want to drop the shape to the canvas and want it to be added "somewhere", then use KoShapeController::addShapesDirect(). If will find a suitable layer for the shape (or create a new one). All that will be done in undoable manner. I hope in the next few iterations we will be able to remove KoShapeControllerBase completely from the Krita's codebase. It doesn't do much useful work anymore. It's main reason for exintence now is to provide KisImage size and DPI value. M +1 -0 libs/flake/CMakeLists.txt M +13 -9 libs/flake/KoShapeController.cpp M +5 -5 libs/flake/KoShapeControllerBase.cpp M +9 -31 libs/flake/KoShapeControllerBase.h M +41 -0 libs/flake/SimpleShapeContainerModel.h A +64 -0 libs/flake/commands/KoAddRemoveShapeCommands.cpp [License: GPL (v2+)] A +54 -0 libs/flake/commands/KoAddRemoveShapeCommands.h [License: GPL (v2+)] M +0 -4 libs/flake/commands/KoPathCombineCommand.cpp M +4 -5 libs/flake/commands/KoShapeClipCommand.cpp M +20 -43 libs/flake/commands/KoShapeCreateCommand.cpp M +4 -6 libs/flake/commands/KoShapeDeleteCommand.cpp M +4 -5 libs/flake/commands/KoShapeUnclipCommand.cpp M +30 -30 libs/flake/tests/MockShapes.h M +14 -21 libs/flake/tests/TestPointMergeCommand.cpp M +21 -20 libs/flake/tests/TestPointRemoveCommand.cpp M +9 -11 libs/flake/tests/TestShapePainting.cpp M +5 -3 libs/ui/actions/kis_selection_action_factories.cpp M +42 -46 libs/ui/flake/kis_shape_controller.cpp M +1 -2 libs/ui/flake/kis_shape_controller.h M +6 -11 libs/ui/flake/kis_shape_layer.cc M +9 -5 plugins/flake/textshape/kotext/commands/AddAnnotationCommand.cpp https://invent.kde.org/graphics/krita/commit/dbbaede31f5b1947957bbfae847c85236818577d |