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

Optimized Rect multi-collision methods #2786

Merged

Conversation

itzpr3d4t0r
Copy link
Member

@itzpr3d4t0r itzpr3d4t0r commented Apr 1, 2024

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:

  • collidelist (40% faster)
  • collidelistall (20-50% faster)
  • collideobjects ( 6-10% faster)
  • collideobjectsall (6-8% faster)
  • collidedict (17% faster)
  • collidedictall (5-15% faster)

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

@itzpr3d4t0r itzpr3d4t0r added Performance Related to the speed or resource usage of the project rect pygame.rect labels Apr 1, 2024
@itzpr3d4t0r itzpr3d4t0r requested a review from a team as a code owner April 1, 2024 18:02
src_c/rect_impl.h Outdated Show resolved Hide resolved
src_c/rect_impl.h Outdated Show resolved Hide resolved
src_c/rect_impl.h Outdated Show resolved Hide resolved
@Starbuck5
Copy link
Member

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Nice clean speedup.

src_c/rect_impl.h Outdated Show resolved Hide resolved
src_c/rect_impl.h Show resolved Hide resolved
src_c/rect_impl.h Outdated Show resolved Hide resolved
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.

LGTM 👍

@itzpr3d4t0r itzpr3d4t0r added this to the 2.5.0 milestone Apr 21, 2024
@itzpr3d4t0r itzpr3d4t0r merged commit 0209311 into pygame-community:main Apr 21, 2024
39 checks passed
@itzpr3d4t0r itzpr3d4t0r deleted the optimize-rect-multicollision branch April 21, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Related to the speed or resource usage of the project rect pygame.rect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants