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
773 stars 120 forks source link

Optimized Rect multi-collision methods #2786

Closed itzpr3d4t0r closed 2 months ago

itzpr3d4t0r commented 3 months ago

This PR swaps a common rect-rect collision function with a simplified version that precalculates some conditions outside the core loop. A side-effect of this PR is also making O(N) cases O(1) where the base rect has 0 width or height.

While this isn't the last step of optimizations for these methods it's still a start and it's a fairly simple change. Future changes will involve using fast sequence looping.

This change affects the following functions:

A graph containing all functions both with and without the change: image

Starbuck5 commented 3 months ago

This is a clever idea for an optimization, nice!

Idea: would it compile better if the argrect 0 length check was embedded into the main boolean logic for whether a rect collided or not? This would be the same logic but it might be easier for the compiler to optimize?

Starbuck5 commented 2 months ago

@itzpr3d4t0r what do you think about putting the argrect 0 check into the macro? It would reduce a lot of code repetition in this PR.

This is my only reservation about approving this PR, that it could be improved in this way. Otherwise it looks good.

itzpr3d4t0r commented 2 months ago

Idea: would it compile better if the argrect 0 length check was embedded into the main boolean logic for whether a rect collided or not? This would be the same logic but it might be easier for the compiler to optimize?

Thanks for the suggestion, I've tried this but I didn't see any speedup, infact it may have been a ltl slower even.

what do you think about putting the argrect 0 check into the macro? It would reduce a lot of code repetition in this PR.

Do you have any idea on how? Because the macro rn is basically an inline bool expression that needs to be fed to an if statement, and each if statement has a different body depending on the function so I find it hard to imagine a way to implement what you're asking, only way would be to add another macro just for that but it would only save 2 lines of code.

Starbuck5 commented 2 months ago

Those two ideas you quoted were the same idea, but framed differently.

#define OPTIMIZED_COLLIDERECT(r)                      \
    (r->w && r->h && left < MAX(r->x, r->x + r->w) && \
     top < MAX(r->y, r->y + r->h) &&                  \
     right > MIN(r->x, r->x + r->w) && \ bottom > MIN(r->y, r->y + r->h))

Right?

It would also need a comment explaining why in there. But that seems better to me than having a required bit of code outside the colliderect macro that's necessary for the macro to give correct results.