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

Sprite scale: Preserve performance #2389

Merged
merged 12 commits into from
Oct 14, 2024
Merged

Sprite scale: Preserve performance #2389

merged 12 commits into from
Oct 14, 2024

Conversation

einarf
Copy link
Member

@einarf einarf commented Oct 6, 2024

The Sprite.scale changes made the property 4-5 times slower. Instead we introduce methods to handle uniform scaling.

@DragonMoffon
Copy link
Collaborator

Id like to try one last thing before we go ahead. Since the biggest cost with the Vec2 is around creation and GC I wonder if making a slotted murable Vec-like with support for piecewise operations would be performant enough. Like working with lists it may have a few hickups, but Ill implement __copy__

@DigiDuncan
Copy link
Collaborator

Id like to try one last thing before we go ahead. [...]

I'm curious to see this before we push this one through -- the DX is a tad clunky and it won't be forward-compatible with 3.1. I'm fine with it if Dragon's proposal doesn't get done/isn't up to snuff, though, I agree it's a better option then lying to the user as to what the underlying scale is.

@einarf
Copy link
Member Author

einarf commented Oct 8, 2024

The amount of things that broke due to this change was minuscule so I'm not too worried. The new methods are also a lot faster than the scale property can be. Especially when adding and multiplying scale simply because it's one callable less compared to += and *=.

This also needs to be checked before merge. Ensure all subclasses of BasicSprite is typed to allow a tuple as well.
#2399

The names we had for the scale methods didn't feel right for arcade/python so they have been changed to be more readable.

While the concern about confusing the user is valid I think the change is worth it. During the interim before we do a larger re-write this is a nicer developer experience than using a method. Most developers set the sprite scale once, and then don't change it in the future.

the `isinstance` check for `AsFloat` is no-where near the cost of creating a vector.

In my ideal world sprite would not have a scale property at all, and instead scale is a method that changes the size of the sprite.
@DigiDuncan DigiDuncan marked this pull request as ready for review October 11, 2024 10:36
Copy link
Collaborator

@DigiDuncan DigiDuncan left a comment

Choose a reason for hiding this comment

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

As much as the functions feel a tad clunky still, the speed can't be denied, and I'm more concerned about the return of scale = float for DX reasons. Seems good to me until 3.1+.

@DragonMoffon DragonMoffon merged commit 0444db5 into development Oct 14, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants