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

Add index_from argument to collidelistall methods #2917

Open
itzpr3d4t0r opened this issue Jun 9, 2024 · 4 comments
Open

Add index_from argument to collidelistall methods #2917

itzpr3d4t0r opened this issue Jun 9, 2024 · 4 comments
Labels
enhancement New API This pull request may need extra debate as it adds a new class or function to pygame Performance Related to the speed or resource usage of the project rect pygame.rect

Comments

@itzpr3d4t0r
Copy link
Member

itzpr3d4t0r commented Jun 9, 2024

Currently the Rect's (and soon Circle's) collidelistall methods return a list of indices of colliding objects. While this is very useful as an optimization over raw python looping (and generally over any other of our multicollision methods) I believe there's still a fundamental improvement still up for grabs here.

What's usually done after calling collidelistall() is to immediately use the returned indices list to index into another container of any kind of object, usually the ones with corresponding rect object as collider.

What I propose is to add an optional argument called index_from which saves the user from manually indexing the container again after the call and saves extra performance (about 40% from what I've tested internally). This would make code more performant, readable and compact.

class MyObj:
    def __init__(...):
        self.r = Rect(...)


my_objects = [ (MyObj) ... ]

# old
colliding_objects = [my_objects[i] for i in r.collidelistall([obj.r for obj in my_objects])]

# proposed
colliding_objects  = r.collidelistall([obj.r for obj in my_objects], my_objects)
@itzpr3d4t0r itzpr3d4t0r added Performance Related to the speed or resource usage of the project enhancement New API This pull request may need extra debate as it adds a new class or function to pygame rect pygame.rect labels Jun 9, 2024
@Starbuck5
Copy link
Member

Isn't this what collideobjectsall is supposed to do?

@itzpr3d4t0r
Copy link
Member Author

Yep but this is way faster than that. It would also allow us to reuse this kwarg in Circle/Line and Polygon in the future to avoid having to implement the collideobjectsall method.

@Starbuck5
Copy link
Member

Yep but this is way faster than that

Does this need to be fast? Are people actually running into this as a significant problem in games?

What if you used collideobjectsall without the key= param and had a .rect attribute instead of a .r attribute? I mean I understand it would still be slower but "way slower" ?

I think if this really needs to be optimized, I'd rather have a new collide method that implements this from the get-go than making it an optional part of collidelistall. For the same reason as I'm pushing for premul_alpha_ip instead of premul_alpha(inplace=True), because I want the return value of a function to be consistent between args.

@Starbuck5
Copy link
Member

Another thing I just thought of about why function returns should be consistent that's more pragmatic than philosophical--
How would this idea by type hinted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New API This pull request may need extra debate as it adds a new class or function to pygame Performance Related to the speed or resource usage of the project rect pygame.rect
Projects
None yet
Development

No branches or pull requests

2 participants