search

ThingPulse / esp8266-oled-ssd1306

Driver for the SSD1306 and SH1106 based 128x64, 128x32, 64x48 pixel OLED display running on ESP8266/ESP32
https://thingpulse.com
Other
2.02k stars 640 forks source link

Corrected Circle drawing algorithm #197

Closed DaveSprague closed 5 years ago

DaveSprague commented 6 years ago

I was using this library on on a NodeMCU 1.0 with the vsCode PlatformIO development environment and I noticed that when I ran the basic example code, the circles were squashed (somewhat more like rounded rectangles than circles). After a few hours of work comparing it to the Adafruit GFX library code and the wikipedia code for the midpoint circle algorithm, I determined that the pre-increments and pre-decrements used in all three circle drawing functions (drawCircle, drawCircleQuads, and fillCircle) should be post-increments and post-decrements. This commit changes the two ++x's and one --y to x++ and y-- respectively in all three funtions.

I've also verified this correction works properly and fixes the same issue when tested in the Arduino IDE.

With this change, this library now draws circles that match pixel by pixel with the calculations used in the Adafruit GFX library. My testing has only covered smaller circles as is used in the example code but I believe it should scale up properly.

mikewalker-cihsolutions commented 6 years ago

+1 for this pull request. I've tested myself and it gives much better results than the current version.

DaveSprague commented 6 years ago

I've done further testing and the fixed version matches pixel for pixel with the Adafruit GFX library for a very wide range of circles sizes.

DaveSprague commented 5 years ago

Is there anything else I need to do or can do to get this fix merged into the master branch?

helmut64 commented 5 years ago

Hi Dave, I verified your changes and the cirle looks much better. Thanks. BTW: I don't understand why it takes almost a year to merge the fix for the maintainer. I have done larger changes to support mbed-os, I need to check what I can do, maybe I should fork the repro and apply the verified pull requests and continue on my own.

DaveSprague commented 5 years ago

Thanks Helmut, this is my first pull request for a code change so I don't have any experience with what's required.

Dave

On Sun, Apr 7, 2019 at 6:32 AM Helmut Tschemernjak notifications@github.com wrote:

Hi Dave, I verified your changes and the cirle looks much better. Thanks. BTW: I don't understand why it takes almost a year to merge the fix for the maintainer. I have done larger changes to support mbed-os, I need to check what I can do, maybe I should fork the repro and apply the verified pull requests and continue on my own.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/197#issuecomment-480590922, or mute the thread https://github.com/notifications/unsubscribe-auth/ABYZClA8JiR21c7o5S09Tav8mhhkfVcEks5vefNigaJpZM4Trodf .

dagnall53 commented 5 years ago

I may be getting confused, but I am not sure that the "corrected" algorithm is in the current OLEDdisplay.cpp. Has the old algorithm crept back in??

void OLEDDisplay::drawCircle(int16_t x0, int16_t y0, int16_t radius) { int16_t x = 0, y = radius; int16_t dp = 1 - radius; do { //THIS is the correct code for circles
if (dp < 0) dp = dp + 2 (++x) + 3; else dp = dp + 2 (++x) - 2 (--y) + 5; / This is the wrong code for circles... if (dp < 0) dp = dp + (++x) << 1 + 3; else dp = dp + (++x) << 1 - (--y) << 1 + 5; */
setPixel(x0 + x, y0 + y); .. etc

DaveSprague commented 5 years ago

Good catch dagnall53. A recent pull request by someone else (https://github.com/ThingPulse/esp8266-oled-ssd1306/pull/236) went through and changed some multiplications by factors of 2 to be done with shifts but also, for an unexplained reason, changed the post increments/decrements back to pre-increments/decrements (probably because they were basing their changes on an older version before my fix was merged? I'll submit another pull request to fix it.

DaveSprague commented 5 years ago

You can see my further comments under the #236 pull request. Not only were the post-increments/decrements inadvertently changed back to pre-increments/decrements but the replacement of multiplications with bit-shift caused further issues with the circle drawing routines because the order of precedence for multiplication is higher than addition but the precedence of left/right bit-shifts is lower than addition. I also did some testing and there doesn't appear to be any performance improvement with the change either. Thanks again, @dagnall53 for catching this. I'm submitting a pull request later on today to revert the code to match up with the version I submitted in this pull request which draws circles correctly.