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

Switching base sprite group container from dictionary to list #2533

Closed
wants to merge 13 commits into from

Conversation

MyreMylar
Copy link
Member

@MyreMylar MyreMylar commented Oct 27, 2023

Also; only build the draw list when changes are made to the group rather than each frame. This should speed up calls to group draw.

Related to #1941

So how this works at present, is that the base container for sprite groups is a dictionary - but the dictionary functionality is only used in a few of the groups lower down the Group inheritance tree, the simpler groups higher up the tree treat the dictionary like a list. Meanwhile, those same groups lower down the tree that use the dictionary were keeping a list copy of the sprites as well for performance reasons.

What this PR does is swap the base container for sprite Groups to be a list so that all Groups can benefit from the iteration speed of lists. As a result the creation of the actual sprite dictionary, actually used by the three Group's lower in the inheritance tree, is moved down to the RenderUpdates group.

I've also added some properties for spritedict so I could wrap some deprecation warnings in case anyone was direct accessing the dictionary instead of using the documented .sprites() method.

I also changed the strategy for building the list of sprites + rects to draw, previously this list of tuples was constructed on every draw call and had some support in there for a time when blits didn't exist. Instead this list is now only constructed when it has been changed, something that is updated by hooks in the sprite class - which should increase the performance of draw calls.

Hopefully this simplifies the sprite groups a bit overall making them easier to understand for future development.

And adding fblits to sprite
Just to make clear the changes, spritedict was being
treated by the documentation as a private attribute
but, like everything in python it is technically public

I have added deprecation advice on what to use instead for classes where it has been pushed out.
Just to make clear the changes, spritedict was being
treated by the documentation as a private attribute
but, like everything in python it is technically public

I have added deprecation advice on what to use instead for classes where it has been pushed out.
Just to make clear the changes, spritedict was being
treated by the documentation as a private attribute
but, like everything in python it is technically public

I have added deprecation advice on what to use instead for classes where it has been pushed out.
@MyreMylar MyreMylar marked this pull request as ready for review October 27, 2023 10:06
@MyreMylar MyreMylar requested a review from a team as a code owner October 27, 2023 10:06
@MyreMylar
Copy link
Member Author

OK - looks like we are passing the tests now.

I've tried to keep changes as minimal as possible so there are as few frills as possible and the focus is on rejigging the base Group/AbstractGroup's containers to use a list in most cases and a 'drawlist' of a paired surface and rectangle specifically for drawing. This should increase performance of using group.draw() on large sprite groups as we will no longer be building the draw list each frame - only when it changes (new sprites are added or removed, or a sprite's .image or .rect attribute is changed.

Some undocumented functionality of the Group class is also deprecated by this - can't really be helped when making changes to python code, mainly because you can access any attribute you like - even if it is dundered as private or protected. The deprecation warnings should steer users back towards documented code. I didn't make any documentation changes for this as the fact you could access 'under the hood' attributes was not documented in the first place.

I feel like we should be dundering more of these undocumented Group attributes - but I wanted to keep this PR as focused as possible.

Please let me know what you think.

Copy link
Member

@zoldalma999 zoldalma999 left a comment

Choose a reason for hiding this comment

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

The new code makes more sense to me, and it also is more performant than the previous.
Maybe a note about should_rebuild_draw_list should be added somewhere in the docs? Or dunder it if we are so sure no one should have to touch it?

@MyreMylar
Copy link
Member Author

MyreMylar commented May 31, 2024

The new code makes more sense to me, and it also is more performant than the previous. Maybe a note about should_rebuild_draw_list should be added somewhere in the docs? Or dunder it if we are so sure no one should have to touch it?

Can't dunder it (well you can, because python lets you break all it's own rules) as it is something the sprites also set on the groups. I mean we could add a public method for the sprites, but that would have the same issue. Sadly the concept of 'friend' class relationships like this doesn't really exist in python.

@MyreMylar MyreMylar requested a review from a team as a code owner June 9, 2024 06:01
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.

I haven't gotten to reviewing this in detail, because I'm worried about backcompat here.

@MyreMylar
Copy link
Member Author

I haven't gotten to reviewing this in detail, because I'm worried about backcompat here.

This is backwards compatible. There are some deprecation warnings set on external usage of undocumented internal details of the Group classes, but those attributes still work after this PR. I think I will change the deprecation warnings to be more vague on when we might remove the backward compatible layer though.

@MyreMylar
Copy link
Member Author

I tweaked the internal back compatability use case a little so if somebody was setting a dictionary of sprites directly on a sprite Group's internal values, that should now still work too rather than just prompting how to fix.

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.

I'm still somewhat skeptical of this PR.

I believe this changes how say, duplicated elements are added. And checking if a sprite is in a group will now be an O(n) operation not O(1)

@MyreMylar
Copy link
Member Author

I think 100+ lines was probably too many changes at once to get through, I'll try to revisit this PR later in smaller chunks if possible,

@MyreMylar MyreMylar closed this Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sprite pygame.sprite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants