Skip to content

Add validation for projection dimensions in Camera2D + projections #2061

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

Merged

Conversation

pushfoo
Copy link
Member

@pushfoo pushfoo commented Apr 12, 2024

TL;DR:

  1. Leave full doc rework until a later PR
  2. Add validation which prevents delayed ZeroDivisionError which @MiCurry discovered
  3. Add tests for validation

Changes

  • Add validation to Camera2D.from_raw_data
    • Add validation
    • Add tests
  • Rename internal *Data attributes for clarity per Discord discussion
    • _data -> _camera_data
    • _projection -> _projection_data
  • Add near and far validation to __init__
  • Add validation to .projection

Why?

  1. Any of the following in an ortho projection cause a divide be zero:
  • left == right
  • top == bottom
  • near == far
  1. It's hidden until you call Camera2D.use for the first time

Example traceback sanitized from the Discord thread:

  File "E:\Projects\SpaceGame\space.py", line 443, in on_draw
    self.cameras[player].use()
  File "C:\Users\User\AppData\Roaming\Python\Python312\site-packages\arcade\camera\camera_2d.py", line 697, in use
    self._ortho_projector.use()
  File "C:\Users\User\AppData\Roaming\Python\Python312\site-packages\arcade\camera\orthographic.py", line 134, in use
    _projection = self._generate_projection_matrix()
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\User\AppData\Roaming\Python\Python312\site-packages\arcade\camera\orthographic.py", line 106, in _generate_projection_matrix
    return Mat4.orthogonal_projection(*_true_projection, self._projection.near, self._projection.far)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\python312\Lib\site-packages\pyglet\math.py", line 627, in orthogonal_projection
    sx = 2.0 / width
         ~~~~^~~~~~~
ZeroDivisionError: float division by zero

Questions

  • Should ZeroProjectionDimension subclass ZeroDivisionError or ValueError? It's a ValueError since the math hasn't taken place yet.

Follow-up work

  • Add validation to future properties after rework

@pushfoo pushfoo marked this pull request as draft April 12, 2024 07:34
@pushfoo pushfoo marked this pull request as ready for review April 12, 2024 20:35
pushfoo and others added 16 commits April 15, 2024 20:33
* Add ZeroProjectionDimension type to arcade.camera.data_types

* Use it in the projection validation in from_raw_data
* Add new test for Camera2D.from_raw_data projection validation

* Expand inheritance safety tests for Camera2D classmethods with parametrization
* Inline the validation logic to go fast

* Update the docstring to be explicitly clear about this behavior
* Turn camera_class into a fixture

* Add simple inline test for from_raw_data's near/far validation
@pushfoo pushfoo force-pushed the camera_immediate_error_on_0_dims branch from 0277038 to 760ae09 Compare April 16, 2024 00:33
@pushfoo
Copy link
Member Author

pushfoo commented Apr 18, 2024

After discussion, consensus from @DragonMoffon and users who've tried 3.0 cameras seems to be:

  1. The current changes are necessary almost entirely as-is
  2. Defer further property validation until after Initial cleanup of arcade.types #2065 and related follow-ups
  3. The only further change needed in this PR is swapping the signatures of Camera2D.__init__ and Camera2D.from_raw_data by:
    • Swapping the signatures and internals
    • Updating the tests
    • Fixing the examples

Copy link
Collaborator

@DragonMoffon DragonMoffon left a comment

Choose a reason for hiding this comment

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

Validation looks good, and renaming is good. There are issues, but they are with the code rather than the PR, and I will fix them later.

@eruvanos eruvanos merged commit 5e4af62 into pythonarcade:development Apr 23, 2024
8 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
Development

Successfully merging this pull request may close these issues.

3 participants