Skip to content

Commit 44d4d93

Browse files
committed
[asimage] Make the two ellipse fill spans mirror each other
asim_ellips2() fills a tilted ellipse by scanline rather than by flood fill: each iteration of the walk emits two horizontal spans, one on the row above the centre and one on the row below, exploiting the shape's 180-degree rotational symmetry. The two spans must therefore be mirror images about the centre, and they were not. The upper span ended at x2-1 while the lower one started at x-x2-1, which mirrors to x2+1 -- a two-column disagreement that went in both directions at once. Two columns end up unpainted inside the shape on the upper rows, and two columns outside it get painted on the lower rows. The gap is visible for every tilted angle and grows with the tilt; at 45 degrees with rx=21, ry=9 it leaves eight unfilled pixels in the interior, including the one the report points at. The straight cases are unaffected because angles of 0, 90, 180 and 270 return early to asim_straight_ellips(). Naming the span end makes the symmetry explicit and lets each branch of the walk set it from its own geometry. Only the line > yr branch needs a different value: it writes the anti-aliased outline at x2+2, so the fill may reach x2+1. The other two branches keep the previous end, so their output does not change. Measured against the interior of the shape's own outline, over 610 combinations of angle and radii: unfilled interior pixels drop from 7698 to 30 and painted exterior pixels from 7496 to 1102, with no combination getting worse on either count. Unfilled ellipses and the straight-ellipse path render bit-identically before and after, at every angle and radius tried. The four filled-ellipse tests were parked in a WILLFAIL target while the fill was broken; with the spans mirrored they pass, and a passing WILLFAIL test is reported as a failure, so they move back into tasimage_draw.cxx and the separate target goes away. tasimage_ellipse_draw.cxx was a strict subset of tasimage_draw.cxx, so it is removed rather than left to run the same four tests twice. Refs #23120 Assisted-by: Claude (Anthropic); the change was AI-assisted, then reviewed, measured and verified by the author.
1 parent 6cfe829 commit 44d4d93

4 files changed

Lines changed: 11 additions & 127 deletions

File tree

builtins/libAfterImage/draw.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1523,6 +1523,7 @@ asim_ellips2( ASDrawContext *ctx, int x, int y, int rx, int ry, int angle, Bool
15231523
{
15241524
double d ;
15251525
int dx1 = 0, dx2 = 0 ;
1526+
int fill_edge = -1 ;
15261527
d = A*(double)x1*(double)x1 + BB +CC*(double)x1;
15271528
#ifdef DEBUG_ELLIPS
15281529
fprintf( stderr, "line = %d, d1 = %f", y-line, d );
@@ -1634,6 +1635,7 @@ asim_ellips2( ASDrawContext *ctx, int x, int y, int rx, int ry, int angle, Bool
16341635
dd += aa ;
16351636
}
16361637
x2 += (dx2>>1)-1 ;
1638+
fill_edge = 1 ;
16371639
last_med_dd2 = med_dd ;
16381640
}
16391641
}else if( line < yr )
@@ -1699,8 +1701,8 @@ asim_ellips2( ASDrawContext *ctx, int x, int y, int rx, int ry, int angle, Bool
16991701
#endif
17001702
if( fill )
17011703
{
1702-
CTX_FILL_HLINE(ctx,x+(x1-2),y-y1,x+x2-1,255);
1703-
CTX_FILL_HLINE(ctx,x-x2-1,y+y1,x-(x1-2),255);
1704+
CTX_FILL_HLINE(ctx,x+(x1-2),y-y1,x+(x2+fill_edge),255);
1705+
CTX_FILL_HLINE(ctx,x-(x2+fill_edge),y+y1,x-(x1-2),255);
17041706
}
17051707

17061708
CC -= 2.*C ;

graf2d/asimage/test/CMakeLists.txt

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,3 @@
55
# For the list of contributors see $ROOTSYS/README/CREDITS.
66

77
ROOT_ADD_GTEST(TASImageDraw tasimage_draw.cxx LIBRARIES ASImage)
8-
9-
# add failing test until fixed
10-
ROOT_ADD_GTEST(TASImageDrawEllipse tasimage_ellipse_draw.cxx WILLFAIL LIBRARIES ASImage)

graf2d/asimage/test/tasimage_draw.cxx

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -169,22 +169,24 @@ TEST(TASImage, FillePolygonLowAlpha)
169169
CheckFilledShapeStaysInside("#100000FF", 2);
170170
}
171171

172-
/*
173-
174-
// comment out all ellpse test while they are failing
172+
// https://github.com/root-project/root/issues/23120
173+
//
174+
// asim_ellips2 walks a tilted ellipse one scanline pair at a time and exploits
175+
// the 180 degree rotational symmetry, so the two spans it fills per iteration
176+
// must mirror each other. The right edge was one pixel short of its partner,
177+
// which left unfilled pixels along the tilted boundary. The point checked below
178+
// sits inside the ellipse and used to stay empty.
175179

176180
TEST(TASImage, FilledEllipsOpaque)
177181
{
178182
CheckFilledShapeStaysInside("#FF2277CC", 3);
179183
}
180184

181-
// Used to leak out of the circle and fill the whole image.
182185
TEST(TASImage, FilledEllipsHighAlpha)
183186
{
184187
CheckFilledShapeStaysInside("#C02277CC", 3);
185188
}
186189

187-
// Used to hang: the colour from the issue report.
188190
TEST(TASImage, FilledEllipsSemiTransparent)
189191
{
190192
CheckFilledShapeStaysInside("#7F2277CC", 3);
@@ -194,4 +196,3 @@ TEST(TASImage, FilleEllipsLowAlpha)
194196
{
195197
CheckFilledShapeStaysInside("#102277CC", 3);
196198
}
197-
*/

graf2d/asimage/test/tasimage_ellipse_draw.cxx

Lines changed: 0 additions & 116 deletions
This file was deleted.

0 commit comments

Comments
 (0)