Bug 359707 - Merging invisible layers unhides them
Summary: Merging invisible layers unhides them
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Unclassified
Component: Layer Stack (show other bugs)
Version: 2.9.11
Platform: Microsoft Windows Microsoft Windows
: NOR normal
Target Milestone: ---
Assignee: Krita Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-23 16:35 UTC by katearcher89
Modified: 2020-11-19 15:49 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description katearcher89 2016-02-23 16:35:10 UTC
If you select some layers, both visible and not, the resulting layer has both that layers combined, i.e. Krita considers invisible layer as visible(although the layer is hidden, i.e. has 0% opacity) and when the merging performs all layers are treated like they are visible and only layers opacity setting is counted in(i.e. if the layer had 45% opacity before it was hidden, it will merge with other layer at 45% opacity).

This is kinda frustrating when combining a lot of layers to reduce memory footprint to go through all the layers and delete those that were hidden before.

How to reproduce:
1. Create two layers
2. Paint anything on both of them
3. turn one layer of
4. merge them.

Expected: If the merging is executed, count hidden layers as layers having 0% opacity.
Comment 1 Halla Rempt 2016-03-07 07:28:08 UTC
Hi,

Thanks for your report. I can confirm the issue in 3.0 (haven't got 2.9 on this laptop). It's a regression, since this used to work correctly.
Comment 2 Dmitry Kazakov 2016-03-15 13:30:48 UTC
Git commit ded5b95703aa4edc0a9392376ebffaef36d0101b by Dmitry Kazakov.
Committed on 15/03/2016 at 13:23.
Pushed by dkazakov into branch 'master'.

Fix merging invisible layers

They should be just deleted and not merged

M  +187  -34   libs/image/kis_layer_utils.cpp
M  +45   -0    libs/image/kis_layer_utils.h
M  +2    -1    libs/ui/kis_layer_manager.cc
M  +18   -95   libs/ui/kis_node_juggler_compressed.cpp

http://commits.kde.org/krita/ded5b95703aa4edc0a9392376ebffaef36d0101b
Comment 3 Halla Rempt 2016-03-16 15:47:49 UTC
Git commit b57216a6556f72e54492331b9e294a94b979bb9b by Boudewijn Rempt, on behalf of Dmitry Kazakov.
Committed on 16/03/2016 at 07:02.
Pushed by rempt into branch 'rempt-osx-port'.

Fix merging invisible layers

They should be just deleted and not merged

M  +187  -34   libs/image/kis_layer_utils.cpp
M  +45   -0    libs/image/kis_layer_utils.h
M  +2    -1    libs/ui/kis_layer_manager.cc
M  +18   -95   libs/ui/kis_node_juggler_compressed.cpp

http://commits.kde.org/krita/b57216a6556f72e54492331b9e294a94b979bb9b
Comment 4 Tiar 2020-11-18 18:47:31 UTC
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 428683, bug 395903, bug 406697

M  +15   -8    libs/image/kis_layer_utils.cpp

https://invent.kde.org/graphics/krita/commit/c1ab1ee97f196e73d8340918e2d9b7a79eafbbca
Comment 5 Tiar 2020-11-19 15:49:29 UTC
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 428683, 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