From c80357a9cc617aa15ea8547f51fa319de1a37204 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 9 Jul 2023 16:33:38 +0200 Subject: [PATCH 1/8] Remove Fill to layer feature --- app/src/bucketoptionswidget.cpp | 28 -------- app/src/bucketoptionswidget.h | 2 - app/src/tooloptionwidget.cpp | 1 - app/ui/bucketoptionswidget.ui | 22 +----- core_lib/src/graphics/bitmap/bitmapbucket.cpp | 10 +-- core_lib/src/managers/toolmanager.cpp | 11 --- core_lib/src/managers/toolmanager.h | 1 - core_lib/src/tool/basetool.cpp | 5 -- core_lib/src/tool/basetool.h | 2 - core_lib/src/tool/buckettool.cpp | 19 ----- core_lib/src/tool/buckettool.h | 1 - core_lib/src/util/pencildef.h | 2 - tests/src/test_bitmapbucket.cpp | 72 ------------------- 13 files changed, 3 insertions(+), 173 deletions(-) diff --git a/app/src/bucketoptionswidget.cpp b/app/src/bucketoptionswidget.cpp index 9f3feedeff..2e9e89e3e0 100644 --- a/app/src/bucketoptionswidget.cpp +++ b/app/src/bucketoptionswidget.cpp @@ -49,10 +49,6 @@ BucketOptionsWidget::BucketOptionsWidget(Editor* editor, QWidget* parent) : ui->colorToleranceSpinbox->setMaximum(MAX_COLOR_TOLERANCE); ui->strokeThicknessSpinBox->setMinimum(1); - ui->fillToLayerComboBox->addItem(tr("Current layer"), 0); - ui->fillToLayerComboBox->addItem(tr("Layer below"), 1); - ui->fillToLayerComboBox->setToolTip(tr("Fill to the current layer or the layer below")); - ui->referenceLayerComboBox->addItem(tr("Current layer", "Reference Layer Options"), 0); ui->referenceLayerComboBox->addItem(tr("All layers", "Reference Layer Options"), 1); ui->referenceLayerComboBox->setToolTip(tr("Refers to the layer that used to flood fill from")); @@ -76,7 +72,6 @@ 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(&QComboBox::currentIndexChanged), mEditor->tools(), &ToolManager::setBucketFillToLayerMode); connect(ui->referenceLayerComboBox, static_cast(&QComboBox::currentIndexChanged), mEditor->tools(), &ToolManager::setBucketFillReferenceMode); connect(ui->blendModeComboBox, static_cast(&QComboBox::currentIndexChanged), mEditor->tools(), &ToolManager::setFillMode); @@ -84,7 +79,6 @@ BucketOptionsWidget::BucketOptionsWidget(Editor* editor, QWidget* parent) : ui->expandSpinBox->setValue(settings.value(SETTING_BUCKET_FILL_EXPAND, 2).toInt()); ui->colorToleranceSlider->setValue(settings.value(SETTING_BUCKET_TOLERANCE, 50).toInt()); ui->colorToleranceSpinbox->setValue(settings.value(SETTING_BUCKET_TOLERANCE, 50).toInt()); - ui->fillToLayerComboBox->setCurrentIndex(settings.value(SETTING_BUCKET_FILL_TO_LAYER_MODE, 0).toInt()); ui->referenceLayerComboBox->setCurrentIndex(settings.value(SETTING_BUCKET_FILL_REFERENCE_MODE, 0).toInt()); ui->blendModeComboBox->setCurrentIndex(settings.value(SETTING_FILL_MODE, 0).toInt()); @@ -110,8 +104,6 @@ void BucketOptionsWidget::updatePropertyVisibility() ui->strokeThicknessSlider->show(); ui->strokeThicknessSpinBox->show(); - ui->fillToLayerComboBox->hide(); - ui->fillToDescLabel->hide(); ui->colorToleranceCheckbox->hide(); ui->colorToleranceSlider->hide(); ui->colorToleranceSpinbox->hide(); @@ -127,8 +119,6 @@ void BucketOptionsWidget::updatePropertyVisibility() ui->strokeThicknessSlider->hide(); ui->strokeThicknessSpinBox->hide(); - ui->fillToLayerComboBox->show(); - ui->fillToDescLabel->show(); ui->referenceLayerComboBox->show(); ui->referenceLayerDescLabel->show(); ui->colorToleranceCheckbox->show(); @@ -137,7 +127,6 @@ void BucketOptionsWidget::updatePropertyVisibility() ui->expandCheckbox->show(); ui->expandSlider->show(); ui->expandSpinBox->show(); - disableFillToLayerComboBox(mEditor->tools()->bucketReferenceModeIsCurrentLayer(ui->referenceLayerComboBox->currentIndex())); ui->blendModeComboBox->show(); ui->blendModeLabel->show(); break; @@ -145,8 +134,6 @@ void BucketOptionsWidget::updatePropertyVisibility() default: ui->strokeThicknessSlider->hide(); ui->strokeThicknessSpinBox->hide(); - ui->fillToLayerComboBox->hide(); - ui->fillToDescLabel->hide(); ui->colorToleranceCheckbox->hide(); ui->colorToleranceSlider->hide(); ui->colorToleranceSpinbox->hide(); @@ -175,8 +162,6 @@ void BucketOptionsWidget::onPropertyChanged(ToolType, ToolPropertyType propertyT setFillExpand(static_cast(p.bucketFillExpand)); break; case ToolPropertyType::USEBUCKETFILLEXPAND: setFillExpandEnabled(p.bucketFillExpandEnabled); break; - case ToolPropertyType::BUCKETFILLLAYERMODE: - setFillToLayerMode(p.bucketFillToLayerMode); break; case ToolPropertyType::BUCKETFILLLAYERREFERENCEMODE: setFillReferenceMode(p.bucketFillReferenceMode); break; case ToolPropertyType::FILL_MODE: @@ -227,23 +212,10 @@ void BucketOptionsWidget::setFillExpand(int value) ui->expandSpinBox->setValue(value); } -void BucketOptionsWidget::setFillToLayerMode(int layerMode) -{ - QSignalBlocker b(ui->fillToLayerComboBox); - ui->fillToLayerComboBox->setCurrentIndex(layerMode); -} - 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) diff --git a/app/src/bucketoptionswidget.h b/app/src/bucketoptionswidget.h index a53644f33f..c8985e2535 100644 --- a/app/src/bucketoptionswidget.h +++ b/app/src/bucketoptionswidget.h @@ -41,14 +41,12 @@ class BucketOptionsWidget : public QWidget void setFillExpand(int value); void setColorTolerance(int tolerance); void setFillReferenceMode(int referenceMode); - void setFillToLayerMode(int layerMode); void setFillMode(int mode); void onPropertyChanged(ToolType, const ToolPropertyType propertyType); void onLayerChanged(int); private: - void disableFillToLayerComboBox(bool state); void updatePropertyVisibility(); Ui::BucketOptionsWidget *ui; diff --git a/app/src/tooloptionwidget.cpp b/app/src/tooloptionwidget.cpp index f6261336b5..5c51c42065 100644 --- a/app/src/tooloptionwidget.cpp +++ b/app/src/tooloptionwidget.cpp @@ -143,7 +143,6 @@ void ToolOptionWidget::onToolPropertyChanged(ToolType, ToolPropertyType ePropert case USETOLERANCE: break; case BUCKETFILLEXPAND: break; case USEBUCKETFILLEXPAND: break; - case BUCKETFILLLAYERMODE: break; case BUCKETFILLLAYERREFERENCEMODE: break; case FILL_MODE: break; default: diff --git a/app/ui/bucketoptionswidget.ui b/app/ui/bucketoptionswidget.ui index b5b20c4d73..49ffe7ee29 100644 --- a/app/ui/bucketoptionswidget.ui +++ b/app/ui/bucketoptionswidget.ui @@ -7,7 +7,7 @@ 0 0 400 - 197 + 221 @@ -35,25 +35,7 @@ 6 - - - - - Fill to - - - - - - - - 0 - 0 - - - - - + diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.cpp b/core_lib/src/graphics/bitmap/bitmapbucket.cpp index 88ba46b223..596149146e 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.cpp +++ b/core_lib/src/graphics/bitmap/bitmapbucket.cpp @@ -49,12 +49,6 @@ BitmapBucket::BitmapBucket(Editor* editor, mTolerance = mProperties.toleranceEnabled ? static_cast(mProperties.tolerance) : 0; - if (properties.bucketFillToLayerMode == 1) - { - auto result = findBitmapLayerBelow(initialLayer, initialLayerIndex); - mTargetFillToLayer = result.first; - mTargetFillToLayerIndex = result.second; - } Q_ASSERT(mTargetFillToLayer); mReferenceImage = *static_cast(initialLayer->getLastKeyFrameAtPosition(frameIndex)); @@ -85,15 +79,13 @@ bool BitmapBucket::allowFill(const QPoint& checkPoint) const QRgb colorOfReferenceImage = mReferenceImage.constScanLine(checkPoint.x(), checkPoint.y()); QRgb targetPixelColor = targetImage.constScanLine(checkPoint.x(), checkPoint.y()); - if (targetPixelColor == mBucketColor &&(mProperties.fillMode == 1 || qAlpha(targetPixelColor) == 255)) + if (targetPixelColor == mBucketColor && (mProperties.fillMode == 1 || qAlpha(targetPixelColor) == 255)) { // Avoid filling if target pixel color matches fill color // to avoid creating numerous seemingly useless undo operations 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)); } diff --git a/core_lib/src/managers/toolmanager.cpp b/core_lib/src/managers/toolmanager.cpp index add88969db..d475c9c314 100644 --- a/core_lib/src/managers/toolmanager.cpp +++ b/core_lib/src/managers/toolmanager.cpp @@ -251,19 +251,8 @@ void ToolManager::setBucketFillExpand(int expandValue) emit toolPropertyChanged(currentTool()->type(), BUCKETFILLEXPAND); } -void ToolManager::setBucketFillToLayerMode(int layerMode) -{ - currentTool()->setFillToLayerMode(layerMode); - emit toolPropertyChanged(currentTool()->type(), BUCKETFILLLAYERMODE); -} - void ToolManager::setBucketFillReferenceMode(int referenceMode) { - // If the bucket reference mode is current layer, enforce fillTo is also set to current layer - if (bucketReferenceModeIsCurrentLayer(referenceMode)) { - currentTool()->setFillToLayerMode(0); - emit toolPropertyChanged(currentTool()->type(), BUCKETFILLLAYERMODE); - } currentTool()->setFillReferenceMode(referenceMode); emit toolPropertyChanged(currentTool()->type(), BUCKETFILLLAYERREFERENCEMODE); } diff --git a/core_lib/src/managers/toolmanager.h b/core_lib/src/managers/toolmanager.h index 02373d2d47..6ba1cd3321 100644 --- a/core_lib/src/managers/toolmanager.h +++ b/core_lib/src/managers/toolmanager.h @@ -77,7 +77,6 @@ public slots: void setTolerance(int); void setBucketColorToleranceEnabled(bool enabled); void setBucketFillExpandEnabled(bool enabled); - void setBucketFillToLayerMode(int layerMode); void setBucketFillReferenceMode(int referenceMode); void setBucketFillExpand(int); void setUseFillContour(bool); diff --git a/core_lib/src/tool/basetool.cpp b/core_lib/src/tool/basetool.cpp index 56bd359bc7..65679d1dfc 100644 --- a/core_lib/src/tool/basetool.cpp +++ b/core_lib/src/tool/basetool.cpp @@ -429,11 +429,6 @@ void BaseTool::setFillExpand(const int fillExpandValue) properties.bucketFillExpand = fillExpandValue; } -void BaseTool::setFillToLayerMode(int layerMode) -{ - properties.bucketFillToLayerMode = layerMode; -} - void BaseTool::setFillReferenceMode(int referenceMode) { properties.bucketFillReferenceMode = referenceMode; diff --git a/core_lib/src/tool/basetool.h b/core_lib/src/tool/basetool.h index 02f33ddecf..533b59491f 100644 --- a/core_lib/src/tool/basetool.h +++ b/core_lib/src/tool/basetool.h @@ -53,7 +53,6 @@ class Properties bool toleranceEnabled = false; int bucketFillExpand = 0; bool bucketFillExpandEnabled = 0; - int bucketFillToLayerMode = 0; int bucketFillReferenceMode = 0; bool useFillContour = false; bool showSelectionInfo = true; @@ -127,7 +126,6 @@ class BaseTool : public QObject virtual void setToleranceEnabled(const bool enabled); virtual void setFillExpand(const int fillExpandValue); virtual void setFillExpandEnabled(const bool enabled); - virtual void setFillToLayerMode(int layerMode); virtual void setFillReferenceMode(int referenceMode); virtual void setUseFillContour(const bool useFillContour); virtual void setShowSelectionInfo(const bool b); diff --git a/core_lib/src/tool/buckettool.cpp b/core_lib/src/tool/buckettool.cpp index 2ef4f01aa5..42585314ac 100644 --- a/core_lib/src/tool/buckettool.cpp +++ b/core_lib/src/tool/buckettool.cpp @@ -62,7 +62,6 @@ void BucketTool::loadSettings() properties.bucketFillExpand = settings.value(SETTING_BUCKET_FILL_EXPAND, 2.0).toInt(); properties.bucketFillExpandEnabled = settings.value(SETTING_BUCKET_FILL_EXPAND_ON, true).toBool(); - properties.bucketFillToLayerMode = settings.value(SETTING_BUCKET_FILL_TO_LAYER_MODE, 0).toInt(); properties.bucketFillReferenceMode = settings.value(SETTING_BUCKET_FILL_REFERENCE_MODE, 0).toInt(); properties.fillMode = settings.value(SETTING_FILL_MODE, 0).toInt(); } @@ -74,7 +73,6 @@ void BucketTool::resetToDefault() setFillMode(0); setFillExpand(2); setFillExpandEnabled(true); - setFillToLayerMode(0); setToleranceEnabled(false); setFillReferenceMode(0); } @@ -163,16 +161,6 @@ void BucketTool::setFillExpand(const int fillExpandValue) settings.sync(); } -void BucketTool::setFillToLayerMode(int layerMode) -{ - properties.bucketFillToLayerMode = layerMode; - - // Update settings - QSettings settings(PENCIL2D, PENCIL2D); - settings.setValue(SETTING_BUCKET_FILL_TO_LAYER_MODE, layerMode); - settings.sync(); -} - void BucketTool::setFillReferenceMode(int referenceMode) { properties.bucketFillReferenceMode = referenceMode; @@ -271,13 +259,6 @@ void BucketTool::paintBitmap() else if (progress == BucketState::DidFillTarget) { mEditor->setModified(layerIndex, frameIndex); - - // Need to invalidate layer pixmap cache when filling anything else but current layer - // otherwise dragging won't show until release event - if (properties.bucketFillToLayerMode == 1) - { - mScribbleArea->invalidatePainterCaches(); - } } }); } diff --git a/core_lib/src/tool/buckettool.h b/core_lib/src/tool/buckettool.h index 631c19938a..4f9660a61c 100644 --- a/core_lib/src/tool/buckettool.h +++ b/core_lib/src/tool/buckettool.h @@ -47,7 +47,6 @@ class BucketTool : public StrokeTool void setWidth(const qreal width) override; void setFillExpand(const int fillExpandValue) override; void setFillExpandEnabled(const bool enabled) override; - void setFillToLayerMode(int layerMode) override; void setFillReferenceMode(int referenceMode) override; void setFillMode(int mode) override; diff --git a/core_lib/src/util/pencildef.h b/core_lib/src/util/pencildef.h index a125b0697a..c47f94d229 100644 --- a/core_lib/src/util/pencildef.h +++ b/core_lib/src/util/pencildef.h @@ -64,7 +64,6 @@ enum ToolPropertyType USETOLERANCE, BUCKETFILLEXPAND, USEBUCKETFILLEXPAND, - BUCKETFILLLAYERMODE, BUCKETFILLLAYERREFERENCEMODE, CAMERAPATH, }; @@ -303,7 +302,6 @@ const static float RotationHandleOffset = 50; #define SETTING_BUCKET_TOLERANCE_ON "BucketToleranceEnabled" #define SETTING_BUCKET_FILL_EXPAND "BucketFillExpand" #define SETTING_BUCKET_FILL_EXPAND_ON "BucketFillExpandEnabled" -#define SETTING_BUCKET_FILL_TO_LAYER_MODE "BucketFillToLayerMode" #define SETTING_BUCKET_FILL_REFERENCE_MODE "BucketFillReferenceMode" #define SETTING_FILL_MODE "FillMode" diff --git a/tests/src/test_bitmapbucket.cpp b/tests/src/test_bitmapbucket.cpp index 3fa0624e30..cf27e52c1d 100644 --- a/tests/src/test_bitmapbucket.cpp +++ b/tests/src/test_bitmapbucket.cpp @@ -75,7 +75,6 @@ TEST_CASE("BitmapBucket - Fill drag behaviour across four segments") dir.mkpath(resultsPath); properties.bucketFillReferenceMode = 0; - properties.bucketFillToLayerMode = 0; properties.bucketFillExpandEnabled = false; properties.fillMode = 0; @@ -92,7 +91,6 @@ TEST_CASE("BitmapBucket - Fill drag behaviour across four segments") // The dragging logic is based around that we only fill on either transparent or the same color as the fill color. SECTION("Filling on current layer - layer is not pre filled") { - properties.bucketFillToLayerMode = 0; Layer* strokeLayer = editor->layers()->currentLayer(); SECTION("When reference is current layer, only transparent color is filled") { @@ -121,7 +119,6 @@ TEST_CASE("BitmapBucket - Fill drag behaviour across four segments") } SECTION("Filling on current layer - layer is pre-filled") { - properties.bucketFillToLayerMode = 0; // Fill mode is set to `replace` because it makes it easier to compare colors... properties.fillMode = 1; @@ -159,73 +156,4 @@ TEST_CASE("BitmapBucket - Fill drag behaviour across four segments") verifyOnlyPixelsInsideSegmentsAreFilled(pressPoint, image, fillColor.rgba()); } } - - // The behaviour changes here because we'll be filling on the layer below, but that layer is blank, - // yet the reference mode tells the flood fill algorithm to fill using the pixel data from the the reference layer. - // In this case it means that all pixels will be filled as we drag across the segments. - SECTION("Filling on layer below - layer is not pre-filled") { - properties.bucketFillToLayerMode = 1; - - Layer* fillLayer = editor->layers()->currentLayer(-1); - SECTION("When reference is current layer, then all pixels not matching the fill color are filled once") - { - properties.bucketFillReferenceMode = 0; - dragAndFill(pressPoint, editor, fillColor, beforeFill.bounds(), properties, 4); - - // Verify that colors are correct on layer below - BitmapImage* image = static_cast(fillLayer)->getLastBitmapImageAtFrame(1); - - image->writeFile(resultsPath + "test4a.png"); - - verifyOnlyPixelsInsideSegmentsAreFilled(pressPoint, image, qPremultiply(fillColor.rgba())); - } - - - SECTION("When reference is all layers, then all pixels not matching the fill color are filled") - { - properties.bucketFillReferenceMode = 1; - - dragAndFill(pressPoint, editor, fillColor, beforeFill.bounds(), properties, 4); - - BitmapImage* image = static_cast(fillLayer)->getLastBitmapImageAtFrame(1); - - image->writeFile(resultsPath + "test5a.png"); - - verifyOnlyPixelsInsideSegmentsAreFilled(pressPoint, image, qPremultiply(fillColor.rgba())); - } - } - - SECTION("Filling on layer below - layer is pre-filled") { - properties.bucketFillToLayerMode = 1; - properties.fillMode = 1; - - SECTION("when reference is all layers, then only pixels matching the fill color are filled") - { - properties.bucketFillReferenceMode = 1; - - Layer* strokeLayer = editor->layers()->currentLayer(); - Layer* fillLayer = editor->layers()->currentLayer(-1); - - // Because the layer is blank, all pixels will be filled once - dragAndFill(pressPoint, editor, fillColor, beforeFill.bounds(), properties, 4); - BitmapImage* image1 = static_cast(strokeLayer)->getLastBitmapImageAtFrame(1); - image1->writeFile(resultsPath + "test6a-first.png"); - - fillColor = QColor(0,255,0,255); - - // Changes fillTo mode to current layer - properties.bucketFillToLayerMode = 0; - - editor->layers()->setCurrentLayer(strokeLayer); - - // Now the layer has been filled with pixel data, so we'll only fill when the color matches the fill color - dragAndFill(pressPoint, editor, fillColor, beforeFill.bounds(), properties, 4); - - BitmapImage* image2 = static_cast(fillLayer)->getLastBitmapImageAtFrame(1); - image1->writeFile(resultsPath + "test6a-second.png"); - image2->writeFile(resultsPath + "test6a-third.png"); - - verifyOnlyPixelsInsideSegmentsAreFilled(pressPoint, image1, fillColor.rgba()); - } - } } From 66574a03d15202f75e14fcb5556eecca8b56c1c9 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 9 Jul 2023 16:46:05 +0200 Subject: [PATCH 2/8] Fix floodfill didn't take camera transform into account #1735 --- core_lib/src/tool/buckettool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core_lib/src/tool/buckettool.cpp b/core_lib/src/tool/buckettool.cpp index 42585314ac..322bdb5563 100644 --- a/core_lib/src/tool/buckettool.cpp +++ b/core_lib/src/tool/buckettool.cpp @@ -183,7 +183,7 @@ void BucketTool::pointerPressEvent(PointerEvent* event) mBitmapBucket = BitmapBucket(mEditor, mEditor->color()->frontColor(), - layerCam ? layerCam->getViewRect() : QRect(), + layerCam ? layerCam->getViewAtFrame(mEditor->currentFrame()).inverted().mapRect(layerCam->getViewRect()) : QRect(), getCurrentPoint(), properties); From 64b8c90e5c4439e9e09bc45c26a8b3c3182c4338 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Mon, 10 Jul 2023 08:10:44 +0200 Subject: [PATCH 3/8] UX: No matter what, allow a fill the very first time It's been a growing annoyance of mine that sometimes a fill won't happen, sometimes caused by a bug, other times because of the rules with are to be applied when filling while dragging the mouse. --- core_lib/src/graphics/bitmap/bitmapbucket.cpp | 4 ++++ core_lib/src/graphics/bitmap/bitmapbucket.h | 1 + 2 files changed, 5 insertions(+) diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.cpp b/core_lib/src/graphics/bitmap/bitmapbucket.cpp index 596149146e..2f87d8fe96 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.cpp +++ b/core_lib/src/graphics/bitmap/bitmapbucket.cpp @@ -86,6 +86,9 @@ bool BitmapBucket::allowFill(const QPoint& checkPoint) const return false; } + if (!mFilledOnce) { + return true; + } return BitmapImage::compareColor(colorOfReferenceImage, mStartReferenceColor, mTolerance, mPixelCache) && (targetPixelColor == 0 || BitmapImage::compareColor(targetPixelColor, mStartReferenceColor, mTolerance, mPixelCache)); } @@ -155,6 +158,7 @@ void BitmapBucket::paint(const QPointF updatedPoint, std::function Date: Mon, 10 Jul 2023 08:11:00 +0200 Subject: [PATCH 4/8] Fix case where undo commands would fill up with no visible changes --- core_lib/src/graphics/bitmap/bitmapbucket.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.cpp b/core_lib/src/graphics/bitmap/bitmapbucket.cpp index 2f87d8fe96..874b8c3eb3 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.cpp +++ b/core_lib/src/graphics/bitmap/bitmapbucket.cpp @@ -84,6 +84,11 @@ bool BitmapBucket::allowFill(const QPoint& checkPoint) const // Avoid filling if target pixel color matches fill color // to avoid creating numerous seemingly useless undo operations return false; + } else if (QColor::fromRgb(targetPixelColor) == QColor::fromRgb(mBucketColor) && mProperties.fillMode == 0) { + // Avoid filling if the target pixel color matches without the alpha component + // this fixes a case where filling a fully opaque pixel with a less opaque version of itself would cause undo + // fills to happen + return false; } if (!mFilledOnce) { From 4078cecbb341d6aea16282480bc3c62168d65f46 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Mon, 10 Jul 2023 18:07:13 +0200 Subject: [PATCH 5/8] Add explanation --- core_lib/src/graphics/bitmap/bitmapbucket.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.cpp b/core_lib/src/graphics/bitmap/bitmapbucket.cpp index 874b8c3eb3..558bcdffe2 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.cpp +++ b/core_lib/src/graphics/bitmap/bitmapbucket.cpp @@ -79,6 +79,11 @@ bool BitmapBucket::allowFill(const QPoint& checkPoint) const QRgb colorOfReferenceImage = mReferenceImage.constScanLine(checkPoint.x(), checkPoint.y()); QRgb targetPixelColor = targetImage.constScanLine(checkPoint.x(), checkPoint.y()); + // A normal click to fill should happen unconditionally, because the alternative is utterly confusing. + if (!mFilledOnce) { + return true; + } + if (targetPixelColor == mBucketColor && (mProperties.fillMode == 1 || qAlpha(targetPixelColor) == 255)) { // Avoid filling if target pixel color matches fill color @@ -91,9 +96,6 @@ bool BitmapBucket::allowFill(const QPoint& checkPoint) const return false; } - if (!mFilledOnce) { - return true; - } return BitmapImage::compareColor(colorOfReferenceImage, mStartReferenceColor, mTolerance, mPixelCache) && (targetPixelColor == 0 || BitmapImage::compareColor(targetPixelColor, mStartReferenceColor, mTolerance, mPixelCache)); } From b4f5de314a07dfe45abc4db2036f1d81b32410d6 Mon Sep 17 00:00:00 2001 From: Jakob Gahde Date: Wed, 9 Aug 2023 00:47:29 +0200 Subject: [PATCH 6/8] Remove dead code --- core_lib/src/graphics/bitmap/bitmapbucket.cpp | 26 ------------------- core_lib/src/graphics/bitmap/bitmapbucket.h | 1 - 2 files changed, 27 deletions(-) diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.cpp b/core_lib/src/graphics/bitmap/bitmapbucket.cpp index 558bcdffe2..1cf3c18e47 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.cpp +++ b/core_lib/src/graphics/bitmap/bitmapbucket.cpp @@ -187,29 +187,3 @@ BitmapImage BitmapBucket::flattenBitmapLayersToImage() } return flattenImage; } - -std::pair BitmapBucket::findBitmapLayerBelow(Layer* targetLayer, int layerIndex) const -{ - bool foundLayerBelow = false; - int layerBelowIndex = layerIndex; - for (int i = layerIndex - 1; i >= 0; i--) - { - Layer* searchlayer = mEditor->layers()->getLayer(i); - Q_ASSERT(searchlayer); - - if (searchlayer->type() == Layer::BITMAP && searchlayer->visible()) - { - targetLayer = searchlayer; - foundLayerBelow = true; - layerBelowIndex = i; - break; - } - } - - if (foundLayerBelow && !targetLayer->keyExists(mEditor->currentFrame())) - { - targetLayer->addNewKeyFrameAt(mEditor->currentFrame()); - emit mEditor->updateTimeLine(); - } - return std::make_pair(targetLayer, layerBelowIndex); -} diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.h b/core_lib/src/graphics/bitmap/bitmapbucket.h index 90a8ce5c99..d4062aafa1 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.h +++ b/core_lib/src/graphics/bitmap/bitmapbucket.h @@ -59,7 +59,6 @@ class BitmapBucket */ bool allowFill(const QPoint& checkPoint) const; - std::pair findBitmapLayerBelow(Layer* targetLayer, int layerIndex) const; BitmapImage flattenBitmapLayersToImage(); Editor* mEditor = nullptr; From cc359c1dbe7983b28b9786e14c84267d1f9829fa Mon Sep 17 00:00:00 2001 From: MrStevns Date: Mon, 28 Aug 2023 19:49:21 +0200 Subject: [PATCH 7/8] Fix more cases where continuous fill would flood the undo stack --- core_lib/src/graphics/bitmap/bitmapbucket.cpp | 66 +++++++++++-------- core_lib/src/graphics/bitmap/bitmapbucket.h | 11 +++- 2 files changed, 48 insertions(+), 29 deletions(-) diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.cpp b/core_lib/src/graphics/bitmap/bitmapbucket.cpp index 1cf3c18e47..d6114445c2 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.cpp +++ b/core_lib/src/graphics/bitmap/bitmapbucket.cpp @@ -48,68 +48,82 @@ BitmapBucket::BitmapBucket(Editor* editor, mTargetFillToLayerIndex = initialLayerIndex; mTolerance = mProperties.toleranceEnabled ? static_cast(mProperties.tolerance) : 0; + const QPoint& point = QPoint(qFloor(fillPoint.x()), qFloor(fillPoint.y())); Q_ASSERT(mTargetFillToLayer); - mReferenceImage = *static_cast(initialLayer->getLastKeyFrameAtPosition(frameIndex)); + BitmapImage singleLayerImage = *static_cast(initialLayer->getLastKeyFrameAtPosition(frameIndex)); if (properties.bucketFillReferenceMode == 1) // All layers { mReferenceImage = flattenBitmapLayersToImage(); + } else { + mReferenceImage = singleLayerImage; } - const QPoint point = QPoint(qFloor(fillPoint.x()), qFloor(fillPoint.y())); mStartReferenceColor = mReferenceImage.constScanLine(point.x(), point.y()); + mUseFillToDrag = canUseFillToDrag(point, color, mProperties, singleLayerImage); mPixelCache = new QHash(); } -bool BitmapBucket::allowFill(const QPoint& checkPoint) const +bool BitmapBucket::canUseFillToDrag(const QPoint& fillPoint, const QColor& bucketColor, const Properties& properties, const BitmapImage& referenceImage) { - if (mProperties.fillMode == 0 && qAlpha(mBucketColor) == 0) - { - // Filling in overlay mode with a fully transparent color has no - // effect, so we can skip it in this case + QRgb pressReferenceColorSingleLayer = referenceImage.constScanLine(fillPoint.x(), fillPoint.y()); + QRgb startRef = qUnpremultiply(pressReferenceColorSingleLayer); + + if (properties.fillMode == 0 && ((QColor(qRed(startRef), qGreen(startRef), qBlue(startRef)) == bucketColor.rgb() && qAlpha(startRef) == 255) || bucketColor.alpha() == 0)) { + // In overlay mode: When the reference pixel matches the bucket color and the reference is fully opaque + // Otherwise when the bucket alpha is zero. + return false; + } else if (properties.fillMode == 2 && qAlpha(startRef) == 255) { + // In behind mode: When the reference pixel is already fully opaque, the output will be invisible. return false; } - Q_ASSERT(mTargetFillToLayer); - - BitmapImage targetImage = *static_cast(mTargetFillToLayer)->getLastBitmapImageAtFrame(mEditor->currentFrame(), 0); - if (!targetImage.isLoaded()) { return false; } - - QRgb colorOfReferenceImage = mReferenceImage.constScanLine(checkPoint.x(), checkPoint.y()); - QRgb targetPixelColor = targetImage.constScanLine(checkPoint.x(), checkPoint.y()); + return true; +} +bool BitmapBucket::allowFill(const QPoint& checkPoint, const QRgb& checkColor) const +{ // A normal click to fill should happen unconditionally, because the alternative is utterly confusing. if (!mFilledOnce) { return true; } - if (targetPixelColor == mBucketColor && (mProperties.fillMode == 1 || qAlpha(targetPixelColor) == 255)) + return allowContinousFill(checkPoint, checkColor); +} + +bool BitmapBucket::allowContinousFill(const QPoint& checkPoint, const QRgb& checkColor) const +{ + if (!mUseFillToDrag) { + return false; + } + + const QRgb& colorOfReferenceImage = mReferenceImage.constScanLine(checkPoint.x(), checkPoint.y()); + + if (checkColor == mBucketColor && (mProperties.fillMode == 1 || qAlpha(checkColor) == 255)) { // Avoid filling if target pixel color matches fill color // to avoid creating numerous seemingly useless undo operations return false; - } else if (QColor::fromRgb(targetPixelColor) == QColor::fromRgb(mBucketColor) && mProperties.fillMode == 0) { - // Avoid filling if the target pixel color matches without the alpha component - // this fixes a case where filling a fully opaque pixel with a less opaque version of itself would cause undo - // fills to happen - return false; } return BitmapImage::compareColor(colorOfReferenceImage, mStartReferenceColor, mTolerance, mPixelCache) && - (targetPixelColor == 0 || BitmapImage::compareColor(targetPixelColor, mStartReferenceColor, mTolerance, mPixelCache)); + (checkColor == 0 || BitmapImage::compareColor(checkColor, mStartReferenceColor, mTolerance, mPixelCache)); } -void BitmapBucket::paint(const QPointF updatedPoint, std::function state) +void BitmapBucket::paint(const QPointF& updatedPoint, std::function state) { - const QPoint point = QPoint(qFloor(updatedPoint.x()), qFloor(updatedPoint.y())); + const QPoint& point = QPoint(qFloor(updatedPoint.x()), qFloor(updatedPoint.y())); const int currentFrameIndex = mEditor->currentFrame(); - if (!allowFill(point)) { return; } + BitmapImage* targetImage = static_cast(mTargetFillToLayer)->getLastBitmapImageAtFrame(mEditor->currentFrame(), 0); + if (targetImage == nullptr || !targetImage->isLoaded()) { return; } // Can happen if the first frame is deleted while drawing - BitmapImage* targetImage = static_cast(mTargetFillToLayer->getLastKeyFrameAtPosition(currentFrameIndex)); + const QRgb& targetPixelColor = targetImage->constScanLine(point.x(), point.y()); - if (targetImage == nullptr || !targetImage->isLoaded()) { return; } // Can happen if the first frame is deleted while drawing + if (!allowFill(point, targetPixelColor)) { + return; + } QRgb fillColor = mBucketColor; if (mProperties.fillMode == 1) diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.h b/core_lib/src/graphics/bitmap/bitmapbucket.h index d4062aafa1..d5502b9655 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.h +++ b/core_lib/src/graphics/bitmap/bitmapbucket.h @@ -43,7 +43,7 @@ class BitmapBucket * @param progress - a function that returns the progress of the paint operation, * the layer and frame that was affected at the given point. */ - void paint(const QPointF updatedPoint, std::function progress); + void paint(const QPointF& updatedPoint, std::function progress); private: @@ -57,7 +57,11 @@ class BitmapBucket * @param checkPoint * @return True if you are allowed to fill, otherwise false */ - bool allowFill(const QPoint& checkPoint) const; + bool allowFill(const QPoint& checkPoint, const QRgb& checkColor) const; + bool allowContinousFill(const QPoint& checkPoint, const QRgb& checkColor) const; + + /** Determines whether fill to drag feature can be used */ + bool canUseFillToDrag(const QPoint& fillPoint, const QColor& bucketColor, const Properties& properties, const BitmapImage& referenceImage); BitmapImage flattenBitmapLayersToImage(); @@ -75,7 +79,8 @@ class BitmapBucket int mTolerance = 0; int mTargetFillToLayerIndex = -1; - int mFilledOnce = false; + bool mFilledOnce = false; + bool mUseFillToDrag = false; Properties mProperties; }; From dc638ed80839d45bb9ce0c5f796d7c5b8f701bf9 Mon Sep 17 00:00:00 2001 From: Jakob Gahde Date: Wed, 30 Aug 2023 02:51:42 +0200 Subject: [PATCH 8/8] Fix typos and such --- core_lib/src/graphics/bitmap/bitmapbucket.cpp | 14 +++++++------- core_lib/src/graphics/bitmap/bitmapbucket.h | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.cpp b/core_lib/src/graphics/bitmap/bitmapbucket.cpp index d6114445c2..60e506dda2 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.cpp +++ b/core_lib/src/graphics/bitmap/bitmapbucket.cpp @@ -60,21 +60,21 @@ BitmapBucket::BitmapBucket(Editor* editor, mReferenceImage = singleLayerImage; } mStartReferenceColor = mReferenceImage.constScanLine(point.x(), point.y()); - mUseFillToDrag = canUseFillToDrag(point, color, mProperties, singleLayerImage); + mUseDragToFill = canUseDragToFill(point, color, singleLayerImage); mPixelCache = new QHash(); } -bool BitmapBucket::canUseFillToDrag(const QPoint& fillPoint, const QColor& bucketColor, const Properties& properties, const BitmapImage& referenceImage) +bool BitmapBucket::canUseDragToFill(const QPoint& fillPoint, const QColor& bucketColor, const BitmapImage& referenceImage) { QRgb pressReferenceColorSingleLayer = referenceImage.constScanLine(fillPoint.x(), fillPoint.y()); QRgb startRef = qUnpremultiply(pressReferenceColorSingleLayer); - if (properties.fillMode == 0 && ((QColor(qRed(startRef), qGreen(startRef), qBlue(startRef)) == bucketColor.rgb() && qAlpha(startRef) == 255) || bucketColor.alpha() == 0)) { + if (mProperties.fillMode == 0 && ((QColor(qRed(startRef), qGreen(startRef), qBlue(startRef)) == bucketColor.rgb() && qAlpha(startRef) == 255) || bucketColor.alpha() == 0)) { // In overlay mode: When the reference pixel matches the bucket color and the reference is fully opaque // Otherwise when the bucket alpha is zero. return false; - } else if (properties.fillMode == 2 && qAlpha(startRef) == 255) { + } else if (mProperties.fillMode == 2 && qAlpha(startRef) == 255) { // In behind mode: When the reference pixel is already fully opaque, the output will be invisible. return false; } @@ -89,12 +89,12 @@ bool BitmapBucket::allowFill(const QPoint& checkPoint, const QRgb& checkColor) c return true; } - return allowContinousFill(checkPoint, checkColor); + return allowContinuousFill(checkPoint, checkColor); } -bool BitmapBucket::allowContinousFill(const QPoint& checkPoint, const QRgb& checkColor) const +bool BitmapBucket::allowContinuousFill(const QPoint& checkPoint, const QRgb& checkColor) const { - if (!mUseFillToDrag) { + if (!mUseDragToFill) { return false; } diff --git a/core_lib/src/graphics/bitmap/bitmapbucket.h b/core_lib/src/graphics/bitmap/bitmapbucket.h index d5502b9655..edd71d6158 100644 --- a/core_lib/src/graphics/bitmap/bitmapbucket.h +++ b/core_lib/src/graphics/bitmap/bitmapbucket.h @@ -58,10 +58,10 @@ class BitmapBucket * @return True if you are allowed to fill, otherwise false */ bool allowFill(const QPoint& checkPoint, const QRgb& checkColor) const; - bool allowContinousFill(const QPoint& checkPoint, const QRgb& checkColor) const; + bool allowContinuousFill(const QPoint& checkPoint, const QRgb& checkColor) const; /** Determines whether fill to drag feature can be used */ - bool canUseFillToDrag(const QPoint& fillPoint, const QColor& bucketColor, const Properties& properties, const BitmapImage& referenceImage); + bool canUseDragToFill(const QPoint& fillPoint, const QColor& bucketColor, const BitmapImage& referenceImage); BitmapImage flattenBitmapLayersToImage(); @@ -80,7 +80,7 @@ class BitmapBucket int mTargetFillToLayerIndex = -1; bool mFilledOnce = false; - bool mUseFillToDrag = false; + bool mUseDragToFill = false; Properties mProperties; };