Camera Redesign #439
Camera Redesign #439
Conversation
| :attr:`Camera.width`, :attr:`Camera.height`, or the | ||
| ``target_game_unit_width`` of the :class:`Camera` constructor) will | ||
| affect both :attr:`Camera.width` and :attr:`Camera.height`. Their ratio | ||
| is determined by the defined window. |
There was a problem hiding this comment.
I think we might need more prose to discuss camera initialization and when scenes can expect to have the camera.
| @@ -111,12 +113,14 @@ def __init__( | |||
| resolution=DEFAULT_RESOLUTION, | |||
| window_title: str = "PursuedPyBear", | |||
| target_frame_rate: int = 30, | |||
| target_camera_width=25, | |||
There was a problem hiding this comment.
This is the default scene width? This is significantly larger than before, right?
There was a problem hiding this comment.
Flipped a coin between a 32 pixel default sprite and a 64. I'm willing to switch it.
There was a problem hiding this comment.
(But yes, this is 25 x ~19, previously, we were 10 x ~7.5)
| # regions they should render to. | ||
| camera = camera_class(self, self.target_camera_width, self.resolution) | ||
| scene.main_camera = camera | ||
| self.scene_cameras[scene] = [camera] |
There was a problem hiding this comment.
Primarily to have handles to adjust resolution when we finally start supporting resizeable windows.
| values = [width, height] | ||
| short_side_index = width > height | ||
| target = self.pixel_ratio * game_unit_size | ||
| target = pixel_ratio * game_object_size |
There was a problem hiding this comment.
Not actually a fan of this rename. The old name was more correct and we're not moving away from game units as a concept.
There was a problem hiding this comment.
Oh, yeah, sorry, I inlined this because pixel_ratio isn't directly a renderer concern anymore, then realized we had a test around this. Can revert.
| assert camera.translate_point_to_game_space(point) == expected | ||
|
|
||
|
|
||
| # @pytest.mark.skip(reason="Test for old camera. Will want to restore this functionality in new camera.") |
There was a problem hiding this comment.
Any reason these tests weren't ported?
There was a problem hiding this comment.
Well, this top one is because I didn't rewrite sprite_in_viewport because it's not used by anything in the library. I'll probably do it sometime in the next couple of months and then restore/rewrite the relevant test.
The one below it is because it's doing a lot, would require translation to the new camera tests, and I'm not entirely sure what all of the steps are there to do. As such, I'd rather rewrite it as its own thing and not tied to the giant pile of other changes going on. (And it's commented out because the old camera got killed, I'd have just left it marked skip otherwise.)
There was a problem hiding this comment.
I think that's a useful utility, although the name isn't great.
The one below that is supposed to be a simple round-trip test: If we convert a point from game units to pixels and back, do we get the same point?
There was a problem hiding this comment.
It is a useful utility! I have no intention of leaving it cut forever. I just wanted to get the new camera working then I'll restore some of the niceties later.
The good news RE: round tripping, the tests for translate_point_to_screen and translate_point_to_game_space use the same values for their tests, just reversing input and output.
There was a problem hiding this comment.
Well, doing a roundtrip test means it's generic and we can throw hypothesis at it.
|
We can drop |
This is the long awaited fix to all the pain points in the Camera. So far, it's just an init.
This represents a breaking change in the API.
This touches on a number of issues, some solved wholly by this change, some only fixes part of the problem.
Resolves #185: pixel_ratio is now derived from the number of game units you want visible and the screen resolution. You can "zoom" by increasing or decreasing the width or height of the camera.
Resolves #310: Camera isn't added until SceneStarted event, but is fully initialized as soon as it's available.
Regarding #358: Did not deal with this directly. That test is currently nullified until we can go over it and determine what is going on.
Regarding #400: Removes the pixel_ratio argument from BaseScene.