Skip to content

Cameras and controller overhaul #457

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 61 commits into from
Mar 17, 2023
Merged

Cameras and controller overhaul #457

merged 61 commits into from
Mar 17, 2023

Conversation

almarklein
Copy link
Member

@almarklein almarklein commented Feb 15, 2023 •
edited
Loading

Addresses #403

Tasks

Contributions outside of cameras and controllers:

  • Introduced WorldObject._look_at which has a flag to indicate whether it is a light or camera.
  • Fixed what's forward for cameras, but not for lights.
  • Fixed a bug in the calculation of the bounding box for grid (texture) geometry.
  • The show() func considers a given camera as already having a good initial view. Plus a few small tweaks in show().

To do:

Wrapping up:

  • Make sure the cameras are documented in the ref docs.
  • Write a piece in the guide on how to use the cameras.
  • Add validation example for camera show methods.
  • Update all examples.
  • Wait for Require pylinalg #484

After this:

  • Camera smoothing.
  • More controllers.
  • Make key-mouse-bindings configurable.
  • Better default bindings? (e.g. panning is not consistent).

Explanation for some decisions

Notion of extent

As I made the controllers work with both the ortho and perpective cameras, I introduced a notion of scale or scene size, which I called "extend". For the ortho camera this is simply the mean of width and height. For the perspective camera, I needed to introduce a new extent prop. It is set by look_at, show_object and the new show_rect.

The extent is an indication of the size of the scene being viewed. It can be used to determine the distance to the scene (taking fov into account), and thus can be used by the orbit camera to know around what point to rotate. Further, it is used to automatically determine the near and far clipping plane to sensible values.

Camera class hierarchy

Interestingly, the extent / aspect pair, and the width / height pair, are two ways to set the same thing! With that insight, the two cameras are kindof the same thing: an orthographic camera is simply a perspective camera with a fov of zero.

So I made the PerspectiveCamera more generic by allowing fov=0, and the OrthographicController a thin wrapper that limits the fov to zero. With a perspective camera and an orbit controller, you can change fov by scrolling while holding alt/option. You can then lower it all the way to zero, smoothly transitioning from a perspective camera to an orthographic one 😄

(At some point I introduced a GenericCamera from which both cameras inherited, but when I started to write the guide, that felt overly complex.)

Controller class hierarchy

The orbit controller has the same method as the panzoom camera, and some more. And they work the same way. So I made the OrbitController inherit from the PanZoomController.

Camera updates

This PR covers everything in #403, except that changes are applied to the camera automatically (and directly). That's a consequence of the fact that the controllers do not have state by themselves.

Korijn
Korijn reviewed Feb 27, 2023
View reviewed changes
@Korijn
Copy link
Collaborator

Korijn commented Feb 28, 2023 •
edited
Loading

Is it a lot of work to also implement fly controls?

  • W/S move forward/backward
  • A/D strafe sideways (left/right)
  • R/F move up/down
  • Mouse movement controls viewing direction (without holding any mouse buttons down)
  • Optional:
    • Q/E rolls camera about the viewing axis

It's much more convenient to navigate scenes like sponza with this kind of control scheme.

@almarklein
Copy link
Member Author

Is it a lot of work to also implement fly controls?

I would love to have that too. But let's do that in a new pr. Made #471 to track.

@Korijn Korijn mentioned this pull request Mar 1, 2023
Merged
@kushalkolar
Copy link
Contributor

kushalkolar commented Mar 6, 2023 •
edited
Loading

This is looking amazing!

What do you think about allowing the Controller to be "enabled" or "disabled". For the PanZoomController it seems like this might be possible by:

  • creating a property enabled which is checked in handle_event().
  • when enabled is set to False call: pan_stop(), zoom_stop() and quickzoom_stop()

Usecase, when using the mouse to move objects (see the dark gray rectangle) it also moves the camera. If a controller can be enabled based on a key event or other event (the user can define their own event handling) it makes this nicer:

selector_controller_mixed-2023-03-05_20.13.07.mp4

@almarklein
Copy link
Member Author

What do you think about allowing the Controller to be "enabled" or "disabled"

I think that's a great idea (was planning for adding this already 😄 )

@kushalkolar kushalkolar mentioned this pull request Mar 9, 2023
Closed
10 tasks
Korijn
Korijn reviewed Mar 9, 2023
View reviewed changes
@almarklein
Copy link
Member Author

Ok, so I managed to hide the extent stuff. Still works but is much simpler to explain.

Can you give an example? How would I determine a proper value for extent, as a user?

So now you can just specify either the width or height in addition to the aspect. But the recommended approach is to use any of the camera.show_xx methods.

I think users will want to use look_at in all kinds of scenarios to update the orientation of a camera, not just to center it on the scene, and will most commonly want to manage the extent themselves. Is this new behavior optional?

Good point. I left the look_up untouched, and introduced show_pos that's basically look_at but it also sets width and height. Now all methods that start with show_ nicely prep the camera for use with a controller.

Remaining point is the registering of events. A proposal:

# Rename add_default_event_handlers -> register_events
controller = gfx.OrbitController(camera)
controller.register_events(renderer_or_viewport)

# And allow as a shortcut
controller = gfx.OrbitController(camera, register_events=renderer_or_viewport)

@Korijn
Copy link
Collaborator

Korijn commented Mar 13, 2023

Proposal is 👍

@almarklein almarklein mentioned this pull request Mar 14, 2023
Merged
@Korijn Korijn mentioned this pull request Mar 14, 2023
Merged
@almarklein
Copy link
Member Author

almarklein commented Mar 14, 2023 •
edited
Loading

@Korijn I want to make sure that you approve the current state of the PR before I update the remaining examples (since that would make the PR harder to review, and I want to avoid duplicate work).

(if needed, take your time, I can work on a fly controller or something)

@Korijn
Copy link
Collaborator

Korijn commented Mar 15, 2023 •
edited
Loading

I think you did a great job here, after playing around with this and reviewing the code I have the following to say:

  • OrbitController: traditionally mouse wheel zooming would be towards the center of orbit, not towards the mouse. Can you use a modifier for zooming to the mouse and make zooming towards the center of orbit the default? It's usually intentional that the camera is fixed on the center of orbit when you pick the orbit controller.
  • I had to really look at the code to discover what features and modifiers are available; maybe add something to the guide/docs about what the controls are?

Other than that, feel free to update the remaining examples. I'm good with this, it looks really nice.

almarklein
almarklein commented Mar 16, 2023
View reviewed changes
@almarklein
Copy link
Member Author

All green 🚀

@Korijn Korijn merged commit 5a7f6d8 into main Mar 17, 2023
@Korijn Korijn deleted the cameras branch March 17, 2023 06:14
@almarklein almarklein mentioned this pull request Mar 17, 2023
Closed
5 tasks
@Korijn Korijn mentioned this pull request Apr 7, 2023
Closed
@kushalkolar kushalkolar mentioned this pull request Apr 14, 2023
Merged
@Korijn Korijn mentioned this pull request Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Korijn Korijn Korijn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to scale camera-axii individually Controller does not request draw on load_state Update projection matrix in camera.show_object
3 participants
@almarklein @Korijn @kushalkolar