color picker updates: restrict histogram fix, overlay drawing change, internal fixes#9835
Conversation
|
Awesome work!
I will do more test on the module pickers |
johnny-bit
added
bugfix
|
First off, many thanks for picking this up and let me congratulate you on your bravery! When I looked at this, the complexity of untangling the many layers daunted me and I stopped at the proxy layer. I've only had a quick look, but the pickers themselves look so much better and thanks for fixing the handle dragging under zoom that I never got round to. Maybe the point picker shouldn't scale with zoom though? Of course this will need a lot of testing as there are many subtleties. There used to be issues with modules either receiving updates too often, or not receiving an initial update at all immediately after a picker was switched on. This was the reason for initialising with NaN for each state switch, to force an update even if the previous and current picker had same (default) selection. Even to test for this is finicky... People are very sensitive to small changes in behaviour, even if they are "objectively" improvements. Don your thick skin! |
johnny-bit
added
difficulty: hard
|
I found one issue, with this PR the overlay gradient line in module graduated density is no longer visible. |
|
@dterrahe @Mark-64 Thank you for notes/tests. I'm grateful for the eyes on this now, even if it means putting up a PR that isn't yet ironclad.
I've wondered about this as well. It would make the draw code much simpler. Having it scale with zoom gives a visual cue that the zoom level has changed. (Box pickers give that cue by increasing/decreasing the box size with zoom.) But to really give this cue, the line width of the scope should also increase, at least when > 1:1 pixel resolution.
And this is yet harder due to the zoomable point picker. Another problem: When the center view is zoomed in, the pixel in the center of the crosshair doesn't actually represent the output sample. This is because the picker is reading the (low-res) preview pipe, and is missing a bunch of extra detail in the region-of-interest. I'll take another look at this.
Interesting idea. I'll see if it's viable -- whether it works with (or against) what Cairo makes easy. And how to present this nicely. As above, we have to be careful as what appears to be in the center of the picker in the center view may not actually be what the picker is using for its sample.
This is a good idea. Are you thinking to highlight it when mouse over the primary picker readout in the left panel (as with live samples)? That should be pretty easy to do. Highlighting it when mouse-over in center view may be possible, but would be harder. If so, it could also highlight live samples on mouse over as well.
I've been wondering about this too. I think the current code makes it hard to restore the picker location for the primary picker, but it should be possible to do a workaround. Another question is if we have an iop picker turned on, then turn click on the primary picker, what should happen when the primary picker activates:
I'm thinking 2) is the best option, and the most straightforward (no special cases).
Thank you for catching. I'll work on this.
I spent a long time not understanding the proxy layer. But this PR should actually increase its importance -- it's very much a key hub which connects and sets behaviors for the picker widgets, the center view, and the actual picking code.
Yes!
Makes sense. I'll look at this carefully.
Will do! |
By all mean do not hesitate to put as much comment in the code to help others grasping this part. TIA. |
I'm not sure that visual clue is needed; it is not provided when there is no picker active. The box size increasing is just, well, because it is more zoomed in. I find the large point picker strange/ungainly. But that's only one opinion of course!
It might be nice to show the color that is actually picked in the center of the picker. That is, create a dot of a few pixels that is filled with the picker result. But maybe that has not been calculated yet when the picker shape is drawn, so this might be hard?
Makes the most sense to me too. Two quick ideas:
One could combined those two to allow passing any picker to another one. |
Yes, let's keep things simple |
This should be available by the time the center view is redrawn. I'll see! Another thought is to make the central square the size of a pixel in the preview pipe, to give a since of the sample resolution. When zoomed out sufficiently, this would have a minimum size (4x4 pixel square?). This would keep some vestige of a visual cue to zoom level, but more importantly would let the user know that the point sample isn't actual sampling pixels 1:1 from the full pixelpipe but rather from preview pixelpipe. Maybe the picker is a crosshairs, with sample-colored square in the middle, and when it is focused/moveable there's a square (or circle, if not looking too much like a rifle sight?) around it.
Yes! The live sample would be sampling the end of the pixelpipe (as always), yes? Not a special case where it sampled at the iop (harder to do & confusing).
Yes! Clicking on the swatch currently "locks" the live sample (a bit of an obscure feature...), but at the least clicking on the readout numbers could copy the sample area. Lots to do, but these all seem good changes to add in to this PR. |
|
Another minor thing: Especially when dragging the point sample, the mouse pointer's "arrow" cursor gets in the way of seeing what's happening. It would be nice to hide the GDK cursor then, but there'd need to be some additional visual clue that the colorpicker is being dragged... |
|
great work, here my findings on a first view: while in vectorscope displaying the picked color is fine (just the white spot) restricting the histogram when using the picker in a processing module (right panel) is strange and not useful in my opinion. If i select the restriction in the left panel it's about evaluating the selected area/point in the histogram; in color picker mode in the right panel it’s about getting color info to set a parametric mask, a controlpoint,... not to evaluate the histogram. |
I like it!
Yes, definitely. Maybe there are educational opportunities here to teach newcomers about sampling colorpickers at different stages of the pipe, but I have no ideas on how to approach that. They may just have to RTFM.
That would be a very subtle difference depending on area. Maybe a right-click on swatch instead? Clicking on the numbers might be better used to implement #9836. Btw I assume you have on your todo that hovering over live point sample still shows zoomed cross (your pace of change is impressive). Other idea (complementary to above); currently right clicking in center view always resets area to full image. What about if you right click within a live sample, it sets current picker to same area as sample? And if overlapping samples, multiple right-clicks cycle between them. And one of the stops in the cycle is full image, so still incorporates current functionality. Right-click in center and right-click on swatch would be consistent/"intuitive". |
Jiyone on IRC asked if it would be possible to sample color very early in the pipe, probably at raw black/white point or demosaic, mainly for callibrating film scans done with camera where red channel of base film needs to be in 90-100% range |
Yes, I see this as well. It's particularly annoying at 400%. I thought this was just how the code worked, but sounds like it's something I broke. I'll look at it.
That's a good point. I'll try flipping the histogram code so it only minds "restrict histogram to selection" if this is the primary (left panel) colorpicker. (Alternative would be some sort of customization of this behavior, but that seems way too fine grained...)
This makes sense.
I did that on purpose, to make the picker extra-noticeable -- I thought showing a color swatch there would be confusing. I can experiment though. I tried making the point picker outline circular, to emphasize that it (ideally) is a sample of pixels which are a sample of an image (samples all the way down). So it looks something like:
When zoomed in to >= 400%, it changes to a square central sample at preview pipe resolution:
And does expand slightly (to allow room for the sample) up to 1600%:
And hover of live sample behavior (here at 400%):
Interesting! I'll try this. If it's not too code intensive!
Another educational opportunity is to see just how untrustworthy a point sample can be, particularly from the preview pixelpipe. For example, from an X-Trans image:
If the input is demosaiced (straight RGB from a film scanner), would it be possible to move RGB Curve iop to the start of the pixelpipe? That gives you a histogram for that point in the pipe, as well as a point/area colorpicker which give numerical readouts (on a 0-255 scale...). |
|
There's a mysterious build error, it looks like CI may have picked up a non-current version of a file to compile...? There's a bunch more details changed:
This gets this close to "feature complete" (read that as "initial plan plus a bunch of ideas which came up along the way"). The one missing thing that I remember from the thread above would be to allow for right-clicking in the center view to copy a live sample's area to the active picker. The overlays for tone equalizer and graduated density iops should be back to working. I sped up the point picker dragging code slightly. Unfortunately, it looks like the slowdown is actually not in the pixelpipe (which does need to partially run to calculate the new sample) but in redrawing the whole center view just to update the picker overlay. This may be worse for hidpi displays. I went back to the code before this PR which also seems to me about as laggy. I notice it a lot at 400%. I'm also wondering if hiding the mouse pointer while dragging makes the lag more noticeable, as nothing now updates smoothly. One way to test this is to set in conf If drawing is indeed the problem, then this is something to be aware of in general as a bottleneck. There may be a way to cache the most recent scaled-to-center-view version of the current image, then only apply the overlays on top, but that doesn't really get to the root of it. There's a bunch of new internal overhaul. Mainly:
No doubt things are broken, as these are so many changes! |
|
CI still appears to be failing with an out-of-date version of |
Store for dt_colorpicker_sample_t structure in dt_lib_colorpicker_t as s/proxy_linked/primary_sample/. Use its data for picker color rgb/lab min/max/mean and size. Goal is not to store primary colorpicker data in colorout at all so that it isn't tied to colorout being focused. For now it is stored in dt_lib_colorpicker_t and in current module. Should continue to be functional, but haven't added new functionality. Still need to get rid of overlapping code and simplify things. Colorpickers in iops seem to continue to work, but need to see if some overlapping code or other test cases are necessary. Note that per-module data still comes from dt_iop_module_t rather than primary colorpicker. Set primary sample point/box in rest of cases on mouse down in center view. Note that this sets primary sample even when per-module picker is active, in hopes that later can make primary sample give a per-module readout. Use primary sample's box/point data when doing color picking for it in pixelpipe, rather than per-module sample.
The internal pixelpipe _pixelpipe_pick_from_image() previously had a bunch of values passed to it. As these are now all in dt_colorpicker_sample_t for both primary colorpicker and live samples, just pass in that struct.
- Restore mean/min/max swatches to tooltip. This was lost in a previous commit. Also some tidying: - Just store 0-255 int values for RGB in histogram profile, as that is how they are used later. - Straighten out some float/double math. - Don't need to check for non-negative values when applying levels or rgblevels. - Live samples in rgb curve appear to be working, so remove a FIXME - Remove some obsolete FIXMEs in darkroom.c
dtorop
force-pushed
the
restrict-histogram-during-edit
branch
from
e331f3c to
8deab03
Compare
Reassure the compiler that variables are initialized.
I'm thinking that it's the time to be finished adding features to this PR. Any further testing/feedback much appreciated. |
|
First of all, this is a really nice work and a great addition to dt ! One comment:
Apart from that it is ok to me and I'd like to merge this soon now for having more field testing because as you say some parts may be broken even if all my testing seems to be working fine. |
The crosshair is now 2x (rather than 3x) the regular size.
Thank you!!
A fair point... I added a commit to reduce it so it is 2x (rather than 3x). It does look better to me. It's easy to tune this more...
Yes! For what it is worth, my greatest fear about the code is that there will be some (very subtle) breakage due to thread safety and shared data. Worst case, as far as I can tell, is that things will "self correct" as the color picker is an interactive tool, but better that things don't come to that. I'm happy if this is merged, and then any bug reports and feature requests are steered toward me... That would also allow for another chance to clean up residual rough edges. |
Generalizes configuration code. At some point it might be nice to eliminate some of the enum boilerplate though....
|
@TurboGit : I've found a few other quirks in this PR, particularly:
I've got fixes for all but the last of these. The last one is a mystery (apparently introduced in c259208), but I at least have a workaround. I'm reluctant to keep adding commits to this PR, as it might complicate reviewing it and the above are more annoyances than major bugs. Would it be better to go with this PR as is, and work up a follow-up PR with fixes? Or just keep trying to fix details here? |
I cannot make this to work, is that me or something is broken? |
|
There is also a regression, on the console each time I click on the global color-picker (point or area) I get: |
That's not good. It must be broken with cLUT display profiles. Is it easy to send over your display profile to me so I can test? I don't have a test profile on hand here.
I disabled that in response to #9835 (comment) from @MStraeten:
|
|
The issue is when I set the screen profile to system and the GNOME color management has been assigned a profile created from DisplayCal. Here is such profile: |
Sorry for missing that and thanks for the clarification. |
Thank you for sending, that's helpful. It looks like the problem is that I added a commit which allows I looked back through the code history, and there's no note of why RGB -> RGB code uses |
Previousely, the conversion in _transform_from_to_rgb_lab_lcms2() would revert to Rec.2020 if the RGB profile was not a work profile. Because dt_colorspaces_init() only allows matrix profiles as work profiles, this means that CLUT profiles cannot be used in _transform_from_to_rgb_lab_lcms2(). LCMS is capabable of transforming CLUT profiles, so it doesn't make sense to not allow them in this transform. This is in line with the behavior of _transform_rgb_to_rgb_lcms2() whioch also uses DT_PROFILE_DIRECTION_ANY. In case of display profile in _transform_from_to_rgb_lab_lcms2(), take care of xprofile_lock work. This change allows for a succinct conversion in _pixelpipe_pick_from_image() from RGB to Lab. Note that the equivalent code before this PR always used LittleCMS, and used profile direction: DT_PROFILE_DIRECTION_OUT | DT_PROFILE_DIRECTION_DISPLAY
dtorop
force-pushed
the
restrict-histogram-during-edit
branch
from
10e931f to
8dae032
Compare
TurboGit
left a comment
TurboGit
left a comment
There was a problem hiding this comment.
Looks ok to me. That being said I must confess that it is difficult to do a proper review as this part is very delicate and I don't have a proper full view on this. My testing are all ok and hopefully remaining glitches will be found soon and well before the 3.8 release. A big thanks for this amazing work !
|
@TurboGit : That's great! I absolutely agree regarding worries about fragility and proper review. I do have a queue of fixes/improvements still in progress. I'll be working on a follow-up PR with these! |
elstoc
added
documentation-complete
7 participants
This is a mix of
Change behavior of color pickers
Internal changes
Caveats/worries
Examples of visual changes
Screenshots are at 200% due to being from a hidpi display. Image is a cloudy sky, to demonstrate an image in which it is somewhat difficult to see the picker overlay
Point colorpicker
Focused
Defocused
Live sample
"Selected" live sample
Box colorpicker
Focused
Defocused
Live sample
"Selected" live sample