Commit af5acee0 authored by Emmet O'Neill's avatar Emmet O'Neill Committed by Dmitry Kazakov

Refactor: Unified Color Picker Code & Removed Some Duplication

Summary:
This patch is part of task T8708, focused on further improving Krita's color picker.

>As of right now, Krita effectively has two near-duplicate paths when it comes to color picking code: the code for the dedicated Color Selector Tool (P) exists mainly in "kis_tool_colorpicker", while the code for the ctrl-key activated color picker that you can pull up while other tools are active [as well as the scratchpad's picker] exists mainly in "kis_tool_utils".

This patch addresses this issue by, as much as possible without some much bigger changes, unifying all of the core color picking functionality within the `KisToolUtils` namespace's `pickColor` function.

Essentially all of the duplicate code was removed from `KisToolColorPicker::pickColor`, which now calls into the same `KisToolUtils::pickColor` function that's used elsewhere creating a single place in the code where things like sampling radius and color picker blending are implemented. As a result, any change to the code in `KisToolUtils::pickColor` will now affect all of Krita's color pickers (the dedicated tool [P], the ctrl-activated picker, and the scratchpad picker) in a uniform and predictable way.

While this patch does solve the most of the code-duplication issue that existed in Krita's color pickers, there are still a few ways in which the dedicated tool and the ctrl-picker behave inconsistently. In my view, more extensive refactoring can be done in the future to create more uniform and predictable behavior across the various pickers, resulting in more color picking code being moved into `KisToolUtils::pickColor` from elsewhere. For now though, I wanted to focus on code-reuse improvements that don't have any user-facing design implications.

Also, some minor code-style cleanup was done here and there.

Test Plan:
Test the various color pickers (dedicated tool, ctrl-picker, scratch pad) for *mostly predictable and equivalent behavior.

*Certain aspects of the dedicated/ctrl picker can't be unified without some bigger changes, such as 'sample from merged/active only'.

Reviewers: #krita, #krita_abyss, rempt, dkazakov

Reviewed By: #krita, #krita_abyss, dkazakov

Subscribers: #krita_abyss, #krita

Tags: #krita_abyss

Differential Revision: https://phabricator.kde.org/D12941
parent 7c639729
......@@ -80,7 +80,7 @@
#include <kis_canvas_resource_provider.h>
KisToolPaint::KisToolPaint(KoCanvasBase * canvas, const QCursor & cursor)
KisToolPaint::KisToolPaint(KoCanvasBase *canvas, const QCursor &cursor)
: KisTool(canvas, cursor),
m_showColorPreview(false),
m_colorPreviewShowComparePlate(false),
......@@ -107,7 +107,7 @@ KisToolPaint::KisToolPaint(KoCanvasBase * canvas, const QCursor & cursor)
m_standardBrushSizes.push_back(maxSize);
}
KisCanvas2 * kiscanvas = dynamic_cast<KisCanvas2*>(canvas);
KisCanvas2 *kiscanvas = dynamic_cast<KisCanvas2*>(canvas);
KisActionManager *actionManager = kiscanvas->viewManager()->actionManager();
// XXX: Perhaps a better place for these?
......@@ -343,7 +343,6 @@ void KisToolPaint::activatePickColorDelayed()
};
repaintDecorations();
}
bool KisToolPaint::isPickingAction(AlternateAction action) {
......@@ -500,10 +499,10 @@ void KisToolPaint::mouseReleaseEvent(KoPointerEvent *event)
QWidget * KisToolPaint::createOptionWidget()
{
QWidget * optionWidget = new QWidget();
QWidget *optionWidget = new QWidget();
optionWidget->setObjectName(toolId());
QVBoxLayout* verticalLayout = new QVBoxLayout(optionWidget);
QVBoxLayout *verticalLayout = new QVBoxLayout(optionWidget);
verticalLayout->setObjectName("KisToolPaint::OptionWidget::VerticalLayout");
verticalLayout->setContentsMargins(0,0,0,0);
verticalLayout->setSpacing(5);
......@@ -523,10 +522,10 @@ QWidget * KisToolPaint::createOptionWidget()
m_optionsWidgetLayout->setSpacing(5);
if (!quickHelp().isEmpty()) {
QPushButton* push = new QPushButton(KisIconUtils::loadIcon("help-contents"), QString(), optionWidget);
QPushButton *push = new QPushButton(KisIconUtils::loadIcon("help-contents"), QString(), optionWidget);
connect(push, SIGNAL(clicked()), this, SLOT(slotPopupQuickHelp()));
QHBoxLayout* hLayout = new QHBoxLayout(optionWidget);
QHBoxLayout *hLayout = new QHBoxLayout(optionWidget);
hLayout->addWidget(push);
hLayout->addItem(new QSpacerItem(0, 0, QSizePolicy::Expanding, QSizePolicy::Fixed));
verticalLayout->addLayout(hLayout);
......@@ -625,7 +624,7 @@ void KisToolPaint::setupPaintAction(KisRecordedPaintAction* action)
{
KisTool::setupPaintAction(action);
action->setOpacity(m_opacity / qreal(255.0));
const KoCompositeOp* op = compositeOp();
const KoCompositeOp *op = compositeOp();
if (op) {
action->setCompositeOp(op->id());
}
......
......@@ -19,13 +19,11 @@
#ifndef KIS_TOOL_PAINT_H_
#define KIS_TOOL_PAINT_H_
#include <vector>
#include "kis_tool.h"
#include <QCursor>
#include <QLayout>
#include <QGridLayout>
#include <QVariant>
#include <QTimer>
#include <QCheckBox>
#include <KoCanvasResourceManager.h>
#include <KoToolBase.h>
......@@ -37,26 +35,19 @@
#include <kis_image.h>
#include "kis_signal_compressor_with_param.h"
#include <brushengine/kis_paintop_settings.h>
#include <resources/KoPattern.h>
#include "kis_tool.h"
#include <QCheckBox>
class QGridLayout;
class KoCompositeOp;
class KoCanvasBase;
class KRITAUI_EXPORT KisToolPaint : public KisTool
{
Q_OBJECT
public:
KisToolPaint(KoCanvasBase * canvas, const QCursor & cursor);
KisToolPaint(KoCanvasBase *canvas, const QCursor &cursor);
~KisToolPaint() override;
int flags() const override;
......@@ -68,9 +59,9 @@ protected:
void setMode(ToolMode mode) override;
void canvasResourceChanged(int key, const QVariant & v) override;
void canvasResourceChanged(int key, const QVariant &v) override;
void paint(QPainter& gc, const KoViewConverter &converter) override;
void paint(QPainter &gc, const KoViewConverter &converter) override;
void activatePrimaryAction() override;
void deactivatePrimaryAction() override;
......
......@@ -29,18 +29,17 @@
namespace KisToolUtils {
bool pick(KisPaintDeviceSP dev, const QPoint &pos, KoColor *color, KoColor *previousColor, int radius, int blend)
bool pickColor(KoColor &out_color, KisPaintDeviceSP dev, const QPoint &pos,
KoColor const *const blendColor, int radius, int blend, bool pure)
{
KIS_ASSERT(dev);
const KoColorSpace* cs = dev->colorSpace();
const KoColorSpace *cs = dev->colorSpace();
KoColor pickedColor(Qt::transparent, cs);
// Ctrl picker sampling radius.
if (radius <= 1) {
dev->pixel(pos.x(), pos.y(), &pickedColor);
} else {
// Sampling radius.
if (!pure && radius > 1) {
QVector<const quint8*> pixels;
const int effectiveRadius = radius - 1;
const QRect pickRect(pos.x() - effectiveRadius, pos.y() - effectiveRadius,
......@@ -59,15 +58,17 @@ namespace KisToolUtils {
const quint8 **cpixels = const_cast<const quint8**>(pixels.constData());
cs->mixColorsOp()->mixColors(cpixels, pixels.size(), pickedColor.data());
} else {
dev->pixel(pos.x(), pos.y(), &pickedColor);
}
// Ctrl picker color blending.
if (previousColor && blend < 100) {
// Color blending.
if (!pure && blendColor && blend < 100) {
//Scale from 0..100% to 0..255 range for mixOp weights.
quint8 blendScaled = static_cast<quint8>(blend * 2.55f);
const quint8 *colors[2];
colors[0] = previousColor->data();
colors[0] = blendColor->data();
colors[1] = pickedColor.data();
qint16 weights[2];
weights[0] = 255 - blendScaled;
......@@ -79,12 +80,11 @@ namespace KisToolUtils {
pickedColor.convertTo(dev->compositionSourceColorSpace());
bool validColorPicked =
pickedColor.opacityU8() != OPACITY_TRANSPARENT_U8;
bool validColorPicked = pickedColor.opacityU8() != OPACITY_TRANSPARENT_U8;
if (validColorPicked) {
pickedColor.setOpacity(OPACITY_OPAQUE_U8);
*color = pickedColor;
out_color = pickedColor;
}
return validColorPicked;
......@@ -200,5 +200,4 @@ namespace KisToolUtils {
radius = props.getInt("radius", 1);
blend = props.getInt("blend", 100);
}
}
......@@ -46,10 +46,21 @@ private:
};
/**
* return the color at the given position on the given paint device.
* Pick a color based on the given position on the given paint device.
*
* out_color - Output parameter returning newly picked color.
* dev - Paint device to pick from.
* pos - Position to pick from.
* blendColor - Optional color to be blended with.
* radius - Picking area radius in pixels.
* blend - Blend percentage. 100% all picked, 0% all blendColor.
* pure - Whether to bypass radius, blending, and active layer settings for pure picking.
*
* RETURN - Returns true if a valid color was picked.
*/
bool KRITAUI_EXPORT pick(KisPaintDeviceSP dev, const QPoint &pos, KoColor *color,
KoColor *previousColor = nullptr, int radius = 1, int blend = 100);
bool KRITAUI_EXPORT pickColor(KoColor &out_color, KisPaintDeviceSP dev, const QPoint &pos,
KoColor const *const blendColor = nullptr, int radius = 1,
int blend = 100, bool pure = false);
/**
* Recursively search a node with a non-transparent pixel
......
......@@ -56,7 +56,7 @@ void KisColorPickerStrokeStrategy::doStrokeCallback(KisStrokeJobData *data)
KoColor color;
KoColor previous = d->currentColor;
bool result = KisToolUtils::pick(d->dev, d->pt, &color, &previous, m_d->radius, m_d->blend);
bool result = KisToolUtils::pickColor(color, d->dev, d->pt, &previous, m_d->radius, m_d->blend);
Q_UNUSED(result);
emit sigColorUpdated(color);
......
......@@ -257,7 +257,7 @@ void KisScratchPad::endPan(KoPointerEvent *event)
void KisScratchPad::pick(KoPointerEvent *event)
{
KoColor color;
if (KisToolUtils::pick(m_paintLayer->projection(), event->point.toPoint(), &color)) {
if (KisToolUtils::pickColor(color, m_paintLayer->projection(), event->point.toPoint())) {
emit colorSelected(color);
}
}
......
......@@ -82,6 +82,7 @@ void KisToolColorPicker::deactivate()
void KisToolColorPicker::pickColor(const QPointF &pos)
{
// Timer check.
if (m_colorPickerDelayTimer.isActive()) {
return;
}
......@@ -94,6 +95,7 @@ void KisToolColorPicker::pickColor(const QPointF &pos)
m_pickedColor.setOpacity(0.0);
// Pick from reference images.
if (m_optionsWidget->cmbSources->currentIndex() == SAMPLE_MERGED) {
auto *kisCanvas = dynamic_cast<KisCanvas2 *>(canvas());
KIS_SAFE_ASSERT_RECOVER_RETURN(kisCanvas);
......@@ -120,67 +122,9 @@ void KisToolColorPicker::pickColor(const QPointF &pos)
dev = currentImage()->projection();
}
// Color sampling radius.
if (m_config->radius == 1) {
QPoint realPos = pos.toPoint();
if (currentImage()->wrapAroundModePermitted()) {
realPos = KisWrappedRect::ptToWrappedPt(realPos, currentImage()->bounds());
}
dev->pixel(realPos.x(), realPos.y(), &m_pickedColor);
}
else {
const KoColorSpace *cs = dev->colorSpace();
int pixelSize = cs->pixelSize();
quint8 *dstColor = new quint8[pixelSize];
QVector<const quint8 *> pixels;
KisRandomConstAccessorSP accessor = dev->createRandomConstAccessorNG(0, 0);
for (int y = -m_config->radius; y <= m_config->radius; y++) {
for (int x = -m_config->radius; x <= m_config->radius; x++) {
if (((x * x) + (y * y)) < m_config->radius * m_config->radius) {
QPoint realPos(pos.x() + x, pos.y() + y);
if (currentImage()->wrapAroundModePermitted()) {
realPos = KisWrappedRect::ptToWrappedPt(realPos, currentImage()->bounds());
}
accessor->moveTo(realPos.x(), realPos.y());
pixels << accessor->oldRawData();
}
}
}
const quint8 **cpixels = const_cast<const quint8 **>(pixels.constData());
cs->mixColorsOp()->mixColors(cpixels, pixels.size(), dstColor);
m_pickedColor = KoColor(dstColor, cs);
delete[] dstColor;
}
// Color blending.
if(m_config->blend < 100){
//Scale from 0..100% to 0..255 range for mixOp weights.
quint8 blendScaled = static_cast<quint8>(m_config->blend * 2.55f);
KoColor previousColor = canvas()->resourceManager()->foregroundColor();
const quint8 *colors[2];
colors[0] = previousColor.data();
colors[1] = m_pickedColor.data();
qint16 weights[2];
weights[0] = 255 - blendScaled;
weights[1] = blendScaled;
const KoMixColorsOp *mixOp = dev->colorSpace()->mixColorsOp();
mixOp->mixColors(colors, weights, 2, m_pickedColor.data());
}
KoColor previousColor = canvas()->resourceManager()->foregroundColor();
m_pickedColor.convertTo(dev->compositionSourceColorSpace());
KisToolUtils::pickColor(m_pickedColor, dev, pos.toPoint(), &previousColor, m_config->radius, m_config->blend); /*!*/
if (m_config->updateColor &&
m_pickedColor.opacityU8() != OPACITY_TRANSPARENT_U8) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment