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

0 sized masks are now possible #493

Merged
merged 4 commits into from Aug 20, 2018
Merged

0 sized masks are now possible #493

merged 4 commits into from Aug 20, 2018

Conversation

@augusto2112
Copy link

@augusto2112 augusto2112 commented Aug 17, 2018

Issue: #491

@illume
Copy link
Member

@illume illume commented Aug 18, 2018

Looks like a few builds are failing. You can see them by clicking the "Details" link next to Appveyor and TravisCI.

Here's one issue on a 32bit python: https://ci.appveyor.com/project/pygame/pygame/build/1.0.584/job/wi2o2m3smiuim2wq

======================================================================
ERROR: test_zero_size_from_surface (pygame.tests.mask_test.MaskModuleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python27\lib\site-packages\pygame\tests\mask_test.py", line 460, in test_zero_size_from_surface
    zero_w_mask = pygame.mask.from_surface(pygame.Surface((0, 100)))
ValueError: Cannot create mask with negative size
@@ -467,6 +470,11 @@ static PyObject* mask_from_surface(PyObject* self, PyObject* args)
return NULL;
}

if (surf->w < 0 || surf->h < 0) {

This comment has been minimized.

@illume

illume Aug 18, 2018
Member

This check should be below surf =.

size_t size;

size = offsetof(bitmask_t,bits);
if (w && h) {

This comment has been minimized.

@illume

illume Aug 18, 2018
Member

For the case where w<=0, or h<=0 this check would pass and the size would be a weird amount. But I guess we check that in the mask.c.

@illume
Copy link
Member

@illume illume commented Aug 18, 2018

@augusto2112 I think it's good. Thanks :)

However, we don't have tests for the different methods operating on zero sized masks. If you want to add tests for those I can leave it open? Otherwise I'll merge it in?

@augusto2112
Copy link
Author

@augusto2112 augusto2112 commented Aug 18, 2018

@illume I'll add the tests 😄

@augusto2112
Copy link
Author

@augusto2112 augusto2112 commented Aug 18, 2018

@illume I'll do it tomorrow though because today I won't have enough time. Or you prefer, you can accept it and tomorrow I submit a new PR with the tests.

@augusto2112
Copy link
Author

@augusto2112 augusto2112 commented Aug 20, 2018

@illume I added a bunch of tests. I'm not sure what should happen when trying to scale a (0, 100) mask by (0, 2) for example. Currently it stays (0, 100).


for size in sizes:
mask = pygame.mask.Mask(size)
mask.scale((2, 3))

This comment has been minimized.

@illume

illume Aug 20, 2018
Member

mask.scale should return a new mask with the requested size.
https://www.pygame.org/docs/ref/mask.html#pygame.mask.Mask.scale

So the test should be something like...

scaled_mask = mask.scale((2, 3))
self.assertEqual(scaled_mask.get_size(), (2, 3))
@illume illume merged commit 516d6c4 into pygame:master Aug 20, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@illume illume mentioned this pull request Oct 16, 2018
4 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants