pygame-ce
pygame-ce copied to clipboard
`pygame.sprite.Group.has()` method behavior for no sprites given
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))
To support his idea, It's like saying the empty set is not included in any other set, which is definitively not correct.
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.)
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?
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.