Issues with ILI9341_t3 gradient routines

F

Fenichel

Well-known member
There are two problems with the fillRectHGradient and fillRectVGradient routines, one trivial and one deeper.
In the fillRectHGradient routine, each h in the line
Code: 
dr=(r2-r1)/h; dg=(g2-g1)/h; db=(b2-b1)/h;
should be w. The fillRectHGradient routine was probably written by copying the fillRectVGradient routine, making (almost all) the changes necessary.

Unfortunately, there is a deeper problem with that line. To see the deeper problem, consider (for definiteness) the problem of filling a 100-pixel line (W = 100) with a gradient running from RedLeft = 0 to RedRight = 9. The expected line will have 10 pixels with Red = 0, 10 with Red = 1, and so on. Conceptually, each pixel will be 0.1 units redder than the previous one, but that will be visible only each time the accumulated steps have added up to one unit.

The code in the fillRectXGradient routines follows that pattern, but in the quoted code that defines the R/G/B steps (dr/dg/db) , all the variables are integers, so in my example dr would be 9/100 in integer arithmetic, or zero.

These routines might seem to work with broad color ranges in narrow rectangles, but not otherwise.
  • The simplest possible fix would be to have the dr/dg/db variables be floating-point.
  • Alternatively, if integer variables must be used, the step size could be implicit, so that (for example) the Red value to be used in Pixel #X in my example could be RedLeft + (X * (RedRight - RedLeft)) / W. Here, (RedRight - RedLeft) could be pre-computed, but the division would need to stay in the loop.
 
Fenichel said:
There are two problems with the fillRectHGradient and fillRectVGradient routines, one trivial and one deeper.
In the fillRectHGradient routine, each h in the line
Code: 
dr=(r2-r1)/h; dg=(g2-g1)/h; db=(b2-b1)/h;
should be w. The fillRectHGradient routine was probably written by copying the fillRectVGradient routine, making (almost all) the changes necessary.

Unfortunately, there is a deeper problem with that line. To see the deeper problem, consider (for definiteness) the problem of filling a 100-pixel line (W = 100) with a gradient running from RedLeft = 0 to RedRight = 9. The expected line will have 10 pixels with Red = 0, 10 with Red = 1, and so on. Conceptually, each pixel will be 0.1 units redder than the previous one, but that will be visible only each time the accumulated steps have added up to one unit.

The code in the fillRectXGradient routines follows that pattern, but in the quoted code that defines the R/G/B steps (dr/dg/db) , all the variables are integers, so in my example dr would be 9/100 in integer arithmetic, or zero.

These routines might seem to work with broad color ranges in narrow rectangles, but not otherwise.
  • The simplest possible fix would be to have the dr/dg/db variables be floating-point.
  • Alternatively, if integer variables must be used, the step size could be implicit, so that (for example) the Red value to be used in Pixel #X in my example could be RedLeft + (X * (RedRight - RedLeft)) / W. Here, (RedRight - RedLeft) could be pre-computed, but the division would need to stay in the loop.
Click to expand...
The second problem noted here is slightly less severe than it seemed to me. The single-color values are integers, but with a unit size of 4. That is, the integer division to compute dr/dg/db will cause bad gradients, but only in relatively extreme cases, like the narrow-gradient wide-rectangle one of my example.
 
I think it's in some ways better, and others worse, than you've stated.

The gradient colours use a 14-bit calculations, so there are 256 or 512 micro-steps per actual step. Only very extreme gradients of one or two steps across the entire screen will fail completely ... except ... I'm pretty sure the lighter end only appears for at best one pixel column (or row, for a vertical gradient). Here's an example:
C++: 
  tft.fillRect(0, 200, 40, 40, tft.color565(10*8,9*4,2*8)); // reference square: second step colour
  tft.fillRectHGradient(10, 210, 220, 20, tft.color565(11*8,10*4,3*8),tft.color565(8,8,8));
Because the gradient starts exactly on the brightest colours, after the first set of deltas has been added the second column has everything dimmed by one colour step; you can see a bright line in the "reference square", then the gradient matches it until it next changes. It's a classic example of a "fencepost error"...

You can also see with this demo that because the channels step at different points, the gradient changes hue as it goes. There's nothing to be done about that, unless you want to write dithering into the gradient code...
 
Fenichel said:
should be w. The fillRectHGradient routine was probably written by copying the fillRectVGradient routine, making (almost all) the changes necessary.
Click to expand...
Have you issued a PR back to Paul on this library? https://github.com/PaulStoffregen/ILI9341_t3

I checked my ILI9341_t3n library and it does not appear to have this issue.

Fenichel said:
The code in the fillRectXGradient routines follows that pattern, but in the quoted code that defines the R/G/B steps (dr/dg/db) , all the variables are integers, so in my example dr would be 9/100 in integer arithmetic, or zero.

These routines might seem to work with broad color ranges in narrow rectangles, but not otherwise.
  • The simplest possible fix would be to have the dr/dg/db variables be floating-point.
  • Alternatively, if integer variables must be used, the step size could be implicit, so that (for example) the Red value to be used in Pixel #X in my example could be RedLeft + (X * (RedRight - RedLeft)) / W. Here, (RedRight - RedLeft) could be pre-computed, but the division would need to stay in the loop.
Click to expand...

I did a quick and dirty update to my zephyr version of the ILI9341_t3n code for using floats...

Code: 
// fillRectVGradient    - fills area with vertical gradient
void ILI9341_GIGA_n::fillRectVGradient(int16_t x, int16_t y, int16_t w, int16_t h,
                                    uint16_t color1, uint16_t color2) {
  x += _originx;
  y += _originy;


  // Rectangular clipping
  if ((x >= _displayclipx2) || (y >= _displayclipy2))
    return;
  if (x < _displayclipx1) {
    w -= (_displayclipx1 - x);
    x = _displayclipx1;
  }
  if (y < _displayclipy1) {
    h -= (_displayclipy1 - y);
    y = _displayclipy1;
  }
  if ((x + w - 1) >= _displayclipx2)
    w = _displayclipx2 - x;
  if ((y + h - 1) >= _displayclipy2)
    h = _displayclipy2 - y;


  int16_t r1, g1, b1, r2, g2, b2;
  float dr, dg, db, r, g, b;
  color565toRGB14(color1, r1, g1, b1);
  color565toRGB14(color2, r2, g2, b2);
  dr = (float)(r2 - r1) / h;
  dg = (float)(g2 - g1) / h;
  db = (float)(b2 - b1) / h;
  r = r1;
  g = g1;
  b = b1;


#ifdef ENABLE_ILI9341_FRAMEBUFFER
  if (_use_fbtft) {
    updateChangedRange(
        x, y, w, h); // update the range of the screen that has been changed;
    if ((x & 1) || (w & 1)) {
      uint16_t *pfbPixel_row = &_pfbtft[y * _width + x];
      for (; h > 0; h--) {
        uint16_t color = RGB14tocolor565(roundf(r), roundf(g), roundf(b));
        uint16_t *pfbPixel = pfbPixel_row;
        for (int i = 0; i < w; i++) {
          *pfbPixel++ = color;
        }
        r += dr;
        g += dg;
        b += db;
        pfbPixel_row += _width;
      }
    } else {
      // Horizontal is even number so try 32 bit writes instead
      uint32_t *pfbPixel_row =
          (uint32_t *)((uint16_t *)&_pfbtft[y * _width + x]);
      w = w / 2; // only iterate half the times
      for (; h > 0; h--) {
        uint32_t *pfbPixel = pfbPixel_row;
        uint16_t color = RGB14tocolor565(r, roundf(g), roundf(b));
        uint32_t color32 = (color << 16) | color;
        for (int i = 0; i < w; i++) {
          *pfbPixel++ = color32;
        }
        pfbPixel_row += (_width / 2);
        r += dr;
        g += dg;
        b += db;
      }
    }
  } else
#endif
  {
    beginSPITransaction(_SPI_CLOCK);
    setAddr(x, y, x + w - 1, y + h - 1);
    writecommand_cont(ILI9341_RAMWR);
    for (y = h; y > 0; y--) {
      uint16_t color = RGB14tocolor565(r, roundf(g), roundf(b));


      for (x = w; x > 1; x--) {
        writedata16_cont(color);
      }
      writedata16_last(color);
      if (y > 1 && (y & 1)) {
        endSPITransaction();
        beginSPITransaction(_SPI_CLOCK);
      }
      r += dr;
      g += dg;
      b += db;
    }
    endSPITransaction();
  }
}


// fillRectHGradient    - fills area with horizontal gradient
void ILI9341_GIGA_n::fillRectHGradient(int16_t x, int16_t y, int16_t w, int16_t h,
                                    uint16_t color1, uint16_t color2) {
  x += _originx;
  y += _originy;


  // Rectangular clipping
  if ((x >= _displayclipx2) || (y >= _displayclipy2))
    return;
  if (x < _displayclipx1) {
    w -= (_displayclipx1 - x);
    x = _displayclipx1;
  }
  if (y < _displayclipy1) {
    h -= (_displayclipy1 - y);
    y = _displayclipy1;
  }
  if ((x + w - 1) >= _displayclipx2)
    w = _displayclipx2 - x;
  if ((y + h - 1) >= _displayclipy2)
    h = _displayclipy2 - y;


  int16_t r1, g1, b1, r2, g2, b2;
  float dr, dg, db, r, g, b;
  uint16_t color;
  color565toRGB14(color1, r1, g1, b1);
  color565toRGB14(color2, r2, g2, b2);
  dr = (float)(r2 - r1) / w;
  dg = (float)(g2 - g1) / w;
  db = (float)(b2 - b1) / w;
  r = r1;
  g = g1;
  b = b1;
#ifdef ENABLE_ILI9341_FRAMEBUFFER
  if (_use_fbtft) {
    updateChangedRange(
        x, y, w, h); // update the range of the screen that has been changed;
    uint16_t *pfbPixel_row = &_pfbtft[y * _width + x];
    for (; h > 0; h--) {
      uint16_t *pfbPixel = pfbPixel_row;
      for (int i = 0; i < w; i++) {
        *pfbPixel++ = RGB14tocolor565(roundf(r), roundf(g), roundf(b));
        r += dr;
        g += dg;
        b += db;
      }
      pfbPixel_row += _width;
      r = r1;
      g = g1;
      b = b1;
    }
  } else
#endif
  {
    beginSPITransaction(_SPI_CLOCK);
    setAddr(x, y, x + w - 1, y + h - 1);
    writecommand_cont(ILI9341_RAMWR);
    for (y = h; y > 0; y--) {
      for (x = w; x > 1; x--) {
        color = RGB14tocolor565(roundf(r), roundf(g), roundf(b));
        writedata16_cont(color);
        r += dr;
        g += dg;
        b += db;
      }
      color = RGB14tocolor565(r, roundf(g), roundf(b));
      writedata16_last(color);
      if (y > 1 && (y & 1)) {
        endSPITransaction();
        beginSPITransaction(_SPI_CLOCK);
      }
      r = r1;
      g = g1;
      b = b1;
    }
    endSPITransaction();
  }
}

And in the test sketch added:
Code: 
tft.fillScreen(ILI9341_BLACK);
    tft.fillRectHGradient(10, 10, 100, 100, tft.color565(0 << 3, 0, 0), tft.color565(9 << 3, 0, 0));
    tft.fillRectHGradient(130, 10, 100, 100, ILI9341_YELLOW, ILI9341_GREEN);
    tft.fillRectVGradient(10, 130, 100, 100, tft.color565(0, 0, 0 << 3), tft.color565(0, 0, 9 << 3));
    tft.fillRectVGradient(130, 130, 100, 100, ILI9341_CYAN, ILI9341_BLUE);
    WaitForUserInput();

1747231357398.png


May play with it a little more... for example check is it faster to do as it does now as one big writeRect like output
or would it be faster to do draw fast VLines or waste some memory for one horizontal row and duplicate outputting it...
 
@KurtE have you read my post #3? There are issues, but not all quite as suggested by @Fenichel …

Using floats seems to be overkill. As it stands the values are 14 bit, and 32 bit (Q5.24 or Q6.24) would actually have more resolution than float.
 
Thanks @h4yn0nnym0u5e - As they say if you have a better solution... feel free to create a PR with it ;)

I understand that floating point may be overkill, but there is hardware support for it... And I might even play with it.
i.e roll my own, like I used to do on older processors: ...
 
Yes, I could put in some effort and make a PR.

But I've been getting increasingly reluctant to do so, because they just get ignored, with the possible exception of those that are so trivial as not to need any review effort whatsoever. Or I thought ... maybe I'm being unfair? So I looked at the first page of Paul's repos, tried to filter out ones not directly part of Teensyduino, and checked the last commit date, open PR count, and oldest PR (oldest on the first page, if there were more than 25 open).

Frankly, it makes for pretty depressing reading:

Repo​
Last commit​
Open PRs​
Oldest PR on first page​
cores​
2025-05-04​
91​
2024-01-18​
ST7735_t3​
2025-05-03​
4​
2020-12-14​
SerialFlash​
2025-04-27​
9​
2017-04-13​
SdFat​
2025-01-16​
4​
2022-09-17​
Encoder​
2025-01-05​
13​
2016-02-04​
USBHost_t36​
2025-01-03​
9​
2018-01-29​
Audio​
2024-12-01​
60​
2022-09-18​
AltSoftSerial​
2024-11-27​
9​
2016-05-28​
LittleFS​
2024-11-13​
3​
2021-09-09​
MTP_Teensy​
2024-11-13​
0​
​
SD​
2024-11-08​
5​
2016-05-28​
FlexIO_t4​
2024-09-10​
0​
​
OneWire​
2024-09-04​
21​
2016-01-31​
TimerOne​
2024-09-03​
8​
2016-06-02​
WS2812Serial​
2024-09-03​
2​
2019-04-25​
SoftwareSerial​
2024-08-26​
3​
2019-03-31​
Time​
2021-06-21​
25​
2015-02-24​
TimeAlarms​
2020-08-09​
9​
2016-08-19​
teensy_loader_cli​
2024-02-27​
19​
2015-03-25​
RadioHead​
2022-09-18​
4​
2018-06-17​
Arduino-EasyTransfer​
2022-02-25​
1​
2024-06-20​

That's 300 PRs open, with the oldest over 10 years old! The most recent commit fixes a bug in the Teensy 2.0 cores - well whoop-te-do, that's so relevant in 2025.

Apologies for the rant ... I just wish there was less reason for it.
 
You must log in or register to reply here.
Back
Top