pygame-community / pygame-ce

🐍🎮 pygame - Community Edition is a FOSS Python library for multimedia applications (like games). Built on top of the excellent SDL library.
https://pyga.me
814 stars 127 forks source link

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

Open aatle opened 1 month ago

aatle commented 1 month ago
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: https://github.com/pygame-community/pygame-ce/blob/cdc27561f973bd062172c2e4b765e91090f6a8d9/src_py/sprite.py#L521-L522

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))
bilhox commented 1 month ago

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

aatle commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.