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

pygame.sprite.Group.has() method behavior for no sprites given #2993

Open
aatle opened this issue Jul 14, 2024 · 4 comments · May be fixed by #2998
Open

pygame.sprite.Group.has() method behavior for no sprites given #2993

aatle opened this issue Jul 14, 2024 · 4 comments · May be fixed by #2998
Labels
bug Not working as intended pygame-ce 3.0 sdl3

Comments

@aatle
Copy link
Contributor

aatle commented Jul 14, 2024

Environment:
Platform:               Windows-11-10.0.22631-SP0
System:                 Windows
System Version:         10.0.22631
Processor:              Intel64 Family 6 Model 167 Stepping 1, GenuineIntel
Architecture:           Bits: 64bit     Linkage: WindowsPE

Python:                 CPython 3.12.4 (tags/v3.12.4:8e8a4ba, Jun  6 2024, 19:30:16) [MSC v.1940 64 bit (AMD64)]
pygame version:         2.5.0
SDL versions:           Linked: 2.30.3  Compiled: 2.30.3
SDL Mixer versions:     Linked: 2.8.0   Compiled: 2.8.0
SDL Font versions:      Linked: 2.22.0  Compiled: 2.22.0
SDL Image versions:     Linked: 2.8.2   Compiled: 2.8.2
Freetype versions:      Linked: 2.11.1  Compiled: 2.11.1

Display Driver:         Display Not Initialized
Mixer Driver:           Mixer Not Initialized

Current behavior:

According to the documentation, pygame.sprite.Group.has() (and __contains__) will return True if all given sprites are contained in the group, False otherwise.
However, for the edge case of no sprites given (or equivalently, empty iterable), this method incorrectly returns False. This is probably what was intended, but is wrong.

In has() method:

pygame-ce/src_py/sprite.py

Lines 521 to 522 in cdc2756

if not sprites:
return False # return False if no sprites passed in

Expected behavior:

The has() method should return True if no sprites (no arguments, empty group, etc.) are given. This is similar to the built-in all() function.
The technical term is "vacuous truth". If you need more explanation, ask me.

Steps to reproduce:

import pygame
group = pygame.sprite.Group()
print('has():', group.has(), group.has([]), group.has(group))
@aatle aatle added the bug Not working as intended label Jul 14, 2024
@bilhox
Copy link
Contributor

bilhox commented Jul 15, 2024

To support his idea, It's like saying the empty set is not included in any other set, which is definitively not correct.

@aatle
Copy link
Contributor Author

aatle commented Jul 16, 2024

One example I can think of currently:
Say the programmer has two groups, all_sprites and enemy_sprites. They want to make sure that they haven't forgotten to add any enemy sprites to all_sprites group, so they add this code:

assert all_sprites.has(enemy_sprites)

Now, if the code is working properly, this should never fail.
However, if all enemies happen to die (leaving enemy_sprites empty), this check will fail unexpectedly. It should pass because all enemies (none exist) are in all_sprites technically and logically.

There's probably better examples, but the point is that there is rarely a case where you want the behavior to be to return False, because it is logically incorrect.
At least, the behavior for no sprites should be documented to avoid confusion.

(I personally do not use pygame.sprite module, but I was looking through the source code and noticed this.)

@oddbookworm
Copy link
Member

Hmmmmm, I'm personally for this change on a logical basis, but this behavior is now 4 years old (this particular change authored by @MyreMylar). I'm curious if Myre remembers the reason for doing so in the first place, I couldn't find any clues in the pull request. But, it would be a pretty big compat break with upstream pygame, so I think it needs to have a solid motivation and strong support to change it. Potential candidate for "small changes" for pygame-ce 3.0.0?

@MyreMylar
Copy link
Member

Hmmmmm, I'm personally for this change on a logical basis, but this behavior is now 4 years old (this particular change authored by @MyreMylar). I'm curious if Myre remembers the reason for doing so in the first place, I couldn't find any clues in the pull request. But, it would be a pretty big compat break with upstream pygame, so I think it needs to have a solid motivation and strong support to change it. Potential candidate for "small changes" for pygame-ce 3.0.0?

I just went a looked through the blame, but my PR there just retained the existing behaviour of returning False when no sprites were passed in to the function. This was the previous version before I edited it:

    def has(self, *sprites):
        """ask if group has a sprite or sprites
        Group.has(sprite or group, ...): return bool
        Returns True if the given sprite or sprites are contained in the
        group. Alternatively, you can get the same information using the
        'in' operator, e.g. 'sprite in group', 'subgroup in group'.
        """
        return_value = False

        for sprite in sprites:
            if isinstance(sprite, Sprite):
                # Check for Sprite instance's membership in this group
                if self.has_internal(sprite):
                    return_value = True
                else:
                    return False
            else:
                try:
                    if self.has(*sprite):
                        return_value = True
                    else:
                        return False
                except (TypeError, AttributeError):
                    if hasattr(sprite, '_spritegroup'):
                        for spr in sprite.sprites():
                            if self.has_internal(spr):
                                return_value = True
                            else:
                                return False
                    else:
                        if self.has_internal(sprite):
                            return_value = True
                        else:
                            return False

        return return_value

Anyway, I agree this behaviour change makes sense - but it would be a backwards compatibility break. I think the current described behaviour of the .has() function is pretty much identical to the .issubset() functionality of a python set, and if you check how that works:

my_set = {1, 2, 3}

if {1, 2}.issubset(my_set):
    print("my_set has 1, 2")

if {3, 4}.issubset(my_set):
    print("my_set has 3, 4")

if set().issubset(my_set):
    print("my_set has the empty set")

Output:

my_set has 1, 2
my_set has the empty set

I agree this is the sort of small thing that should be rolled into the 'compatibility break' release. It probably won't affect many people, but it might break a handful of older games wo happen to be relying on the current behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Not working as intended pygame-ce 3.0 sdl3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants