Skip to content

Conversation

@aatle
Copy link
Member

@aatle aatle commented Nov 3, 2024

The current sprite collision functions code has some code repetition. This implementation structures it differently, which has a few effects.

  • If dokill is true in spritecollide or groupcollide, the sprites are killed after all colliding sprites have been determined, instead of during iteration (should be unnoticeable)
  • As a consequence of the previous thing, in these cases, iteration over group.sprites() is replaced by iteration over group itself, resulting in the following:
    • Avoids a list copy of the group, improving performance slightly in around 1/8 of possible cases
    • It is no longer an error to use a non-group iterable of sprites here (already no error if dokill is False)
      • I advocate for typing the sprite collide functions as supporting all iterables of sprites, instead of just sprite groups
    • "Old-style" groups that don't support iteration raise error now here (already errors if dokill is False)
  • It is now easy to implement an ignore_self parameter, e.g., with the new structure

Performance tested in 3.13.0, no regression (similar performance).
Renamed a few local variables for clarity.

@aatle aatle requested a review from a team as a code owner November 3, 2024 03:20
@yunline yunline added Code quality/robustness Code quality and resilience to changes sprite pygame.sprite labels Nov 3, 2024
@Starbuck5
Copy link
Member

Starbuck5 commented Nov 3, 2024

I heard somewhere (can't find a back reference) that the old "pull out a bound function into a local variable" trick is actually slower in the most recent versions of Python, I'd be interested to see you look into it with this case.

EDIT: Found it-- https://youtu.be/z0-4EwIFeJo?si=sTOUIHgumGYGiyZl&t=1112

@aatle
Copy link
Member Author

aatle commented Nov 3, 2024

Thanks. I looked into it and found no noticeable performance increase nor penalty from either using sprite_rect.colliderect instead of sprite_rect_collide or spritecollide global instead of sprite_collide_func local, on python 3.13.0.
The most likely thing is that the impact is very minimal. The talk mentions that for pure python methods, the cached method may still be faster by 3% on 3.11.

Since I saw no performance increase from removing the cached methods/functions, I will leave it in just in case python 3.9 or 3.10 needs it.

@MyreMylar MyreMylar closed this Dec 31, 2024
@MyreMylar MyreMylar reopened this Dec 31, 2024
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

OK, LGTM 👍

Nice simplification. I renamed the 'crashed' variable because it's name didn't make much sense and this was a good opportunity.

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🎉

After looking around for a bit trying to think of any case where doing a kill later would change behaviour I have not come up with anything. So I think this is safe to merge

@ankith26 ankith26 added this to the 2.5.6 milestone Jun 7, 2025
@ankith26 ankith26 merged commit ab1e929 into pygame-community:main Jun 7, 2025
23 checks passed
@aatle aatle deleted the sprite-collision branch June 7, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code quality/robustness Code quality and resilience to changes sprite pygame.sprite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants