NotificationsYou must be signed in to change notification settings
Fork3.8k
Star8.4k

scale(), scale_by(), smoothscale(), smoothscale_by() reorganization and speed up#3319

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Starbuck5 merged 4 commits intopygame:mainfromitzpr3d4t0r:scale_scaleby_improvements

Aug 17, 2022

Merged

scale(), scale_by(), smoothscale(), smoothscale_by() reorganization and speed up#3319

Starbuck5 merged 4 commits intopygame:mainfromitzpr3d4t0r:scale_scaleby_improvements

Aug 17, 2022

Conversation

Copy link

Contributor

itzpr3d4t0r commentedJul 17, 2022•
edited
Loading

Implements#3315.
Results are a bit strange, but the functions do work. I used this code:

fromtimeitimporttimeitimportpygamefrompygame.transformimportscale,scale_by,smoothscale,smoothscale_bypygame.init()WIDTH,HEIGHT=500,500win=pygame.display.set_mode((WIDTH,HEIGHT))IMAGE=pygame.image.load("images/cell.png").convert()GLOB= {"scale":scale,"IMAGE":IMAGE,"scale_by":scale_by,"smoothscale":smoothscale,"smoothscale_by":smoothscale_by}deftest(test_str:str):print(timeit(test_str,globals=GLOB))print("SCALE: ",end="")test("scale(IMAGE, (100, 100))")print("SCALE_BY: ",end="")test("scale_by(IMAGE, 2.45)")print("SMOOTHSCALE: ",end="")test("smoothscale(IMAGE, (100, 100))")print("SMOOTHSCALE_BY: ",end="")test("smoothscale_by(IMAGE, 2.45)")

and got these results:
OLD

SCALE: 10.217117799999869SCALE_BY: 13.330813199999284SMOOTHSCALE: 17.059421400000247SMOOTHSCALE_BY: 22.5625871000002

NEW

SCALE: 7.131523500000185SCALE_BY: 10.155133100000057SMOOTHSCALE: 16.981359200000043SMOOTHSCALE_BY: 22.357186100000035

itzpr3d4t0r added3 commits

July 17, 2022 16:02

transform.scale() and transform.scale_by() now use internal C scale_t…

03376e8

…o function.transform.smoothscale() and transform.smoothscale_by() now use internal C smoothscale_to function.

Updated pointers type to be the appropriate one.

54f16f5

Removed unused variable

Re added int cast.

17a6106

Copy link

Contributor

Starbuck5 commentedAug 10, 2022•
edited
Loading

I didn't measure much of a performance difference at all, I guess this is more of a code quality PR 🙂

Maybe 1.5% better performance in some cases, but the benchmarks are shaky. (Although I've just realized I did test with a large surface, so differences may be masked.)

Starbuck5 reviewed

Aug 10, 2022

View reviewed changes

src_c/transform.c OutdatedShow resolvedHide resolved

Starbuck5 reviewed

Aug 10, 2022

View reviewed changes

src_c/transform.c OutdatedShow resolvedHide resolved

Starbuck5 reviewed

Aug 10, 2022

View reviewed changes

src_c/transform.c OutdatedShow resolvedHide resolved

Starbuck5 reviewed

Aug 10, 2022

View reviewed changes

src_c/transform.c OutdatedShow resolvedHide resolved

Starbuck5 reviewed

Aug 10, 2022

View reviewed changes

Copy link

Contributor

Starbuck5 left a comment

Choose a reason for hiding this comment

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

Besides my formatting nitpicks, this PR looks good to me. It's hard to review though, since the diff is so gnarly. (Not your fault)

MyreMylar approved these changes

Aug 14, 2022

View reviewed changes

Copy link

Contributor

MyreMylar left a comment

Choose a reason for hiding this comment

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

Had a quick look over the code and it looks reasonable with a bit less duplication.

The tests pass and as far as I can tell it has made little to no difference to performance.

made requested changes

e0ae4c5

Starbuck5 approved these changes

Aug 17, 2022

View reviewed changes

Copy link

Contributor

Starbuck5 left a comment

Choose a reason for hiding this comment

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

Thanks!

As a final check to make sure outputs hadn't changed, I scaled and smoothscaled a surface into a png file before and after, and verified the file hash hadn't changed.

illume reviewed

Aug 20, 2022

View reviewed changes

Copy link

Member

illume left a comment

Choose a reason for hiding this comment

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

Very nice. 👍
It's common to see different speeds depending on image size and CPU cache size/architecture.

ps, as an aside; have you seehttps://halide-lang.org/ andhttps://github.com/pydata/numexpr ? The videos that halide provides are especially interesting.