Fix a few cases where click fill and fill drag behaviour didn't work as expected #1667
Conversation
- Filling on layer below when reference layer was all layers, would not allow stroke filling - Filling with overlay would result in multiple undo operations for one paint operation... - Click fill with tablet would result in two fill events.
There was a problem hiding this comment.
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.
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.
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.
|
Thanks for reviewing Scribble 🙏
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.
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.
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. |
- Use compareColor instead of raw pixel comparison for the ability to fill when the pixel is in average the same.
b818f6c to
895fae0
Compare
895fae0 to
a197d52
Compare
a46e3b7 to
e278c1e
Compare
e278c1e to
27d73c0
Compare
|
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 |
|
Will be looking at this PR soon. |
|
I would suggest making another PR for the mouse/table event things next time. Let's focus on one thing at a time. |
|
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 |
|
Any news regarding this PR, would it make it easier to review if I moved the double click event logic out? |
|
I’ll take a look in a moment. |
There was a problem hiding this comment.
Sorry for the wait, I needed to do something else for a while because thinking about all the different cases to handle made my head spin. Like I said earlier, I’ve been able to reproduce the last two issues and can verify that they’re fixed, but not the first one. Thanks for helping me with that!
Right now there are still some cases that don’t seem quite right to me. I’ve also made some cosmetic changes which I’ll push in a moment. Other than that the changes look good to me all in all.
The real problem causing this has been fixed via pencil2d#1729
Layer setup: Bitmap Layer Stroke Bitmap Layer Fill 1. draw a circle on the stroke layer and slice it up with lines, so you can fill individual parts 2. set fillTo mode to "layer below" and reference to "all layers" 3. fill and drag a color, eg. red.. across the slices 4. Change fillTo mode to "current layer" 5. repeat step 3 with a different color
|
PR should be ready for review again |
There was a problem hiding this comment.
Tbh, the way the effect of the fill tool varies based on the target layer setting doesn’t seem quite right. Either that or I just have a different concept of how target and reference layer should affect the fill behaviour. Anyway, I’m starting to develop a strong distaste of the fill tool… there’s just so many combinations of parameters to take into account… I feel like at some point we should add some human-readable documentation (and not just tests) on how the tool is supposed to behave (though not necessarily in this PR).
Code looks fine all in all… but I’m at a point where I’d like a (non-code) review of the functionality from at least one other person before we proceed with this PR. For now, thanks for all your hard work!
|
Basically, I like the fill tool. I like the expand fill, and I also like the possibility to fill on the layer below. |
|
I’ve cherry-picked the tablet event fix onto master. @davidlamhauge Have you tried the fill behaviour changes from this PR? |
|
@J5lx No, I was commenting on your worries about too many combinations, and the possibility to make mistakes. |
|
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 |
|
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. |
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. |
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 😅
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 😄
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. Understanding the featureThe 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.
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: Bucket settings:Blend mode: replace Filling on the same layer
Filling on layer below
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. |
|
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. |
There was a problem hiding this comment.
I finally got around to taking a good look at this again (it’s taken me far too long, sorry!). Unfortunately it seems the bug with the multiple undo operations seems to have crept back in, and occurs irrespective of the blend mode now. But for a mercy, at least with the current version, I think fixing it might be relatively simple unless I’m overlooking something, so I’ll just make a PR. Other than that, the code seems to be fundamentally quite sound now in the sense that it seems to work as intended and there is no other unintended behaviour left.
However, I also agree with your verdict on the fill-to feature. I think you summed it up pretty accurately, and I don’t have much to add there. That said, keeping your previous comment about (not) derailing the PR in mind, if my PR finds your approval, I’d say it’s fine to merge this once those changes are in, since the current version is quite coherent. Then the fill-to feature can potentially be discussed separately. Does that sound alright to you?
|
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 |
- Also fixed a few confusing variables
…-bugs Fix / simplify allowFill()
3b14075 to
343dfe9
Compare
Uh oh!
There was an error while loading. Please reload this page.