Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix aalines overlap #2912

Merged
merged 6 commits into from
Oct 3, 2024
Merged

Conversation

mzivic7
Copy link
Contributor

@mzivic7 mzivic7 commented Jun 6, 2024

As seen on the picture bellow, there is a problem with draw.aalines.
bad
This is happening because draw.aalines require each aaline to be drawn 1px shorter (in some cases 2px - see bellow), so two connected draw_aaline are not overlapping.

What is changed and how it works:

draw_aaline has 2 endpoints that needs to be disabled for draw.aalines only:
First endpoint is disabled when it is first line when aalines is open.
And when from_x value is not decimal (if it is, then line is 2px shorter).
Second endpoint is disabled when it is last line when aalines is open.

As a result of disabling first endpoint, now there is special case where 1px is missing between two lines, where one is steep and other line is not.
This is detected in draw.aalines.
And resolved in draw_aaline by drawing one px at place where end point should be.
At what end point will be drawn pixel: first point when line is not inverted, second point when line is inverted.

Here are some results (might need to zoom to see the difference better):
results
Top line is modified.
Middle line is original.
Bottom line is difference.

Code used to draw this:
import pygame
import numpy as np

def curve_points(a, b, pea, t):
    sin_p = np.sin(pea)
    cos_p = np.cos(pea)
    x = a * np.cos(t)
    y = b * np.sin(t)
    x_rot = x * cos_p - y * sin_p
    y_rot = y * cos_p + x * sin_p
    return np.stack((x_rot, y_rot), axis=1)

points1 = [[200.1, 300],
           [220.1, 300.1],
           [240.1, 300.2],
           [260.1, 300.4],
           [280.1, 300.6],
           [300.1, 300.8]]

points2 = [[400.7, 300.1],
           [420.3, 300.5],
           [440.6, 305.1],
           [460.4, 315.2],
           [480.2, 330.5],
           [500.1, 360.7],
           [550.4, 360.5],
           [550.8, 400.2]]

points_num = 150
t = np.linspace(-np.pi, np.pi, points_num)
points3 = curve_points(250, 150, 0.3, t) + np.array([350, 350])

pygame.init()
screen = pygame.display.set_mode((700, 700))
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, (150, 150, 150), False, points1)
    pygame.draw.aalines(screen, (150, 150, 150), False, points2)
    pygame.draw.aalines(screen, (150, 150, 150), False, points3)
    pygame.display.flip()
    clock.tick(60)
pygame.quit()

Results were merged for comparison in gimp.

mzivic7 added 2 commits June 5, 2024 19:52
Pixel was missing between steep and not-steep line, when line is 
inverted
@mzivic7 mzivic7 requested a review from a team as a code owner June 6, 2024 00:53
@ankith26 ankith26 added draw pygame.draw bugfix PR that fixes bug labels Jun 6, 2024
src_c/draw.c Outdated Show resolved Hide resolved
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is asking for something that won't be fun, but I'd like to see some regression test(s) put in place that demonstrate this change, and will flag if the change gets broken by a future commit

- 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)
@mzivic7 mzivic7 requested a review from oddbookworm June 19, 2024 22:23
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nitpick (that applies to both tests), but otherwise this looks good to me!

test/draw_test.py Outdated Show resolved Hide resolved
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still looks good to me

@mzivic7 mzivic7 mentioned this pull request Jul 19, 2024
19 tasks
@bilhox bilhox added this to the 2.5.2 milestone Aug 28, 2024
mzivic7 and others added 2 commits September 5, 2024 16:03
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
@bilhox
Copy link
Contributor

bilhox commented Sep 14, 2024

So I tried to understand how the algorithm works, but even after a lot of doliprane, I couldn't understand the algorithm because of lack of graphical explanations. I believe it would be better if @mzivic7 or anyone who understood the algorithm, make a video that explains step by step the algorithm for the sake of others mental health. Like that we can push this PR faster to its merge.

Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, LGTM 👍

Output looks a lot better (Once I zoomed in, my old eyes couldn't see shit at normal resolution) and the changes make sense to me to reduce the overlapping in the original algorithm.

@MyreMylar MyreMylar merged commit 9ef9855 into pygame-community:main Oct 3, 2024
26 checks passed
@mzivic7 mzivic7 deleted the fix_aalines_overlap branch October 3, 2024 21:49
@mzivic7 mzivic7 mentioned this pull request Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug draw pygame.draw
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants