Refactor Camera2D to separate property and display updates

[madmiraal]

Currently, almost any property change to Camera2D and almost all internal notifications call _update_scroll() which updates the Viewport transform by calling _get_camera_transform(). However, _get_camera_transform() is responsible for:

1. Updating the camera's target position (adjusted for drag margins and limits)
2. Updating the camera's current screen position (adjusted for smooth scrolling and limits).
3. Creating the Viewport's new Transform2D

This causes updates and recalculations to occur not only unnecessarily, but also undesirably. For example, changes to Camera2D's properties shouldn't cause updates to the current screen position. Therefore, currently, every time a property is updated, the changes to the current screen position need to be undone (see #2764, #39567, #63580 required to fix #2074, #16323 and #63083 respectively). Furthermore, making _get_camera_transform() responsible for everything results in various difficult to troubleshoot bugs (see #44358, #56336, #63330, #63572, #63738, etc.). To make matters worse, users call force_update_scroll() or even the hidden, but bound, method _update_scroll() to try workaround bugs.

This PR refactors the private methods _update_scroll() and _get_camera_transform() and adds:

• void _update_viewport()
• void _update_size()
• void _update_position()

_update_position() is used to update the camera's target position based on the Node2D position and adjusted for drag margins and limits. It is called every time the position is updated or any of the properties (drag margins and limits) that affect the target position are changed.

_update_scroll() is now only called by the internal process notifications and only does anything when smoothing_enabled is true. It is used to close the gap between the current camera position and the target position in a consistent manner.

_get_camera_transform() now only calculates the Viewport transform based on the current camera position, zoom and, if rotating is true, rotation.

_update_viewport() is used instead of _update_scroll() to update the Viewport or the editor.

_update_size() is used to update the new screen_size variable when the screen size or zoom changes; instead of recalculating it every time it is needed.

This PR also includes forward ports of:

• Improve process logic in Camera2D #46717
• Camera2D: Update scrolls when the Viewport is resized #46829
• [3.x] Fix crash when deleting Camera2D and Viewport assigned to it #53459

In addition, some internal variables have been renamed to make them easier to understand:

• camera_pos -> target_position
• smoothed_camera_pos -> current_position
• smoothing -> smoothing_speed (along with its getter and setter)
• first -> initialised (and reversed the logic)

And unneeded methods have been removed or unbound:

• _get_camera_screen_size(): removed
• force_update_scroll(): removed
• _update_scroll(): unbound

Fixes #44358
Fixes #49747
Fixes #63572
Fixes #63738
Closes #56336

This PR also includes some additional enhancements:

1. When the Viewport size is larger than the camera limits, which can happen when zooming out, it interpolates the camera position between the limits; instead of the current arbitrary behaviour (Camera2D limits don't clamp camera position #62441 (comment)) of locking it to the top-right corner due to the internal order of calculation.
2. As identified here and here, it ensures that scroll updates do not overshoot.
3. It updates the editor to only allow sensible values for smoothing_speed between 0 and 60 and drops the incorrect px/s suffix.
4. Finally, in addition to fixing Camera2D smoothing calculation appears incorrect #49747 i.e. reverts the change made by 1063ddf to smoothing_speed to describe it as the speed is in pixels per second when it isn't, it also makes some minor documentation updates to hopefully make other methods and properties clearer.

[MewPurPur]

poke poke?

[lawnjelly]

This looks great. 👍

I'm wondering whether it would be possible to have separate commits (or separate PR, and then build on it in this one?) for the existing forward ports of the 3 PRs from 3.x, then a separate PR for the improvements here? That might make it a little easier to review.

[YuriSizov]

I'm wondering whether it would be possible to have separate commits (or separate PR, and then build on it in this one?) for the existing forward ports of the 3 PRs from 3.x, then a separate PR for the improvements here? That might make it a little easier to review.

If you plan on reviewing them, I don't see why not. cc @madmiraal Can you split your work into forward ports and enhancements?

[lawnjelly]

Isn't the point of this that if current is false, there's no need for clear_current() to call_group etc?
i.e. should there be a noop check at the top?
if (p_current == current) return or something like that? Or must it be bypassed?

[KoBeWi]

Yeah, call_group doesn't need to be called. In fact, it will remove any current camera, which is probably not desired.

[madmiraal]

Okay. I've updated the PR with a refactored set_current() to make things simpler and clearer.
Basically, when calling set_current(true) it now calls _clear_current() on all cameras in the Viewport, which just sets current to false and redraws it; It then sets this camera to be current.
When calling set_current(false) it just sets this camera to not be current.

[lawnjelly]

Generally it looks pretty good I think. Probably would benefit from @KoBeWi or anyone else having a look through too.

Will require a bunch of testing with 2D projects in 4.x, and I can't really do this easily, I'm presuming @madmiraal has done this as much as possible.

I suspect ultimately it will probably have to be tried as one of those YOLO merges, and then watching for regressions, as the logic and order of operations are quite fiddly. But I agree it is worth refactoring and cleaning up Camera2D at this stage, before it becomes too difficult to change. 👍

[KoBeWi]

Needs rebase to make testing easier.

EDIT:
I looked through the code and it will conflict a lot with my PR: #65698
Especially the part about changing is_current() to current is not necessary would be reverted by that PR.

[KoBeWi]

I think you could just add an inline method is_smoothing_active().

[lawnjelly]

That's fine too. (That was ported from my PR actually lol 😁 ).

[madmiraal]

Yes, this is from the forward port of #46717.

[akien-mga]

Would be good to reach consensus on whether this can be merged soon for 4.0. There have been a few other minor compat breaking changes to the Camera2D API in the meantime (rotation smoothing, possibly further renaming of (follow) smoothing: #65779) which would need to be integrated too.

Feedback welcome from more users and contributors too to make sure this is what we want the Camera2D API to be like in 4.0. From a cursory look it seems to make sense to me but I'm no expert nor heavy user.

[akien-mga]

We sadly didn't really reach further consensus on this and it's too late to assess/merge now for 4.0, which is close to RC stage.

Let's aim to review/address those issues for 4.x, and see how to handle compatibility breakage (either accept to break compat anyway in a future 4.x release, or preserve it somehow if it doesn't make the API too awkward).

[awardell]

This doesn't fix #44358, but it shouldn't need to.