Skip to content
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.

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

Fix Camera2D frame delay #46697

Merged
merged 1 commit into from
Mar 5, 2021
Merged

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Mar 5, 2021

Fixes camera frame delay by always doing a scroll update except when smoothing is active.

From discussion with reduz we identified the problem of the frame delay comes down to two things:

  1. The scroll update when receiving transform notification is only done when process and internal process are inactive, which was probably a longhand for when the smoothing was off.
  2. Users can create scripts which turn on process (and this PR Fix continious update with Camera2D #4407 possibly), or something is (see below) thus this logic fails.

The solution was to check directly a flag for when smoothing is switched on.

Fixes #28492
Fixes #43800

Notes

  • Also although in theory it should be on all the time because it is a bug, I want to sound out whether we should have a bool to allow the previous delay, which could be seen as a feature(!) by some 😁
  • This is alternative approach to that in Fix 2d camera frame delay #46569
  • Also I've spotted that in _update_process_mode() it sets the process mode as a one-off depending on whether you have it set to CAMERA2D_PROCESS_IDLE or not. This could also be the root cause of the bug, because it is not switching this on and off according to whether smoothing is on or off, so causing the logic to fail.

Fixes camera frame delay by always doing a scroll update except when smoothing is active.
@lawnjelly lawnjelly requested a review from a team as a code owner March 5, 2021 15:52
@lawnjelly lawnjelly requested a review from reduz March 5, 2021 16:26
@YeldhamDev YeldhamDev added this to the 3.2 milestone Mar 5, 2021
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! As discussed we'll have to do an in-depth review of the (internal) processing logic in this class as it seems to be a bit inconsistent, which can definitely lead to bugs that look like delays.

I don't think we need to add an option to preserve the old behavior. Let's fix it and see if anyone complains about the fix :)

@akien-mga akien-mga merged commit b993feb into godotengine:3.2 Mar 5, 2021
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the camera_2d_delay_logic branch March 5, 2021 19:17
@reduz
Copy link
Member

reduz commented Mar 5, 2021

Yes, processing is enabled wrongly, so this is not a complete fix, it may break during process.

@oeleo1
Copy link

oeleo1 commented Mar 6, 2021

@lawnjelly Let me get this right. We previously introduced the suggested workaround consisting in applying in the camera's process() function the player's global transform to the camera's global transform and calling force_update_scroll() manually because it wasn't called (we knew that for sure) due to the bug.

Now with this fix, it sounds like keeping this workaround code would cause calling force_update_scroll() twice, which is presumably expensive, once manually and once internally at the end of process() but in some circumstances which are beyond me it may still not be called internally. Is that correct? Or is it "safe" to drop the manual workaround code? Or is it safe to keep it without the double call penalty?

The above comments sound like there is no clean way out now with a guaranteed 1 call only to "force_update_scroll()", be it manual or internal. What is the suggested path going forward? Maybe it's better for me to forget about all this right away and trust The Engine... :-)

@lawnjelly
Copy link
Member Author

Haha. I shouldn't worry too much. 😁

With the fixed camera, there should be no need to manually call force_update_scroll(). I'm not entirely convinced it was as expensive as first thought (I believe it just sets a transform in the visual server, when not in the editor).

With smoothing turned on, reduz believes there should be no need for the workaround. Actually we should probably test this in practice just in case, you know what they say about assumption...

The only gotcha is that if you had a game where you did like the frame delay behaviour (for cosmetic reasons) it will no longer be present, and you'd have to put it in manually. There could have been an option to emulate the old behaviour but the others didn't think this necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants