Automatically create transform masks on layer types that cannot be transformed
Needs RevisionPublic

Authored by scottpetrovic on Feb 6 2019, 7:15 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Summary

This addresses this usability issue that is mentioned on this request

File Layer doesn't support transform tool, but leaves user guessing
https://phabricator.kde.org/T4595

This patch tries to grab all the layer types that run into this situation including group layers, file layers, and clone layers. If someone activates the transform tool when over these layer types, a new transform layer is automatically created as a child of it and selected.

Test Plan
  1. Add a group layer and put a paint layer in it. Then try to select the group layer and press Ctrl + T to activate the transform tool. Results: transform layer is created in group layer
  1. Add a file layer. Press Ctrl+T to activate the transform tool. Results: A transform layer is created as a child of the file layer
  1. Add a paint layer then clone it. Press Ctrl + T to activate the transform tool. Results: A transform layer is created as a child of the clone layer.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
scottpetrovic created this revision.Feb 6 2019, 7:15 PM
Restricted Application added a project: Krita. · View Herald TranscriptFeb 6 2019, 7:15 PM
scottpetrovic requested review of this revision.Feb 6 2019, 7:15 PM
dkazakov requested changes to this revision.Feb 8 2019, 9:54 AM
dkazakov added a subscriber: dkazakov.

Hi, @scottpetrovic!

I'm sorry, but there are a few bugs in the patch:

  1. createAndUseTransformIfNeeded() should be called from startStroke(), not from activate(). activate() is called only once, when the tool is switched into, but startStroke() is called every time the user clicks om the canvas
  2. Group layers are successfully transformed by the transform tool, when the tool is in recursive mode. So it should be considered in the check.
  3. Creation of the mask should be created either with KisNodeCommandsAdapter (returned by KisNodeManager) or manually inside the transform stroke (very complicated). Right now the node is created without undo information, which breaks undo.
This revision now requires changes to proceed.Feb 8 2019, 9:54 AM

@dkazakov

I took care of most of the points, so the the undo works now with it. I moved the function to the startStroke area as well. I also fixed a recent regression where transforming doesn't work on things like clone layers and vector layers.

The one point I need a little assistance on is knowing if a group layer is in "recursive mode". I see a function in the KisGrouplayer class, but it is not clear what is going on or how I would do a check that state of the group layer

dkazakov requested changes to this revision.Mar 29 2019, 2:42 PM

From a quick glance, it seems like point 2 is not fixed yet:

Group layers are successfully transformed by the transform tool, when the tool is in recursive mode. So it should be considered in the check.

Basically, in recursive mode, group layers should be transformed directly, without any masks

This revision now requires changes to proceed.Mar 29 2019, 2:42 PM

Updated logic with recursive mode and group layers. The added mask is skipped now in that situation

scottpetrovic added a comment.EditedMar 29 2019, 3:52 PM

Just a note about that "work recursive" toggle on the tool options for the transform tool.

That property seems to be forcing its value on and off with every start stroke...which is making using it kind of confusing.

if (m_optionsWidget) {
    m_workRecursively = m_optionsWidget->workRecursively() ||
        !currentNode->paintDevice();
}

I am not sure why this logic is there in the startStroke(), but maybe there is a better way to manage the state of that bool

Just a note about that "work recursive" toggle on the tool options for the transform tool.

That property seems to be forcing its value on and off with every start stroke...which is making using it kind of confusing.

if (m_optionsWidget) {
    m_workRecursively = m_optionsWidget->workRecursively() ||
        !currentNode->paintDevice();
}

I am not sure why this logic is there in the startStroke(), but maybe there is a better way to manage the state of that bool

The logic under this check was that Group Layers doesn't have a paint device and, therefore, should be transformed recursively. I guess you should just change `!currentNode->paintDevice()` into `dynamic_cast<KisGroupLayer*>(currentNode.data())`. I don't think that group layers need this auto-transform-mask functionality.

dkazakov requested changes to this revision.Apr 1 2019, 9:55 AM

Hi, @scottpetrovic!

There are two very small issues still:

  1. When clicking with the transform tool on a Clone Layer, the click only creates the mask, but doesn't start the transformation. The user should click the second time to start the thransformation. I would expect the transformation to be started automatically right after the first click.
  1. [recursive + group layers] There are two solutions:
    • Whatever the state of the "recursive" option, group layers are always transformed recursively. Mask is never created.
    • In recursive mode group layers are transformed recursively. In non-recursive mode, a transform mask is created.

I guess the latter approach is more logical, but I have no strong preference. Please choose whatever approach you like more and remove wither the checks in startStroke() or in createAndUseTransformIfNeeded()

Otherwise the patch looks fine and can be pushed as soon as these minor issues are cleared.

plugins/tools/tool_transform2/kis_tool_transform.cc
1028

If you decide that group layers should always we transformed without transform mask, then this check should go. If you don't decide, then not :)

This revision now requires changes to proceed.Apr 1 2019, 9:55 AM

I mostly just added a bit of logic to get the handles to show up faster. The issue with the handles not appearing instantly was because layer changes on the transform tool messes up the paint() from fully rendering.

I tried a couple different approaches for this, but the one I am submitting seemed to work the best. I first tried to have some signal being fired when the layer changes, but things couldn't get in sync and the paint() was not firing enough.

This fixes the mask layer instantly showing the handles...and it also makes handles instantly show when changing between two normal paint layers. You don't have to click on the canvas to make them show any longer.

With the group layers and the handles immedietly showing up...I noticed a group layer initially doesn't show the handles. If you select a child layer, then select the group layer again, it shows up.

There must be some calculation a group layer does that prevents the handles from showing up immediately when it is originally trying to be transformed.

dkazakov requested changes to this revision.Apr 2 2019, 10:33 AM

Hi, @scottpetrovic!

Now there are two severe regressions :(

  1. When pressing Enter key during transform, the transformation is applied and in a moment after that started again. So basically, there is no way to finish the transform without switching the tool.
  1. When clicking on a Clone Layer multiple times, multiple transform masks are created.
This revision now requires changes to proceed.Apr 2 2019, 10:33 AM

We might need a bit of discussion about both of these points to figure out what needs to happen.

For point 1 and "finishing" the transform. This might need some discussion with artists. We might need to change our definition of what "finishing" a transform means. I think some people are not expecting the transform tool to finish until they move to another tool. I didn't even think about this until I saw a @gdquest video on how he is switching layers and doing multiple transform while staying in the transform tool. Right now he has to click on the canvas again to "re-activate" the transform handles...which for him is a limitation. Here is the video at the time... https://youtu.be/33Hym2srxpo?t=238

For point 2, I am not sure what a better UX would be. Right now, you cannot do transforms on a clone layer, so clicking a clone layer just leaves the person guessing (referenced in this ticket https://phabricator.kde.org/T4595). Maybe unexpected is a better word than regression. It either needs to go to the existing transform mask if one exists, or it needs to do what it is doing now. I have a preference to leaving it the way my patch is. The reasoning is that we don't know if someone wants to create a new mask. If they are selecting the clone layer, that might mean they do not want to use the existing mask...but to create a new one.

Hi, @scottpetrovic!

For point 1 and "finishing" the transform. This might need some discussion with artists. We might need to change our definition of what "finishing" a transform means. I think some people are not expecting the transform tool to finish until they move to another tool. I didn't even think about this until I saw a @gdquest video on how he is switching layers and doing multiple transform while staying in the transform tool. Right now he has to click on the canvas again to "re-activate" the transform handles...which for him is a limitation. Here is the video at the time... https://youtu.be/33Hym2srxpo?t=238

The problem is that the person cannot see the final hi-quality result until he presses Enter key and "finishes" the transform operation. So we must let the user to finish it.

The thing mentioned by @gdquest is a bit different and unrelated. It is about activation the tool again, not about finishing the transform :)

For point 2, I am not sure what a better UX would be. Right now, you cannot do transforms on a clone layer, so clicking a clone layer just leaves the person guessing (referenced in this ticket https://phabricator.kde.org/T4595). Maybe unexpected is a better word than regression. It either needs to go to the existing transform mask if one exists, or it needs to do what it is doing now. I have a preference to leaving it the way my patch is. The reasoning is that we don't know if someone wants to create a new mask. If they are selecting the clone layer, that might mean they do not want to use the existing mask...but to create a new one.

Each transform mask in a stack slows down rendering significantly. More than that the user might even collapse the clone layer (why keeping this mask visible, if I can just click on a layer and transform it?) and still transform it with the transform tool. I believe teh tool should reuse the existing mask as much as possible.

Here is video illustration:

@dkazakov -- I think I understand your points. I think the video was meant to be for artists for them to help decide how they want this tool and functionality to work. The current functionality in stable is not how some artists want it. Maybe I can make a video to help communicate the different directions that the tool could go.

Where my changes start overlapping with @gdquest's idea is it has to do with layer changing. When a clone layer creates a mask and moves to it, the transform tool gets "de-activated", so the bounding box disappears. You are asking me to change how the transform tool deals with layer changes. I am trying to figure out what needs to happen. Should the previous transform operation be reset? Should it automatically commit and let you start transforming the new layer? I am not sure and was hoping artists could help decide based on how they use it.

The same situation goes for point 2. You seem to be wanting to limit the transform layers to 1 for clone layers because of performance. I am ok with this, but wanted to get other artists opinions on how they think this should work.

Where my changes start overlapping with @gdquest's idea is it has to do with layer changing.

I still believe that @gdquest meant a bit different thing.

When a clone layer creates a mask and moves to it, the transform tool gets "de-activated", so the bounding box disappears. You are asking me to change how the transform tool deals with layer changes. I am trying to figure out what needs to happen. Should the previous transform operation be reset? Should it automatically commit and let you start transforming the new layer?

The current behavior (before the patch) is consistent: when some interfering action happens: either layer is changed or some other user action is started, the result of the transformation is committed. No information is lost.

With the patch, even Esc key doesn't work, which can make the user (e.g. me) very angry.

The same situation goes for point 2. You seem to be wanting to limit the transform layers to 1 for clone layers because of performance. I am ok with this, but wanted to get other artists opinions on how they think this should work.

Please don't forget why we started to work on this feature in the first place: we don't want the user "to create a transform mask anywhere", instead, we want to let the user "edit uneditable layers". The user doesn't want to know anything about the transform masks or any weird things like that. He just wants to click on a layer and transform that. If the user can do that without knowing what "transform mask" is, and without shooting into his own foot (by creating a dozen of junk masks), then our goal is reached. Don't overcomplicate things :)

This patch currently does layer changing to get to the newly created mask, which is what would be overlapping with gdquest's situation. If we want the new mask to automatically activate, I will have to figure out a different way to automatically activate it. Currently in Krita switching to a different layer de-activates the transform tool boundaries