Fix pygame.draw.arc

[cyuria]

Fix a rendering bug with pygame.draw.arc

This bug already has a pull request open (#1981), however there has been no activity on that PR for four months (except for a comment asking about its revival a few days ago).

I used the following sources for testing code: pygame/pygame#3020 (comment) as well as #1981 (review)

I intend to make a few more performance based commits, as there are a number of different ways I can think of to significantly improve performance.
I figured I should open the PR now as I have a working solution and I figured it would be good to do so at this point.

EDIT:

I have finished making performance improvements and am now just waiting for a review before we can merge this PR. I'd be happy to implement any other changes anyone can think of

[cyuria]

difference:
old:


new:

[cyuria]

I noticed quite quickly that it fails a few tests, which in hindsight I probably should have run before submitting the PR, so I'll do that before anything else

[cyuria]

new tests:

[cyuria]

Could I get someone else's opinion on this?

The reason the tests are failing is because of some differences between the new arc algorithm and the old one when the difference between the start and stop angles is greater than 2 Pi radians, or 360 degrees.

At this point I will probably add a clause to my algorithm to satisfy the tests.

However, I think my algorithm's way of handling the edge case could be preferable if documented. Here's a comparison:
Old:

New:

The code I used to test this:

import math
import time

import pygame

pygame.init()
font = pygame.font.Font(size=32)

def drawarc(screen):
    t = time.time() % (4 * math.pi)
    screen.fill((0, 0, 0))
    pygame.draw.arc(screen, pygame.Color('white'), (50, 50, 300, 300), 0, t, 50)
    screen.blit(font.render(str(round(t / math.pi, 1)) + ' Pi radians', True, pygame.Color('white')), (0, 0))
    pygame.display.flip()

def runanim():
    screen = pygame.display.set_mode((400, 400))
    while True:
        event = pygame.event.poll()
        drawarc(screen)
        if event.type == pygame.NOEVENT:
            continue
        if event.type == pygame.QUIT:
            break
        if event.type == pygame.KEYDOWN:
            print(time.time() % (4 * math.pi))
            continue
        if event.type == pygame.MOUSEBUTTONDOWN:
            print(event.pos)
            continue


if __name__ == "__main__":
    runanim()

[oddbookworm]

Haven't dug too deep, but here are a couple comments

[oddbookworm]

The reason the tests are failing is because of some differences between the new arc algorithm and the old one when the difference between the start and stop angles is greater than 2 Pi radians, or 360 degrees.

At this point I will probably add a clause to my algorithm to satisfy the tests.

Generally, we try to avoid changes that might break someone's project. I also would like feedback from other reviewers about which behavior should be the "correct" behavior. IMO if there is a difference of more than 2pi radians, then a full circle should be drawn because it has important information in some cases. I could potentially be persuaded otherwise though

[oddbookworm]

In particular, @MightyJosip is our resident draw expert iirc (maybe @zoldalma999 too?). But one of our steering committee should probably chime in about the behavior diff

[cyuria]

I've been working a bit more and I believe I have run into a place where the old algorithm (as well as one of the tests) are either inconsistent with the docs or just broken, if I fix the start angle at -17 radians and the stop angle at -5.5, (the only currently failing test), the following happens:

Unless I am making a mistake, the documentation says otherwise:

pygame-ce/docs/reST/ref/draw.rst

Lines 270 to 278 in 6ebeb19

	| if ``start_angle < stop_angle``, the arc is drawn in a
	counterclockwise direction from the ``start_angle`` to the
	``stop_angle``
	| if ``start_angle > stop_angle``, tau (tau == 2 * pi) will be added
	to the ``stop_angle``, if the resulting stop angle value is greater
	than the ``start_angle`` the above ``start_angle < stop_angle`` case
	applies, otherwise nothing will be drawn
	| if ``start_angle == stop_angle``, nothing will be drawn
	|

[cyuria]

wait nvm I'm an idiot

[cyuria]

seeing as all tests passed, I started working a bit on performance, using the following benchmark script:

import math
import time

import pygame

def drawarc(surface, t):
    pygame.draw.arc(surface, pygame.Color('white'), (0, 0, 1000, 1000), 0, t, 300)

def runbench():
    screen = pygame.display.set_mode((50, 50))
    surface = pygame.Surface((1000, 1000))
    starttime = time.time_ns()

    for c in range(100):
        for i in range(200):
            drawarc(surface, i * math.pi / 100)

        # give the user some kind of feedback
        screen.fill((256 * c // 100,)*3)
        pygame.display.flip()

    endtime = time.time_ns()

    print(f"starttime:  {starttime / 1000000000}s")
    print(f"endtime:    {endtime / 1000000000}s")
    print(f"difference: {(endtime - starttime) / 1000000000}s")

if __name__ == "__main__":
    runbench()

The following are my results for the entire test

algorithm	time
old	47s
new (1)	69s
new (2)	54s

(1) the first full implementation which passed all tests (commit a9522a6)
(2) with bounds checking before running the algorithm (commit a31b616)

It is also possible to visualize the bounds checking by adding the following code to the draw_arc function anywhere after the minima and maxima are calculated:

    draw_line(surf, minx + x_center, miny + y_center,
              minx + x_center, maxy + y_center, color, drawn_area);
    draw_line(surf, minx + x_center, maxy + y_center,
              maxx + x_center, maxy + y_center, color, drawn_area);
    draw_line(surf, maxx + x_center, maxy + y_center,
              maxx + x_center, miny + y_center, color, drawn_area);
    draw_line(surf, maxx + x_center, miny + y_center,
              minx + x_center, miny + y_center, color, drawn_area);

doing so gives the following result on my machine with the animated test program (albeit with red for the bounding box lines)

There are still two more performance improvements I can think of which I will try to implement soon.

• reducing the amount of per pixel checking by using alternate methods to calculate the easier parts of the arc
• directly setting the pixels instead of updating drawn area and using the bounding box given values to calculate the drawn area

[cyuria]

The latest performance enhancements are documented in the two above commits. Here is the updated benchmark table:

algorithm	time
old	47s
new (1)	69s
new (2)	54s
new (3)	48s
new (4)	35s

(1) the first full implementation which passed all tests (commit a9522a6)
(2) with bounds checking before running the algorithm (commit a31b616)

(3) Use the calculated bounding box to obtain the drawn_area instead of calculating it for each drawn pixel with set_and_check_rect (commit 2e35853) -- NOTE this commit fails the draw arc bounding box test due to an out by one error on some arcs, will be fixed in the next commit
(4) Bypass set_at to avoid checking surface clip rect and check it when calculating the bounding box instead (commit bef7f71) -- NOTE this commit fails the draw arc clip rect box due to an out by one error, will be fixed in the next commit

[Starbuck5]

So this takes the old algorithm from mostly unusable (because of the holes) to twice as fast? Amazing!

Cyuria, are you on the pygame community discord?

[cyuria]

Cyuria, are you on the pygame community discord?

I am now :)

Also there are a few weird interactions with the test edge cases which I'm still trying to fix, currently zero width rects are doing weird stuff

[oddbookworm]

I am now :)

You should check out the contributing channel under the "development" category :)

[cyuria]

I fixed the test cases, here's the updated performance table:

algorithm	time
old	47s
new (1)	69s
new (2)	54s
new (3)	48s
new (4)	35s
new (5)	36s

(5) up to the latest commit with test cases (commit f77a891)

The timings can vary pretty badly, I ran some of the tests a few times and got up to a few seconds difference for each run, so don't necessarily trust the extra +1s (although there definitely should be slightly lower performance because I'm running quite a few extra checks, especially on arcs which are not fully inside the clipping rect of the drawn surface)

[cyuria]

I think I'm done with this PR unless someone has anything else to say, I still need a review.

The performance table for each commit is as follows:

algorithm	time
old	47s
new (1)	69s
new (2)	54s
new (3)	48s
new (5)	36s
new (6)	30s

(1) the first full implementation which passed all tests (commit a9522a6)

(2) with bounds checking before running the algorithm (commit a31b616)

(3) Use the calculated bounding box to obtain the drawn_area instead of calculating it for each drawn pixel with set_and_check_rect (commit 2e35853) -- NOTE fails some tests, was fixed in the commit after

(5) Bypass set_at to avoid checking surface clip rect and check it when calculating the bounding box instead (commit bef7f71) -- Fixes issues with previous commit

(6) commit a7b3cd3, redo algorithm somewhat to use faster circle line drawing techniques

[itzpr3d4t0r]

I suppose there should be some tests to check the behaviour stays fixed in the future, or do we already have those elsewhere?

[cyuria]

I suppose there should be some tests to check the behaviour stays fixed in the future, or do we already have those elsewhere?

If the behaviour ever somehow gets reverted to the "loading screen" behaviour, a couple of the tests will fail because they assume that arcs of length 5.5 and 10 iirc are full circles. The relevant sections of the code are also commented, saying which section of the docs they relate to.

I don't think it's a problem but if you want I can add some tests.

[MyreMylar]

Overall this looks good to me. It passes the unit tests locally, seems to have fixed the original issue, runs faster and passes all the test cases I could come up with fiddling around with the function.

I added some minor comment & spelling things you could fix but nothing that should stop this from being merged. Whoever does merge this - don't forget to also close #1981 that this supercedes.

[dr0id]

LGTM, all tests pass as they are! Thanks for the PR.

[Starbuck5]

I checked a bunch of before/after behavior on possible edge cases locally, and this got a positive note from JoKing (our draw expert) on discord, so let's go ahead and merge it.

[Starbuck5]

I did a squash and merge since there were so many commits on the branch, to keep main a bit more tidy.

Great work on this @cyuria, this was an impressive PR!