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

…as expected (#1667)

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

- 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.

* Fix variable case

* Use pixelformat to check if color is premultiplied when blending occurs

- Use compareColor instead of raw pixel comparison for the ability to fill when the pixel is in average the same.

* Fix linux compilation error

* Fix compiler error caused by inline function definition

* Cleanup

* Fix now unnecessary check for premult

The real problem causing this has been fixed via #1729

* Fix filling was not allowed when fillTo mode was "layer below"

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

* Rewrite test cases

* Simplify allowFill function

* Rework drag to fill behavior again

* Fix / simplify allowFill()

* Reference mode should enforce fillTo mode when currentLayer is set.

- Also fixed a few confusing variables

* Fix build error

Co-authored-by: scribblemaniac <scribblemaniac@users.noreply.github.com>
Co-authored-by: Jakob Gahde <j5lx@fmail.co.uk>

app/src/bucketoptionswidget.cpp

@@ -76,7 +76,7 @@ BucketOptionsWidget::BucketOptionsWidget(Editor* editor, QWidget* parent) :
     connect(mEditor->tools(), &ToolManager::toolPropertyChanged, this, &BucketOptionsWidget::onPropertyChanged);
     connect(mEditor->layers(), &LayerManager::currentLayerChanged, this, &BucketOptionsWidget::onLayerChanged);
 
-    connect(ui->fillToLayerComboBox, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), mEditor->tools(), &ToolManager::setBucketFillToLayer);
+    connect(ui->fillToLayerComboBox, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), mEditor->tools(), &ToolManager::setBucketFillToLayerMode);
     connect(ui->referenceLayerComboBox, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), mEditor->tools(), &ToolManager::setBucketFillReferenceMode);
     connect(ui->blendModeComboBox, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), mEditor->tools(), &ToolManager::setFillMode);
 
@@ -123,7 +123,7 @@ void BucketOptionsWidget::updatePropertyVisibility()
         ui->blendModeComboBox->hide();
         ui->blendModeLabel->hide();
         break;
-    case Layer::BITMAP:
+    case Layer::BITMAP: {
         ui->strokeThicknessSlider->hide();
         ui->strokeThicknessSpinBox->hide();
 
@@ -135,11 +135,11 @@ void BucketOptionsWidget::updatePropertyVisibility()
         ui->expandCheckbox->show();
         ui->expandSlider->show();
         ui->expandSpinBox->show();
-        ui->referenceLayerComboBox->show();
-        ui->referenceLayerDescLabel->show();
+        disableFillToLayerComboBox(mEditor->tools()->bucketReferenceModeIsCurrentLayer(ui->referenceLayerComboBox->currentIndex()));
         ui->blendModeComboBox->show();
         ui->blendModeLabel->show();
         break;
+    }
     default:
         ui->strokeThicknessSlider->hide();
         ui->strokeThicknessSpinBox->hide();
@@ -235,6 +235,13 @@ void BucketOptionsWidget::setFillReferenceMode(int referenceMode)
 {
     QSignalBlocker b(ui->referenceLayerComboBox);
     ui->referenceLayerComboBox->setCurrentIndex(referenceMode);
+    disableFillToLayerComboBox(mEditor->tools()->bucketReferenceModeIsCurrentLayer(referenceMode));
+}
+
+void BucketOptionsWidget::disableFillToLayerComboBox(bool state)
+{
+    ui->fillToLayerComboBox->setDisabled(state);
+    ui->fillToDescLabel->setDisabled(state);
 }
 
 void BucketOptionsWidget::setStrokeWidth(qreal value)

app/src/bucketoptionswidget.h

@@ -48,6 +48,7 @@ class BucketOptionsWidget : public QWidget
     void onLayerChanged(int);
 
 private:
+    void disableFillToLayerComboBox(bool state);
     void updatePropertyVisibility();
 
     Ui::BucketOptionsWidget *ui;

core_lib/src/graphics/bitmap/bitmapbucket.cpp

@@ -34,7 +34,6 @@ BitmapBucket::BitmapBucket(Editor* editor,
                            QPointF fillPoint,
                            Properties properties):
     mEditor(editor),
-    mBucketStartPoint(fillPoint),
     mMaxFillRegion(maxFillRegion),
     mProperties(properties)
 
@@ -48,6 +47,8 @@ BitmapBucket::BitmapBucket(Editor* editor,
     mTargetFillToLayer = initialLayer;
     mTargetFillToLayerIndex = initialLayerIndex;
 
+    mTolerance = mProperties.toleranceEnabled ? static_cast<int>(mProperties.tolerance) : 0;
+
     if (properties.bucketFillToLayerMode == 1)
     {
         auto result = findBitmapLayerBelow(initialLayer, initialLayerIndex);
@@ -61,17 +62,14 @@ BitmapBucket::BitmapBucket(Editor* editor,
     {
         mReferenceImage = flattenBitmapLayersToImage();
     }
-
     const QPoint point = QPoint(qFloor(fillPoint.x()), qFloor(fillPoint.y()));
+    mStartReferenceColor = mReferenceImage.constScanLine(point.x(), point.y());
 
-    BitmapImage* image = static_cast<LayerBitmap*>(mTargetFillToLayer)->getLastBitmapImageAtFrame(frameIndex, 0);
-    mReferenceColor = image->constScanLine(point.x(), point.y());
+    mPixelCache = new QHash<QRgb, bool>();
 }
 
-bool BitmapBucket::shouldFill(QPointF checkPoint) const
+bool BitmapBucket::allowFill(const QPoint& checkPoint) const
 {
-    const QPoint point = QPoint(qFloor(checkPoint.x()), qFloor(checkPoint.y()));
-
     if (mProperties.fillMode == 0 && qAlpha(mBucketColor) == 0)
     {
         // Filling in overlay mode with a fully transparent color has no
@@ -84,60 +82,34 @@ bool BitmapBucket::shouldFill(QPointF checkPoint) const
 
     if (!targetImage.isLoaded()) { return false; }
 
-    BitmapImage referenceImage = mReferenceImage;
+    QRgb colorOfReferenceImage = mReferenceImage.constScanLine(checkPoint.x(), checkPoint.y());
+    QRgb targetPixelColor = targetImage.constScanLine(checkPoint.x(), checkPoint.y());
 
-    QRgb pixelColor = referenceImage.constScanLine(point.x(), point.y());
-    QRgb targetPixelColor = targetImage.constScanLine(point.x(), point.y());
-
-    if (mProperties.fillMode == 2 && pixelColor != 0)
+    if (targetPixelColor == mBucketColor &&(mProperties.fillMode == 1 || qAlpha(targetPixelColor) == 255))
     {
-        // don't try to fill because we won't be able to see it anyway...
+        // Avoid filling if target pixel color matches fill color
+        // to avoid creating numerous seemingly useless undo operations
         return false;
     }
 
-    // First paint is allowed with no rules applied
-    if (mFirstPaint)
-    {
-        return true;
-    }
-
-    // Ensure that when dragging that we're only filling on either transparent or same color
-    if ((mReferenceColor == targetPixelColor && targetPixelColor == pixelColor) || (pixelColor == 0 && targetPixelColor == 0))
-    {
-        return true;
-    }
-
-    // When filling with various blending modes we need to verify that the applied color
-    // doesn't match the target color, otherwise it will fill the same color for no reason.
-    // We still expect to only fill on either transparent or same color.
-    if (mAppliedColor != targetPixelColor && pixelColor == 0)
-    {
-        return true;
-    }
-
-    return false;
+    // Allow filling if the reference pixel matches the start reference color, and
+    // the target pixel is either transparent or matches the start reference color
+    return BitmapImage::compareColor(colorOfReferenceImage, mStartReferenceColor, mTolerance, mPixelCache) &&
+           (targetPixelColor == 0 || BitmapImage::compareColor(targetPixelColor, mStartReferenceColor, mTolerance, mPixelCache));
 }
 
 void BitmapBucket::paint(const QPointF updatedPoint, std::function<void(BucketState, int, int)> state)
 {
-    const Layer* targetLayer = mTargetFillToLayer;
-    int targetLayerIndex = mTargetFillToLayerIndex;
-    QRgb fillColor = mBucketColor;
-
     const QPoint point = QPoint(qFloor(updatedPoint.x()), qFloor(updatedPoint.y()));
-    const QRect cameraRect = mMaxFillRegion;
-    const int tolerance = mProperties.toleranceEnabled ? static_cast<int>(mProperties.tolerance) : 0;
     const int currentFrameIndex = mEditor->currentFrame();
-    const QRgb origColor = fillColor;
 
-    if (!shouldFill(updatedPoint)) { return; }
+    if (!allowFill(point)) { return; }
 
-    BitmapImage* targetImage = static_cast<BitmapImage*>(targetLayer->getLastKeyFrameAtPosition(currentFrameIndex));
+    BitmapImage* targetImage = static_cast<BitmapImage*>(mTargetFillToLayer->getLastKeyFrameAtPosition(currentFrameIndex));
 
     if (targetImage == nullptr || !targetImage->isLoaded()) { return; } // Can happen if the first frame is deleted while drawing
 
-    BitmapImage referenceImage = mReferenceImage;
-
+    QRgb fillColor = mBucketColor;
     if (mProperties.fillMode == 1)
     {
         // Pass a fully opaque version of the new color to floodFill
@@ -153,11 +125,11 @@ void BitmapBucket::paint(const QPointF updatedPoint, std::function<void(BucketSt
 
     int expandValue = mProperties.bucketFillExpandEnabled ? mProperties.bucketFillExpand : 0;
     bool didFloodFill = BitmapImage::floodFill(&replaceImage,
-                           &referenceImage,
-                           cameraRect,
+                           &mReferenceImage,
+                           mMaxFillRegion,
                            point,
                            fillColor,
-                           tolerance,
+                           mTolerance,
                            expandValue);
 
     if (!didFloodFill) {
@@ -166,7 +138,7 @@ void BitmapBucket::paint(const QPointF updatedPoint, std::function<void(BucketSt
     }
     Q_ASSERT(replaceImage != nullptr);
 
-    state(BucketState::WillFillTarget, targetLayerIndex, currentFrameIndex);
+    state(BucketState::WillFillTarget, mTargetFillToLayerIndex, currentFrameIndex);
 
     if (mProperties.fillMode == 0)
     {
@@ -181,19 +153,16 @@ void BitmapBucket::paint(const QPointF updatedPoint, std::function<void(BucketSt
         // fill mode replace
         targetImage->paste(replaceImage, QPainter::CompositionMode_DestinationOut);
         // Reduce the opacity of the fill to match the new color
-        BitmapImage properColor(replaceImage->bounds(), QColor::fromRgba(origColor));
+        BitmapImage properColor(replaceImage->bounds(), QColor::fromRgba(mBucketColor));
         properColor.paste(replaceImage, QPainter::CompositionMode_DestinationIn);
         // Write reduced-opacity fill image on top of target image
         targetImage->paste(&properColor);
     }
 
-    mAppliedColor = targetImage->constScanLine(point.x(), point.y());
-    mFirstPaint = false;
-
     targetImage->modification();
     delete replaceImage;
 
-    state(BucketState::DidFillTarget, targetLayerIndex, currentFrameIndex);
+    state(BucketState::DidFillTarget, mTargetFillToLayerIndex, currentFrameIndex);
 }
 
 BitmapImage BitmapBucket::flattenBitmapLayersToImage()

core_lib/src/graphics/bitmap/bitmapbucket.h

@@ -57,27 +57,27 @@ class BitmapBucket
      * @param checkPoint
      * @return True if you are allowed to fill, otherwise false
      */
-    bool shouldFill(QPointF checkPoint) const;
+    bool allowFill(const QPoint& checkPoint) const;
 
     std::pair<Layer*, int> findBitmapLayerBelow(Layer* targetLayer, int layerIndex) const;
     BitmapImage flattenBitmapLayersToImage();
 
     Editor* mEditor = nullptr;
     Layer* mTargetFillToLayer = nullptr;
 
+    QHash<QRgb, bool> *mPixelCache;
+
     BitmapImage mReferenceImage;
     QRgb mBucketColor = 0;
-    QRgb mReferenceColor = 0;
-    QRgb mAppliedColor = 0;
+    QRgb mStartReferenceColor = 0;
 
-    QPointF mBucketStartPoint;
     QRect mMaxFillRegion;
 
+    int mTolerance = 0;
+
     int mTargetFillToLayerIndex = -1;
 
     Properties mProperties;
-
-    bool mFirstPaint = true;
 };
 
 #endif // BITMAPBUCKET_H

core_lib/src/graphics/bitmap/bitmapimage.cpp

@@ -18,7 +18,6 @@ GNU General Public License for more details.
 
 #include <cmath>
 #include <QDebug>
-#include <QtMath>
 #include <QFile>
 #include <QFileInfo>
 #include <QPainterPath>
@@ -764,48 +763,6 @@ void BitmapImage::clear(QRect rectangle)
     modification();
 }
 
-/** Compare colors for the purposes of flood filling
- *
- *  Calculates the Eulcidian difference of the RGB channels
- *  of the image and compares it to the tolerance
- *
- *  @param[in] newColor The first color to compare
- *  @param[in] oldColor The second color to compare
- *  @param[in] tolerance The threshold limit between a matching and non-matching color
- *  @param[in,out] cache Contains a mapping of previous results of compareColor with rule that
- *                 cache[someColor] = compareColor(someColor, oldColor, tolerance)
- *
- *  @return Returns true if the colors have a similarity below the tolerance level
- *          (i.e. if Eulcidian distance squared is <= tolerance)
- */
-bool BitmapImage::compareColor(QRgb newColor, QRgb oldColor, int tolerance, QHash<QRgb, bool> *cache)
-{
-    // Handle trivial case
-    if (newColor == oldColor) return true;
-
-    if(cache && cache->contains(newColor)) return cache->value(newColor);
-
-    // Get Eulcidian distance between colors
-    // Not an accurate representation of human perception,
-    // but it's the best any image editing program ever does
-    int diffRed = static_cast<int>(qPow(qRed(oldColor) - qRed(newColor), 2));
-    int diffGreen = static_cast<int>(qPow(qGreen(oldColor) - qGreen(newColor), 2));
-    int diffBlue = static_cast<int>(qPow(qBlue(oldColor) - qBlue(newColor), 2));
-    // This may not be the best way to handle alpha since the other channels become less relevant as
-    // the alpha is reduces (ex. QColor(0,0,0,0) is the same as QColor(255,255,255,0))
-    int diffAlpha = static_cast<int>(qPow(qAlpha(oldColor) - qAlpha(newColor), 2));
-
-    bool isSimilar = (diffRed + diffGreen + diffBlue + diffAlpha) <= tolerance;
-
-    if(cache)
-    {
-        Q_ASSERT(cache->contains(isSimilar) ? isSimilar == (*cache)[newColor] : true);
-        (*cache)[newColor] = isSimilar;
-    }
-
-    return isSimilar;
-}
-
 bool BitmapImage::floodFill(BitmapImage** replaceImage,
                             const BitmapImage* targetImage,
                             const QRect& cameraRect,

core_lib/src/graphics/bitmap/bitmapimage.h

@@ -19,6 +19,8 @@ GNU General Public License for more details.
 
 #include <QPainter>
 #include "keyframe.h"
+#include <QtMath>
+#include <QHash>
 
 
 class BitmapImage : public KeyFrame
@@ -73,7 +75,6 @@ class BitmapImage : public KeyFrame
     void clear(QRect rectangle);
     void clear(QRectF rectangle) { clear(rectangle.toRect()); }
 
-    static inline bool compareColor(QRgb newColor, QRgb oldColor, int tolerance, QHash<QRgb, bool> *cache);
     static bool floodFill(BitmapImage** replaceImage, const BitmapImage* targetImage, const QRect& cameraRect, const QPoint& point, const QRgb& fillColor, int tolerance, const int expandValue);
     static bool* floodFillPoints(const BitmapImage* targetImage,
                                 QRect searchBounds, const QRect& maxBounds,
@@ -120,6 +121,48 @@ class BitmapImage : public KeyFrame
 
     Status writeFile(const QString& filename);
 
+    /** Compare colors for the purposes of flood filling
+     *
+     *  Calculates the Eulcidian difference of the RGB channels
+     *  of the image and compares it to the tolerance
+     *
+     *  @param[in] newColor The first color to compare
+     *  @param[in] oldColor The second color to compare
+     *  @param[in] tolerance The threshold limit between a matching and non-matching color
+     *  @param[in,out] cache Contains a mapping of previous results of compareColor with rule that
+     *                 cache[someColor] = compareColor(someColor, oldColor, tolerance)
+     *
+     *  @return Returns true if the colors have a similarity below the tolerance level
+     *          (i.e. if Eulcidian distance squared is <= tolerance)
+     */
+    static inline bool compareColor(QRgb newColor, QRgb oldColor, int tolerance, QHash<QRgb, bool> *cache)
+    {
+        // Handle trivial case
+        if (newColor == oldColor) return true;
+
+        if(cache && cache->contains(newColor)) return cache->value(newColor);
+
+        // Get Eulcidian distance between colors
+        // Not an accurate representation of human perception,
+        // but it's the best any image editing program ever does
+        int diffRed = static_cast<int>(qPow(qRed(oldColor) - qRed(newColor), 2));
+        int diffGreen = static_cast<int>(qPow(qGreen(oldColor) - qGreen(newColor), 2));
+        int diffBlue = static_cast<int>(qPow(qBlue(oldColor) - qBlue(newColor), 2));
+        // This may not be the best way to handle alpha since the other channels become less relevant as
+        // the alpha is reduces (ex. QColor(0,0,0,0) is the same as QColor(255,255,255,0))
+        int diffAlpha = static_cast<int>(qPow(qAlpha(oldColor) - qAlpha(newColor), 2));
+
+        bool isSimilar = (diffRed + diffGreen + diffBlue + diffAlpha) <= tolerance;
+
+        if(cache)
+        {
+            Q_ASSERT(cache->contains(isSimilar) ? isSimilar == (*cache)[newColor] : true);
+            (*cache)[newColor] = isSimilar;
+        }
+
+        return isSimilar;
+    }
+
 protected:
     void updateBounds(QRect rectangle);
     void extend(const QPoint& p);