Change in Camera2D.current behavior in 3.3.2

[noidexe]

Godot version

3.3.2.stable.official

System information

Windows 10, GLES 3, GTX 2060

Issue description

In Godot 3.2.3 and previous versions, if you had more than one camera in a scene with "current" activated, the last one in tree order would become the current camera.

For example here lvl_camera shoud become current when the scene loads.

In Godot 3.3.2 it seems it's no longer the case. The first camera in tree order seems to be the one that becomes current. There is something even more weird. In the same example if I move the Player node below the lvl_camera node like this:

When the scene loads lvl_camera will be current but as soon as the player moves, the camera inside the player will become current.

Another thing I noticed is that if I check the remote tree both cameras have current set to true. It also seems to happen only when the camera is part of an instanced scene.
Maybe something wrong here? https://github.com/godotengine/godot/blame/ee115d7b2d5385a80710c0f0a16009640193ff93/scene/2d/camera_2d.cpp#L417

Steps to reproduce

1. Open Node2D.tscn in the provided project
2. run the scene
   Expected: Only one camera should have current set to true. lvl_camera should be active
   Actual: Both cameras have current set to true. player/camera is active

Alternative test:

1. Open Node2D.tscn in the provided project
2. Swap the tree order of player and lvl_camera so that player comes last:
   
3. Run the scene
4. Move the player using arrow keys
   Expected: Only one camera should have current set to true. player/camera should be active
   Actual: Both cameras have current set to true. lvl_camera is active until you move the player, then player/camera becomes active. Both still show current ☑ in the remote tree

Minimal reproduction project

CameraBug.zip

[Calinou]

Can you test the 3.2.4 betas/RCs and 3.3 RCs to know when the regression started? This will greatly speed up troubleshooting 🙂

[noidexe]

Can you test the 3.2.4 betas/RCs and 3.3 RCs to know when the regression started? This will greatly speed up troubleshooting 🙂

It was introduced in 3.2.4 rc 4
I don't have time today to build from source and test but I think it must be something in either of this two commit by @lawnjelly :
8763d89
136f94f

[lawnjelly]

Most likely to be the process logic PR, I can take a look tomorrow.

[akien-mga]

Diff between 3.2.4 RC 3 and RC 4: 4f891b7...dc99f04

Yes 8763d89 seems like a likely candidate, though I don't immediately see how it would impact current.

[lawnjelly]

Am looking at this now. It is possible that the changes exposed a pre-existing bug (or not dealt with properly situation). What doesn't seem to be considered at all is this situation where you have a camera that is current that is added to a scene where another camera is already current, allowing two cameras to have their bool set.

Yes I can confirm in 3.2.4 RC3 (before the changes) both cameras were also current (as seen in the remote scene tree). So the old behaviour was also buggy, but the result seen was different due to when _scroll_update is called between the two camera nodes.

This means that whenever either camera receives a transform notification, it tries to _update_scroll and change the viewport position (which is ultimately how the camera works), so the results are not well defined, and accidental in either the old or new behaviour.

Rule for adding new current camera to the scene tree

The proper thing to do is have some logic to deal with the situation where a current camera is added to the scene tree and there is another camera current already. What should happen? Should the newly loaded camera take priority? Or should the old one stay current and the new one be set to not current?

This looks fairly easy to fix but we need to decide on a standard. The current behaviour (and that before 3.2.4 rc4) may have been undefined, and just produced accidental results as seen. The only snag is this could be breaking behaviour in some cases, whichever standard we go with. Mind you as the old behaviour is essentially undefined, that is not a reason not to fix it.

I suspect the best behaviour to go for is that the most recent camera with current bool set to receive NOTIFICATION_ENTER_TREE should call make_current, which will set any other current cameras to non-current. i.e. The most recent loaded camera wins.

Let us know thoughts on this. If we can come to an agreement, it should be easy to make a PR.

[akien-mga]

Fixed by #50112.