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

Remove and deprecate cythonised sprite module #2546

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Conversation

MyreMylar
Copy link
Member

fixes #2342

Motivation:
After PR #2533 the normal sprite submodule's Group.draw() call is faster than the current cythonised sprite submodule, having a second cython-version of the sprite submodule is an additional pain to maintain. The general approach we are transitioning towards in pygame-ce is to use the C extension API where it helps with performance and keep other stuff in pure python where we can leverage the performance work of the CPython team - this is going to mean the gradual elimination of the current middle layer that is our Cython code.

Maintenance is the biggest hurdle in keeping most open source projects going and hopefully this is one tiny step in making pygame-ce a little bit easier to mainatain.

@MyreMylar MyreMylar added the sprite pygame.sprite label Nov 4, 2023
@MyreMylar MyreMylar requested a review from a team as a code owner November 4, 2023 15:18
@MyreMylar
Copy link
Member Author

MyreMylar commented Nov 4, 2023

This was the performance test code I was using:

import timeit

time_taken = timeit.timeit(
    "draw_sprites(display_surf, test_sprite_group)",
    setup="import pygame\n\n"
    "pygame.init()\n\n"
    "display_surf = pygame.display.set_mode((800, 600))\n\n"
    "test_sprite_group = pygame.sprite.Group()\n\n"
    "for i in range(300):\n"
    "   test_sprite = pygame.sprite.Sprite(test_sprite_group)\n"
    "   test_sprite.image = pygame.Surface((64,64))\n"
    "   test_sprite.image.fill('red')\n"
    "   test_sprite.rect = pygame.Rect((i*2,i*2), (64, 64))\n"
    "def draw_sprites(display_surface, sprite_group):\n"
    "    display_surface.fill((255, 255, 255))\n"
    "    sprite_group.draw(display_surface)\n"
    "    pygame.display.flip()\n",
    number=1000,
)
print("time taken to do 1000 warm-up sprite draws (don't count these as perf takes a while to warm up)", time_taken)


time_taken = timeit.timeit(
    "draw_sprites(display_surf, test_sprite_group)",
    setup="import pygame\n"
    "import pygame._sprite\n\n"
    "pygame.init()\n\n"
    "display_surf = pygame.display.set_mode((800, 600))\n\n"
    "test_sprite_group = pygame._sprite.Group()\n\n"
    "for i in range(300):\n"
    "   test_sprite = pygame._sprite.Sprite(test_sprite_group)\n"
    "   test_sprite.image = pygame.Surface((64,64))\n"
    "   test_sprite.image.fill('red')\n"
    "   test_sprite.rect = pygame.Rect((i*2,i*2), (64, 64))\n"
    "def draw_sprites(display_surface, sprite_group):\n"
    "    display_surface.fill((255, 255, 255))\n"
    "    sprite_group.draw(display_surface)\n"
    "    pygame.display.flip()\n",
    number=1000,
)
print("time taken to do 1000 cython sprite draws", time_taken)

time_taken = timeit.timeit(
    "draw_sprites(display_surf, test_sprite_group)",
    setup="import pygame\n\n"
    "pygame.init()\n\n"
    "display_surf = pygame.display.set_mode((800, 600))\n\n"
    "test_sprite_group = pygame.sprite.Group()\n\n"
    "for i in range(300):\n"
    "   test_sprite = pygame.sprite.Sprite(test_sprite_group)\n"
    "   test_sprite.image = pygame.Surface((64,64))\n"
    "   test_sprite.image.fill('red')\n"
    "   test_sprite.rect = pygame.Rect((i*2,i*2), (64, 64))\n"
    "def draw_sprites(display_surface, sprite_group):\n"
    "    display_surface.fill((255, 255, 255))\n"
    "    sprite_group.draw(display_surface)\n"
    "    pygame.display.flip()\n",
    number=1000,
)
print("time taken to do 1000 sprite draws", time_taken)

On current main the cython sprite module slightly outperforms the normal sprite module in drawing, but when I try it with the newly optimised sprite module in #2533 the normal sprite is now better than cython sprite.

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.

I'm good to see this removed, I like that you've justified this with performance and maintainability, and I concur.

@Starbuck5
Copy link
Member

Another bonus of this is that it makes the installed wheel smaller since it doesn't have the _sprite object. For me it's a 300kb difference.

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.

LGTM, thanks for the PR! Seeing 44k lines being removed is always a nice feeling

@ankith26 ankith26 added this to the 2.4.0 milestone Nov 6, 2023
@ankith26 ankith26 merged commit 55180bb into main Nov 6, 2023
30 checks passed
@ankith26 ankith26 deleted the deprecate_cython_sprite branch November 6, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate pygame.sprite to C (and/or remove the Cythonized sprite)?
4 participants