Issue #1207 - Bucket features #1630

MrStevns · 2021-04-18T12:28:43Z

Inspired by davids work I set myself to see if I could improve on his expansion algorithm, as I found it problematic that it would be slower the more it needed to expand. I had recently bumped into an interesting image processing algorithm that could tell me the nearest pixels to a given color. This meant that by only searching the image once I could find pixels closest to the color and afterwards simply threshold based on the expand value. The result of this means the algorithm takes the same amount of time to expand a given region as we still only search the image once, compared to looping through the images x times which is not so problematic if you work with a relatively small image or only expand using 2-3 pixels but any more will take significantly longer.

PR content

I believe the PR fulfills most of the checkboxes in #1207 thus I think the issue can be closed after this PR has been merged.
What do you think?

Here's the UI when on bitmap layer

New feature - Fill to can be either:

Current layer
Layer below (old pencil behavior)

An example of how "Layer below" works:

New feature - Reference can be either:

Current layer
All layers

An example of how "all layers" reference mode works:

The all layer functionality basically flattens all bitmap layers of the current keyframe before it is run through by our floodfill algorithm, thus we can use all layers as reference to fill and in the end decide where we want to fill to.

Updated feature - Color tolerance:

Can be toggled on/off

New feature - Expand fill:

Can be toggled on/off
Easily changed using the slider

New feature - Drag to fill

From a technical perspective I have moved all bucket related tool property functionality to its own widget.
The bitmap bucket logic has been moved to a separate BitmapBucket class so it doesn't clutter the tool.

Pre-compiled builds are available to test here:
https://drive.google.com/drive/folders/0B7zQuPZEO_64NlFsM0JTTmwzdlk

closes #1207

- Uses manhattan distance algorithm to find pixels in neighborhood and then simply expands based on that data. It's very fast. O(n^2)

- Move bucket related code to own widget - Implements fill expand with - enable state - slider - spinbox - Improves tolerance by adding same checkstate to easily enable/disable the functionality.

- Add missing license header

- Implements UI and functionality to fill to current or layer below. - Implement UI and functionality to use current layer as reference or use all layers as reference!

davidlamhauge · 2021-04-20T20:35:48Z

I've tested this feature thoroughly and finds it working as expected. There were some minor issues, that CF has taken care of.
This feature will certainly improve the bucket-fill quality of Pencil2D, and I look forward to see someone review and merge it.

This won't do as we don't backup layer movement or anything. If you create three drawings on layer 1, then go to layer two, swap the layers and undo.. the logic will (based on the index) try to undo on the wrong layer... I've made just enough changes to make the undo/redo feature work for filling on a different layer. There might be cases i've missed but I don't want to put too much energy into this because of the undo/redo rewrite...

Jose-Moreno · 2021-04-22T23:25:22Z

FYI, i'm testing this. So far these are the issues I've found that I'll need to share a video to showcase the issues accurately so please hold off merging this before that. I'll provide the video before the weekend.

Jose-Moreno · 2021-04-24T00:09:10Z

@candyface Alright here's the video I promised. I was perhaps too enthusiastic so apologies if I fumble some words.

https://youtu.be/qdeTicqTN6E

I'll summarize the main issues:

[Bug] Clicking on a color area is not replacing the pixels when using new colors, probably due to a compositing mode mismatch. This is evidenced after noticing how the expansion keeps working when filling a layer without lineart.
[UX] Fill to: layer below option only solves a particular problem where each lineart layer will have a color layer pair. To solve other problem patterns we need at least a "custom layer" target option.
[UX] Reference: All layers option doesn't take into account layer visibility so it undermines the feature usefulness when handling many lineart levels.
[Feature Request]: Implement as a future PR the capability to fill pixels on top (front) or below (back) current layer existing pixels. Right now as per the mentioned fill bug, it seems the back fill is working, so having the ability to select between back/front fill would be ideal.
[Bug] Windows Ink is messing up with the application of the color while using a stylus. It seems to duplicate the tablet event. Mouse events work properly, and shutting down windows ink via the tablet drivers options also work properly. There seems to be a problem with Qt 5.15 at least, since other PR's i've tested (i.e David's Camera Interpolation) had a problem with right clicking through the timeline cursor.

- Add additional "Behind" fill behavior as discussed with Jose.

- Fixed when Reference was current layer and fillTo was current layer, it would not fill within correct boundaries. - Fixed when reference was current layer and fillTo was current layer - Fixed various cases where dragging and filling would spill

Which of course doesn't exist at this point...

- Added check to always allow filling once - Simplify shouldFill logic - Fixes a few cases where filling with transparent with overlay would keep filling.. the new mAppliedColor should prevent that from happening.

- Fixed painting occurring outside region of interest - Fixed calling state even though floodfill didn't happen - Added test to make sure spilling doesn't occur.

Would happen when painting, then undo the steps and then fill again.. since the layer cache is expected to be valid in 90% cases, which doesn't include undo/redo.

MrStevns · 2021-05-03T15:07:28Z

PR is ready to be reviewed

chchwy · 2021-07-13T06:36:53Z

app/src/tooloptionwidget.cpp

+    if (tool->type() == BUCKET) {
+        disableAllOptions();
+        if (mBucketOptionsWidget == nullptr) {
+            mBucketOptionsWidget = new BucketOptionsWidget(editor(), this);


Personally, I prefer to create the widget at the initial stage. Especially when the widget will not be destroyed until the application ends.

chchwy · 2021-07-13T06:37:37Z

core_lib/src/graphics/bitmap/bitmapbucket.cpp

+    }
+
+    if (mTargetFillToLayer == nullptr) {
+        Q_ASSERT(true);


Do you mean Q_ASSERT(false)? it's the case that should never happen, right?

And looks like the assert shouldn't be put here as the mTargetFillToLayer could be null.

I'm assuming based off of the changes you made that you realize that it can't be null. editor->layers()->currentLayer() asserts the return is not null, so initialLayer can't be null. findBitmapLayerBelow will return initialLayer or another layer which is allso asserted not to be null.

Yes, I finally got it :p

chchwy · 2021-07-13T06:37:49Z

core_lib/src/graphics/bitmap/bitmapbucket.cpp

+    }
+
+    if (mTargetFillToLayer == nullptr) {
+        Q_ASSERT(true);


same as the above comment.

chchwy · 2021-07-13T06:54:37Z

core_lib/src/tool/basetool.h

+    int bucketFillExpand = 0;
+    bool bucketFillExpandEnabled = 0;
+    int bucketFillToLayerMode = 0;
+    int bucketFillReferenceMode = 0;


Looks like all these newly added modes are exclusively for the bucket tool. I wonder if these variables could be moved to somewhere else better? And use enum instead?

Yup I considered that too, maybe we should do that. I simply added it there since we have most of the tool properties in basetool.

chchwy · 2021-07-13T07:38:42Z

I think this PR is working as expected and generally good to go. I encountered once a bug that bucketFillToLayerMode value doesn't match the dropdown list item but I can't replicate it consistently. probably it only happens in a special scenario.

Before I merge this PR, I will make some minor code changes mentioned above. won't touch the main part.

Great effort, well done.

MrStevns · 2021-07-13T07:45:44Z

Sounds good, thanks for reviewing :) - I generally agree with your comments. I am away from the computer for the day, so apply the changes you see fit 👍

MrStevns added 12 commits April 17, 2021 15:11

Implement expand algorithm

10580d0

- Uses manhattan distance algorithm to find pixels in neighborhood and then simply expands based on that data. It's very fast. O(n^2)

pencil2d#1207 - Implement Fill expand UI interaction

23d5240

- Move bucket related code to own widget - Implements fill expand with - enable state - slider - spinbox - Improves tolerance by adding same checkstate to easily enable/disable the functionality.

Move vector stroke thickness to BucketOptionsWidget

aab0d65

- Add missing license header

Implement controls to fill to a layer and use layer below as reference

15bf27a

- Implements UI and functionality to fill to current or layer below. - Implement UI and functionality to use current layer as reference or use all layers as reference!

Add tooltips

0db1fe5

Adjust stretching

bcf366b

Add documentation

8bbc010

Remove unused function

76db88a

Move destructor method below constructor

77a18d2

Fix BucketOptionsWidget cpp/h/ui placement

9de9f8a

Merge branch 'master' into iss-1207-bucket-features

5371677

Fix keyframe not added when filling using "layer below"

df5c710

MrStevns added 7 commits April 24, 2021 09:09

Merge branch 'master' into iss-1207-bucket-features

32d5ea8

Fix features doesn't take layer visibility into account

90ebbe1

Fix filling with transparency not working

b6ff150

Refactor and simplify

ec94ef3

Fix typo in documentation

56110b5

Change transparency to blend mode

ffca137

- Add additional "Behind" fill behavior as discussed with Jose.

Fix targetLayerIndex not being properly set with layer below

8893310

MrStevns force-pushed the iss-1207-bucket-features branch from 24637bf to 8893310 Compare April 25, 2021 07:51

MrStevns added 6 commits April 25, 2021 11:31

Don't show floodfill options on camera layer

6127949

Implement drag to fill ability

21cfe61

Rework bucket logic and move into BitmapBucket class

a67f26d

Implement BitmapBucket tests

8321332

Remove redundant code based on test results

7bc955c

J5lx added Feature Request Paint Bucket labels Apr 28, 2021

Fix crash on deinit because of strong reference to pointer

42c3cd4

Which of course doesn't exist at this point...

MrStevns added 8 commits April 29, 2021 20:03

Simplify and fix cases where filling would occur more than once

f4f1987

- Added check to always allow filling once - Simplify shouldFill logic - Fixes a few cases where filling with transparent with overlay would keep filling.. the new mAppliedColor should prevent that from happening.

Fix test not working as expected

fd1a18a

- Fixed painting occurring outside region of interest - Fixed calling state even though floodfill didn't happen - Added test to make sure spilling doesn't occur.

Fix github actions failing

f69be8f

Fix cache update issue when painting on layer below

e51a2a3

Remove redundant code

9d3eb4d

Check before creating keyframe on layer below

16f35c0

Fix cache issue when drawing on layer below

e5e6705

Would happen when painting, then undo the steps and then fill again.. since the layer cache is expected to be valid in 90% cases, which doesn't include undo/redo.

Remove old comment

93db5e5

chchwy reviewed Jul 13, 2021

View reviewed changes

core_lib/src/graphics/bitmap/bitmapbucket.cpp

}

if (mTargetFillToLayer == nullptr) {

Q_ASSERT(true);

Copy link

Member

chchwy Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as the above comment.

chchwy reviewed Jul 13, 2021

View reviewed changes

chchwy added this to the 0.7.0 milestone Jul 13, 2021

chchwy merged commit 0913fd6 into pencil2d:master Jul 13, 2021

MrStevns modified the milestones: 0.7.0, v0.6.7 Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1207 - Bucket features #1630

Issue #1207 - Bucket features #1630

MrStevns commented Apr 18, 2021 •

edited

Loading

davidlamhauge commented Apr 20, 2021

Jose-Moreno commented Apr 22, 2021

Jose-Moreno commented Apr 24, 2021

MrStevns commented May 3, 2021

chchwy Jul 13, 2021

chchwy Jul 13, 2021

chchwy Jul 13, 2021

scribblemaniac Jul 24, 2021

chchwy Jul 24, 2021

chchwy Jul 13, 2021

chchwy Jul 13, 2021 •

edited

Loading

MrStevns Jul 13, 2021

chchwy commented Jul 13, 2021

MrStevns commented Jul 13, 2021

Issue #1207 - Bucket features #1630

Issue #1207 - Bucket features #1630

Conversation

MrStevns commented Apr 18, 2021 • edited Loading

PR content

davidlamhauge commented Apr 20, 2021

Jose-Moreno commented Apr 22, 2021

Jose-Moreno commented Apr 24, 2021

MrStevns commented May 3, 2021

chchwy Jul 13, 2021

Choose a reason for hiding this comment

chchwy Jul 13, 2021

Choose a reason for hiding this comment

chchwy Jul 13, 2021

Choose a reason for hiding this comment

scribblemaniac Jul 24, 2021

Choose a reason for hiding this comment

chchwy Jul 24, 2021

Choose a reason for hiding this comment

chchwy Jul 13, 2021

Choose a reason for hiding this comment

chchwy Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

MrStevns Jul 13, 2021

Choose a reason for hiding this comment

chchwy commented Jul 13, 2021

MrStevns commented Jul 13, 2021

MrStevns commented Apr 18, 2021 •

edited

Loading

chchwy Jul 13, 2021 •

edited

Loading