search

Plonq / bevy_panorbit_camera

A simple pan and orbit camera for the Bevy game engine
Apache License 2.0
186 stars 36 forks source link

Add zoom smoothing #30

Closed ghost closed 1 year ago

ghost commented 1 year ago

See https://github.com/Plonq/bevy_panorbit_camera/pull/29 for previous discussion

ghost commented 1 year ago

I have bound the scale of the Orthographic camera to radius, which means the behavior of the Orthographic camera in the PR code will be different. Should the original behavior be preserved?

origin

new

Plonq commented 1 year ago

I have bound the scale of the Orthographic camera to radius, which means the behavior of the Orthographic camera in the PR code will be different. Should the original behavior be preserved?

Yes original behaviour should be preserved, to avoid clipping like your video shows. In other words, for orthographic cameras, the radius should never change when zooming.

ghost commented 1 year ago

It's almost there - both pixel and line scrolling work, including for ortho cameras. Let's just make sure radius is not modified for orthographic cameras, and then I think it'll be ready.

I have added the scale and target_scale fields, restoring the behavior of the orthographic camera. Additionally, I have modified the code related to Constraints, reusing the apply_limits function (previously apply_zoom_limits).

Plonq commented 1 year ago

It's almost there - both pixel and line scrolling work, including for ortho cameras. Let's just make sure radius is not modified for orthographic cameras, and then I think it'll be ready.

I have added the scale and target_scale fields, restoring the behavior of the orthographic camera. Additionally, I have modified the code related to Constraints, reusing the apply_limits function (previously apply_zoom_limits).

This is getting a bit out of hand, and I think there's too much scope creep now. I don't think adding new fields for scale is a good idea, when radius can be used for both. But it's not obvious how to structure things accordingly.

How about this. Let's revert back to before you differentiated between pixel and line scrolling. I.e. apply smoothing across the board. Once merged, I'll look into solving the lag issue with smoothing the pixel scrolling separately.

Don't worry, your work won't be wasted!

ghost commented 1 year ago

It's almost there - both pixel and line scrolling work, including for ortho cameras. Let's just make sure radius is not modified for orthographic cameras, and then I think it'll be ready.

I have added the scale and target_scale fields, restoring the behavior of the orthographic camera. Additionally, I have modified the code related to Constraints, reusing the apply_limits function (previously apply_zoom_limits).

This is getting a bit out of hand, and I think there's too much scope creep now. I don't think adding new fields for scale is a good idea, when radius can be used for both. But it's not obvious how to structure things accordingly.

How about this. Let's revert back to before you differentiated between pixel and line scrolling. I.e. apply smoothing across the board. Once merged, I'll look into solving the lag issue with smoothing the pixel scrolling separately.

Don't worry, your work won't be wasted!

I think in perspective and orthographic cameras, radius and scale have different roles. Merging them could lead to confusion. Even in orthographic cameras, we still need radius to represent the position of the camera. For clarity and ease of use, it's better to keep them separate.

Plonq commented 1 year ago

I think in perspective and orthographic cameras, radius and scale have different roles. Merging them could lead to confusion. Even in orthographic cameras, we still need radius to represent the position of the camera. For clarity and ease of use, it's better to keep them separate.

After thinking about it some more, and investigating what it would take to use radius for both, I have come to agree overall. There's still one thing I don't like about splitting the values, and that is if you want to control the 'zoom' of the camera, and dynamically switch between normal and orthographic projection, you would also need to switch between radius and scale. But that's likely an edge case, and isn't all that bad.

I'm happy with the PR as it is. Let's get this merged!

edit: just need a rustfmt. Also, it would be good to add some tests for the new util functions.

ghost commented 1 year ago

I think in perspective and orthographic cameras, radius and scale have different roles. Merging them could lead to confusion. Even in orthographic cameras, we still need radius to represent the position of the camera. For clarity and ease of use, it's better to keep them separate.

After thinking about it some more, and investigating what it would take to use radius for both, I have come to agree overall. There's still one thing I don't like about splitting the values, and that is if you want to control the 'zoom' of the camera, and dynamically switch between normal and orthographic projection, you would also need to switch between radius and scale. But that's likely an edge case, and isn't all that bad.

I'm happy with the PR as it is. Let's get this merged!

edit: just need a rustfmt. Also, it would be good to add some tests for the new util functions.

I've run rustfmt and have added tests for apply_limits and approx_equal.

Plonq commented 1 year ago

Thanks again for the contribution! I'll get a release out soon.