Rockbox

files didn't show up for some reason

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

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.

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

kfazz: I'm not sure I understood correctly... cb-synch.patch is by midgey and grad-sync.patch is by you, am I right ?

nico_p, yes, that's how it is.

I'd vote for this up.. I like it. Commit please! :)

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).

Gets my vote. I think it looks great

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.

I preferred it when it stretched over the full width (i.e. "covered the icon").

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.

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)

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.

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))

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...

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).

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 ?

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)

The gradient doesn't show good in ipod nano's build (both sim and real build) anyone know what's wrong?

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?

The code should use LCD_RGBPACK() instead of _RGBPACK() which is for 24bit RGB packing. Attached patch fixes the bug.

Woops, the unpack should also be changed (this fixes the gradient for non white/gray colors). Here it comes:

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 ?

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.

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 ?

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)