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
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
|
Detailscb-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
Thursday, 27 September 2007, 15:46 GMT
Reason for closing: Accepted
Additional comments about closing: Committed
http://www.rockbox.org/irc/reader.pl?date=20070730#09:26:31 and http://www.pastebin.ca/638960
and fixes a typo in grad-sync-fg.patch that made the line selector change colors when it scrolled
* 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).
* 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.
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.
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)
Awesome patch though still. Thumbs up.
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...
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).
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 ?
maybe the gradient code needs to use a different packing macro for it?
* 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 ?
+ 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.
Attached is the patch including dionoea's ideas. I think it's ready to be committed... any more comments before I do ?
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)