Page MenuHome

Add eyedropper to colorramp node.
ClosedPublic

Authored by LazyDodo (LazyDodo) on Oct 19 2017, 3:12 AM.

Details

Summary

This diff adds the eye dropper tool to the color ramp node and allows you to sample a color range from any ui element or bitmap.

why it's useful : https://www.youtube.com/watch?v=IY_uwlOGokg

beyond that, very little to say about this, I made sure check_style_c.py didn't complain and tested all corner cases such as single clicking, exceeding the maximum number of stops (32) and canceling.

I kinda winged the reviewers, if someone else is more suited, please change.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D2886
Build Status
Buildable 1030
Build 1030: arc lint + arc unit

Event Timeline

Nice! This can come in very handy.

Is the number of stops arbitrary?

Suggestion: Since the color picker is already a modal tool, why not keep the tool active after the first click, allowing the user to pick more colors and then exit with Escape/Right Click. This way is more intuitive and allows more flexibility to create a ramp of colors even picking from completely different parts of Blender.

Is the number of stops arbitrary?

Yes, no, half. you can as sample as many points as you want, but due to a limitation of 32 stops in the color ramp node, if you go over 32, it'll pick 32 points evenly spaced out along your path.

Suggestion: Since the color picker is already a modal tool, why not keep the tool active after the first click, allowing the user to pick more colors and then exit with Escape/Right Click. This way is more intuitive and allows more flexibility to create a ramp of colors even picking from completely different parts of Blender.

I build it in the way i wanted to use it, but your idea sounds useful too, i'll look into it

Updated the diff with:

  • Live update while you sample.
  • Undo any changes when canceling the operator.
  • Hotkey E over the color ramp, starts the 'dragging sampler'
  • Hotkey Alt-E over the color ramp , starts the 'point sampler'
    • left mouse click samples a point
    • back space removes the last point
    • escape cancels
    • enter confirms

video of the pointsampling mode

https://www.youtube.com/watch?v=4GLfHZHL97I&feature=youtu.be

Konstantin (Ko) rescinded a token.
Konstantin (Ko) awarded a token.

This is fantastic. Thanks so much!

I don't know how's the procedure regarding patches like this (that do not touch many areas) nowadays with 2.8 going on and all that. Maybe @Campbell Barton (campbellbarton) knows?

Looks fine to me (there is possible improvement regarding *3, which @Campbell Barton (campbellbarton) is currently typing in). Here is a patch to fix strict compiler warnings:

1diff --git a/source/blender/editors/interface/interface_eyedropper.c b/source/blender/editors/interface/interface_eyedropper.c
2index 5e3ab20615c..9a062f0fd01 100644
3--- a/source/blender/editors/interface/interface_eyedropper.c
4+++ b/source/blender/editors/interface/interface_eyedropper.c
5@@ -595,7 +595,7 @@ static bool eyedropper_colorband_sample_callback(int mx, int my, void *userdata)
6 return true;
7 }
8
9-static void eyedropper_colorband_sample(bContext *C, EyedropperColorband *eye, int mx, int my)
10+static void eyedropper_colorband_sample(bContext *UNUSED(C), EyedropperColorband *eye, int mx, int my)
11 {
12 /* since the mouse tends to move rather rapidly we use BLI_bitmap_draw_2d_line_v2v2i to */
13 /* interpolate between the reported coordinates */
14@@ -619,9 +619,9 @@ static void eyedropper_colorband_exit(bContext *C, wmOperator *op)
15 static void eyedropper_colorband_apply_noresample(bContext *C, wmOperator *op)
16 {
17 EyedropperColorband *eye = (EyedropperColorband *)op->customdata;
18- int stops = min(32, eye->color_buffer_index / 3);
19+ int stops = min_ii(32, eye->color_buffer_index / 3);
20 if (stops) {
21- float step = 1.0f / max(stops - 1, 1);
22+ float step = 1.0f / max_ff(stops - 1, 1);
23 eye->band->tot = stops;
24 for (int i = 0; i < stops; i++) {
25 eye->band->data[i].r = eye->color_buffer[(i * 3)];

This revision is now accepted and ready to land.Dec 6 2017, 4:01 PM

Generally fine, noted minor issues.

source/blender/editors/interface/interface_eyedropper.c
540

Better use float (*color_buffer)[3] - avoid annoying multiplications all over.

622

All uses of 32 should be MAXCOLORBAND

627–629

can use copy_v3_v3().

  • Applied patch from @Sergey Sharybin (sergey)
  • Removed all the * 3 math
  • used copy_v3_v3 where possible.
  • removed hardcoded 32 with MAXCOLORBAND
  • Addressed one last side case where there's no sampled colors , it now stubs one black stop in that case.
source/blender/editors/interface/interface_eyedropper.c
102

Would rather keep the return value, even if its not used in this case, its a convention for modal keymap functions.

529

defines should be uppercase, also think doubling each re-alloc may be good-enough here, it's not likely to be a noticeable bottleneck.

643–665

This could be re-written to pick color points better, but think it's OK as-is.

Similar to other simplification code in Blender (decimate for eg)

  • use a min-heap that stores cost of removing each color based on it's difference with the interpolated value.
  • remove the lowest cost colors until the number is 32, maybe continue removing until the error introduced is perceptible (over some threshold).

Think this will give noticeably better outcome.

To avoid writing custom code for this, it's possible to use curve-fit-nd library the do the simplification for us, though it would need an option to fit without adjusting handle length (simple to implement).

646

interface_eyedropper.c:646:57: error: "/*" within comment [-Werror=comment]

source/blender/editors/interface/interface_eyedropper.c
544

Prefer CamelCaps for types, not members.

587

This is a bug waiting to happen. Rather modify eyedropper_color_sample_fl not to take the second argument instead of passing NULL.

Campbell Barton (campbellbarton) retitled this revision from Add eyedropper to colorramp node. to Add eyedropper to colorramp node..
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
  • Move re-sampling function into BKE_colorband.
  • Set alpha value (wasn't being set at all).
  • Split keymap function in two, so each can return the resulting keymap.
  • Remove context from struct.
  • Name vars in keeping with existing code.
  • Remove unused struct member
Campbell Barton (campbellbarton) requested changes to this revision.EditedDec 7 2017, 6:54 AM

Went over this patch in more detail:

  • Alt-E for picking individual colors feels very obscure, perhaps this could be supported differently?

    Eg - initial click event to sample points, click drag to sample line.
  • At first I didn't think it was so important to clean-up redundant colors when sampling, however many stops can be added all the same color in quite common cases.

    Think it would be good to at least remove colors which have no visible difference, eg: the RGB difference is <= 1 / 255.

    For the time to add basic color array simplification, think it may be not much more effort to add a version that discards the colors that contribute least. - If not, can be done separately.
  • When dragging a line, it would be nice to draw the line too, see IMAGE_OT_sample_line where it's being done already (more of a TODO but think its handy)
  • Currently when pressing E near the color band, it can crash:
==518==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000003e0 (pc 0x55ad13195960 bp 0x7ffc6d3cfa60 sp 0x7ffc6d3cfa20 T0)
==518==The signal is caused by a READ memory access.
==518==Hint: address points to the zero page.
    #0 0x55ad1319595f in eyedropper_colorband_init /src/blender/source/blender/editors/interface/interface_eyedropper.c:580
    #1 0x55ad13198110 in eyedropper_colorband_invoke /src/blender/source/blender/editors/interface/interface_eyedropper.c:740
    #2 0x55ad1230d521 in wm_operator_invoke /src/blender/source/blender/windowmanager/intern/wm_event_system.c:1143
This revision now requires changes to proceed.Dec 7 2017, 6:54 AM

Removing redundant colors is definitely cleaning up the color ramp nicely, however removing the least significant color works well for a smallish set, but when things get larger it's just behaving awkwardly. this especially shows when you're sampling a checkerboard like the uv grid.

/* I'm somehow unable to make a diff with just these 2 functions, this one goes in source\blender\blenlib\intern\math_vector_inline.c */
MINLINE float len_squared_v4v4(const float a[4], const float b[4])
{
	float d[4];

	sub_v4_v4v4(d, b, a);
	return dot_v4v4(d, d);
}

/* this one in colorband.c */
void BKE_colorband_init_from_table_rgba(
        ColorBand *coba,
        const float (*array)[4], const int array_len)
{
	if (!array_len) {
		/* coba is empty, set 1 black stop */
		zero_v3(&coba->data[0].r);
		coba->data[0].a = 1.0f;
		coba->cur = 0;
		coba->tot = 1;
                return;
	}

	float *stops = MEM_mallocN(sizeof(float)*array_len, "Colorband_stop_table");
	bool *active = MEM_mallocN(sizeof(bool)*array_len, "colorband_active_table");

	/* Step 1, assign every color it's stop so it won't change the sampling distribution */
	/* later on when we remove colors */
	float step = 1.0f / max_ff(array_len - 1, 1);
	for (int i = 0; i < array_len; i++) {
		stops[i] = i * step;
		active[i] = true;
	}

	int current_array_len = 1; /* first color is always active */
	
	/* step 2,even if we have less than MAXCOLORBAND stops de-activate any color that */
	/* has the same color as *both* it's neighbors */
	for (int i = 1; i < array_len-1; i++) {
		float distance_left = len_squared_v4v4(array[i], array[i - 1]);
		float distance_right = len_squared_v4v4(array[i], array[i + 1]);
		active[i] = distance_left > 0 || distance_right > 0;
		if (active[i])
			current_array_len++;
	}

	/*  step 3 loop though all stops and eliminate the one closest to its neighbors until */
	/*  we have reached the number of stops needed */
	while (current_array_len >= MAXCOLORBAND) {
		float min_distance = FLT_MAX;
		int current_elimination_target = -1;
		for (int i = 1; i < array_len - 1; i++) {
			if (active[i]) {
				int left_neighbor, right_neighbor;

				/* this can surely be optimzed a little further, but given the array size */
				/* will be rather small most of the time and this code will run rather */
				/* infrequently guess this will have to do for now */

				for (left_neighbor = i - 1; left_neighbor >= 0; left_neighbor--) {
					if (active[left_neighbor])
						break;
				}
				
				for (right_neighbor = i + 1; right_neighbor < array_len; right_neighbor++) {
					if (active[right_neighbor])
						break;
				}
				BLI_assert(active[left_neighbor]);
				BLI_assert(active[right_neighbor]);
				float distance_left = len_squared_v4v4(array[i], array[left_neighbor]);
				float distance_right = len_squared_v4v4(array[i], array[right_neighbor]);
				float total_distance = distance_left + distance_right;
				if (total_distance < min_distance) {
					min_distance = total_distance;
					current_elimination_target = i;
				}
			}
		}
		BLI_assert(current_elimination_target != -1);
		active[current_elimination_target] = false;
		current_array_len--;
	}

	/* step 3 , we have guaranteed less than MAXCOLORBAND stops now, apply the colorband */
	int current_stop = 0;
	coba->tot = current_array_len;
	for (int i = 0; i < array_len; i++) {
		if (active[i]) {
			copy_v4_v4(&coba->data[current_stop].r, array[i]);
			coba->data[current_stop].pos = stops[i];
			coba->data[current_stop].cur = current_stop;
			current_stop++;
		}
	}
	coba->tot = current_stop;
	coba->cur = 0;

	MEM_freeN(stops);
	MEM_freeN(active);
}

maybe it's just better to make a 1xN bitmap and have the bitmap scaling functions work it out for us?

The bitmap scaling function seems like a good idea and might be a more efficient way to reduce the steps to a minimum. Scale down to 32 pixels and then run through each pixel in order whilst ignoring any matches.

@LazyDodo (LazyDodo)

Instead of comparing the distance to each adjacent point, the interpolated point should be calculated, then measure the distance to that - otherwise points will be kept which are matching the interpolated values, where detail could be better used elsewhere.

A disadvantage to down-sampling is any detail below 1/32nd will be lost. In practice it might be good-enough though.

  • De-duplicate color for the simple case
  • Fix crash when no button is found

@Campbell Barton (campbellbarton) ah rats, I fixed most of those during the day today.

The tip about the interpolated value was good, but it really needed lo look along the whole area between the points, this method works *REALLY* well, however.

  1. The fudge factor shouldn't be there, but it's *SO* much better with.
  1. it's neglecting alpha
void BKE_colorband_init_from_table_rgba(
        ColorBand *coba,
        const float (*array)[4], const int array_len)
{
	if (!array_len) {
		/* coba is empty, set 1 black stop */
		zero_v3(&coba->data[0].r);
		coba->data[0].a = 1.0f;
		coba->cur = 0;
		coba->tot = 1;
	}

	float *stops = MEM_mallocN(sizeof(float)*array_len, "Colorband_stop_table");
	bool *active = MEM_mallocN(sizeof(bool)*array_len, "colorband_active_table");

	/* Step 1, assign every color it's stop so it won't change the sampling distribution */
	/* later on when we remove colors */
	float step = 1.0f / max_ff(array_len - 1, 1);
	for (int i = 0; i < array_len; i++) {
		stops[i] = i * step;
		active[i] = true;
	}

	int current_array_len = 1; /* first color is always active */
	
	/* step 2,even if we have less than MAXCOLORBAND stops de-activate any color that */
	/* has the same color as *both* it's neighbors */
	for (int i = 1; i < array_len-1; i++) {
		float distance_left = len_squared_v4v4(array[i], array[i - 1]);
		float distance_right = len_squared_v4v4(array[i], array[i + 1]);
		active[i] = distance_left > 0 || distance_right > 0;
		if (active[i])
			current_array_len++;
	}

	/*  step 3 loop though all stops and eliminate the one closest to its neighbors until */
	/*  we have reached the number of stops needed */
	while (current_array_len >= MAXCOLORBAND)
	{
		float min_error = FLT_MAX;
		int current_elimination_target = -1;
		for (int i = 1; i < array_len - 1; i++) {
			if (active[i]) {
				int left_neighbor, right_neighbor;

				/* this can surely be optimzed a little further, but given the array size */
				/* will be rather small most of the time and this code will run rather */
				/* infrequently guess this will have to do for now */

				for (left_neighbor = i - 1; left_neighbor >= 0; left_neighbor--) {
					if (active[left_neighbor])
						break;
				}
				
				for (right_neighbor = i + 1; right_neighbor < array_len; right_neighbor++) {
					if (active[right_neighbor])
						break;
				}
				BLI_assert(active[left_neighbor]);
				BLI_assert(active[right_neighbor]);
				
				/* if we were to remove this stop, the error introduced would be the size of the tringle in 3d space between all 3 points involved */
				float error = area_tri_v3(array[i], array[left_neighbor], array[right_neighbor]);

				/* in theory this should have done the trick, but somehow if we fudge it like this, it visually looks better */
				error = fabs(error - len_v3v3(array[left_neighbor], array[right_neighbor]));

				/* adjust the error for distance between stops, ie stops closer together have a smaller error */
				error = error * (stops[right_neighbor] - stops[left_neighbor]);

				if (error < min_error) {
					min_error = error;
					current_elimination_target = i;
				}
			}
		}
		BLI_assert(current_elimination_target != -1);
		active[current_elimination_target] = false;
		current_array_len--;
	}

	/* step 3 , we have guaranteed less than MAXCOLORBAND stops now, apply the colorband */
	int current_stop = 0;
	coba->tot = current_array_len;
	for (int i = 0; i < array_len; i++) {
		if (active[i]) {
			copy_v4_v4(&coba->data[current_stop].r, array[i]);
			coba->data[current_stop].pos = stops[i];
			coba->data[current_stop].cur = current_stop;
			current_stop++;
		}
	}
	coba->tot = current_stop;
	coba->cur = 0;

	MEM_freeN(stops);
	MEM_freeN(active);
}

Write color-band simplification function using a min-heap

Committed to master: rB7ae4c3a01923cccfa372072b880507c58557f45a

Wrote a re-sampling function that removes elements weighted by their combine RGBA area: rBf7a1a1a700d76a253afdbcb04decb0099be34cc8

Mostly it works well in my tests, although noise in the image can show up in the color-ramp - not sure how picky users will be about this?
if it's a problem a smoothing pass give nicer results for photo's that contain some noise.

This revision is now accepted and ready to land.Dec 12 2017, 3:26 AM

@LazyDodo (LazyDodo), ack, missed your message - committed a function that works similar to yours, just that it treats each channel as a 2D area and combines, so it works for RGBA,
Even though it needs to run 4x times, this has an advantage over 3D area since it avoids a sqrt. Also uses a min-heap to avoid searching for the best item to remove each time.

There is still some room for improvement in this patch IMHO.

  • Drawing the mouse stroke thats being used as a sample.
  • Making picking points more discoverable than Alt-E.
  • Possibly smoothing noisy input as mentioned already - although this is more of a 'nice-to-have'.