Rockbox

Tasklist

FS#7808 - color bar/gradient patch for color targets

Attached to Project: Rockbox
Opened by Ken Fazzone (kfazz) - Friday, 21 September 2007, 14:51 GMT
Last edited by Nicolas Pennequin (nicolas_p) - Thursday, 27 September 2007, 15:46 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System Sansa e200
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 5
Private No

Details

cb-synch.patch adds settable colors for line selector color and selected text color
this isn't mine, found it via google on japanese pastebin

grad-sync.patch changes the selection bar color into a gradient from selection color to black

these patches should work for all color targets, but ive only tested on e200

forum thread is here: http://forums.rockbox.org/index.php?topic=12783.0
This task depends upon

Closed by  Nicolas Pennequin (nicolas_p)
Thursday, 27 September 2007, 15:46 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed
Comment by Ken Fazzone (kfazz) - Friday, 21 September 2007, 14:53 GMT
files didn't show up for some reason
Comment by Jonas Häggqvist (rasher) - Friday, 21 September 2007, 17:00 GMT
As low_light pointed out on IRC, this was done by midgey:

http://www.rockbox.org/irc/reader.pl?date=20070730#09:26:31 and http://www.pastebin.ca/638960
Comment by Ken Fazzone (kfazz) - Sunday, 23 September 2007, 05:29 GMT
this version does the drawing of the gradient in the foreground, so if a background image is set, the gradient doesn't get covered up.
Comment by Ken Fazzone (kfazz) - Sunday, 23 September 2007, 08:11 GMT
this fixes the line selector setting not saving
and fixes a typo in grad-sync-fg.patch that made the line selector change colors when it scrolled
Comment by Nicolas Pennequin (nicolas_p) - Monday, 24 September 2007, 11:52 GMT
kfazz: I'm not sure I understood correctly... cb-synch.patch is by midgey and grad-sync.patch is by you, am I right ?
Comment by Ken Fazzone (kfazz) - Monday, 24 September 2007, 14:48 GMT
nico_p, yes, that's how it is.
Comment by Max Kelley (maxkelley) - Monday, 24 September 2007, 20:04 GMT
I'd vote for this up.. I like it. Commit please! :)
Comment by Nicolas Pennequin (nicolas_p) - Monday, 24 September 2007, 20:16 GMT
I've made a few changes to this patch (I might have forgotten some):
* Fix: The selected area was one pixel too tall.
* Added a solid colour setting (STYLE_COLORBAR).
* Renamed STYLE_HIGHLIGHT to STYLE_GRADIENT.
* Moved the line selector colour settings so that they are right under the type setting.
* Code policing and cleaning up.

The attached files are the patch and two screendumps showing the gradient and solid color modes.

What do you think? I'm hoping to commit this soonish once it gets enough review (assuming it doesn't get vetoed, of course).
Comment by Robert Kukla (roolku) - Monday, 24 September 2007, 21:51 GMT
Gets my vote. I think it looks great
Comment by Nicolas Pennequin (nicolas_p) - Monday, 24 September 2007, 23:26 GMT
Some fixes:
* The line wasn't displayed correctly (it's not supposed to cover the icon, it should only cover the text. See the screendumps).
* The line didn't appear on remote screens. This is fixed.
* Made the recording screen use the setting.
Comment by Robert Kukla (roolku) - Monday, 24 September 2007, 23:30 GMT
I preferred it when it stretched over the full width (i.e. "covered the icon").
Comment by Frank Grisafi (JobVanDam) - Tuesday, 25 September 2007, 01:31 GMT
Rob, no one is stopping from using a older version of the patch if you like the covering of the icon.

Nic, you provide all the patches the young kids really like. You had a hand in this patch and the mighty Slider Progressbar. Keep it up.
Can't wait to use this patch, makes me feel giddy just thinking about the possibilities.
Comment by Antoine Cellerier (dionoea) - Tuesday, 25 September 2007, 08:10 GMT
Looks nice. I was just wondering if this had any (noticeable) impact on performance when drawing the screen (Since, as far as I understand, you compute the gradient values every time using a bunch of divisions).

Another issue is that you assume that h_{r,g,b} - height * ( h_{r,g,b} / height ) = 0 in the drawing loop. Due to integer rounding issues that won't always be true. (So the last line might end up being not quite black ...). I'm not sure if that matters though since most dark colors look black anyway (it would if both sides of the gradient colors were configurable)
Comment by Frank Grisafi (JobVanDam) - Tuesday, 25 September 2007, 15:26 GMT
Just been experimenting with the patch... the bottom color of the gradient, the black, is it configurable? Can I change it?

Awesome patch though still. Thumbs up.
Comment by Antoine Cellerier (dionoea) - Tuesday, 25 September 2007, 15:28 GMT
No, the bottom color is not configurable. (It uses the top color and does a linear interpolation of the red, green and blue components to 0, 0 and 0 (which is black))
Comment by Nicolas Pennequin (nicolas_p) - Tuesday, 25 September 2007, 15:54 GMT
I think I'll make it configurable though...
dionoea: how can I solve the rounding problems ? Also, do you think there is a better way to draw the gradient that calculate it each time ? I haven't seen any noticeable impact on drawing speed, but the only target I can test with for now is a gigabeat...
Comment by Antoine Cellerier (dionoea) - Tuesday, 25 September 2007, 16:22 GMT
Hum, looks like I was kind of mistaken about the rounding error, since it's not doing a linear interpolation but something exponential. Which also means that the last row's color isn't black but some shade of gray rougly equal to ( exp(-1)*R, exp(-1)*G, exp(-1)*B ) (if you discard any integer rounding issues, and consider that the height is big enough (error is 5% for height=10, 3% for height=20, ...)).

About the optimisation thing, other than caching the values I don't see what could be done (you'd only need to reset the cache every time the gradient color or font changes). I'm not sure if this is worth it though.
Edit: Well you could also precompute (1-1/height) and do fixed point computing (so you'd only have multiplications and shifts instead of the division).
Comment by Nicolas Pennequin (nicolas_p) - Tuesday, 25 September 2007, 21:03 GMT
dionoea: Maybe linear interpolation would be better if I want to make the second colour configurable ? I'm not quite sure what the code to calculate each step of the gradient would be in that case (I didn't write the original gradient code and haven't given it much thought yet...)

About the optimisation, my reasoning was the same. I also thought of saving the gradient as a bitmap in memory, so that we'd only need to draw that, but I think that can be done later unless performance is a real problem (I'd like to make using a custom bitmap another possibility, so when I implement that, saving the gradient should be quite easy).

I didn't get your comment about 1-1/height... Maybe it's also because I'm clueless about integer arithmetic.
EDIT: actually I think I get it, but it won't apply if I move to linear interpolation, will it ?
Comment by Antoine Cellerier (dionoea) - Wednesday, 26 September 2007, 06:55 GMT
Making the second color configurable with the current exponential gradient indeed seems tougher than with a "stupid" linear interpolation between two colors. I'm not sure if it'd look as nice though (it's worth a try). (You could use some fixed point arithmetic for the linear gradient too to reduce the number of divisions needed, but as you said optimisation isn't really needed if performance is not an issue)
Comment by Tri Nguyen (tri170391) - Wednesday, 26 September 2007, 13:13 GMT
The gradient doesn't show good in ipod nano's build (both sim and real build) anyone know what's wrong?
Comment by Ken Fazzone (kfazz) - Wednesday, 26 September 2007, 14:45 GMT
just a random guess, but does the nano lcd use a different color format?
maybe the gradient code needs to use a different packing macro for it?
Comment by Antoine Cellerier (dionoea) - Wednesday, 26 September 2007, 15:34 GMT
The code should use LCD_RGBPACK() instead of _RGBPACK() which is for 24bit RGB packing. Attached patch fixes the bug.
Comment by Antoine Cellerier (dionoea) - Wednesday, 26 September 2007, 15:41 GMT
Woops, the unpack should also be changed (this fixes the gradient for non white/gray colors). Here it comes:
Comment by Nicolas Pennequin (nicolas_p) - Wednesday, 26 September 2007, 16:55 GMT
Here is an updated patch

* Make the second colour configurable and do linear interpolation between the two colours.
* Make the lang strings specific to colour screen targets.
* Include dionoea's macro fixes.

Rounding errors cause the actual end colour of the gradient to be slightly different from the one the user chose. Maybe it could be solved by redoing the division on each step of the for loop, but that would be less efficient than doing only one division. Another option I prefer would be to use the user selected colour to draw the last line of the gradient... Any better ideas ?
Comment by Antoine Cellerier (dionoea) - Thursday, 27 September 2007, 07:03 GMT
You could try something like this to prevent too many rounding errors accumulating:
+ int h_r = RGB_UNPACK_RED(lss_pattern)<<16;
+ int h_b = RGB_UNPACK_BLUE(lss_pattern)<<16;
+ int h_g = RGB_UNPACK_GREEN(lss_pattern)<<16;
+ int count = 0;
+ int rstep = (h_r - ((signed)RGB_UNPACK_RED(lse_pattern))<<16) / h;
+ int gstep = (h_g - ((signed)RGB_UNPACK_GREEN(lse_pattern))<<16) / h;
+ int bstep = (h_b - ((signed)RGB_UNPACK_BLUE(lse_pattern))<<16) / h;
+ for(count = 0; count < h; count++) {
+ lcd_hline(xpos, LCD_WIDTH, ypos + count);
+ h_r -= rstep;
+ h_g -= gstep;
+ h_b -= bstep;
+ fg_pattern = LCD_RGBPACK(h_r>>16, h_g>>16, h_b>>16);
+ }
This is of course untested (but it should prevent accumulating rounding errors bigger than 1/(1<<16) on each step. So the overall error on the last line would be something like height/(1<<16) which seems small enough)


Btw, you only need to declare (and initialize) the h_{r,g,b} and count variables when drawing the gradient.
Comment by Nicolas Pennequin (nicolas_p) - Thursday, 27 September 2007, 11:32 GMT
Thanks for the suggestion ! It seems to display better colours now.
Attached is the patch including dionoea's ideas. I think it's ready to be committed... any more comments before I do ?
Comment by Antoine Cellerier (dionoea) - Thursday, 27 September 2007, 12:05 GMT
Shouldn't we save the original style in s->invert (well ... rename that structure member to s->style I guess) in lcd-16bit.c? That would prevent introducing the s->invert == 1, 2 or 3 convention and allow using the STYLE_{INVERT,COLORBAR,GRADIENT} macro constants instead.

The "Line Selector Start Color" and "Line Selector End Color" configuration item labels can also be a bit confusing if the user chooses a solid color line selector type. (Is it possible to hide/show configuration items in the menus depending on another item's value? I'm not familiar enough with the menu code to know.)

Edit: those color configuration items are also confusing when using one of the two old line selector types, since none of the color options apply (not even the text color)

Loading...