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

WiP: Convert camera tests to Hypothesis #220

Open
wants to merge 7 commits into
base: canon
Choose a base branch
from
Open

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Mar 31, 2019

  • test_camera_viewport
  • test_camera_point_in_viewport_not_at_origin
  • test_camera_translate_to_frame
  • test_camera_translate_to_viewport
  • test_sprite_in_viewport
  • test_viewport_change_affects_frame_height

@nbraud
Copy link
Contributor Author

nbraud commented Apr 1, 2019

I'm having some trouble converting test_camera_point_in_viewport_not_at_origin to an Hypothesis test.

Now I'm unsure I understand correctly what Camera.point_in_viewport is supposed to do (it's either that or there's a bug in it, given that I can't get Hypothesis not to explode on that test), and I can't find doc that clarifies it :(

@pathunstrom
Copy link
Collaborator

I think the correct answer to what it's supposed to do is "be overengineering made manifest"

It literally checks if a given vector exists in the viewport/window the camera is attached to. As far as I can tell, it's not been used and existed to handle the future case of multiple cameras.

@nbraud nbraud requested a review from a team as a code owner April 4, 2019 11:04
@nbraud
Copy link
Contributor Author

nbraud commented Apr 4, 2019

@pathunstrom OK; I still don't quite get what it's supposed to do (or in which coordinate system it operates, frame or viewport) so I will leave that test alone for now.

@pathunstrom
Copy link
Collaborator

Pretty sure it's pure viewport, not even a translation from one to the other. That's my bad.

@nbraud
Copy link
Contributor Author

nbraud commented Apr 4, 2019

FWIW, CI is failing because I needed Vector.isclose, which isn't in a released version of ppb-vector yet.

Copy link
Member

@AstraLuma AstraLuma left a comment

Choose a reason for hiding this comment

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

Seems good so far.

@AstraLuma
Copy link
Member

Looking this over, I don't think #439 covers everything this does/aspires to be.

So I'm going to leave this open for now.

@pathunstrom pathunstrom changed the base branch from master to canon June 27, 2020 11:02
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