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

Initial cleanup of arcade.types #2065

Merged
merged 31 commits into from
Apr 23, 2024

Conversation

pushfoo
Copy link
Member

@pushfoo pushfoo commented Apr 18, 2024

Changes

TL;DR: Spring cleaning to make 3.0-critical fixes easier

  1. Make arcade.types a folder module
  2. Move Color and related to arcade.types.color
  3. Delete or reduce use of redundant or infrequent aliases and types:
    • Remove NamedPoint
      • only used in shapes_perf.py
      • Doesn't appear to be needed, but we can use Vec2 once non-advertised API issues are resolved
    • Reduce usage of IPoint in favor of clearer Size2D[int]
    • Rename arcade.types.Vector to Velocity
  4. Small pyglet 2.1 & pyright-related changes in arcade.math:
    • Remove Vec* from V_2D and V_3D aliases since pyglet.math.Vec* types are now tuples
    • Use arcade.types.AsFloat in a few places, such as arcade.,math.lerp
  5. Update doc build to account for new type module structure

Why?

TL;DR: Multiple 3.0-mandatory issues need good rectangle abstraction(s)

  1. Issues listed below share a need for rectangle abstractions (or the all-important LRWH one if we have more)
  2. We can refactor GUI's Rect into our rectangle abstraction
  3. To make this easier for devs, we need to split and clean the over-stuffed arcade.types module
  • Navigating the current module is painful
  • None of the following really overcome the huge volume of stuff in the module:
    • PyCharm / VSCode's module structure tree views
    • Command palette jump-to
    • Old-school search by string / regex
  • Less experienced devs can help with type cleanup if we:
    1. Split the module to be more legible
    2. Structure our docs / types well
    3. Give them clear instructions and guidance on how to proceed

The 3.0 issues which need good rectangles

The following 3.0-mandatory issues would all be massively helped along by sharing rectangle handling code to avoid bugs:

1. Cameras

Due to lack of good rectangle abstractions, cameras individually compute and expose both raw and scaled values by:

  1. Numerically indexing tuple elements individually (ie, value[0])
  2. Mixing the data, often with similarly-accessed attributes from camera *Data classes.

As a result of this, we have:

  1. Hard-to-read code
  2. 4+ known bugs and we keep finding more:
  3. Wasted function calls and . accesses due to individual recalculation of values

For example, if we want to check what's in the player's view, scaled projection boundaries currently waste 4 function calls and repeated . accesses recalculating the values:

def projection_left_scaled(self) -> float:
"""
The left edge of the projection in world space.
This is not adjusted with the camera position.
NOTE this IS scaled by zoom.
If this isn't what you want,
use projection_left instead.
"""
return self._projection.left / self._data.zoom
@projection_left_scaled.setter
def projection_left_scaled(self, _left: float) -> None:
self._projection.left = _left * self._data.zoom
@property
def projection_right(self) -> float:
"""
The right edge of the projection in world space.
This is not adjusted with the camera position.
NOTE this IS NOT scaled by zoom.
If this isn't what you want,
use projection_right_scaled instead.
"""
return self._projection.right
@projection_right.setter
def projection_right(self, _right: float) -> None:
self._projection.right = _right
@property
def projection_right_scaled(self) -> float:
"""
The right edge of the projection in world space.
This is not adjusted with the camera position.
NOTE this IS scaled by zoom.
If this isn't what you want,
use projection_right instead.
"""
return self._projection.right / self._data.zoom
@projection_right_scaled.setter
def projection_right_scaled(self, _right: float) -> None:
self._projection.right = _right * self._data.zoom
@property
def projection_bottom(self) -> float:
"""
The bottom edge of the projection in world space.
This is not adjusted with the camera position.
NOTE this IS NOT scaled by zoom.
If this isn't what you want,
use projection_bottom_scaled instead.
"""
return self._projection.bottom
@projection_bottom.setter
def projection_bottom(self, _bottom: float) -> None:
self._projection.bottom = _bottom
@property
def projection_bottom_scaled(self) -> float:
"""
The bottom edge of the projection in world space.
This is not adjusted with the camera position.
NOTE this IS scaled by zoom.
If this isn't what you want,
use projection_bottom instead.
"""
return self._projection.bottom / self._data.zoom
@projection_bottom_scaled.setter
def projection_bottom_scaled(self, _bottom: float) -> None:
self._projection.bottom = _bottom * self._data.zoom
@property
def projection_top(self) -> float:
"""
The top edge of the projection in world space.
This is not adjusted with the camera position.
NOTE this IS NOT scaled by zoom.
If this isn't what you want,
use projection_top_scaled instead.
"""
return self._projection.top
@projection_top.setter
def projection_top(self, _top: float) -> None:
self._projection.top = _top
@property
def projection_top_scaled(self) -> float:
"""
The top edge of the projection in world space.
This is not adjusted with the camera position.
NOTE this IS scaled by zoom.
If this isn't what you want,
use projection_top instead.
"""
return self._projection.top / self._data.zoom

Instead, we should be recalculating the scaled rectangle bounds once relative to the projection centerpoint and returning it. For bound checking, we'll:

  • always need all the values (left <= x <= right and bottom <= y <= top)
  • can encapsulate bound checking inside a rectangle type, as we already do for points

2. The 3.0-mandatory create_text_sprite rework

I've paused work on #1410 since I don't want to buggily rewrite bounds checking and scaling code.

3. (Not actually 3.0) Screenshots

To complete #2039 to satisfaction, we'll want region handling. Rectangles can do that.

Just how nasty is our type situation?

Rectangles are very nasty

First, we have at least 4 different ways of using / defining "rectangle" data. We won't fix this in this PR, but we'll clear room for it.

Two of the rectangle definitions are very bad:

  1. The current Rect type alias inside arcade.types:
    • is labelled as LBWH in the comments
    • gets used as LRBT elsewhere
    • wastes space which could be expressed in a clean annotation / alias
  2. arcade.math.rand_in_rect takes bottom_left: Point, width: float, height: float

We also have two good ones:

  1. The LBWH Rect inside the GUI

    • It's a reasonable start
    • It implements __contains__ as point bound-checking tool
    • It has a scaling helper which may need point-relative scaling
  2. Separate left, right, bottom, top arguments to functions in draw_commands

    • These are beginner friendly so the format is okay
    • They're also great debug tools

The rest is cluttered

This PR tries to clean up some of the various redundant or one-off definitions cluttering arcade.types. It also moves Color and related defs into their own submodule.

Future work / PRs

3.0 Critical Work

  1. Nailing down what our abstraction should look like, probably involving:
    • A Rect type which computes not just axis bound values (LRBT), but also width and height
      • You usually want the whole viewport bounds during real game development
      • width & height are useful too: if something's size is bigger than the viewport, skip detailed bounding checks
  • A Protocol with properties
  • Using typing.NamedTuple for base classes due to these being fastest for named access according to the pyglet team's tests
  1. Refactoring our GUI's Rect into the decided reusable component
  2. Following up with that rectangle in:
    • Current PRs
    • Future ones
  3. Refactor arcade.particles:
    • (3.0 mandatory) Particles don't care about frame time #1137
    • While we're there, consider cleaning up arcade.types.Velcocity
      • Only remaining uses are in the particle emitters
      • It's consistent enough that this PR will get even bigger if we try to fix it here

As time allows

  1. Moving Arcade-specific exceptions out of utils
    • This might be good for cameras
    • Might not be entirely useful since we may want camera exceptions to go in the camera module
  2. The math module needs cleaning
    • In-shape-bound randomization functions might be best on types?
    • If pyglet 2.1 vector issues are resolved without vendoring, we'll want to do some refactoring

* Remove unused TypeVar import

* Move BufferProtocol conditional import to the top

* Make backport-related imports conditional instead of universal
* Remove Vec2 and Vec3 from the unions since pyglet's Vec tpyes are now tuples

* Remove Vec2 and Vec3 imports from arcade.math
* Add subscriptable PathOr alias

* Remove Optional from PathOrTexture

* Wrap usages of PathOrTexture in Optional

* Add bytes to PathLike Union alias

* Clean up imports in some PathOrTexture-using files

* Move the if TYPE_CHECKING import of Texture closer to the usage
@DigiDuncan
Copy link
Contributor

Praying to the merging gods we can get this in ASAP. Super important. Also, glorious write up, push.

@eruvanos eruvanos merged commit efd215b into pythonarcade:development Apr 23, 2024
8 checks passed
eruvanos pushed a commit that referenced this pull request Apr 23, 2024
* Replace old bloated color validation with Color's from_iterable + normalize

* Improve readability & fail speed in draw_commands

* Move validation & unpacking ctx members to locals to top of functions

* Use Color's from_iterable + normalize

* Use named variables for goal buffer sizes to make buffer expansion clearer

* Add comments and attempt to make math more readable

* Finish dedupe of logic / name shortening in draw_lines

* Skip tuple redundant tuple conversion since array.array accepts appropriate iterables

* Don't re-divide when we already know the size

* Explain safety // 8

* Separate steps better in draw_points

* Copy to ctypes array earlier

* Skip tuple conversion for array.array since it takes generators

* Attempt to make draw_polygon_outline's current logic more readable

* Sequence.extend doesn't exist, so annotate local variable as List[Point]
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.

None yet

4 participants