Skip to content

Commit

Permalink
fix some darktable-org#16534 regressions dragging pickerbox when rota…
Browse files Browse the repository at this point in the history
…ted 45°

- grab any corner
- change only cursor corner
- right+click in live sample to select
  • Loading branch information
dterrahe committed Jun 12, 2024
1 parent 5177c59 commit 70f42cd
Show file tree
Hide file tree
Showing 10 changed files with 80 additions and 90 deletions.
73 changes: 33 additions & 40 deletions src/common/color_picker.c
Original file line number Diff line number Diff line change
Expand Up @@ -276,26 +276,20 @@ void dt_color_picker_backtransform_box(dt_develop_t *dev,
const float ht = dev->preview_pipe->iheight;
const float wdp = dev->preview_pipe->processed_width;
const float htp = dev->preview_pipe->processed_height;
const gboolean box = num == 2;
int out_num = num == 2 ? 4 : 1;
if(wd < 1.0f || ht < 1.0f || wdp < 1.0f || htp < 1.0f)
{
for(int i = 0; i < num; i++)
out[i] = in[i];
return;
}

dt_boundingbox_t fbox = { wdp * in[0],
htp * in[1],
box ? wdp * in[2] : 0.0f,
box ? htp * in[3] : 0.0f };
dt_dev_distort_backtransform(dev, fbox, num);

out[0] = fbox[0] / wd;
out[1] = fbox[1] / ht;
if(box)
for(int i = 0; i < out_num; i++)
{
out[i * 2 ] = wdp * in[(i % 3 > 0) * 2];
out[i * 2 + 1] = htp * in[(i % 2) * 2 + 1];
}
dt_dev_distort_backtransform(dev, out, out_num);
for(int i = 0; i < out_num; i++)
{
out[2] = fbox[2] / wd;
out[3] = fbox[3] / ht;
out[i * 2 ] /= wd;
out[i * 2 + 1] /= ht;
}
}

Expand All @@ -320,10 +314,14 @@ static void _sort_coordinates(float *fbox)
void dt_color_picker_transform_box(dt_develop_t *dev,
const int num,
const float *in,
float *out)
float *out,
gboolean scale)
{
const float wd = dev->preview_pipe->iwidth;
const float ht = dev->preview_pipe->iheight;
const float wdp = scale ? dev->preview_pipe->processed_width : 1.0f;
const float htp = scale ? dev->preview_pipe->processed_height : 1.0f;

const gboolean box = num == 2;
if(wd < 1.0f || ht < 1.0f)
{
Expand All @@ -332,26 +330,27 @@ void dt_color_picker_transform_box(dt_develop_t *dev,
return;
}

const float x0 = wd * in[0];
const float y0 = ht * in[1];
const float x1 = wd * in[2];
const float y1 = ht * in[3];
dt_pickerbox_t fbox;
for(int i = 0; i < 8; i += 2)
{
fbox[i] = wd * in[i ];
fbox[i + 1] = ht * in[i + 1];
}

float fbox[8] = { x0,y0, x0,y1, x1,y0, x1,y1 };
dt_dev_distort_transform(dev, fbox, box ? 4 : 1);

if(box) // sort the 4 point coordinates
{
_sort_coordinates(fbox);
out[0] = 0.5f * (fbox[0] + fbox[2]);
out[1] = 0.5f * (fbox[1] + fbox[3]);
out[2] = 0.5f * (fbox[4] + fbox[6]);
out[3] = 0.5f * (fbox[5] + fbox[7]);
out[0] = 0.5f * (fbox[0] + fbox[2]) / wdp;
out[1] = 0.5f * (fbox[1] + fbox[3]) / htp;
out[2] = 0.5f * (fbox[4] + fbox[6]) / wdp;
out[3] = 0.5f * (fbox[5] + fbox[7]) / htp;
}
else
{
out[0] = fbox[0];
out[1] = fbox[1];
out[0] = fbox[0] / wdp;
out[1] = fbox[1] / htp;
}
}

Expand All @@ -374,23 +373,17 @@ gboolean dt_color_picker_box(dt_iop_module_t *module,
const int height = roi->height;
const gboolean isbox = sample->size == DT_LIB_COLORPICKER_SIZE_BOX;

const float bx0 = wd * sample->box[0];
const float by0 = ht * sample->box[1];
const float bx1 = wd * sample->box[2];
const float by1 = ht * sample->box[3];

const float sx = sample->point[0] * wd;
const float sy = sample->point[1] * ht;

/* get absolute pixel coordinates in final preview image.
we transform back all 4 corner locations to current module coordinates,
sort the coordinates, and use average of 2 highest and 2 lowest for the
resulting rectangle.
*/
float fbox[8] = { isbox ? bx0 : sx, isbox ? by0 : sy,
isbox ? bx0 : sx, isbox ? by1 : sy,
isbox ? bx1 : sx, isbox ? by0 : sy,
isbox ? bx1 : sx, isbox ? by1 : sy };

This comment has been minimized.

Copy link
@dterrahe

dterrahe Jun 12, 2024

Author Owner

@jenshannoschwalm @TurboGit

Using a square box in raw-space can cause all four transformed points to move when one corner is dragged. Store all back-transformed corners.

This comment has been minimized.

Copy link
@jenshannoschwalm

jenshannoschwalm Jun 13, 2024

Right :-) Didn't yet understand all your changes from reading the diffs.

It seems to me that this overall keeps the non-dragged corners at fixed locations, correct?
But do these corners reflect the area actually used when reading picker data? Currently we always use a rectangle, wouldn't we have to change that algo to read data "per line" with line borders calculated from all-corners then?

This comment has been minimized.

Copy link
@dterrahe

dterrahe Jun 13, 2024

Author Owner

It seems to me that this overall keeps the non-dragged corners at fixed locations, correct?

It keeps the opposite corner in fixed position, even with "extreme" transformations (e.g. 45° rotation), like it used to do.

Currently we always use a rectangle

dt_color_picker_transform_box still always produces a rectangle. But during dragging it will be the rectangle with the same dragged and fixed opposite corners, rather than a strangely "adjusted" one that moves about somewhat detached from the cursor.

If you have no problem with how color pickers behave on a tilted image in master then there's no need to test this "solution" and I'll just keep it in my personal patch set.

This comment has been minimized.

Copy link
@jenshannoschwalm

jenshannoschwalm Jun 16, 2024

Sorry i didn't see you comment earlier. I also think the visualizing currently is somewhat annoying so i would be glad if you do some pr.

Or do i misunderstand you here? Do you want me to test and do a pr with your code? You don't want to PR yourself? For sure i wouldn't oppose a better solution ...

This comment has been minimized.

Copy link
@dterrahe

dterrahe Jun 16, 2024

Author Owner

Feel free to do a PR, if you are happy after testing.

Fixed picker initialisation (by merging _backtransform_box with dt_color_picker_backtransform_box).

This comment has been minimized.

Copy link
@jenshannoschwalm

jenshannoschwalm Jun 16, 2024

I did test and it's really good.

  1. Any reason you wouldn't want to do the PR?
  2. I also saw your great OMP work presented by ralf
  3. I just had a short overview on things you have in your local stuff, there is much more interesting work :-)

This comment has been minimized.

Copy link
@dterrahe

dterrahe Jun 16, 2024

Author Owner
  1. A PR implies that the submitter

    • states it is ready for public consumption (has been tested sufficiently, multi-platform etc)
    • is willing to engage with the "community" to provide commentary/explanations and make reasonable adjustments to get it merged (coding style, general applicability/configurability, common aesthetics/style etc)
    • will provide support/bug fixes for any regressions

    I'm not willing to imply any of these.

  2. @ralfbrown completed it and did all of the above, so it is as much his work as mine. (And if you find a bug, it is only his ;-) )

  3. Feel free to take any of it. Some of it has no general use at all and some would lead to discussions I don't want to be copied into. No need to @ me and if you make any changes you don't need to preserve my name as the committer.
    EDIT: Some of the earlier ones have already been previously proposed for upstream inclusion by myself and/or Pascal and rejected, so you wouldn't want to create PRs for those again.

This comment has been minimized.

Copy link
@jenshannoschwalm

jenshannoschwalm Jun 17, 2024

Well, for this commit i would certainly prefer to keep you as the author as you did the work for something i should have done - didn't do so because i didn't find the right solution .

This comment has been minimized.

Copy link
@ralfbrown

ralfbrown Jun 17, 2024

I did the same for the OpenMP refactor. Using the commandline git, add --author=dterrahe when committing to retain authorship. If you're using a GUI wrapper, it will presumably have an equivalent option.

This comment has been minimized.

Copy link
@jenshannoschwalm

jenshannoschwalm Jun 17, 2024

i used the command line :-)

dt_pickerbox_t fbox;
for(int i = 0; i < 8; i += 2)
{
fbox[i] = wd * (isbox ? sample->box[i ] : sample->point[0]);
fbox[i + 1] = ht * (isbox ? sample->box[i + 1] : sample->point[1]);
}

dt_dev_distort_transform_plus
(dev, dev->preview_pipe, module->iop_order,
Expand Down
3 changes: 2 additions & 1 deletion src/common/color_picker.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ void dt_color_picker_backtransform_box(dt_develop_t *dev,
void dt_color_picker_transform_box(dt_develop_t *dev,
const int num,
const float *in,
float *out);
float *out,
gboolean scale);
gboolean dt_color_picker_box(dt_iop_module_t *module,
const dt_iop_roi_t *roi,
const dt_colorpicker_sample_t *const sample,
Expand Down
1 change: 1 addition & 0 deletions src/common/darktable.h
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ struct dt_colorspaces_t;
struct dt_l10n_t;

typedef float dt_boundingbox_t[4]; //(x,y) of upperleft, then (x,y) of lowerright
typedef float dt_pickerbox_t[8];

typedef enum dt_debug_thread_t
{
Expand Down
2 changes: 1 addition & 1 deletion src/gui/color_picker_proxy.c
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ static gboolean _record_point_area(dt_iop_color_picker_t *self)
}
}
else if(sample->size == DT_LIB_COLORPICKER_SIZE_BOX)
for(int k = 0; k < 4; k++)
for(int k = 0; k < 8; k++)
{
if(self->pick_box[k] != sample->box[k])
{
Expand Down
2 changes: 1 addition & 1 deletion src/gui/color_picker_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ typedef struct dt_iop_color_picker_t
// the picker request for the primary picker when this picker is
// activated, and will remember the most recent picker position
float pick_pos[2];
dt_boundingbox_t pick_box;
dt_pickerbox_t pick_box;
gboolean changed;
} dt_iop_color_picker_t;

Expand Down
4 changes: 2 additions & 2 deletions src/libs/colorpicker.c
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,12 @@ static void _update_samples_output(dt_lib_module_t *self)
/* set sample area proxy impl */

static void _set_sample_box_area(dt_lib_module_t *self,
const dt_boundingbox_t box)
const dt_pickerbox_t box)
{
dt_lib_colorpicker_t *data = self->data;

// primary sample always follows/represents current picker
for(int k = 0; k < 4; k++)
for(int k = 0; k < 8; k++)
data->primary_sample.box[k] = box[k];

_update_size(self, DT_LIB_COLORPICKER_SIZE_BOX);
Expand Down
2 changes: 1 addition & 1 deletion src/libs/colorpicker.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ typedef struct dt_colorpicker_sample_t
// whether from colorpicker lib or an iop. They are used for showing
// the sample in the center view, and sampling in the pixelpipe.
float point[2];
dt_boundingbox_t box;
dt_pickerbox_t box;
dt_lib_colorpicker_size_t size;
gboolean denoise;
gboolean pick_output;
Expand Down
2 changes: 1 addition & 1 deletion src/libs/lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -1466,7 +1466,7 @@ gchar *dt_lib_get_localized_name(const gchar *plugin_name)
}

void dt_lib_colorpicker_set_box_area(dt_lib_t *lib,
const dt_boundingbox_t box)
const dt_pickerbox_t box)
{
if(!lib->proxy.colorpicker.module || !lib->proxy.colorpicker.set_sample_box_area) return;
lib->proxy.colorpicker.set_sample_box_area(lib->proxy.colorpicker.module, box);
Expand Down
2 changes: 1 addition & 1 deletion src/libs/lib.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ gboolean dt_lib_presets_can_autoapply(dt_lib_module_t *mod);

/** set the colorpicker area selection tool and size, box[k] 0.0 - 1.0 */
void dt_lib_colorpicker_set_box_area(dt_lib_t *lib,
const dt_boundingbox_t box);
const dt_pickerbox_t box);

/** set the colorpicker point selection tool and position */
void dt_lib_colorpicker_set_point(dt_lib_t *lib,
Expand Down
79 changes: 37 additions & 42 deletions src/views/darkroom.c
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ static void _darkroom_pickers_draw(dt_view_t *self,
if(sample->size == DT_LIB_COLORPICKER_SIZE_BOX)
{
dt_boundingbox_t fbox;
dt_color_picker_transform_box(dev, 2, sample->box, fbox);
dt_color_picker_transform_box(dev, 2, sample->box, fbox, FALSE);
x = fbox[0];
y = fbox[1];
double w = fbox[2];
Expand Down Expand Up @@ -311,7 +311,7 @@ static void _darkroom_pickers_draw(dt_view_t *self,
but this gets particularly tricky to do with iop pickers with transformations after them in the pipeline
*/
dt_boundingbox_t fbox;
dt_color_picker_transform_box(dev, 1, sample->point, fbox);
dt_color_picker_transform_box(dev, 1, sample->point, fbox, FALSE);
x = fbox[0];
y = fbox[1];
cairo_user_to_device(cri, &x, &y);
Expand Down Expand Up @@ -2999,20 +2999,21 @@ void mouse_moved(dt_view_t *self, double x, double y, double pressure, int which
float delta_y = 1.0f / (float) dev->full.pipe->processed_height;

dt_boundingbox_t pbox = { zoom_x, zoom_y };
dt_boundingbox_t zb;
dt_color_picker_backtransform_box(dev, 1, pbox, zb);

if(sample->size == DT_LIB_COLORPICKER_SIZE_BOX)
{
sample->box[0] = MAX(0.0, MIN(sample->point[0], zb[0]) - delta_x);
sample->box[1] = MAX(0.0, MIN(sample->point[1], zb[1]) - delta_y);
sample->box[2] = MIN(1.0, MAX(sample->point[0], zb[0]) + delta_x);
sample->box[3] = MIN(1.0, MAX(sample->point[1], zb[1]) + delta_y);
float corner[2];
dt_color_picker_transform_box(dev, 1, sample->point, corner, TRUE);

pbox[0] = MAX(0.0, MIN(corner[0], zoom_x) - delta_x);
pbox[1] = MAX(0.0, MIN(corner[1], zoom_y) - delta_y);
pbox[2] = MIN(1.0, MAX(corner[0], zoom_x) + delta_x);
pbox[3] = MIN(1.0, MAX(corner[1], zoom_y) + delta_y);
dt_color_picker_backtransform_box(dev, 2, pbox, sample->box);
}
else if(sample->size == DT_LIB_COLORPICKER_SIZE_POINT)
{
sample->point[0] = zb[0];
sample->point[1] = zb[1];
dt_color_picker_backtransform_box(dev, 1, pbox, sample->point);
}
dev->preview_pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dt_control_queue_redraw_center();
Expand Down Expand Up @@ -3136,44 +3137,34 @@ int button_pressed(dt_view_t *self,
const int procw = dev->preview_pipe->backbuf_width;
const int proch = dev->preview_pipe->backbuf_height;

dt_boundingbox_t pbox = { zoom_x, zoom_y };
dt_boundingbox_t zb;
dt_color_picker_backtransform_box(dev, 1, pbox, zb);

if(which == 1)
{
sample->point[0] = zb[0];
sample->point[1] = zb[1];
sample->point[0] = zoom_x;
sample->point[1] = zoom_y;

if(sample->size == DT_LIB_COLORPICKER_SIZE_BOX)
{
const float eps = 0.1f / MAX(0.1f, zoom_scale);

gboolean on_corner_prev_box = TRUE;
float opposite_x = 0.f, opposite_y = 0.f;

const float dx0 = fabsf(zb[0] - sample->box[0]);
const float dx1 = fabsf(zb[0] - sample->box[2]);
const float dy0 = fabsf(zb[1] - sample->box[1]);
const float dy1 = fabsf(zb[1] - sample->box[3]);
dt_boundingbox_t sbox;
dt_color_picker_transform_box(dev, 2, sample->box, sbox, TRUE);

const gboolean px0 = dx0 < eps && dx0 < dx1;
const gboolean px1 = dx1 < eps && dx0 > dx1;
const gboolean py0 = dy0 < eps && dy0 < dy1;
const gboolean py1 = dy1 < eps && dy0 > dy1;
const float handle_px = 6.0f;
float hx = handle_px / (procw * zoom_scale);
float hy = handle_px / (proch * zoom_scale);

if(px0) opposite_x = sample->box[2];
else if(px1) opposite_x = sample->box[0];
else on_corner_prev_box = FALSE;
const float dx0 = fabsf(zoom_x - sbox[0]);
const float dx1 = fabsf(zoom_x - sbox[2]);
const float dy0 = fabsf(zoom_y - sbox[1]);
const float dy1 = fabsf(zoom_y - sbox[3]);

if(py0) opposite_y = sample->box[3];
else if(py1) opposite_y = sample->box[1];
else on_corner_prev_box = FALSE;
const gboolean px0 = dx0 < hx && dx0 < dx1;
const gboolean px1 = dx1 < hx && dx0 > dx1;
const gboolean py0 = dy0 < hy && dy0 < dy1;
const gboolean py1 = dy1 < hy && dy0 > dy1;

if(on_corner_prev_box)
if((px0 || px1) && (py0 || py1))
{
sample->point[0] = opposite_x;
sample->point[1] = opposite_y;
sample->point[0] = sbox[px0 ? 2 : 0];
sample->point[1] = sbox[py0 ? 3 : 1];
}
else
{
Expand All @@ -3185,6 +3176,7 @@ int button_pressed(dt_view_t *self,
dt_control_change_cursor(GDK_FLEUR);
}

dt_color_picker_backtransform_box(dev, 1, sample->point, sample->point);
dev->preview_pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dt_control_queue_redraw_center();
return 1;
Expand All @@ -3200,11 +3192,13 @@ int button_pressed(dt_view_t *self,
for(GSList *samples = darktable.lib->proxy.colorpicker.live_samples; samples; samples = g_slist_next(samples))
{
dt_colorpicker_sample_t *live_sample = samples->data;
dt_boundingbox_t sbox;
if(live_sample->size == DT_LIB_COLORPICKER_SIZE_BOX
&& (picker->flags & DT_COLOR_PICKER_AREA))
{
if(zb[0] < live_sample->box[0] || zb[0] > live_sample->box[2]
|| zb[1] < live_sample->box[1] || zb[1] > live_sample->box[3])
dt_color_picker_transform_box(dev, 2, live_sample->box, sbox, TRUE);
if(zoom_x < sbox[0] || zoom_x > sbox[2] ||
zoom_y < sbox[1] || zoom_y > sbox[3])
continue;
dt_lib_colorpicker_set_box_area(darktable.lib, live_sample->box);
}
Expand All @@ -3215,7 +3209,8 @@ int button_pressed(dt_view_t *self,
float slop_px = MAX(26.0f, roundf(3.0f * zoom_scale));
const float slop_x = slop_px / (procw * zoom_scale);
const float slop_y = slop_px / (proch * zoom_scale);
if(!feqf(zb[0], live_sample->point[0], slop_x) || !feqf(zb[1], live_sample->point[1], slop_y))
dt_color_picker_transform_box(dev, 1, live_sample->point, sbox, TRUE);
if(!feqf(zoom_x, sbox[0], slop_x) || !feqf(zoom_y, sbox[1], slop_y))
continue;
dt_lib_colorpicker_set_point(darktable.lib, live_sample->point);
}
Expand All @@ -3231,7 +3226,7 @@ int button_pressed(dt_view_t *self,
// default is hardcoded this way
// FIXME: color_pixer_proxy should have an dt_iop_color_picker_clear_area() function for this
dt_boundingbox_t reset = { 0.02f, 0.02f, 0.98f, 0.98f };
dt_boundingbox_t box;
dt_pickerbox_t box;
dt_color_picker_backtransform_box(dev, 2, reset, box);
dt_lib_colorpicker_set_box_area(darktable.lib, box);
dev->preview_pipe->status = DT_DEV_PIXELPIPE_DIRTY;
Expand Down

0 comments on commit 70f42cd

Please sign in to comment.