T3589: Create smart fill brush task. Added new tool to seamlessly fill in areas in the image.
Demo is available here: https://youtu.be/jI87VzDtkPY
Details
Details
- Reviewers
dkazakov rempt - Group Reviewers
Krita - Commits
- R37:d1bba92b0c1a: Healing brush tool
R37:ec5837d18ab5: Healing brush tool
R37:aca2d1bcef71: Healing brush tool
Note that this code runs much slower when compiled in debug mode than in release mode. Please, test release for performance issues.
- Press the "bandaid" tool.
- Use brush to paint over the area you want patched.
- Area should be replaced with reasonable image that blends with the image around it.
Diff Detail
Diff Detail
- Repository
- R37 Krita
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Works great! I've put a few questions inline, but nothing major.
| libs/ui/kis_mask_manager.cc | ||
|---|---|---|
| 234 | Shouldn't this be called createInpaintMask or something like that? | |
| libs/ui/widgets/KisVisualColorSelector.cpp | ||
| 331 ↗ | (On Diff #13172) | If the assert is no longer relevant, please just remove the line. |
| plugins/tools/tool_smart_patch/kis_inpaint.cpp | ||
| 471 | Does this mean the inpainting tool should be disabled for now when the color depth is other than 8 bits? | |
| 806 | Page 6 of what? | |
Comment Actions
I added support for all color depths to inpaint tool. It was planned all along but slipped my mind.
Addressed reviewers comments.
Comment Actions
Hi, @eingerman!
There is an architectural problem with the tool atm, which is extremely dangerous. The tool creates a node (inpaint mask) without any locking, syncing or undoing. Actually, we dropped an idea of having tool previews in a layer stack about 4 years ago. Instead of having previews in the stack, one should use tool overlay. Technically, override KisTool::paint() method and draw the stuff.
Please also note that you cannot call KisToolFreehand::beginPrimaryAction(event) when a macro is started. You cannot just start a stroke inside a legacy macro action. It is undefined behavior that can lead to hangups and crashes.
Basically, you should follow the pattern of KisLiquifyTransformStrategy, where we reimplement painting, instead of reusing too-heavy freehand tool. And in the end of the action just start a stroke-based action that does all your work in a threaded way.
One more question is, why you create your own ImageData class when we already have KisFixedPaintDevice for the same purpose?
We can discuss it in IRC/hangouts if needed.
Comment Actions
I guess that one reason to use a mask and not a canvas decoration is that the inpaint tool can be used with any brush engine. Another reason might be that the colorize tool also uses a mask?
There do seem to be update problems, though, after the stroke is applied, and I wonder whether it makes sense to have an explicit apply button instead of applying the effect after mouse release -- that would make it easier to mask out some area very precisely.
Comment Actions
I doubt there is any use in allowing multiple brush engines for painting healing selection. It'll create more troubles than benefits, like in Colorize mask, where it is still in todo to disable incompatible presets for it. I guess simple pixel brush is the only thing compatible with it.
Another reason might be that the colorize tool also uses a mask?
Colorize mask is a final data, not preview. It information is saved into .kra and can be reused by the user. Here it is just a preview. Last time we used masks as a preview were selection masks. It was around 2010 and it was almost impossible to make them render correctly. It was exactly the case why that functionality was disabled until we put it into Kickstarter 2014 stretchgoal and rewrote them using the overlay.
There do seem to be update problems, though, after the stroke is applied, and I wonder whether it makes sense to have an explicit apply button instead of applying the effect after mouse release -- that would make it easier to mask out some area very precisely.
The update problems might be easily caused undefined behavior of the incorrect usage of strokes system. Every new code write-accessing the image should be wrapped into a stroke strategy or applied using KisProcessingApplicator. Right now the legacy and stroke interfaces are interleaved. I'm actually a bit surprised it doesn't crash. I thought there were an assert for it.
Here is the official documentation about strokes system:
http://dimula73.narod.ru/krita_strokes/strokes_documentation.html
As far as I can see the filtering itself can be implemented using a KUndo2Command passed to KisProcessingApplicator. That will solve the threading issues. And the implementation of preview depends on whether the "healing brush" supports semi-transparent masks. If yes, then the mask device can be created inside the tool itself. If not, one can even use the QPainterPath for storing the stroke and rendering it on screen.
Comment Actions
I guess that one reason to use a mask and not a canvas decoration is that the inpaint tool can be used with any brush engine.
I doubt there is any use in allowing multiple brush engines for painting healing selection. It'll create more troubles than benefits, like in Colorize mask, where it is still in todo to disable incompatible presets for it. I guess simple pixel brush is the only thing compatible with it.
When I was implementing the tool I was trying to keep user experience consistent with paint tool. In addition to brushes, I saw in freehand tool implementation a lot of code related to path smoothing and dynamics, which I didn't want to re-implement, but thought was important for consistency. Another feature I thought may be important was pressure sensitivity when using tablet (press harder - brush expands).
Liquify transform tool UX looked counterintuitive because you see something that looks like a round brush, but you don't change it's size where regular brush controls are - you do it in tool menu.
Another reason might be that the colorize tool also uses a mask?Colorize mask is a final data, not preview. It information is saved into .kra and can be reused by the user. Here it is just a preview. Last time we used masks as a preview were selection masks. It was around 2010 and it was almost impossible to make them render correctly. It was exactly the case why that functionality was disabled until we put it into Kickstarter 2014 stretchgoal and rewrote them using the overlay.
There do seem to be update problems, though, after the stroke is applied, and I wonder whether it makes sense to have an explicit apply button instead of applying the effect after mouse release -- that would make it easier to mask out some area very precisely.
The update problems might be easily caused undefined behavior of the incorrect usage of strokes system. Every new code write-accessing the image should be wrapped into a stroke strategy or applied using KisProcessingApplicator. Right now the legacy and stroke interfaces are interleaved. I'm actually a bit surprised it doesn't crash. I thought there were an assert for it.
I decided against an apply button fairly early. One of the uses of the healing brush is to fill in dust spots in image and remove small stray objects. This means that the brush may be applied dozens of times, so clicking apply would be annoying.
I thought I was avoiding the update problems because processing is done in main UI thread. After user releases the mouse button, control doesn't return until image is completely painted. For the same reason I thought I don't need to implement strokes.
Here is the official documentation about strokes system:
http://dimula73.narod.ru/krita_strokes/strokes_documentation.htmlAs far as I can see the filtering itself can be implemented using a KUndo2Command passed to KisProcessingApplicator. That will solve the threading issues. And the implementation of preview depends on whether the "healing brush" supports semi-transparent masks. If yes, then the mask device can be created inside the tool itself. If not, one can even use the QPainterPath for storing the stroke and rendering it on screen.
Undo was the part I was not sure about. The problem I was solving is that freehand tool creates a state on the stack and then healing brush creates another state on the stack. I needed to combine the two states into a single undo. I guess I don't understand the undo code well enough. I wasn't aware of deprecated parts.
I'm going to try canvas decorator option and look again into undo handling.
The reason for ImageData class is that the algorithm does a lot of random access of image data in the inner loop, so performance was critical. I looked into fixed paint device and I didn't see a fast way of accessing pixels. Example of access in setPixel/pixel in kis_fixed_paint_device looked very slow.
Comment Actions
When I was implementing the tool I was trying to keep user experience consistent with paint tool. In addition to brushes, I saw in freehand tool implementation a lot of code related to path smoothing and dynamics, which I didn't want to re-implement, but thought was important for consistency. Another feature I thought may be important was pressure sensitivity when using tablet (press harder - brush expands).
Does the new tool support semi-transparent masks for healing? And healing with custom mask shape? Does the algorithm takes brush mask pixel's opacity into account? If so, then, yes, using a pixel brush might be a good idea. Otherwise, it'll just create troubles.
Liquify transform tool UX looked counterintuitive because you see something that looks like a round brush, but you don't change it's size where regular brush controls are - you do it in tool menu.
In Liquify you still can use the Shift-gesture for resizing...
Anyway, could you answer the questions above? The answer defines how we should proceed with implementing the tool. We can reuse the freehand tool's code, but do it in a bit different way (like in KisScratchPad).
I thought I was avoiding the update problems because processing is done in main UI thread. After user releases the mouse button, control doesn't return until image is completely painted. For the same reason I thought I don't need to implement strokes.
Don't forget that the user might never release the mouse (tool switch, lost focus and etc). The user might also cancel the stroke by pressing Esc key. The strokes system implements the transactional approach for accessing the image. A stroke will either fully complete or will be fully canceled, whatever happens in the meantime.
Undo was the part I was not sure about. The problem I was solving is that freehand tool creates a state on the stack and then healing brush creates another state on the stack. I needed to combine the two states into a single undo. I guess I don't understand the undo code well enough. I wasn't aware of deprecated parts.
If you run the preview stroke on a separate strokes facade with fake undo store (like it is done in KisScratchPad), then you will not have to think about undo for it :) And that is only needed if the answer to the questions above is positive. Otherwise we can just limit ourselves to a QPainterPath approach.
The reason for ImageData class is that the algorithm does a lot of random access of image data in the inner loop, so performance was critical. I looked into fixed paint device and I didn't see a fast way of accessing pixels. Example of access in setPixel/pixel in kis_fixed_paint_device looked very slow.
It also has data() method, like in ImageView, so you can access the data directly. As far as I can see, the only difference between KisFixedPaintDevice and ImageView is that the latter one has a move constructor :) I'm not sure if it is used in the code, but I guess we will be perfectly fine with adding it to KisFixedPaintDevice :)
Comment Actions
Rewrote healing brush to use decorator. Changed ui to adjust brush size using keyboard and toolbar size control. Rewrote image access using Applicator based undo/redo interface.
Comment Actions
Hi, @eingerman!
The patch looks almost fine from the threading point of view! :) The only trouble is waitForDone() in the end of the operation that can be easily removed.
I also added an example of how to make the undo work correctly (see the inline comment). Please also check whether tempImageDev, tempMaskDev and originalImageDev in patchImage() are still needed. I guess they can be removed now :)
For the rest the tool looks nice now :)
PS:
I also guess KisInpaintMask can now be removed, right? :)
| plugins/tools/tool_smart_patch/kis_tool_smart_patch.cpp | ||
|---|---|---|
| 58 | Outch, we already have an undo system for pixel access operations: You should use KisTransactionBasedCommand. You can check en example in e.g. ApplyToPixelSelection command. Basically, you should override paint() method of that command and return the transaction command from it. In pseudocode it looks like this: class KisToolSmartPatch::InpaintCommand : public KisTransactionBasedCommand {
...
KUndo2Command* paint() override {
KisTransaction transaction(m_imageDev);
// m_modifiedRect is not needed here, because the transaction emits all the update sugnals itself
m_modifiedRect = patchImage(m_imageDev, m_maskDev, m_patchRadius, m_accuracy, m_originalImageDev, m_patchedImageDev);
return transaction.endAndTake();
}
}; | |
| 181 | m_d.isNull() sounds weird. I don't think it is needed, especially, when you already accessed it in line 176 | |
| 194 | As far as I can see waitForDone() is not needed here. Does InpaintCommand access any members of the tool from the inside? The only trouble I can see is m_d->maskDev and m_d->imageDev. But the former is already passed as a copy, so it won't be any problem. And m_d->imageDev is not used anywhere outside this function, so I guess it can be easily removed. Just pass currentNode()->paintDevice() directly. | |
Comment Actions
Thank you for the careful review. Using KisTransactionBasedCommand simplified undo handling a lot!
I kept waitForDone though. I set "busy" cursor while processing content aware fill. I need to restore it after image is patched. Otherwise UI "feels odd" - busy cursor disappears, but the image changes a second later.
Does it make sense to add a function to KisProcessingApplicator that would block until all processing in the applicator is done (i.e. all thread processing strokes complete)?
Comment Actions
Hi, @eingerman!
I think the patch is now ok for pushing. There are a few bugs still, but they can be fixed after the main fix is in :)
- Brush size doesn't change when you change the size in the toolbox
- There is a warning in the console when you draw with the healing brush: Unsupported composition mode
- I really don't like the idea of blocking the GUI for the tool calculation. Ideally, it should not block, like it is done in every other tool. If you really want it, you can show a special dialog...
- As far as I can tell, you don't need to change the default pixel of the device in getMaskBoundingBox(), nonDefaultPixelArea() doesn't care about it.
| libs/ui/kis_paintop_box.cc | ||
|---|---|---|
| 1101 | What does this change do? Is it also needed for Liquify tool? | |