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
926 stars 154 forks source link

Add `index_from` argument to `collidelistall` methods #2917

Open itzpr3d4t0r opened 5 months ago

itzpr3d4t0r commented 5 months ago

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)
Starbuck5 commented 4 months ago

Isn't this what collideobjectsall is supposed to do?

itzpr3d4t0r commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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?