From 63d3f3f59c3b394581f0221145a6ec40c9c7db42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20=C5=BDivi=C4=87?= Date: Wed, 5 Jun 2024 19:52:07 +0200 Subject: [PATCH 1/5] Fixed draw.aalines overlaping --- src_c/draw.c | 212 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 158 insertions(+), 54 deletions(-) diff --git a/src_c/draw.c b/src_c/draw.c index 1d5cbd2cba..42f8aa7245 100644 --- a/src_c/draw.c +++ b/src_c/draw.c @@ -46,7 +46,9 @@ draw_line(SDL_Surface *surf, int x1, int y1, int x2, int y2, Uint32 color, int *drawn_area); static void draw_aaline(SDL_Surface *surf, Uint32 color, float startx, float starty, - float endx, float endy, int *drawn_area); + float endx, float endy, int *drawn_area, + int disable_first_endpoint, int disable_second_endpoint, + int extra_pixel_for_aalines); static void draw_arc(SDL_Surface *surf, int x_center, int y_center, int radius1, int radius2, int width, double angle_start, double angle_stop, @@ -159,7 +161,7 @@ aaline(PyObject *self, PyObject *arg, PyObject *kwargs) return RAISE(PyExc_RuntimeError, "error locking surface"); } - draw_aaline(surf, color, startx, starty, endx, endy, drawn_area); + draw_aaline(surf, color, startx, starty, endx, endy, drawn_area, 0, 0, 0); if (!pgSurface_Unlock(surfobj)) { return RAISE(PyExc_RuntimeError, "error unlocking surface"); @@ -255,9 +257,13 @@ aalines(PyObject *self, PyObject *arg, PyObject *kwargs) SDL_Surface *surf = NULL; Uint32 color; float pts[4]; + float pts_prev[4]; float *xlist, *ylist; float x, y; int l, t; + int extra_px; + int steep_prev; + int steep_curr; PyObject *blend = NULL; int drawn_area[4] = {INT_MAX, INT_MAX, INT_MIN, INT_MIN}; /* Used to store bounding box values */ @@ -344,19 +350,81 @@ aalines(PyObject *self, PyObject *arg, PyObject *kwargs) return RAISE(PyExc_RuntimeError, "error locking surface"); } - for (loop = 1; loop < length; ++loop) { + /* first line - if open, add endpoint pixels.*/ + pts[0] = xlist[0]; + pts[1] = ylist[0]; + pts[2] = xlist[1]; + pts[3] = ylist[1]; + + /* Previous points. + * Used to compare previous and current line.*/ + pts_prev[0] = pts[0]; + pts_prev[1] = pts[1]; + pts_prev[2] = pts[2]; + pts_prev[3] = pts[3]; + if (closed) { + draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 1, + 1, 0); + } + else { + draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 0, + 1, 0); + } + + for (loop = 2; loop < length - 1; ++loop) { pts[0] = xlist[loop - 1]; pts[1] = ylist[loop - 1]; pts[2] = xlist[loop]; pts[3] = ylist[loop]; - draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area); + + /* Comparing previous and current line. + * If one is steep and other is not, extra pixel must be drawn.*/ + steep_prev = + fabs(pts_prev[2] - pts_prev[0]) < fabs(pts_prev[3] - pts_prev[1]); + steep_curr = fabs(pts[2] - pts[0]) < fabs(pts[3] - pts[1]); + extra_px = steep_prev != steep_curr; + pts_prev[0] = pts[0]; + pts_prev[1] = pts[1]; + pts_prev[2] = pts[2]; + pts_prev[3] = pts[3]; + + draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 1, + 1, extra_px); + } + + /* Last line - if open, add endpoint pixels. */ + pts[0] = xlist[length - 2]; + pts[1] = ylist[length - 2]; + pts[2] = xlist[length - 1]; + pts[3] = ylist[length - 1]; + steep_prev = + fabs(pts_prev[2] - pts_prev[0]) < fabs(pts_prev[3] - pts_prev[1]); + steep_curr = fabs(pts[2] - pts[0]) < fabs(pts[3] - pts[1]); + extra_px = steep_prev != steep_curr; + pts_prev[0] = pts[0]; + pts_prev[1] = pts[1]; + pts_prev[2] = pts[2]; + pts_prev[3] = pts[3]; + if (closed) { + draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 1, + 1, extra_px); } + else { + draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 1, + 0, extra_px); + } + if (closed && length > 2) { pts[0] = xlist[length - 1]; pts[1] = ylist[length - 1]; pts[2] = xlist[0]; pts[3] = ylist[0]; - draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area); + steep_prev = + fabs(pts_prev[2] - pts_prev[0]) < fabs(pts_prev[3] - pts_prev[1]); + steep_curr = fabs(pts[2] - pts[0]) < fabs(pts[3] - pts[1]); + extra_px = steep_prev != steep_curr; + draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 1, + 1, extra_px); } PyMem_Free(xlist); @@ -1256,7 +1324,9 @@ set_and_check_rect(SDL_Surface *surf, int x, int y, Uint32 color, static void draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, - float to_x, float to_y, int *drawn_area) + float to_x, float to_y, int *drawn_area, + int disable_first_endpoint, int disable_second_endpoint, + int extra_pixel_for_aalines) { float gradient, dx, dy, intersect_y, brightness; int x, x_pixel_start, x_pixel_end; @@ -1361,66 +1431,100 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, * The line is not a mathematical line of thickness zero. The same * goes for the endpoints. The have a height and width of one pixel. */ /* First endpoint */ - x_pixel_start = (int)from_x; - y_endpoint = intersect_y = from_y + gradient * (x_pixel_start - from_x); - if (to_x > clip_left + 1.0f) { - x_gap = 1 + x_pixel_start - from_x; - brightness = y_endpoint - (int)y_endpoint; - if (steep) { - x = (int)y_endpoint; - y = x_pixel_start; - } - else { - x = x_pixel_start; - y = (int)y_endpoint; - } - if ((int)y_endpoint < y_endpoint) { + if (!disable_first_endpoint) { + x_pixel_start = (int)from_x; + y_endpoint = intersect_y = + from_y + gradient * (x_pixel_start - from_x); + if (to_x > clip_left + 1.0f) { + x_gap = 1 + x_pixel_start - from_x; + brightness = y_endpoint - (int)y_endpoint; + if (steep) { + x = (int)y_endpoint; + y = x_pixel_start; + } + else { + x = x_pixel_start; + y = (int)y_endpoint; + } + if ((int)y_endpoint < y_endpoint) { + pixel_color = get_antialiased_color(surf, x, y, color, + brightness * x_gap); + set_and_check_rect(surf, x, y, pixel_color, drawn_area); + } + if (steep) { + x--; + } + else { + y--; + } + brightness = 1 - brightness; pixel_color = get_antialiased_color(surf, x, y, color, brightness * x_gap); set_and_check_rect(surf, x, y, pixel_color, drawn_area); + intersect_y += gradient; + x_pixel_start++; } - if (steep) { - x--; - } - else { - y--; + } + else { + /* Handle extra pixel for aalines. + * It is drawn only when one line is steep and other is not.*/ + if (extra_pixel_for_aalines) { + x_pixel_start = (int)from_x; + y_endpoint = intersect_y = + from_y + gradient * (x_pixel_start - from_x); + if (to_x > clip_left + 1.0f) { + x_gap = 1 + x_pixel_start - from_x; + brightness = y_endpoint - (int)y_endpoint; + if (steep) { + x = (int)y_endpoint; + y = x_pixel_start; + } + else { + x = x_pixel_start; + y = (int)y_endpoint; + } + if ((int)y_endpoint < y_endpoint) { + pixel_color = get_antialiased_color(surf, x, y, color, + brightness * x_gap); + set_and_check_rect(surf, x, y, pixel_color, drawn_area); + } + } } - brightness = 1 - brightness; - pixel_color = - get_antialiased_color(surf, x, y, color, brightness * x_gap); - set_and_check_rect(surf, x, y, pixel_color, drawn_area); - intersect_y += gradient; - x_pixel_start++; + /* To be sure main loop skips first endpoint.*/ + x_pixel_start = (int)ceil(from_x); + intersect_y = from_y + gradient * (x_pixel_start - from_x); } /* Second endpoint */ x_pixel_end = (int)ceil(to_x); - if (from_x < clip_right - 1.0f) { - y_endpoint = to_y + gradient * (x_pixel_end - to_x); - x_gap = 1 - x_pixel_end + to_x; - brightness = y_endpoint - (int)y_endpoint; - if (steep) { - x = (int)y_endpoint; - y = x_pixel_end; - } - else { - x = x_pixel_end; - y = (int)y_endpoint; - } - if ((int)y_endpoint < y_endpoint) { + if (!disable_second_endpoint) { + if (from_x < clip_right - 1.0f) { + y_endpoint = to_y + gradient * (x_pixel_end - to_x); + x_gap = 1 - x_pixel_end + to_x; + brightness = y_endpoint - (int)y_endpoint; + if (steep) { + x = (int)y_endpoint; + y = x_pixel_end; + } + else { + x = x_pixel_end; + y = (int)y_endpoint; + } + if ((int)y_endpoint < y_endpoint) { + pixel_color = get_antialiased_color(surf, x, y, color, + brightness * x_gap); + set_and_check_rect(surf, x, y, pixel_color, drawn_area); + } + if (steep) { + x--; + } + else { + y--; + } + brightness = 1 - brightness; pixel_color = get_antialiased_color(surf, x, y, color, brightness * x_gap); set_and_check_rect(surf, x, y, pixel_color, drawn_area); } - if (steep) { - x--; - } - else { - y--; - } - brightness = 1 - brightness; - pixel_color = - get_antialiased_color(surf, x, y, color, brightness * x_gap); - set_and_check_rect(surf, x, y, pixel_color, drawn_area); } /* main line drawing loop */ From 552725dc52874ef806b2e702e4fc87db5d482edf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20=C5=BDivi=C4=87?= Date: Thu, 6 Jun 2024 02:23:53 +0200 Subject: [PATCH 2/5] Added missing pixel Pixel was missing between steep and not-steep line, when line is inverted --- src_c/draw.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src_c/draw.c b/src_c/draw.c index 42f8aa7245..35d1ab8000 100644 --- a/src_c/draw.c +++ b/src_c/draw.c @@ -387,7 +387,6 @@ aalines(PyObject *self, PyObject *arg, PyObject *kwargs) pts_prev[1] = pts[1]; pts_prev[2] = pts[2]; pts_prev[3] = pts[3]; - draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 1, 1, extra_px); } @@ -1333,6 +1332,7 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, Uint32 pixel_color; float x_gap, y_endpoint, clip_left, clip_right, clip_top, clip_bottom; int steep, y; + int line_inverted; dx = to_x - from_x; dy = to_y - from_y; @@ -1365,6 +1365,7 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, swap(&clip_right, &clip_bottom); } if (dx < 0) { + line_inverted = 1; swap(&from_x, &to_x); swap(&from_y, &to_y); dx = -dx; @@ -1468,7 +1469,7 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, else { /* Handle extra pixel for aalines. * It is drawn only when one line is steep and other is not.*/ - if (extra_pixel_for_aalines) { + if (extra_pixel_for_aalines && !line_inverted) { x_pixel_start = (int)from_x; y_endpoint = intersect_y = from_y + gradient * (x_pixel_start - from_x); @@ -1526,6 +1527,29 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, set_and_check_rect(surf, x, y, pixel_color, drawn_area); } } + else { + /* Handle extra pixel for aalines. + * It is drawn only when one line is steep and other is not.*/ + if (extra_pixel_for_aalines && line_inverted) { + if (from_x < clip_right - 1.0f) { + y_endpoint = to_y + gradient * (x_pixel_end - to_x); + x_gap = 1 - x_pixel_end + to_x; + brightness = y_endpoint - (int)y_endpoint; + if (steep) { + x = (int)y_endpoint - 1; + y = x_pixel_end; + } + else { + x = x_pixel_end; + y = (int)y_endpoint - 1; + } + brightness = 1 - brightness; + pixel_color = get_antialiased_color(surf, x, y, color, + brightness * x_gap); + set_and_check_rect(surf, x, y, pixel_color, drawn_area); + } + } + } /* main line drawing loop */ for (x = x_pixel_start; x < x_pixel_end; x++) { From c5c48bac005a2a91232a3f5629e2b489692a0d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20=C5=BDivi=C4=87?= Date: Wed, 19 Jun 2024 23:44:41 +0200 Subject: [PATCH 3/5] Added tests - test_aalines__overlap and - test_aalines__steep_missing_pixel - Added missing pixel when first line in list is steep and second is not (earlier, it was working only when it is not first line in list) --- src_c/draw.c | 8 +++++-- test/draw_test.py | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src_c/draw.c b/src_c/draw.c index 35d1ab8000..84a8431c67 100644 --- a/src_c/draw.c +++ b/src_c/draw.c @@ -362,13 +362,17 @@ aalines(PyObject *self, PyObject *arg, PyObject *kwargs) pts_prev[1] = pts[1]; pts_prev[2] = pts[2]; pts_prev[3] = pts[3]; + steep_prev = + fabs(pts_prev[2] - pts_prev[0]) < fabs(pts_prev[3] - pts_prev[1]); + steep_curr = fabs(xlist[2] - pts[2]) < fabs(ylist[2] - pts[1]); + extra_px = steep_prev > steep_curr; if (closed) { draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 1, - 1, 0); + 1, extra_px); } else { draw_aaline(surf, color, pts[0], pts[1], pts[2], pts[3], drawn_area, 0, - 1, 0); + 1, extra_px); } for (loop = 2; loop < length - 1; ++loop) { diff --git a/test/draw_test.py b/test/draw_test.py index 3f00a9319e..1ea981b005 100644 --- a/test/draw_test.py +++ b/test/draw_test.py @@ -3646,6 +3646,60 @@ class DrawAALinesTest(AALinesMixin, DrawTestCase): class to add any draw.aalines specific tests to. """ + def test_aalines__overlap(self): + """Ensures that two adjacent antialiased lines are not overlapping. + + Draws two lines, and checks if 2 pixels, at shared point between those + two lines, are too bright. + + See: #2912 + """ + line_color = (150, 150, 150) + max_expected_colors = ((70, 70, 70), (100, 100, 100)) + test_points = [[20.1, 25.4], [25.1, 25.6], [30.1, 25.8]] + + surface = pygame.display.set_mode((50, 50)) + self.draw_aalines(surface, line_color, False, test_points) + + for i, y in enumerate((25, 26)): + check_color = tuple(surface.get_at((25, y))) + self.assertLess( + check_color, + max_expected_colors[i], + f"aalines are overlapping, pos={(25, y)}", + ) + + def test_aalines__steep_missing_pixel(self): + """Ensures there are no missing pixels between steep and non-steep lines. + + Draws two adjacent lines: first is not steep and second is, then + checks if there is missing pixel at shared point between those two lines. + + See: #2912 + """ + line_color = (150, 150, 150) + min_expected_color = (40, 40, 40) + test_points = [[11.2, 8.5], [17.1, 25.7], [35.8, 25.5], [47.6, 41.8]] + + surface = pygame.display.set_mode((50, 50)) + self.draw_aalines(surface, line_color, False, test_points) + + # First line is steep, and other line is not steep + check_color = tuple(surface.get_at((17, 26))) + self.assertGreater( + check_color, + min_expected_color, + "Pixel is missing between steep and non-steep line, pos=(17, 26)", + ) + + # First line is not steep, and other line is steep + check_color = tuple(surface.get_at((36, 25))) + self.assertGreater( + check_color, + min_expected_color, + "Pixel is missing between non-steep and steep line, pos=(26, 25)", + ) + ### Polygon Testing ########################################################### From 29f43fa2edb05c56647f6205d97fe97f07069a6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20=C5=BDivi=C4=87?= Date: Thu, 20 Jun 2024 03:53:23 +0200 Subject: [PATCH 4/5] Added this pr url to tests --- test/draw_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/draw_test.py b/test/draw_test.py index 1ea981b005..ecec278ce1 100644 --- a/test/draw_test.py +++ b/test/draw_test.py @@ -3652,7 +3652,7 @@ def test_aalines__overlap(self): Draws two lines, and checks if 2 pixels, at shared point between those two lines, are too bright. - See: #2912 + See: https://github.com/pygame-community/pygame-ce/pull/2912 """ line_color = (150, 150, 150) max_expected_colors = ((70, 70, 70), (100, 100, 100)) @@ -3675,7 +3675,7 @@ def test_aalines__steep_missing_pixel(self): Draws two adjacent lines: first is not steep and second is, then checks if there is missing pixel at shared point between those two lines. - See: #2912 + See: https://github.com/pygame-community/pygame-ce/pull/2912 """ line_color = (150, 150, 150) min_expected_color = (40, 40, 40) From a94e88d55defa536f75f023943f76b32eaae47b0 Mon Sep 17 00:00:00 2001 From: Marko Zivic Date: Thu, 5 Sep 2024 18:23:46 +0200 Subject: [PATCH 5/5] Fixed gaps in line and simplified code In some cases when adding extra pixel between steep and non-steep aaline, one extra pixel is not enough, causing gaps to appear between aalines Instead both endpoint pixels are needed This allowed to merge extra pixel handling with endpoint handling --- src_c/draw.c | 61 ++++++---------------------------------------------- 1 file changed, 7 insertions(+), 54 deletions(-) diff --git a/src_c/draw.c b/src_c/draw.c index a0f5a29111..ec8012a916 100644 --- a/src_c/draw.c +++ b/src_c/draw.c @@ -1329,7 +1329,6 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, Uint32 pixel_color; float x_gap, y_endpoint, clip_left, clip_right, clip_top, clip_bottom; int steep, y; - int line_inverted; dx = to_x - from_x; dy = to_y - from_y; @@ -1362,7 +1361,6 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, swap(&clip_right, &clip_bottom); } if (dx < 0) { - line_inverted = 1; swap(&from_x, &to_x); swap(&from_y, &to_y); dx = -dx; @@ -1427,9 +1425,11 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, /* Handle endpoints separately. * The line is not a mathematical line of thickness zero. The same - * goes for the endpoints. The have a height and width of one pixel. */ + * goes for the endpoints. The have a height and width of one pixel. + * Extra pixel drawing is requested externally from aalines. + * It is drawn only when one line is steep and other is not.*/ /* First endpoint */ - if (!disable_first_endpoint) { + if (!disable_first_endpoint || extra_pixel_for_aalines) { x_pixel_start = (int)from_x; y_endpoint = intersect_y = from_y + gradient * (x_pixel_start - from_x); @@ -1463,38 +1463,14 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, x_pixel_start++; } } - else { - /* Handle extra pixel for aalines. - * It is drawn only when one line is steep and other is not.*/ - if (extra_pixel_for_aalines && !line_inverted) { - x_pixel_start = (int)from_x; - y_endpoint = intersect_y = - from_y + gradient * (x_pixel_start - from_x); - if (to_x > clip_left + 1.0f) { - x_gap = 1 + x_pixel_start - from_x; - brightness = y_endpoint - (int)y_endpoint; - if (steep) { - x = (int)y_endpoint; - y = x_pixel_start; - } - else { - x = x_pixel_start; - y = (int)y_endpoint; - } - if ((int)y_endpoint < y_endpoint) { - pixel_color = get_antialiased_color(surf, x, y, color, - brightness * x_gap); - set_and_check_rect(surf, x, y, pixel_color, drawn_area); - } - } - } - /* To be sure main loop skips first endpoint.*/ + /* To be sure main loop skips first endpoint.*/ + if (disable_first_endpoint) { x_pixel_start = (int)ceil(from_x); intersect_y = from_y + gradient * (x_pixel_start - from_x); } /* Second endpoint */ x_pixel_end = (int)ceil(to_x); - if (!disable_second_endpoint) { + if (!disable_second_endpoint || extra_pixel_for_aalines) { if (from_x < clip_right - 1.0f) { y_endpoint = to_y + gradient * (x_pixel_end - to_x); x_gap = 1 - x_pixel_end + to_x; @@ -1524,29 +1500,6 @@ draw_aaline(SDL_Surface *surf, Uint32 color, float from_x, float from_y, set_and_check_rect(surf, x, y, pixel_color, drawn_area); } } - else { - /* Handle extra pixel for aalines. - * It is drawn only when one line is steep and other is not.*/ - if (extra_pixel_for_aalines && line_inverted) { - if (from_x < clip_right - 1.0f) { - y_endpoint = to_y + gradient * (x_pixel_end - to_x); - x_gap = 1 - x_pixel_end + to_x; - brightness = y_endpoint - (int)y_endpoint; - if (steep) { - x = (int)y_endpoint - 1; - y = x_pixel_end; - } - else { - x = x_pixel_end; - y = (int)y_endpoint - 1; - } - brightness = 1 - brightness; - pixel_color = get_antialiased_color(surf, x, y, color, - brightness * x_gap); - set_and_check_rect(surf, x, y, pixel_color, drawn_area); - } - } - } /* main line drawing loop */ for (x = x_pixel_start; x < x_pixel_end; x++) {