Persistent color picker tool / Lockable Color Picker #1812
Comments
Reported by |
|
That would be useful! It is in darktable, sometimes i use it to equalize skin tones between people. |
Beep6581
added
the
type: enhancement
|
I don't know how you would use this feature in practice, but I'd like to know if it has to be a tool (stored in the pp3) or just a volatile tool that will reset itself at each image loading ? |
|
That would be useful feature. 👍 if the impact on processing time in gui is low. If there are changes in gui processing pipeline which decrease performance I will take a look to solve them. Ingo |
|
thanks for the offer @heckflosse . The tool will only look at the image data at the end of the pipeline, so there shouldn't be any impact on the processing. Any comment for the pp3 vs volatile choice ? |
|
@Hombre57 I think volatile would be sufficient |
I think so too. And doing a Tool (i.e. saved in the pp3) for this would add much work in fact. |
Volatile, shouldn't touch the PP3. Let's say I want to make some region a specific hue, I would just drop the picker on that spot and move some widget such as the HH curve, until the hue is reached. The pickers could be just little circles "o" over the preview, and clicking a circle removes it. Opening the next photo would also remove all of the circles, the same as with the little "Detail" zoom windows. |
|
Ok. So here is my plan : I'd add a new button in the top toolbar of the Editor panel, next to the Hand or Straighten tool. When pressed, you'll only able to add/move/resize/delete the color pickers. When unpressed, everything will remain visible bu you won't be able to interact with the pickers anymore. The color picker will be a square shape (I know that a circle would be better, but that may be done later) with the detected color displayed inside, of a modifiable size and with values displayed below. I know that it will hide part of the context next to the picker, but is this really important ? We have to find some place to display the values anyway... You'll be able to set the font size in Preferences, but the font shape will be the same than the Navigator tool. The expected behavior is (when in Color Picker mode) :
I think it will be a good starting point. I'll start this patch this week, beside finishing the Soft-proofing patch (with the help of Marty if he's okay to help). |
|
And... should we be able to show/hide the pickers in some ways ? Then I'd suggest to right click on the Color Picker button, with some obvious logic. I know that this is not intuitive, but a tooltip will be there to help. For the on preview behavior, users will have to RTFM or explore by themselves. |
|
@Hombre57 For the circle shape maybe I can point you to the code of focus mask (where the shape also was a square before I made circles) Ingo |
|
@heckflosse I didn't even knew that the focus mask (for focus peaking ???) uses circles 😅 . That may help depending on what it really is. I need an algorithm to read values inside a defined circle size, I guess that I'll find something using polar coordinates... |
|
@Hombre57 focus mask uses a very simple (and fast) approach to draw small circles. Maybe you can use this too. |
|
Just a comment to say that the patch is progressing well. It's more difficult than expected but I've found a solution for every problem I think. Still 20% of work to be done, it won't be finished by the end of the month but I'm confident for a mid-october first release (or before if I have more free time). It also depend on the requirment of the Soft-proofing branch for RT5 that seem to have a higher priority, see issue #3300. |
|
For those of you who are interested in this feature, I've create a new The displayed values are those from the internal Working Space image (have to check that...) i.e. not color corrected. If you use this tool to compare different images, this should not be a problem. What's already working fine :
If you create a picker in a Detail window, it should be shown in its Detail window only. This doesn't work for now, the picker become black when dragged outside it. Will be solved soon. Other flaws to solve and add features to add (but the most part is done) :
Don't care about reporting bug as of now (unless you experience a crash), I have to finish it first then the testing phase can start. |
|
Oh, I forgot to add : the user should also be able to increase the picker size. Not done yet. Furthermore, as you can see, the picker has a fixed size whatever your zoom rate is. It means that you can collect more data by zooming out, thanks to the scaling. |
May I also post a patch instead of creating a new issue ;-) ? |
|
@heckflosse In SETM, opening a new image while in Color Picker mode will disable this mode, so when double clicking on a new image I have to wait that the image is displayed, then press 'z', then click on the Color Picker tool, then place a picker and drag it -> no crash. I'm using a debug build, and Gtk3.22 compiled with Gcc 6.xx. I can't say better.
...ok...
Thanks, works fine ! |
|
@heckflosse With the BT you posted above, can you see if @Floessie Should I protect the IPC from being accessed asynchronously by the GUI with a Mutex ? On line 789 of editorpanel.cc, |
Well, I think we didn't understood ourselves. I consider seeing the color corresponding to the displayed RGB values as correct (and it's true that they can be very different than what's displayed in the preview). You consider that whatever the displayed RGB values are, we should display the preview's color, it would be a color mismatch for me. [Time to go to sleep, we'll continue discussion tomorrow, good night] |
But what you did by doing this is displaying it in preview in a wrong colour space. Assume working profile is ProphotoRgb, then the rgb values (in working profile) are very different to e.g. srgb. But simply using the ProhotoRgb values in preview (which in my assumption is sRGB) leads to a colour inside the circle which is of no use (at least of no use I'm aware of) Imho we should display the correct values (for working profile or output profile), but in preview the colour of the circle should match the preview colour behind it. |
|
I can reproduce it with a debug build on Debian Stretch with GCC 6.1.1 and GTK 2.24.
No, it's rather an unregistered callback like the one in #3048. |
|
@Hombre57 Don't look any further, I think I've found it. Preparing a patch... |
|
The problem was, that although HTH diff --git a/rtgui/cropwindow.cc b/rtgui/cropwindow.cc
index df9cb9f..8b3210f 100644
--- a/rtgui/cropwindow.cc
+++ b/rtgui/cropwindow.cc
@@ -66,13 +66,13 @@ ZoomStep zoomSteps[] = {
#define MAXZOOMSTEPS 20
#define ZOOM11INDEX 13
-CropWindow::CropWindow (ImageArea* parent, rtengine::StagedImageProcessor* ipc_, bool isLowUpdatePriority_, bool isDetailWindow)
+CropWindow::CropWindow (ImageArea* parent, bool isLowUpdatePriority_, bool isDetailWindow)
: ObjectMOBuffer(parent), state(SNormal), press_x(0), press_y(0), action_x(0), action_y(0), pickedObject(-1), pickModifierKey(0), rot_deg(0), onResizeArea(false), deleted(false),
fitZoomEnabled(true), fitZoom(false), isLowUpdatePriority(isLowUpdatePriority_), hoveredPicker(nullptr), cropLabel(Glib::ustring("100%")),
backColor(options.bgcolor), decorated(true), isFlawnOver(false), titleHeight(30), sideBorderWidth(3), lowerBorderWidth(3),
upperBorderWidth(1), sepWidth(2), xpos(30), ypos(30), width(0), height(0), imgAreaX(0), imgAreaY(0), imgAreaW(0), imgAreaH(0),
imgX(-1), imgY(-1), imgW(1), imgH(1), iarea(parent), cropZoom(0), zoomVersion(0), exposeVersion(0), cropgl(NULL),
- pmlistener(NULL), pmhlistener(NULL), observedCropWin(NULL), ipc(ipc_)
+ pmlistener(NULL), pmhlistener(NULL), observedCropWin(NULL)
{
Glib::RefPtr<Pango::Context> context = parent->get_pango_context () ;
Pango::FontDescription fontd = context->get_font_description ();
@@ -110,7 +110,7 @@ CropWindow::CropWindow (ImageArea* parent, rtengine::StagedImageProcessor* ipc_,
minWidth = bsw + iw + 2 * sideBorderWidth;
cropHandler.setDisplayHandler(this);
- cropHandler.newImage (ipc_, isDetailWindow);
+ cropHandler.newImage (parent->getImProcCoordinator(), isDetailWindow);
}
CropWindow::~CropWindow ()
@@ -904,13 +904,13 @@ void CropWindow::pointerMoved (int bstate, int x, int y)
screenCoordToImage (x, y, imgPos.x, imgPos.y);
if (imgPos.x < 0) {
imgPos.x = 0;
- }else if (imgPos.x >= ipc->getFullWidth()) {
- imgPos.x = ipc->getFullWidth()-1;
+ }else if (imgPos.x >= iarea->getImProcCoordinator()->getFullWidth()) {
+ imgPos.x = iarea->getImProcCoordinator()->getFullWidth()-1;
}
if (imgPos.y < 0) {
imgPos.y = 0;
- }else if (imgPos.y >= ipc->getFullHeight()) {
- imgPos.y = ipc->getFullHeight()-1;
+ }else if (imgPos.y >= iarea->getImProcCoordinator()->getFullHeight()) {
+ imgPos.y = iarea->getImProcCoordinator()->getFullHeight()-1;
}
imageCoordToCropImage(imgPos.x, imgPos.y, cropPos.x, cropPos.y);
float r=0.f, g=0.f, b=0.f;
@@ -2056,10 +2056,10 @@ void CropWindow::buttonPressed (LWButton* button, int actionCode, void* actionDa
} else if (button == bZoom100) { // zoom 100
zoom11 ();
} else if (button == bClose) { // close
- if(ipc->updateTryLock()) {
+ if(iarea->getImProcCoordinator()->updateTryLock()) {
deleted = true;
iarea->cropWindowClosed (this);
- ipc->updateUnLock();
+ iarea->getImProcCoordinator()->updateUnLock();
}
}
}
diff --git a/rtgui/cropwindow.h b/rtgui/cropwindow.h
index 089fcb8..5796a78 100644
--- a/rtgui/cropwindow.h
+++ b/rtgui/cropwindow.h
@@ -94,7 +94,6 @@ class CropWindow : public LWButtonListener, public CropDisplayHandler, public Ed
std::list<CropWindowListener*> listeners;
CropWindow* observedCropWin; // Pointer to the currently active detail CropWindow
- rtengine::StagedImageProcessor* ipc;
bool onArea (CursorArea a, int x, int y);
void updateCursor (int x, int y);
@@ -113,7 +112,7 @@ class CropWindow : public LWButtonListener, public CropDisplayHandler, public Ed
public:
CropHandler cropHandler;
- CropWindow (ImageArea* parent, rtengine::StagedImageProcessor* ipc_, bool isLowUpdatePriority_, bool isDetailWindow);
+ CropWindow (ImageArea* parent, bool isLowUpdatePriority_, bool isDetailWindow);
~CropWindow ();
void setDecorated (bool decorated)
diff --git a/rtgui/imagearea.cc b/rtgui/imagearea.cc
index c09051e..125968b 100644
--- a/rtgui/imagearea.cc
+++ b/rtgui/imagearea.cc
@@ -88,7 +88,7 @@ void ImageArea::on_resized (Gtk::Allocation& req)
{
if (ipc && get_width() > 1) { // sometimes on_resize is called in some init state, causing wrong sizes
if (!mainCropWindow) {
- mainCropWindow = new CropWindow (this, ipc, false, false);
+ mainCropWindow = new CropWindow (this, false, false);
mainCropWindow->setDecorated (false);
mainCropWindow->setFitZoomEnabled (true);
mainCropWindow->addCropWindowListener (this);
@@ -106,7 +106,12 @@ void ImageArea::on_resized (Gtk::Allocation& req)
}
}
-void ImageArea::setImProcCoordinator (rtengine::StagedImageProcessor* ipc_)
+rtengine::StagedImageProcessor* ImageArea::getImProcCoordinator() const
+{
+ return ipc;
+}
+
+void ImageArea::setImProcCoordinator(rtengine::StagedImageProcessor* ipc_)
{
if( !ipc_ ) {
focusGrabber = NULL;
@@ -398,7 +403,7 @@ void ImageArea::addCropWindow ()
return; // if called but no image is loaded, it would crash
}
- CropWindow* cw = new CropWindow (this, ipc, true, true);
+ CropWindow* cw = new CropWindow (this, true, true);
cw->zoom11();
cw->setCropGUIListener (cropgl);
cw->setPointerMotionListener (pmlistener);
diff --git a/rtgui/imagearea.h b/rtgui/imagearea.h
index deabe8a..71d4007 100644
--- a/rtgui/imagearea.h
+++ b/rtgui/imagearea.h
@@ -73,7 +73,8 @@ public:
ImageArea (ImageAreaPanel* p);
~ImageArea ();
- void setImProcCoordinator (rtengine::StagedImageProcessor* ipc_);
+ rtengine::StagedImageProcessor* getImProcCoordinator() const;
+ void setImProcCoordinator(rtengine::StagedImageProcessor* ipc_);
void setPreviewModePanel(PreviewModePanel* previewModePanel_)
{
previewModePanel = previewModePanel_; |
|
@Floessie Great that you fixed the bug, I'll let @heckflosse and @Beep6581 test it, since I didn't experienced it.
I understand better now. I'll integrate your patch then. |
|
@Floessie wrote
Your patch builds fine and fixes the crash. Thank you! |
Please wait. I just detected that it doesn't always work correctly |
|
@Hombre57 I updated the link from above with the corrected code |
|
Last commit added the bugfix from @heckflosse and @Floessie (thanks a lot for the help guys) + the request from @Beep6581 . Please test for new bugs, but feature wise it's complete now. |
|
Added a bugfix and code simplification in a new commit. |
|
@Floessie I tend to forget that kind of useful things. Will do next time. |
|
@Hombre57 Great, works fine now |
|
@Hombre57 I found another issue |
when the Color Picker mode is off. see issue #1812
|
Imho now it's right to merge :) |
|
Done ! Thanks for the debugging :) |
|
@Hombre57 Thank you for implementing this great tool. I especially like it in combination with hsv curves to partly desaturate an image. Though for this use case it would be an improvement to show the hue value at the x-axis of hsv curve. But that's a different issue. Well done! Ingo |
|
Yes, i also wanted to say thanks for your efforts @Hombre57. The tool was implemented very nicely and it will be a pleasure to use. Looking forward to seeing it in the official release..... Anyone have a clue when that might be? |
We are in the process of releasing RT5, which should be due by the end of the month. As the lockable color pickers have already been merged to master, they will be part of that release. |
Originally reported on Google Code with ID 1828
Reported by
entertheyonion 2013-04-05 13:21:45The text was updated successfully, but these errors were encountered: