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

Expose Camera2D in arcade's top level namespace #2055

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

pushfoo
Copy link
Member

@pushfoo pushfoo commented Apr 8, 2024

Changes

TL;DR: Enable arcade.Camera2D and from arcade import Camera2D

  • Import Camera2D + supporting classes to arcade/__init__.py
  • Add Camera2D + supporting classes to arcade.__all__

Why

  1. Good DX
  2. Make subsequent examples + porting guide PRs easier to read

@DigiDuncan
Copy link
Contributor

Good idea. We have other "core" objects exposed (Text, Window, View, Sprite) and this makes sense to have. You want more complex cameras, dig into arcade.camera.

@pushfoo pushfoo mentioned this pull request Apr 8, 2024
5 tasks
@pushfoo pushfoo force-pushed the expose_Camera2D_at_arcade_toplevel branch from c7a991e to 2623830 Compare April 9, 2024 15:16
@einarf
Copy link
Member

einarf commented Apr 10, 2024

It does make sense historically, but I'm a little bit conflicted here. Should we not try to slim down the arcade module when possible? It already contains so much.

@DigiDuncan
Copy link
Contributor

It does make sense historically, but I'm a little bit conflicted here. Should we not try to slim down the arcade module when possible? It already contains so much.

I would say our most important objects (and most accessible ones) should be top-level. Camera2D, Text, Window, View, Sprite, Sound, etc. The more advanced or specialized versions (PerspectiveProjector, or BasicSprite, etc.) can be accessed in their sub-modules.

@pushfoo
Copy link
Member Author

pushfoo commented Apr 15, 2024

@einarf
TL;DR: We should fix Camera2D.__init__'s arguments + improve underlying camera behavior.

Should we not try to slim down the arcade module when possible?

The current custom viewport options for Camera2D are somewhat ugly. We have a choice of telling beginners to:

  • use Camera2D.from_raw_data to create cameras
  • set camera2d_instance.projection_viewport after creation
  • use the camera.*Data classes with Camera2D.__init__

I think we should flip Camera2D's __init__ and from_raw_data arguments:

  • It eliminates requiring a top-level exposure for the arcade.camera.*Data classes
  • Beginners will be confused by the extra classes anyway
  • Sharing CameraData between cameras is an advanced technique

@pushfoo pushfoo force-pushed the expose_Camera2D_at_arcade_toplevel branch from 2623830 to 58bdfcd Compare April 16, 2024 00:49
@pushfoo
Copy link
Member Author

pushfoo commented Apr 16, 2024

The recent commits pushed:

  1. Rebase onto development to get Eliminate pyright errors #2062's fixes
  2. Remove the *Data classes from arcade.__all__

@einarf
Copy link
Member

einarf commented Apr 16, 2024

Might also be a good idea to update imports in examples here?

@pushfoo
Copy link
Member Author

pushfoo commented Apr 18, 2024

Might also be a good idea to update imports in examples here?

Yep. I think this might wait till after the following:

From what I remember of trying to test something like this locally, it also caused some havoc when trying to build doc in certain cases. That may have been due to trying to squeeze cameras into the current doc build system clumsily and in a rush. I'll revisit this PR in the coming days after the above.

@einarf
Copy link
Member

einarf commented Apr 23, 2024

Maybe we should just merge this quick to at least get the Camera2D doc generated? It's better than having no docs on it right now.

@pushfoo
Copy link
Member Author

pushfoo commented Apr 23, 2024

TL;DR: It might actually be very broken despite tests seeming to pass.

I haven't it recently and so I don't remember the details, but there was at some point a wall of red in build output related to this. From what I remember:

  • They weren't the PDF build
  • They were related to class detection
  • I think replacing the quick index tooling as you suggested would've solved them

I was going to wait until the following are in place:

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

3 participants