Cleanup and simplify camera override API

[rxlecky]

• Make the camera override 2D and 3D APIs use the same terminology
• Make both 2D and 3D override use Camera nodes under the hood to make the code simpler and cleaner
• Make camera override 2D use global canvas transform to make the override camera behave identically to the 2D editor camera, i.e. make CanvasLayers move with the camera regardless of their follow_viewport property value
• Makes all code depending on the viewport current camera, i.e. VisibleOnScreenNotifiers, work with the camera override out of the box

The change to using camera nodes also has potential downsides. If there's gameplay code depending on current camera, it might cause unexpected behaviour when the current camera suddenly changes to the overriden camera. I had a person ask me whether using the camera override will make the environment unload if they are using visibility culling, causing, for example, the player character to fall through the floor. Similar arguments were the reason, why I originally opted for only creating camera on the RenderingServer without creating the actual camera node. That way, everything would work as if the override camera never existed.

However, reevaluating that now, duplicate code had to be written to support the same functions that the camera nodes already support. Additionally, there are cases where we do want the override camera to work as normal camera, example would be the case described in the issue #49334. All these would have to be catered to separately if the camera override didn't use the camera nodes, whereas now they all "just work". Lastly, to follow up on the visibility culling argument, I think the camera override wouldn't be really that useful if nothing besides the area in player's close vicinity would be loaded and rendered anyway.

Any feedback is welcome. If there's another approach that I haven't thought of, I'm happy to rework this. Cheers!

Closes #49334

[rxlecky]

Oh, actually, #49334 won't be fixed since that's a 3.3.2 bug. Nonetheless, the same bug is present in master which this will fix and I'm happy to backport this once it's approved and merged.

[akien-mga]

ObjectID is default initialized so you don't need to assign a new one here (same below).

[rxlecky]

Ah, good spot! Thanks, fixed now. 🙂

[rxlecky]

@akien-mga unless there are further comments on code cleanliness, this should be good to be merged, I'm just finishing the merge conflict resolution, it looks rather trivial.

But I'd still wait before merging. I'm not entirely sure whether it was @reduz 's suggestion back when this was first implemented to not use nodes or it was my initiative. Also, besides switching to nodes, this PR also switches from the regular canvas_transform to the global_canvas_transform for the 2D camera override. The former makes it behave and look like the gameplay camera, whereas the latter will make it behave more like the editor camera.

It would be good to get his - or other experienced member's, maybe @Calinou since this is partially a UX change as well - opinion on these aspects before merging.

[rxlecky]

@akien-mga do you think this could get reviewed sometime soon? I'm happy to merge and rebase but I'm putting it off for now until someone has the time for review.

[KoBeWi]

Camera overrides were replaced by the Game tab: #97257
The linked issue still needs fixing though.

[rxlecky]

Alright, I got around to doing this quite a bit later than I wanted but I think I've got the changes ready now. I started from scratch but the original goals stayed mostly unchanged, including fixing the linked issue.

The only goal that's changed is switching to using the global transform for override 2D. The change made sense at the time when the only way to control the override camera was via the 2D editor camera which uses global transform. The difference in behaviours made for a bit jarring user experience. But now that the override camera can be controlled directly from the Game tab, I think leaving it as is is the better option to leave it consistent with regular cameras. My understanding is that the editor camera style of control is only left for compatibility and/or systems that don't support window embedding anyway.

[rxlecky]

Now, let's see if I can wrangle git to reset the branch to be up to date with master first. 😆

[rxlecky]

Alright, I believe all check failures should be resolved now. The only real thing I had to change was wrap the camera override code with DEBUG_ENABLED guard instead of TOOLS_ENABLED that I used initially. This is because the RuntimeNodeSelect class that handles the Game tab view interactions in the running game and uses the override is also using DEBUG_ENABLED. And I guess while the camera override is primarily used in the editor, it is a debug feature, not an editor-bound feature, so using DEBUG_ENABLED makes more sense regardless .

[KoBeWi]

2D zooming is broken. It does not zoom on cursor.

godot.windows.editor.dev.x86_64_tdZfnu3n85.mp4

Expected (in editor):

r2ionMO8pk.mp4

[rxlecky]

Well spotted, thanks for testing! I'll have a look at fixing this asap.

[rxlecky]

Alright, 2D zooming should be fixed now.

[rxlecky]

Related to that, I'd be keen to discuss with someone responsible for Camera2D code if it would be viable to do a refactor of get_camera_transform() function, decoupling it from the update responsibility that it's serving right now and exposing it as public member function. There are two problems I'm encountering at the moment:

• There's no way of getting the true, combined camera transform
• All other transform-related members that are accessible - e.g. get_camera_screen_center() and get_camera_position() - rely on get_camera_transform() being run every frame to update their cached values. If it isn't - e.g. due to the camera not being current - they report stale values.

[rxlecky]

I guess it's also stemming from the fact that there's currently no way to access to the combined camera transform why functions such as get_camera_screen_center() and - to an extent - the newly proposed get_camera_current_rotation() (#106690) are being added just to fill in that gap.

[KoBeWi]

Works correctly, though I tested mainly 2D.
I checked the code and looks like nice simplification.

[Calinou]

I had a person ask me whether using the camera override will make the environment unload if they are using visibility culling, causing, for example, the player character to fall through the floor.

This particular scenario can be handled in the character controller by freezing the character if they stand in an unloaded area. The floor will reappear below their feet when the camera gets close enough again to reload the area.

[Calinou]

Tested locally, it works as expected.

Example in the 3D Waypoints demo, which uses projected Controls in 2D space to match the 3D world:

Before

waypoints_camera_override_before.mp4

After

waypoints_camera_override_after.mp4

However, when I uncheck the camera override after moving the camera and switching to Input mode, I will always get a script error:

Video that shows how to reproduce the error (it's most reproducible if you toggle the camera override on and off quickly):

waypoints_camera_override_script_error.mp4

Adding a null check in the project code avoids it, but not every project will have this check in place. Also, if I add a null check to early return at the top of _process(), the whole block will eventually stop running when I would have gotten an error without the null check:

waypoints_camera_override_null_check.mp4

[rxlecky]

Thanks for the review guys! I addressed your remarks and went over the code and fixed a few more formatting and spelling mistakes. I will do a final pass once the changes are finalized and the proposed changes are accepted.

On a side note, shouldn't codespell catch spelling mistakes - such as overriden -> overridden in my case? Or should it not be relied upon as much?

[rxlecky]

I had a person ask me whether using the camera override will make the environment unload if they are using visibility culling, causing, for example, the player character to fall through the floor.

This particular scenario can be handled in the character controller by freezing the character if they stand in an unloaded area. The floor will reappear below their feet when the camera gets close enough again to reload the area.

I'm sure there's a way to work around this. It just feels wrong to me that in order to use an editor tool the user needs to add code to handle that special case. 🤔 I'm not saying we shouldn't follow through with the changes for now but it may be worth looking into possible out-of-the-box solutions. For example, investigating a viability of adding a feature in visibility notifiers/enablers that would allow choosing whether they should only consider the override camera or both the override and overridden cameras for determining visibility.

Also, I intentionally didn't expose the override API in the scripting bindings because, again, I think the usage of the tool should ideally be transparent to the user and I find it to be quite an elegant solution that Viewport reports the override camera as it would report any other current camera. But if we expect users having to handle the override camera as a special case, maybe it would be worth exposing the override API to give users the tools they need to handle it?

[rxlecky]

when I uncheck the camera override after moving the camera and switching to Input mode, I will always get a script error

The error is thrown because the camera instance is freed when override is turned off and a new one is created when it's turned on again. I find it more straightforward to just start with a fresh instance rather than keeping the old one around to be reused later. I can change it if caching instances is the more common approach in the codebase or more desirable in this situation. But I would argue that in this case the problem is that the script is caching and using an instance that gives no guarantees about its lifetime. The correct approach would be to either verify it's still valid using is_instance_valid(...) before use or query Viewport for the current camera every time it's needed.

[rxlecky]

Also a side-effect of this change is that the override cameras now show up in the remote scene tree when camera override is in use:

... and frankly, I'm not the biggest fan of this. It's an unnecessary visual clutter in the best case and a way for user to unwittingly change things and break the tool or crash the editor in the worst. It made me wonder if internal child nodes should not be hidden in the remote tree? Or would that cause an issue if such node ever needs to be displayed in the inspector?

[KoBeWi]

Remote tree is supposed to show all nodes.
The camera is named, so it shouldn't be a problem. You can't delete remote nodes and changing it won't crash anything.