pygame-community / pygame-ce

🐍🎮 pygame - Community Edition is a FOSS Python library for multimedia applications (like games). Built on top of the excellent SDL library.
https://pyga.me
939 stars 155 forks source link

Add draw.aapolygon #3126

Open mzivic7 opened 1 month ago

mzivic7 commented 1 month ago

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.

Screenshot_From_2024-09-28_12-44-51 Left: draw.polygon+draw.aalines Right: draw.aapolygon

Sample code ```py 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 commented 1 month ago

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

damusss commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

bilhox commented 1 month ago

okay then I agree with keeping it a different function.

damusss commented 1 month ago

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

bilhox commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

damusss commented 1 month ago

@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

mzivic7 commented 1 month ago

@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 commented 1 month ago

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

mzivic7 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

image

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 commented 1 month ago

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 commented 1 month ago

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

MyreMylar commented 1 month ago

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 https://github.com/pygame-community/pygame-ce/pull/3126#issuecomment-2395110824, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGDGGUZYWGAF6ENRWUOQATZ2AIIZAVCNFSM6AAAAABPAUPQRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGEYTAOBSGQ . You are receiving this because your review was requested.Message ID: @.***>

Starbuck5 commented 1 week ago

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