Add draw.aapolygon

[mzivic7]

This PR adds draw.aapolygon function, with filled option.
It was not possible to overlap draw.poylgon and draw.aalines without issues:
function draw_fillpoly sometimes draws polygon slightly larger, and those fully opaque pixels are hiding antialiased pixels from aalines
So I made separate, modified, draw_fillpoly_for_aapolygon that makes borders slightly thinner at those problematic places.
I didn't want to change original draw_fillpoly: to not add extra arguments and if-s inside loops, which would make it slower.
draw.aapolygon is added to docs, examples and tests.

Left: draw.polygon+draw.aalines
Right: draw.aapolygon

Sample code

import pygame

points = [[100, 100],
        [200, 160],
        [120, 255],
        [70, 121]]
points2 = [[220, 100],
         [320, 160],
         [240, 255],
         [190, 121]]
pygame.init()
screen = pygame.display.set_mode((400, 400))
clock = pygame.time.Clock()

run = True
while run:
  for event in pygame.event.get():
      if event.type == pygame.QUIT:
          run = False
  screen.fill("black")
  pygame.draw.aalines(screen, "white", True, points)
  pygame.draw.polygon(screen, "white", points)
  pygame.draw.aapolygon(screen, "white", points2)
  pygame.display.flip()
  clock.tick(60)
pygame.quit()

[bilhox]

What about adding a kwarg called antlialiased in pygame.draw.polygon, just like in Font.render, rather than an actual different function ?

[bilhox]

good job

[damusss]

What about adding a kwarg called antlialiased in pygame.draw.polygon, just like in Font.render, rather than an actual different function ?

First of all, font antialiasing should have been an attribute. Secondly, while it would technically be better imo, it would surely be incosistent with the other 3 antialiased draw functions.

[bilhox]

Secondly, while it would technically be better imo, it would surely be incosistent with the other 3 antialiased draw functions.

While we are at it, let's make it a kwarg. I believe ankith or starbuck told us we should make it kwarg now. we're not in C, we're on python, so let's make it more friendly.

[damusss]

While we are at it, let's make it a kwarg. I believe ankith or starbuck told us we should make it kwarg now. we're not in C, we're on python, so let's make it more friendly.

it sounds good. what about aacircle, aaline and aalines?

[damusss]

@bilhox only problem is that aapolygon has filled argument while polygon has width argument. they are not compatible

[bilhox]

okay then I agree with keeping it a different function.

[damusss]

I suppose if this one has no width there must be a specific reason, which I'm curious to know

[bilhox]

I suppose if this one has no width there must be a specific reason, which I'm curious to know

Currently there is no implementation with antialiased line, however, I hope the man who made the video I sent on discord will make a video for antialiased line with width.

[mzivic7]

Implementing line width is much more complex, because it has to grow inwards. The problem is in finding points inside polygon, and this is not simple thing. There is a whole library for shrinking polygons: https://pypi.org/project/shapely/

I can make draw.aapolygon with draw.aalines, and width will grow in both sides, same as draw.polygon.
But that would leave gaps where endpoints are, this is already happening with draw.polygon.

For draw.aalines, I would make draw.aaline with width arg, and some complicated modifications.

[mzivic7]

Also, note on merging draw.aa<function> into draw.<function>:
This would require almost all draw functions to be merged: line, lines, circle, ellipse, polygon
It will not make much problems in c functions, except they will just get bigger and less readable,
But merging tests (with draw.aaellipse incl)... Almost 9000 LOC, around 300 tests that each needs to be checked, merged and modified.
Then, it will break someones code, because draw.aalines and draw.aaline are old functions. So to keep consistency with them better keep all functions separated, then in the future, they can all be merged with one separate PR.

[bilhox]

Now it works fine ! Good job :)
Before approving it, I would like to see some code refactoring (requested changes).

[MyreMylar]

LGTM 👍

I noticed one small issue in the docs, where they are technically correct - but still wrong.

I tested this locally and it all works well in my tinkering around, tests all pass locally as well. I like the interface chosen, it should neatly replace everything gfxdraw.aapolygon() does and adds filling as well which is great. 👍👍

[damusss]

@mzivic7 I see this version has the filled argument. Do you plan on replacing it with a width, based on your testing, or do you think it's not possible/not worth it/other reason? Just curious, to make sure you intend for the PR to be ready

[bilhox]

Good job for C part ! :)

Just left some minor requested changes concerning docs, and a question.

[bilhox]

Here it would be interesting to refer to ColorLike from pygame.typing, using a link to its paragraph in the docs.

[mzivic7]

It would be better to add this to all functions in docs at once, in one separate PR. Like just one find-and-replace.

[bilhox]

Why it should be in a different PR when it concerns directly aapolygon ?

[mzivic7]

@mzivic7 I see this version has the filled argument. Do you plan on replacing it with a width, based on your testing, or do you think it's not possible/not worth it/other reason? Just curious, to make sure you intend for the PR to be ready

I finished width for draw.aalines but only in python code form. Unfortunately it is very complex and I'm afraid it will be slow.
It has 150LOC, cyclomatic complexity = 14, and there is a ton of math, perhaps someone should review that python code.
For one draw.aalines with width, it calls: x4 draw.aalines, x1 draw.lines, xn to x2n draw.polygon where n is number of points.
If this is acceptable I can do same "upgrade" to draw.lines since it also has gaps on edges (which includes draw.polygon).
If this is not acceptable, then i can make it with draw.aalines and their width arg (which will come soon in another PR), but it will work same as draw.lines and have those gaps on edges.

[bilhox]

Maybe I didn't see correctly, why is the PR reverting the changes of Coordinate to Point ?

[mzivic7]

Maybe I didn't see correctly, why is the PR reverting the changes of Coordinate to Point ?

Forgot to sync all my branches. Sorry.

[MyreMylar]

These are the two tests failing:

  ======================================================================
  FAIL: test_draw_diamond (pygame.tests.draw_test.DrawAAPolygonTest)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-prw312qk\cp38-win_amd64\venv-test\lib\site-packages\pygame\tests\draw_test.py", line 4722, in test_draw_diamond
      self.assertEqual(self.surface.get_at((x, y)), GREEN, msg=str((x, y)))
  AssertionError: Color(255, 0, 0, 255) != Color(0, 255, 0, 255) : (5, 3)
  
  ======================================================================
  FAIL: test_illumine_shape (pygame.tests.draw_test.DrawAAPolygonTest)
  non-regression on pygame-ce issue #328
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-prw312qk\cp38-win_amd64\venv-test\lib\site-packages\pygame\tests\draw_test.py", line 4828, in test_illumine_shape
      self.assertEqual(self.surface.get_at((x, 0)), GREEN)  # upper border
  AssertionError: Color(255, 0, 0, 255) != Color(0, 255, 0, 255)
  
  ----------------------------------------------------------------------

[MyreMylar]

It looks like the change is clipping off the right hand side for this test comparing the draw polygon 'diamond' versus the 'gfxdraw' one:

Test program:

import pygame
import pygame.gfxdraw

pygame.init()
screen = pygame.display.set_mode((640, 480), 0)

white_background = pygame.Surface(screen.get_rect().size)
white_background.fill((255, 255, 255))

running = True

while running:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            running = False

    screen.blit(white_background, (0, 0))

    pygame.draw.aapolygon(
         screen, pygame.Color("#70AA70"), [(1, 3), (3, 5), (5, 3), (3, 1)], filled=False
    )

    pygame.gfxdraw.aapolygon(
        screen, [(11, 13), (13, 15), (15, 13), (13, 11)], pygame.Color("#70AA70")
    )

    pygame.display.flip()


pygame.quit()

[mzivic7]

It looks like the change is clipping off the right hand side for this test comparing the draw polygon 'diamond' versus the 'gfxdraw' one:

Actually its not draw.aapolygon issue. Remember this PR: #2912, I did not test it with 2px long lines. I will fix it soon and make new PR for that.
But still we should wait with merging this PR, until I implement width argument to draw.aalines, so draw.aapolygon will have width too.

[mzivic7]

Ok to fix it I just have to disable my overlapping-aalines-fix when this and/or previous point coordinates are ints.
Should I:

• Open new PR with new branch for this (its just ~15 lines),
• Do it in upcoming PR for draw.aalines width, because it will get significantly modifying
• Or just do it here?

[MyreMylar]

New PR I think.

…

On Sat, 5 Oct 2024, 17:34 Marko Zivic, ***@***.***> wrote: Ok to fix it I just have to disable my overlapping-aalines-fix when this and/or previous point coordinates are ints. Should I: - Open new PR with new branch for this (its just ~15 lines), - Do it in upcoming PR for draw.aalines width, because it will get significantly modifying - or just do it here? — Reply to this email directly, view it on GitHub <#3126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADGDGGUZYWGAF6ENRWUOQATZ2AIIZAVCNFSM6AAAAABPAUPQRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGEYTAOBSGQ> . You are receiving this because your review was requested.Message ID: ***@***.***>

[Starbuck5]

I was told on discord by Mzivic they are planning to modify the algorithm for this, so I am marking as a draft.