Fix a few cases where click fill and fill drag behaviour didn't work as expected

[MrStevns]

• Filling on layer below when reference layer was all layers, would not allow stroke filling when applying on non transparent color
• Filling with overlay would result in multiple undo operations for one paint operation...
• Click fill with tablet would result in two fill events. (reported as fixed by Jose)

[MrStevns]

Thanks for reviewing Scribble 🙏

I tried to review this, but I need to understand the intent of this a little bit more first. Also I am unable to replicate either the multiple tablet event issues so @Jose-Moreno will have to test those parts at least.

I tested the implementation with Jose and we both concluded that it seems to work as expected now. My understanding is that it's an Windows only issue but I'm not sure why it happens... presumably a Qt bug. I haven't experienced it on my macbook or virtual linux machine either.

This is a question for the team: How do you expect the fill drag behavior to work? To me, when you first press down, the color of the pixel at that location on the reference image (let's call this the origin color) is the only color filled for the duration of the fill drag. Each time the pointer moves it checks if the pixel color at the current location of the reference image, and if it matches the origin pixel (or perhaps matches within the color tolerance if that is enabled), then it does a flood fill using the reference layer to determine the fill region, and writing to the target layer using the specified blend mode. Based on the code, it doesn't look like it was doing that, and it is definitely not do that now. Not only is it filling all transparent areas, its behavior is tied to the the contents of the target layer which I just don't understand why that would be desirable.

The way I envisioned it is also based on the way that the behavior works in Clip Studio Paint (from my own testing and comparison), where even if the the reference mode doesn't look at all layers, you should never fill multiple times on the same color, even if that color ends up on a different layer.

In practice it should mean that there's no difference between the reference mode when it's about determining whether it should fill while dragging, which imo. improves the user experience.

When it comes to undoing and fill drag, the behavior I have observed (both on master and in this PR) is one undo event per region filled while dragging. Is this what we want? I think it's nice this way, but if you're filling a lot of smaller regions it could quickly fill up the undo/redo stack.

I too think this is a nice behavior, however we could merge the undo elements based on whether they are created within a given timespan, like if you move quickly and it fills 10 regions within 1 second or so, then it will count as one undo action, otherwise a new one is created. This might be out of scope for the current undo implementation but it's possible with the undo/redo rework PR.

[MrStevns]

It would be nice to have this merged as it's also contains some fixes related to tablet behavior. I still believe that the implementation works as intended and there's tests to back it up but of course we should agree on the changes nevertheless

[chchwy]

Will be looking at this PR soon.

[chchwy]

I would suggest making another PR for the mouse/table event things next time. Let's focus on one thing at a time.

[MrStevns]

Noted 👍 . At the time it made sense for me because the tablet bug was reproducible and somewhat related to the drag behavior but yes in hindsight I should have done that

[MrStevns]

Any news regarding this PR, would it make it easier to review if I moved the double click event logic out?

[J5lx]

I’ll take a look in a moment.

[MrStevns]

PR should be ready for review again

[davidlamhauge]

Basically, I like the fill tool. I like the expand fill, and I also like the possibility to fill on the layer below. On the other hand, I must side with Jakob, that there are many combinations, and it can be confusing. A week ago I myself ruined a background for a project, because I accidently made a fill on the layer below, and when I realised what I'd done, the background was beyond repair. It was 100% my fault, but I'd wish there was some warning or indication, that I was about to fill on another layer. I will probably not be the only one making that mistake. Besides this bad experience, I am very pleased with the fill tool.

[J5lx]

I’ve cherry-picked the tablet event fix onto master.

@davidlamhauge Have you tried the fill behaviour changes from this PR?

[davidlamhauge]

@J5lx No, I was commenting on your worries about too many combinations, and the possibility to make mistakes. I have time to test it tomorrow. Anything I should test in particular?

[MrStevns]

Thanks for reviewing again Jakob and for the comments David 😃

However, I feel like talking about how the feature should behave is a little too late or not relevant as far as this PR goes.. the functionality is already there, this PR was only meant to fix bugs regarding existing functionality, not reinvent or remove the existing implementation. Can't we take this discussion in some other way that doesn't involve stalling the PR or derailing its purpose?

@J5lx As far as the tool goes, you don't have to use all its features, just like blending modes, each feature is a something that can be used for a certain task or workflow. Of course if you feel like you're developing a distaste for the tool because it's getting too complex, then we should look into that.

[MrStevns]

I will try to write up how I envisioned the feature, with some examples, so we can all get a clear understanding of the logic. I should have some time later today.

[J5lx]

Can't we take this discussion in some other way that doesn't involve stalling the PR or derailing its purpose?

I’m very sorry. I didn’t want to make you feel like your work was being artificially held up or sidetracked. I need to be more careful with my words.

The entire remark about the complexity of the fill tool has nothing to do with the changes introduced by this PR and is just an expression of my frustration at the unique challenges of reviewing them. Not because these particular changes are especially complex or anything, but because the fill tool in general, with its various parameters and the many possible combinations of them, is complex per se (in regard to its inner workings, not so much in regard to its usefulness in various workflows). And I’m not even sure if there’s much that can be done about it. That is, I don’t necessarily think it’s too complex, I just think it’s complex.

The reason I was still talking about intended functionality is not because I want to change it or turn this PR into something else, but because I feel that I should understand the intended functionality in order to properly assess whether this code implements it and I’m unsure if my current understanding of it is correct. More specifically, the current behaviour (of this PR) didn’t feel right for me and I wasn’t sure if that was because there’s actually something wrong with it or just because I misunderstood the intention behind it. The behaviour has changed throughout this PR in response to feedback and I wanted to make sure that all of it is still part of the fixes.

Maybe I’m being too fastidious. Or maybe this stuff is just too much for me and I should have left the review for someone more competent on this matter. Either way, I’m sorry I came off the wrong way. I’ll try to be more careful in the future and I’ll see if I can work on my approach to be more targeted. I’m looking forward to your write-up.

[MrStevns]

I’m very sorry. I didn’t want to make you feel like your work was being artificially held up or sidetracked. I need to be more careful with my words.

It's hard to convey emotions through text, and depending on what time of the day one reads and answers something, intend and understanding may differ. No need to apologize 😅

The entire remark about the complexity of the fill tool has nothing to do with the changes introduced by this PR and is just an expression of my frustration at the unique challenges of reviewing them. Not because these particular changes are especially complex or anything, but because the fill tool in general, with its various parameters and the many possible combinations of them, is complex per se (in regard to its inner workings, not so much in regard to its usefulness in various workflows). And I’m not even sure if there’s much that can be done about it. That is, I don’t necessarily think it’s too complex, I just think it’s complex.

That one is mostly on me, I should have written down the functionality the first time I made this proposal, so it would be clear how I intended it to work and so that in the future we would be to read up on its functionality. Hopefully the following write up makes it clearer 😄

The reason I was still talking about intended functionality is not because I want to change it or turn this PR into something else, but because I feel that I should understand the intended functionality in order to properly assess whether this code implements it and I’m unsure if my current understanding of it is correct. More specifically, the current behavior (of this PR) didn’t feel right for me and I wasn’t sure if that was because there’s actually something wrong with it or just because I misunderstood the intention behind it. The behavior has changed throughout this PR in response to feedback and I wanted to make sure that all of it is still part of the fixes.

That also makes sense, it's difficult to review something if it's too complex to understand from code alone or simply has too many combinations to understand the intend. It's true that the behavior has changed a few times during the lifecycle of the PR, either to address a bug or me realizing an odd behavior.. so it's a good thing to have this clarified once a for all and not basing it on loose assumptions or what I say and don't say, is right.

Understanding the feature

The idea behind the drag to fill behavior is that you should be able to apply several fills in an easy and less error prone manner than manually clicking on a pixel several times, and we can do that because we know where you started. To keep things simpler, we should be able to apply the following rules in all cases:

1. You should be able to drag across and not fill any other pixel than transparent when the start drag position is a transparent pixel.
2. When the start drag position is a colored pixel, only a pixel of similar (based on tolerance) color or an area which is transparent will be filled.

Also, the rules assume that the pixel to be compared against is found on the reference layer, ie. either the current or all layers based on the reference setting. This was how I envisioned it anyway...

How I tested:

Original image: project: fill-drag-pr.pclx.zip

Bucket settings:

Blend mode: replace Reference mode: current layer

Filling on the same layer

Rule	Master	PR	Functionality	Conclusion
You should be able to drag across and not fill any other pixel than transparent when the start drag position is a transparent pixel.			same	Works as intended
When the start drag position is a colored pixel, only a pixel of similar (based on tolerance) color or an area which is transparent will be filled.:			same	Works as intended

Filling on layer below

Rule	Master	PR	Functionality	Conclusion
You should be able to drag across and not fill any other pixel than transparent when the start drag position is a transparent pixel.			Differ, on master the target layer avoids filling where the reference color is, even though it's a different layer. On the PR branch, we don't take this into account anymore.	PR algorithm is more loose about where to fill, is this better or worse? I'd say worse.. I think the master implementation is better.
When the start drag position is a colored pixel, only a pixel of similar (based on tolerance) color or an area which is transparent will be filled.:			Differs, on master the start pixel is filled and all other colors that are not the same or transparent are ignored. In the PR, the white background is kept, and filling happens on all reference points, because the fill to layer is blank.	PR algorithm doesn't work entirely, the white pixels should have been filled and as with the Rule1, we should try to be smarter about where we paint, even if we're working on a different layer.

In other words... if we consider the initial implementation the best outcome, then there's still code to be written in order to make sure the rules apply, when painting on the layer below.

[MrStevns]

After having tried a few things, I'm pretty sure i have the algorithm sorted out now... now the only problem is that I don't like the fillTo feature anymore 😆

The reason is that when it's correctly implemented (ie. not the result of master...) then it's only usable when the reference is all layers, at least if you consider the rules I wrote down in the previous post and the test project.

For example:

I start by filling the layer below with a green color, at this point the reference is set to 'all layers', and both reference and target pixel is transparent, thus it works. Next I change the color to Purple and fill again, as I try it out with reference: 'all layers', it continues to work fine because we're scanning all layers, because both the reference and target color is found. This however doesn't work when I afterwards change to reference: 'current layer', because in this case the reference pixel is still transparent, only the target pixel is green and therefore it doesn't fill.

I thought it was a pretty cool feature in the beginning, because you would be able to both draw the lines of your animation and do the filling efficiently after, without having to jump between layers but the additional complexity it adds to the drag algorithm, makes me think otherwise.

[MrStevns]

Thanks for reviewing again Jacob, I've made a few smaller changes to make up for the more limited functionality of the fillTo layer mechanism now and fixed a bunch of typos. I have also merged your PR, so I'd say it's ready to be reviewed again