Add fill tool transparency mode switch #1590
Conversation
|
This looks very good! I haven't looked at the code, but it is certainly a great improvement regarding UX. |
|
The feature as of right now seems a bit difficult to understand imo. mostly because it's only useful when filling with a color that is not fully opaque. It introduces two concepts: replace and overlay but with no prior knowledge of the implementation, the user won't know what the old default behavior is. Maybe we could call it 'Fill behavior' and then have Another consideration is also to separate the fill with transparency feature, since that in itself could be pretty useful. Making it more explicit that you can do this instead of being aware that filling with replace and 0 alpha erases the existing content. |
This is exactly why I chose the wording I did. With it labelled as "Transparency", what is the user going to think it does? Well they might not immediately understand what it does, but they will at least know the conditions where it makes a difference. I don't think it could be made any more clear that it's only useful when filling with a color that is not fully opaque than labelling it this way. Anyone who is concerned about the behavior of non-opaque filling will know that this has something to do with it and will be able to easily test to understand it and see which behavior they are looking for.
Does it matter what the old behavior is? There isn't really a clear "normal" behavior here as some programs use the replace method as their default or even only fill algorithm. Normal doesn't really convey what it actually does, especially if you haven't worked with translucent filling on Pencil2D before.
I'm not sure what exactly you're envisioning here from a UX perspective. I do agree that making the behavior more explicit (perhaps with an erase mode or something like that) would be a good idea, but I'm not sure what the best way to do that would be yet. I think it's an enhancement that can be added at a later time though. |
I think it matter and I agree it doesn't convey the behaviour, it is however conventional that "source over" is known as "normal" behaviour among blending modes, and source over is what we've used so far, so that should be seen is the normal mode as that is what the users has come to expect from filling.
Fair enough let's ignore this for now. I did not envision anything when I wrote my last comment, I just thought it was more useful to have erase using fill as an actual feature rather than a somewhat hidden one that can be discovered by filling with zero alpha. I will review the PR now, maybe in the future we can improve on it by making more conventional blending modes but for now I think it's fine. |
MrStevns
left a comment
There was a problem hiding this comment.
Functionality looks fine, however I would prefer if we could move the fill mode logic out of the floodfill function.
The algorithm is complex enough in itself and this does not really have anything to do with flood filling but simply changes how it is applied to the targetImage which happens to exist inside the floodfill function.
To be clear, this does not change the default logic. The default fill mode is still the same as it was before. |
| if (properties.fillMode == 0) | ||
| { | ||
| if (qAlpha(fillColor) == 0) | ||
| { | ||
| // Filling in overlay mode with a fully transparent color has no | ||
| // effect, so we can skip it in this case | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Just a minor thing but this could be simplified to:
| if (properties.fillMode == 0) | |
| { | |
| if (qAlpha(fillColor) == 0) | |
| { | |
| // Filling in overlay mode with a fully transparent color has no | |
| // effect, so we can skip it in this case | |
| return; | |
| } | |
| } | |
| if (properties.fillMode == 0 && qAlpha(fillColor) == 0) | |
| { | |
| // Filling in overlay mode with a fully transparent color has no | |
| // effect, so we can skip it in this case | |
| return; | |
| } |
It is however not a must have change though, I will merge the PR.
There was a problem hiding this comment.
This was intentional so the else if check won't be run if fillMode == 0 and qAlpha(fillColor) != 0. A premature and useless optimization? Yes absolutely, but it's not really any worse readability-wise so 🤷
There was a problem hiding this comment.
I'd argue that nested branches for the sake of premature optimization is unnecessary complexity and does decrease readability, eg. I find it easier to read one branch with logic operators (to a certain extend of course) than to consider a nested branch logic but I guess that's highly subjective. It only goes through the branch once, there is no useful optimization to win here, since you're simply checking if the color is alpha based, which is extremely fast.
You're right with only one branch it's not an issue but it is the beginning of unnecessary complexity by branching.
There was a problem hiding this comment.
I also feel like this might be changed to a switch statement at a later time if we get more modes (such as that erase mode), in which case a nested if statement would make it a bit more natural to notice and make that change. I'm not really defending this choice, it's neither here nor there for me. If you want to change it go ahead.
This adds a new "Transparency" combo box to the bucket tool with two options: Overlay, Replace. this changes how the mode in which the tool fills the region, and this is only noticable when you have a translucent or transparent new color. Overlay is the same as the current behavior, where it will draw the new color over top of the existing color of the image. With an opaque color it just seems like it's getting overwridden, but say you are filling an opaque region with a translucent color, then you will still have an opaque region, but with a slighly different color as if the translucent fill was on a layer above. The replace option adds a new alternative behavior for this scenario. It basically clears the region before adding the new color. So if you tried to fill an opaque region with a translucent color, you the opaque region would be replaced with a translucent color matching the color you chose, no blending. This is especially useful if you set the color alpha value to 0, and then it effectively acts as a flood fill eraser.
Here's a more visual description:
UI:
