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

draw.aaline: blend correctly #582

Merged
merged 5 commits into from Oct 28, 2018

Conversation

Projects
None yet
3 participants
@dlon
Copy link
Member

dlon commented Oct 27, 2018

#575

Difference:
0
1

dlon added some commits Oct 27, 2018

@dlon dlon force-pushed the dlon:aaline-blend branch from fa6e8d9 to 53d0177 Oct 27, 2018

@@ -102,10 +102,6 @@ aaline(PyObject *self, PyObject *arg)
return NULL;
surf = pgSurface_AsSurface(surfobj);

if (surf->format->BytesPerPixel != 3 && surf->format->BytesPerPixel != 4)

This comment has been minimized.

Copy link
@illume

illume Oct 27, 2018

Member

oh cool. I guess this means it supports more than 32/24 bit now.

This comment has been minimized.

Copy link
@dlon

dlon Oct 27, 2018

Author Member

I think it's always between 1 and 4, but 2 and 1 also work now

@illume

This comment has been minimized.

Copy link
Member

illume commented Oct 27, 2018

Very nice :)

  • I guess we need a test case. Maybe something like what @artifactz wrote in the issue.
  • Are any of the expectedFailures in draw_test fixed by this?
  • using PG_INLINE on those functions might be a good idea (in transform.c, but probably should be moved into _pygame.h)

Here's an unfinished test I started before I realised there are a bunch of expected failure tests which maybe test this already? @e1000 ?

def test_aaline_blend(self):
        surface = Surface(800, 600)
        surface.fill(pg.Color(0, 0, 0))
        pg.draw.aaline(surface, pg.Color(255, 255, 255), (25, 20), (780, 580), 1)
        pg.draw.aaline(surface, pg.Color(255, 255, 255), (20, 80), (780, 20), 1)
        # the expected_color's here need to be fixed.
        for x_y, expected_color in [
            (95, 73), (0, 0, 0, 255),
            (96, 73), (0, 0, 0, 255),
            (97, 73), (0, 0, 0, 255)
        ]:
            self.assertEqual(surface.get(x_y, expected_color)

Modified code to print out color of points where you click the mouse...

import pygame as pg

pg.init()
surface = pg.display.set_mode((20, 20))
clock = pg.time.Clock()
while True:
    for event in pg.event.get():
        if event.type == pg.KEYDOWN:
            if event.key == pg.K_q or event.key == pg.K_ESCAPE:
                exit(0)
        elif event.type == pg.QUIT:
            exit(0)
        if event.type == pg.MOUSEBUTTONDOWN:
            print(event, surface.get_at(event.pos))

    surface.fill(pg.Color(0, 255, 0))

    pg.draw.aaline(surface, pg.Color(255, 255, 255), (2, 2), (18, 18), 1)
    pg.draw.aaline(surface, pg.Color(255, 255, 255), (2, 18), (18, 2), 1)

    pg.display.flip()
    clock.tick(60)
static void
set_pixel_32(Uint8 *pixels, SDL_PixelFormat *format, Uint32 pixel)
{
switch (format->BytesPerPixel) {

This comment has been minimized.

Copy link
@e1000

e1000 Oct 27, 2018

Cool! It looks similar to the code in drawline now, but why it is much more simple here? Can we use this code in drawline ?

This comment has been minimized.

Copy link
@dlon

dlon Oct 27, 2018

Author Member

In drawline, the line is drawn for each case inside the switch statement, probably because it might be faster.

br * colorptr[1], \
br * colorptr[2], \
br * colorptr[3])); \
}}

This comment has been minimized.

Copy link
@e1000

e1000 Oct 27, 2018

maybe @illume would like to apply some clang-format ? ;)

@e1000

This comment has been minimized.

Copy link

e1000 commented Oct 27, 2018

about the tests, it might be that test_short_line_anti_aliasing passes with this;
test_short_non_antialiased_lines should pass except the diagonal I guess.

the others are about not going well to the border (algorithm is cutting one pixel all around the rect);
and a test about float coordinates that must be fixed after we clearly defined this case (cf #522)

@illume : I don't understand your test test_aaline_blend ; why do you test that some pixel are black?

@dlon

This comment has been minimized.

Copy link
Member Author

dlon commented Oct 27, 2018

Some measurements:

Not inlined:
18985 calls/s
Inlined:
18854 calls/s
Replaced trunc
23240 calls/s
Replaced trunc + inlined:
22826 calls/s

Inlining seems to make little difference when I try it, but replacing trunc() made a difference.

Also, the code for big endian might be wrong.

Tests still look like this:

loading test.draw_test
xxxx...........xxxx......

@illume illume merged commit 8c4bbd3 into pygame:master Oct 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illume illume referenced this pull request Oct 28, 2018

Merged

Aaline blend test #585

@e1000

This comment has been minimized.

Copy link

e1000 commented Oct 30, 2018

  • x is for expected Failure,
  • u is for unexpected Success,

so it seems, no unexpected success :-/ I will investigate that "ASAP"...

@dlon dlon deleted the dlon:aaline-blend branch Oct 30, 2018

@dlon dlon referenced this pull request Mar 23, 2019

Closed

1.9.5 release notes. #561

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.