sajattack / st7735-lcd-rs

Rust library for displays using the ST7735 driver
https://docs.rs/st7735-lcd
MIT License
37 stars 28 forks source link

Wrong calculation of rectangle area in draw_rectangle #11

Closed nitroxis closed 4 years ago

nitroxis commented 4 years ago

I've been playing around with the the "Longan Nano" RISC-V board which comes with a 160x80 LCD. There I noticed that rectangles always seem to be a few pixels "short" (i.e. there are pixels missing at the end of a fill).

I believe the calculation of the rectangle area here is wrong: https://github.com/sajattack/st7735-lcd-rs/blob/0d67a318975fddcb175567d5f7ebfabe7a8c2924/src/lib.rs#L289-L291

I think this should be:

let rect_width = shape.bottom_right.x - item.primitive.top_left.x + 1;
let rect_height = shape.bottom_right.y - item.primitive.top_left.y + 1;
let rect_size = rect_width * rect_height;

Because if you e.g. fill a rectangle from (0,0) to (1,1), the size is 2x2 = 4 pixels.

LongHairedHacker commented 4 years ago

I can confirm that there's something off with the rectangles.

My code draws a red rectangle from 0,0 to 10,10 and then puts a green pixel at 10,10. 10x10

Then it draws a red rectangle from 0,0 to 10,20 and then puts a green pixel at 10,20. 10x20 (Sorry for the lack of sharpness, since the last android update my smart phone camera won't focus properly on close stuff.)

The change proposed above makes sense because the coordinates in you rect will go from 0 to 10 (including the 10) along each axis, that's 11 pixels.

We can also do some pixel counting in my 'screenshots': Using the calculations from the code, we would want to fill 100pixels for 10x10. With the +1 variant we'd end with 11*11 = 121. So we should be about one full row of 11 pixels and the another ten pixels short, which perfectly explains the single red pixel in the last row of the rect.

For the 10x20 rect, we get 200 and 11*21 = 231, that's 31 pixels short. So again two full rows of 11 pixels and 9 pixels short, which again perfectly matches the two lonesome pixels in my 'screenshot'.

So as far is can tell @nitroxis is right.