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

SDL 1 CI build fixes #1832

Merged
merged 8 commits into from May 22, 2020
Merged

SDL 1 CI build fixes #1832

merged 8 commits into from May 22, 2020

Conversation

MyreMylar
Copy link
Contributor

This is a grab bag of small fixes to try and get the SDL 1 build building and passing tests again on Travis CI.

There is more detail in this draft PR where I discovered most of these errors in the process of trying to write some tests for the overlay module:

#1830

// sse2neon.h is from here: https://github.com/DLTcollab/sse2neon
#include "include/sse2neon.h"
#else
#if IS_SDLv1
Copy link
Member

Choose a reason for hiding this comment

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

👍

@MyreMylar
Copy link
Contributor Author

MyreMylar commented May 21, 2020

Looks like two of the new mask tests are also failing on SDL 1, is that because of this bit of code perhaps:

pygame/src_c/mask.c

Lines 2072 to 2076 in f439234

#if IS_SDLv1
SDL_SRCALPHA,
#else
PGS_SRCALPHA,
#endif

Maybe @charlesej will know.

EDIT: The failed SDL 1 tests:

======================================================================
FAIL: test_to_surface__different_srcalphas (pygame.tests.mask_test.MaskTypeTest)
Ensures an exception is raised when surfaces have different SRCALPHA
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/pygame-2.0.0.dev9-py3.5-linux-x86_64.egg/pygame/tests/mask_test.py", line 4573, in test_to_surface__different_srcalphas
    mask.to_surface(surface, setsurface, unsetsurface)
AssertionError: ValueError not raised
======================================================================
FAIL: test_to_surface__different_srcalphas_with_created_surfaces (pygame.tests.mask_test.MaskTypeTest)
Ensures an exception is raised when surfaces have different SRCALPHA
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.5.6/lib/python3.5/site-packages/pygame-2.0.0.dev9-py3.5-linux-x86_64.egg/pygame/tests/mask_test.py", line 4595, in test_to_surface__different_srcalphas_with_created_surfaces
    mask.to_surface(setsurface=setsurface, unsetsurface=unsetsurface)
AssertionError: ValueError not raised
----------------------------------------------------------------------

@charlesej
Copy link
Contributor

@MyreMylar I think there needs to be a check added to the check_surface_pixel_format function for SDL 1.

pygame/src_c/mask.c

Lines 2023 to 2036 in f439234

static int
check_surface_pixel_format(SDL_Surface *surf, SDL_Surface *check_surf)
{
if ((surf->format->BytesPerPixel != check_surf->format->BytesPerPixel) ||
(surf->format->BitsPerPixel != check_surf->format->BitsPerPixel)
#if IS_SDLv2
|| (surf->format->format != check_surf->format->format)
#endif
) {
return 0;
}
return 1;
}

Something like:

static int
check_surface_pixel_format(SDL_Surface *surf, SDL_Surface *check_surf)
{
    if ((surf->format->BytesPerPixel != check_surf->format->BytesPerPixel) ||
        (surf->format->BitsPerPixel != check_surf->format->BitsPerPixel)
#if IS_SDLv2
        || (surf->format->format != check_surf->format->format)
#else
        || ((surf->flags & SDL_SRCALPHA) != (check_surf->flags & SDL_SRCALPHA))
#endif
    ) {
        return 0;
    }

    return 1;
}

@MyreMylar MyreMylar marked this pull request as ready for review May 22, 2020 09:03
@MyreMylar
Copy link
Contributor Author

Hooray, with that last fix from Charles it all runs. Probably should merge it in quick before we all break it again in every other PR 🍻 🤦

dst_has_alpha=False,
))

# These tests trigger some of the weird SDL1 alpha blending behaviour
Copy link
Member

Choose a reason for hiding this comment

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

It makes me a little nervous that these aren't correct on SDL1. Are the tests correct, or is SDL1 incorrect? Has the behaviour changed in these blit routines now? Do the SDL2 ones not match the SDL1 ones?

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need a response necessarily :) I was just thinking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you are right to ask I'm definitely going to poke at it and see if I can figure out why this works differently.

I mean in theory you don't need to do pre-multiplied alpha blending in any of those cases because both surfaces don't have alpha but I believe I would expect it to just work boringly without going through the fancy intrinsic code....

Anyway, it just didn't seem the moment to go off on a tangent figuring that out in this PR.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

👍 thanks, and sorry I messed this up before.

@illume illume merged commit 1737ab2 into pygame:master May 22, 2020
@MyreMylar MyreMylar deleted the sdl1-build-fixes branch June 5, 2020 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants