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

Name scale properties on Sprites to things that make sense #2021

Merged
merged 29 commits into from
Jul 1, 2024

Conversation

DigiDuncan
Copy link
Collaborator

@DigiDuncan DigiDuncan commented Mar 13, 2024

What does it do?

  • Allows setting scale to a float for backwards compatible codefeel/easy proportional scaling
  • Getting scale returns a tuple[float, float]
  • Renames scale -> scale_x
  • Renames scale_xy -> scale
  • Adds scale_y

Why should this be done?

  • The current implementation lies to you. scale as it is returns scale_x, which is incorrect if x/y are scaled independently.
  • There is precedent. Sprite currently returns position¹ as a 2-tuple, and center_x and center_y as floats.
  • Learning wrong is worse than not learning. The current implementation is more confusing for children in the long run. Most game libraries represent scale in both dimensions, and Arcade should aim to provide transferable knowledge.
  • It's confusing for a developer. If you are a games dev, understanding why .scale returns x, scale_y returns y, scale_xy returns a tuple, but setting scale sets both x and y, is very confusing and makes code harder to read and reeks havoc on codefeel.

¹I'd argue position should be center as well, but that's for another day.

@pushfoo
Copy link
Member

pushfoo commented Mar 15, 2024

TL;DR: This will be a non-breaking change & efficiency upgrade with pyglet 2.1, which seems to be coming sooner than expected.

Benefits:

  1. sprite_instance.scale *= 2.0 will work with scalars as it does now
  2. sprite_instance.scale *= 2.0, 3.0:

Steps required:

  • Bump to pyglet 2.1 (Switch to Pyglet 2.1 or dev releases #2043)
  • Make BasicSprite.scale return pyglet 2.1's tuple-based Vec2s
  • Improve doc to use/review position to teach tuples, then lead into scale
    1. cute diagram with scale x and scale y, stretching a snake image
    2. scale example with interactive GUI sliders to scale sprite
    3. add links to healthbar examples
  • Revert any example temp fixes to pass CI / typing

@einarf einarf self-requested a review March 25, 2024 23:12
@einarf einarf added this to the 3.0 mandatory milestone Mar 25, 2024
@pushfoo pushfoo mentioned this pull request Apr 4, 2024
8 tasks
@einarf
Copy link
Member

einarf commented Apr 16, 2024

What about the initializer? Should scale still only be a float there?

@pushfoo
Copy link
Member

pushfoo commented Apr 16, 2024

Imo, it could be either without an isinstance check:

if hasattr(scale, "__len__"):
    self._scale = Vec2(*scale)
else:
    self._scale = Vec2(scale, scale)

@einarf
Copy link
Member

einarf commented Jun 30, 2024

I resolved the conflicts. There was a lot of changes to unit test so these needs to be looked into.

* Annotate HitBox scale __init__ arguments as Point2

* Annotate BasicSprite scale return as Point2
* Make .scale convert to Vec2 on return

* Update scale unit tests to use tuples
* Remove if check around texture since we are guaranteed to have one now

* Rename scale_x argument from new_value to new_scale_x

* Unpack self._scale into old_scale_*

* Remove redundant scale setting for y

* Apply new scale to the hitbox first to raise exceptions earlier
* Remove if check around texture since we are guaranteed to have one now

* Rename scale_x argument from new_value to new_scale_x

* Unpack self._scale into old_scale_*

* Remove redundant scale setting for x

* Apply new scale to the hitbox first to raise exceptions earlier
* Rename new_value to new_scale

* Assign to scale_x and scale_y instead of immediate tuple creation

* Add comments about hot code path asking not to DRY it

* Add exception checks for unpack

* Reorder and reduce use of dot and index acccess

* Remove extra line
* Significantly redeuce dot and index access in rescale_relative_to_point

* Precache re-used quantities

* Comments for clarity
* Use scale instead of removed scale_xy

* Convert to more pyglet/Google-style
* Add vector unpack check logic to rescale_relative_to_point

* Update signature annotations

* Update docstring

* Rename factor argument to scale_by

* Delete body of rescale_xy_relative_to_point

* Update docstring to point to rescale_relative_to_point with deprecation

* Add @warning wrapper to rescale_xy_relative_to_point
@pushfoo pushfoo merged commit 3c03b23 into pythonarcade:development Jul 1, 2024
7 checks passed
@pushfoo
Copy link
Member

pushfoo commented Jul 1, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants