ThingPulse/minigrafx

MiniGrafx::drawRect() calculates borders wrongly

spiro-trikaliotis opened this issue · 1 comments

In

void MiniGrafx::drawRect(int16_t x, int16_t y, int16_t width, int16_t height) {
  drawHorizontalLine(x, y, width);
  drawVerticalLine(x, y, height);
  drawVerticalLine(x + width, y, height);
  drawHorizontalLine(x, y + height, width);
}

(minigrafx/MiniGrafx.cpp, lines 203ff), the two last lines are wrong.

This results in a rect that has the lower right corner missing one pixel, AND the right and top line being one pixel too much to the right or to the bottom. Also, drawing a rect with fillRect() and drawing the same coordinates with drawRect show that the right line is one pixel too much at the right, and the bottom line being one pixel too much to the bottom.

Why?

The first drawHorizontalLine() draws a line at y coord y, and x in the range from x to x + width - 1.
The next drawVerticalLine() draws a line at x coord x, and y in the range from y to y + height - 1.

Now, the next line drawVerticalLine() adds a vertical line at x position x + width. However, the first line stopped at x + width - 1. Thus, it adds another pixel to the right of what the first drawHorizontalLine() drawed, with y going from y to y + height - 1.

Likewise, the next line drawHorizontalLine() adds a horizontal line at y position y + height, but the second draw stopped at y + height - 1. Thus, it adds another bottom line, with x going from x to x + width - 1.

As becomes obvious now, the position (x + width, y + height) is never painted and is missing in the rect.

Of course, one can argue of the right and bottom lines are too much to the right and bottom, or if just the one pixel at the lower right corner is missing. I tend to the first choice, as I expect a drawRect() and a fillRect() with the same coordinates to also fill the same area of the screen. Furthermore, with the second variant, a drawRect at (0,0) with height 320 (the maximum) and width 240 (the minimum) would result in two lines (right and bottom) being off-screen, and, thus, not visible.

So, my proposal to fix is to change the function to:

void MiniGrafx::drawRect(int16_t x, int16_t y, int16_t width, int16_t height) {
  drawHorizontalLine(x, y, width);
  drawVerticalLine(x, y, height);
  drawVerticalLine(x + width - 1, y, height);
  drawHorizontalLine(x, y + height - 1, width);
}

EDIT: If you agree, I can submit a PR.

The first PR (c4aace7) was wrong, as it included PR #59. Created a new one in #60.