Fix off-by-one bug in drawCircle - #5798
Conversation
The next decision variable should be calculated based on the *current* x/y values
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe ChangesCircle drawing
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai please compare our drawCircle (non-aliased) implementation - including changes from this PR - to reference implementations. Is our code working correctly now? Bonus question: why are the numbers "4", "6" and "10" appearing in our code? I did not see them in the reference. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Map the affected source file, then print the drawCircle implementation and PR diff.
ast-grep outline wled00/FX_2Dfcn.cpp --items all --match drawCircle --view expanded || true
rg -n -A100 -B20 '(^|[^[:alnum:]_])drawCircle\s*\(' wled00/FX_2Dfcn.cpp
git diff --find-renames -- wled00/FX_2Dfcn.cpp
git show HEAD^:wled00/FX_2Dfcn.cpp | rg -n -A100 -B20 '(^|[^[:alnum:]_])drawCircle\s*\(' || trueLength of output: 15116 🤖 web_search executed:
Length of output: 15998
int d = 3 - 2 * radius;
int y = radius;
int x = 0;
while (y >= x) {
// Plot the eight symmetric pixels.
if (d > 0) {
d += 4 * (x - y) + 10;
y--;
} else {
d += 4 * x + 6;
}
x++;
}The critical requirement is that the update of The implementation plots these eight symmetric offsets for each point in one octant: This is the normal eight-way symmetry used by the supplied reference algorithms. The Wikipedia and Rosetta Code variants use a different orientation. They start at The German compact variant is also equivalent. It combines the E and SE decisions in one error-variable implementation. Its error variable and update order differ, but its plotted circle is the same Bresenham midpoint circle. The values Start with the implicit circle equation: At each point, the algorithm chooses between: It evaluates the midpoint between these choices. Expanding the equation gives these changes to the decision value: The WLED implementation multiplies these by Therefore:
The Comment image is consistent with this result. The fixed row removes the increasing asymmetry in the shipped implementation, especially from radius 7 onward. Minor pixel differences from Pillow can still occur because Pillow can use a different rasterization and tie-breaking policy. Those differences do not indicate an error in the corrected Bresenham implementation. You are interacting with an AI system. |
|
@coderabbitai how about https://en.wikipedia.org/wiki/Midpoint_circle_algorithm#Jesko's_method ? Could we adopt this method to be faster? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline wled00/FX_2Dfcn.cpp --items all --match drawCircle --view expanded || true
rg -n -A130 -B25 '(^|[^[:alnum:]_])drawCircle\s*\(' wled00/FX_2Dfcn.cpp
rg -n -A30 -B10 '\b(?:setPixelColor|blendPixelColor|XY)\s*\(' wled00/FX_2Dfcn.cpp | head -n 260
git diff --find-renames -- wled00/FX_2Dfcn.cppLength of output: 14133 🤖 web_search executed:
Length of output: 11796 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
def wled(r):
points = set()
d = 3 - 2 * r
x, y = 0, r
while y >= x:
for i in range(4):
dx = -x if (i & 1) else x
dy = -y if (i & 2) else y
points.add((dx, dy))
points.add((dy, dx))
if d > 0:
d += 4 * (x - y) + 10
y -= 1
else:
d += 4 * x + 6
x += 1
return points
def jesko(r):
points = set()
t1 = r // 16
x, y = r, 0
while x >= y:
for px, py in (
( x, y), ( x, -y), (-x, y), (-x, -y),
( y, x), ( y, -x), (-y, x), (-y, -x),
):
points.add((px, py))
y += 1
t1 += y
t2 = t1 - x
if t2 >= 0:
t1 = t2
x -= 1
return points
different = []
for r in range(1, 256):
a, b = wled(r), jesko(r)
if a != b:
different.append((r, len(a), len(b), sorted(a - b), sorted(b - a)))
print(f"radii compared: 1..255")
print(f"identical point sets: {255 - len(different)}")
print(f"different point sets: {len(different)}")
for r, na, nb, only_wled, only_jesko in different[:12]:
print(f"r={r}: WLED={na}, Jesko={nb}, "
f"WLED-only={only_wled[:12]}, Jesko-only={only_jesko[:12]}")
if len(different) > 12:
print(f"... {len(different)-12} additional differing radii")
PY
rg -n -A100 -B15 '\b(?:void|bool|uint32_t)\s+Segment::setPixelColorXY\s*\(' wled00
rg -n -A100 -B15 '\bsetPixelColorXY\s*\(' wled00/FX.h wled00/FX.cpp 2>/dev/null || trueLength of output: 178051
However, we should not replace WLED’s current recurrence with it. I compared the two algorithms for every valid Examples:
Therefore, Jesko’s method can be adopted only as a different circle rasterizer. It cannot be treated as a transparent optimization of the current output. There can still be a speed benefit in the decision loop. The published method uses five counted arithmetic operations per octant step. But My recommendation is:
The Comment image favors the current result because it tracks the selected standard Bresenham reference. Jesko’s method would create a different output contract. You are interacting with an AI system. |
|
@coderabbitai please also check the aliased variant of drawCircle() against the reference Wu's Algorithm. Is there a similar off-by-one error in our aliased circle implementation? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Map the affected implementation before reading the relevant ranges.
ast-grep outline wled00/FX_2Dfcn.cpp --items all --match drawCircle --view expanded || true
# Show the complete circle implementation and the pixel-writing helpers it calls.
rg -n -A220 -B30 '(^|[^[:alnum:]_])(?:void|bool|uint16_t|uint32_t)\s+Segment::drawCircle\s*\(' wled00/FX_2Dfcn.cpp
rg -n -A120 -B20 '\b(?:setPixelColorXY|blendPixelColorXY|blendPixelColor)\s*\(' wled00/FX_2Dfcn.cpp wled00/FX.h wled00/FX.cpp 2>/dev/null || true
# Capture the PR-specific change for comparison with the soft branch.
git diff --find-renames -- wled00/FX_2Dfcn.cppLength of output: 152147 🤖 web_search executed:
Length of output: 14301 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate all drawCircle definitions precisely.
rg -nP '^\s*(?:void|bool|uint\d+_t)\s+(?:Segment::)?drawCircle\s*\(' wled00
# Print a bounded source range around each definition in wled00/FX_2Dfcn.cpp.
line="$(rg -nP '^\s*void\s+Segment::drawCircle\s*\(' wled00/FX_2Dfcn.cpp | head -n1 | cut -d: -f1)"
if [ -z "$line" ]; then
echo "Segment::drawCircle definition not found in wled00/FX_2Dfcn.cpp" >&2
exit 1
fi
end=$((line + 180))
sed -n "${line},${end}p" wled00/FX_2Dfcn.cpp
# Print the focused PR diff without broad helper searches.
git diff --unified=80 -- wled00/FX_2Dfcn.cppLength of output: 6668 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate the blend helper definition or its documentation. The argument order determines
# the exact source-color weight in the soft circle branch.
rg -n -A30 -B10 '\bcolor_blend\s*\(' wled00 -g '*.{h,hpp,c,cc,cpp,ino}' | head -n 260
# Compare the WLED loop's (x, y, fade) trace with a direct Wu-circle reference.
python3 - <<'PY'
import math
def quantized_fade(radius, x):
yf = math.sqrt(radius * radius - x * x)
return int(255.0 * (math.ceil(yf) - yf)) # C++ float-to-uint8 truncation
def wled_trace(radius):
x, y, old_fade = 0, radius, 0
out = []
while x < y:
fade = quantized_fade(radius, x)
if old_fade > fade:
y -= 1
old_fade = fade
# `y` is floor(yf); y-1 is ceil(yf)-1 in WLED's pixel-centre convention.
out.append((x, y, fade, 255 - fade))
x += 1
return out
def wu_reference_trace(radius):
# Wu's circle recurrence in WLED's orientation and pixel-centre convention:
# calculate fraction -> detect wrap/decrement -> emit the two complementary samples.
x, y, previous = 0, radius, 0
out = []
while x < y:
coverage_outer = quantized_fade(radius, x)
if coverage_outer < previous:
y -= 1
previous = coverage_outer
out.append((x, y, coverage_outer, 255 - coverage_outer))
x += 1
return out
def post_plot_decrement_trace(radius):
# This models the ordering error relevant to the non-soft branch.
x, y, old_fade = 0, radius, 0
out = []
while x < y:
fade = quantized_fade(radius, x)
out.append((x, y, fade, 255 - fade))
if old_fade > fade:
y -= 1
old_fade = fade
x += 1
return out
mismatches = []
post_plot_differences = []
for radius in range(1, 256):
actual = wled_trace(radius)
expected = wu_reference_trace(radius)
if actual != expected:
mismatches.append(radius)
if actual != post_plot_decrement_trace(radius):
post_plot_differences.append(radius)
print(f"radii compared: 1..255")
print(f"WLED / Wu-reference trace mismatches: {len(mismatches)}")
print(f"radii affected by a post-plot decrement ordering: {len(post_plot_differences)}")
print("first affected radii:", post_plot_differences[:20])
for radius in (2, 3, 4, 7, 8, 12):
print(f"r={radius}: {wled_trace(radius)}")
PYLength of output: 20691
The two branches use different update rules. In float yf = sqrtf(float(rsq - x*x));
uint8_t fade = float(0xFF) * (ceilf(yf) - yf);
if (oldFade > fade) y--;
oldFade = fade;This order is correct:
The early decrement is intentional. It ensures that
The
color_blend(col, pixCol, fade) // color contribution: 255 - fade
color_blend(pixCol, col, fade) // color contribution: fadeI compared the WLED For contrast, if this branch plotted first and decremented Therefore, the non-aliased bug does not apply to the aliased implementation. The PR does not modify the aliased branch. You are interacting with an AI system. |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
On a related note, I also found that the non-aliased It's possible to use Bresenham's in |
@HampusAdolfsson yes, I think this was the reason why we didn't use Bresenham's in fillCircle() ... directly using the "x^2 + y^2 <= r^2" criterion was faster, and it produced nicer results. |
Side-topic: There is indeed some potential for optimization here - setPixelColorXY() is the "slow" variant that always tests |
2 participants
Bresenham's algorithm (used in the non-soft version of
drawCircle) calculates the next decision variable based on the current x- and y-values.drawCircleincorrectly adjusts x and y before calculating the next decision variable, making the arc slightly flatter than it should be.I wrote a python script to compare the implementations, with the python library
Pillowused as a reference:As you can see, this change makes WLED agree with Pillow. Everything from
r=7and up looks rounder after this change. The lower radii are more a matter of taste (and which ones I prefer depend on the size of each pixel).Summary by CodeRabbit