| Summary: | New Layer From Visible not working properly in 4.4.0 onwards | ||
|---|---|---|---|
| Product: | [Applications] krita | Reporter: | Ahab Greybeard <ahab.greybeard> |
| Component: | Layer Stack | Assignee: | Tiar <tamtamy.tymona> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | halla, raymond.d.lucas, rg0001 |
| Priority: | NOR | Keywords: | regression |
| Version: | nightly build (please specify the git hash!) | ||
| Target Milestone: | --- | ||
| Platform: | Debian stable | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/graphics/krita/commit/d25e08a3c6298f98850e1f22284f30536425eec1 | Version Fixed In: | |
| Sentry Crash Report: | |||
Git commit c1ab1ee97f196e73d8340918e2d9b7a79eafbbca by Agata Cacko. Committed on 18/11/2020 at 18:45. Pushed by tymond into branch 'master'. Fix assert on New Layer From Visible on invisible active layer Before this commit, Krita would assert on New Layer from Visible when used on invisible layer. This commit fixes this. Explanation: Buckle up, it's going to be a ride :) In 2016 there was a problem that invisible layers would show up when they were merged, see bug 359707. https://bugs.kde.org/show_bug.cgi?id=359707 As a solution, "just remove the layers separately instead of merging" strategy was used. In 2018 it turned out that in case when putAfter (the layer Krita puts the merged result above of) is invisible, it gets removed, so then it's null and Krita tries to merge other layers in instead of putAfter, see bug 395903. https://bugs.kde.org/show_bug.cgi?id=395903 As a solution, if putAfter was invisible, one of the visible nodes-to-be-merged were assigned to putAfter to replace the original putAfter node, to make sure that when original putAfter is removed, some other layer is doing its job. (The fact that it's a soon-to-be-merged layer is not a problem). In 2020 it turned out that there is quite a lot of actions that are possible in locked groups. It required some more changes to the code. See bug 406697. https://bugs.kde.org/show_bug.cgi?id=406697 A solution included a change to the relevant code such that invisible putAfter would be unconditionally replaced with one of the nodes from the to-be-merged list, and then if putAfter had no parent, Krita would encounter an assert. Problem with this behaviour: In case of newLayerFromVisible(), mergedNodes list would only contain a root node (which has no parent). If the putAfter is invisible, then it's replaced with "one of" mergedNodes, which means the root node. Which means it's going to assert. However it's not always necessary to replace putAfter with one of the to-be-merged nodes. It's only needed when it's going to be removed. Hence moving the replacing into the if. Because root is not invisible, the list of invisible nodes will be empty and nothing will be replaced and nothing will be removed. NOTE: I also added a new condition: '&& cleanupNodes'. It's not stricly necessary to cover all usecases of mergeMultipleNodesImpl, because the only usecase where cleanupNodes is false is the newLayerFromVisible case (which will fail the first condition). However it makes it more future-proof since the only case those nodes needs to be removed is when cleanupNodes is true (otherwise removing nodes would be a dataloss bug). NOTE2: It's possible that putAfter replacement needs one more check (to see if it is in fact in the invisibleNodes list). I couldn't find a situation where it was needed, though, so I left it as it is. Related: bug 359707, bug 395903, bug 406697 M +15 -8 libs/image/kis_layer_utils.cpp https://invent.kde.org/graphics/krita/commit/c1ab1ee97f196e73d8340918e2d9b7a79eafbbca Git commit d25e08a3c6298f98850e1f22284f30536425eec1 by Agata Cacko. Committed on 19/11/2020 at 15:49. Pushed by tymond into branch 'krita/4.3'. Fix assert on New Layer From Visible on invisible active layer Before this commit, Krita would assert on New Layer from Visible when used on invisible layer. This commit fixes this. Explanation: Buckle up, it's going to be a ride :) In 2016 there was a problem that invisible layers would show up when they were merged, see bug 359707. https://bugs.kde.org/show_bug.cgi?id=359707 As a solution, "just remove the layers separately instead of merging" strategy was used. In 2018 it turned out that in case when putAfter (the layer Krita puts the merged result above of) is invisible, it gets removed, so then it's null and Krita tries to merge other layers in instead of putAfter, see bug 395903. https://bugs.kde.org/show_bug.cgi?id=395903 As a solution, if putAfter was invisible, one of the visible nodes-to-be-merged were assigned to putAfter to replace the original putAfter node, to make sure that when original putAfter is removed, some other layer is doing its job. (The fact that it's a soon-to-be-merged layer is not a problem). In 2020 it turned out that there is quite a lot of actions that are possible in locked groups. It required some more changes to the code. See bug 406697. https://bugs.kde.org/show_bug.cgi?id=406697 A solution included a change to the relevant code such that invisible putAfter would be unconditionally replaced with one of the nodes from the to-be-merged list, and then if putAfter had no parent, Krita would encounter an assert. Problem with this behaviour: In case of newLayerFromVisible(), mergedNodes list would only contain a root node (which has no parent). If the putAfter is invisible, then it's replaced with "one of" mergedNodes, which means the root node. Which means it's going to assert. However it's not always necessary to replace putAfter with one of the to-be-merged nodes. It's only needed when it's going to be removed. Hence moving the replacing into the if. Because root is not invisible, the list of invisible nodes will be empty and nothing will be replaced and nothing will be removed. NOTE: I also added a new condition: '&& cleanupNodes'. It's not stricly necessary to cover all usecases of mergeMultipleNodesImpl, because the only usecase where cleanupNodes is false is the newLayerFromVisible case (which will fail the first condition). However it makes it more future-proof since the only case those nodes needs to be removed is when cleanupNodes is true (otherwise removing nodes would be a dataloss bug). NOTE2: It's possible that putAfter replacement needs one more check (to see if it is in fact in the invisibleNodes list). I couldn't find a situation where it was needed, though, so I left it as it is. Related: bug 359707, bug 395903, bug 406697 (cherry picked from commit c1ab1ee97f196e73d8340918e2d9b7a79eafbbca) M +15 -8 libs/image/kis_layer_utils.cpp https://invent.kde.org/graphics/krita/commit/d25e08a3c6298f98850e1f22284f30536425eec1 *** Bug 427476 has been marked as a duplicate of this bug. *** |
SUMMARY Tested with 4.3.0, 4.4.0, 4.4.1 and the Nov 03 4.4.1 alpha (git 23aa4ae) appimages. (Should that be the 4.4.2 alpha?) If the top layer of the layer stack has visibility turned off then the New Layer From Visible action does not work. It works in 4.3.0 but not 4.4.0 onwards STEPS TO REPRODUCE 1. Make two or more paint layers with content. 2. Turn off the visibility of the top layer 3. Do New Layer From Visible OBSERVED RESULT 3. New Layer From Visible does not happen The log has the folowing entry associated with this: 04 Nov 2020 12:26:11 +0000: SAFE ASSERT (krita): "putAfter->parent()" in file /home/appimage/workspace/Krita_Stable_Appimage_Build/krita/libs/image/kis_layer_utils.cpp, line 1376 EXPECTED RESULT 3. A new layer containing currently visible content should be created. SOFTWARE/OS VERSIONS Krita Version: 4.4.1-alpha (git 23aa4ae) Languages: en_GB, en, en, en_GB, en Hidpi: false Qt Version (compiled): 5.12.9 Version (loaded): 5.12.9 OS Information Build ABI: x86_64-little_endian-lp64 Build CPU: x86_64 CPU: x86_64 Kernel Type: linux Kernel Version: 4.19.0-12-amd64 Pretty Productname: Debian GNU/Linux 10 (buster) Product Type: debian Product Version: 10 Desktop: MATE OpenGL Info Vendor: "NVIDIA Corporation" Renderer: "GeForce GTX 750 Ti/PCIe/SSE2" Version: "4.6.0 NVIDIA 450.66" Shading language: "4.60 NVIDIA" Requested format: QSurfaceFormat(version 3.0, options QFlags<QSurfaceFormat::FormatOption>(DeprecatedFunctions), depthBufferSize 24, redBufferSize 8, greenBufferSize 8, blueBufferSize 8, alphaBufferSize 8, stencilBufferSize 8, samples -1, swapBehavior QSurfaceFormat::DoubleBuffer, swapInterval 0, colorSpace QSurfaceFormat::DefaultColorSpace, profile QSurfaceFormat::CompatibilityProfile) Current format: QSurfaceFormat(version 4.6, options QFlags<QSurfaceFormat::FormatOption>(DeprecatedFunctions), depthBufferSize 24, redBufferSize 8, greenBufferSize 8, blueBufferSize 8, alphaBufferSize 8, stencilBufferSize 8, samples -1, swapBehavior QSurfaceFormat::DoubleBuffer, swapInterval 0, colorSpace QSurfaceFormat::DefaultColorSpace, profile QSurfaceFormat::CompatibilityProfile) Version: 4.6 Supports deprecated functions true is OpenGL ES: false QPA OpenGL Detection Info supportsDesktopGL: true supportsOpenGLES: true isQtPreferOpenGLES: false Hardware Information GPU Acceleration: auto Memory: 16039 Mb Number of Cores: 8 Swap Location: /tmp ADDITIONAL INFORMATION