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 2d camera frame delay #46569

Closed
wants to merge 1 commit into from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Mar 1, 2021

2d cameras currently often give a frame delay between moving the camera and see the effects being updated.

This PR adds an option to use a more optimized scroll_update (guaranteed no more than 1 per frame) which occurs after all other scene tree updates, removing the frame delay. This is exposed as a property of the camera.

NEW : The property is now called delay and defaults to true, which is compatibility with the old camera. You will have to switch it off to get the improved camera. I think this makes sense for compatibility and how little time we have during RCs, and is safer against any regression.

Fixes #28492
Fixes #43800

Camera Notes

Although #43995 works, it is not very practical for merging as is as we need to have this switchable, in case of problems (at least to start with).

Using a dirty flag arrangement allows protection against multiple transform updates in a single frame hitting the Camera2D, which would result in several calls to _update_scroll in #43995. On the other hand deferring the _update_scroll requires keeping a list of pending cameras, and some care as to when the flush is made so as to work consistently with the existing order of operations.

(You may need to download these gifs to see at 60fps, depending on your browser. The demo is based on @Securas2010 project from #46504, with a camera child of the player, which should in theory give a static player in the centre of screen. This doesn't work correctly at present (because of the frame delay) but does with the PR.)

Existing Camera2D (delay present, causing ugly jitter)

cam_delay

This PR with delay switched off for Camera2D

cam_no_delay

Test project

Run as is to see the jitter problem as a result of camera delay. Then select the camera, and turn off delay, and rerun, to see the new method.
test_camera_delay.zip

@lawnjelly
Copy link
Member Author

This PR is now separate from the snapping, which is now in 2 separate optional PRs. I shouldn't really have combined them originally as they are only indirectly related.

The no delay option for the cameras is a vast improvement in some games, and should be fine for 3.2.4.

@lawnjelly lawnjelly changed the title [WIP] Fix 2d camera frame delay + more snapping options Fix 2d camera frame delay Mar 3, 2021
@lawnjelly lawnjelly marked this pull request as ready for review March 3, 2021 10:28
@lawnjelly lawnjelly requested review from a team as code owners March 3, 2021 10:28
doc/classes/Camera2D.xml Outdated Show resolved Hide resolved
Comment on lines +580 to +581
// flush pending camera scrolls
Camera2D::flush_pending_scrolls();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not fond of how this breaks encapsulation by having the SceneTree responsible for flushing a particular node. Might be OK as a temporary workaround but I'm not sure it's a good design for the long term.

Copy link
Member

Choose a reason for hiding this comment

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

This could be moved to Viewport. Somewhat related: #38317

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't absolutely sure on this either. It just needs a good place to flush it, so welcome ideas. Reduz also suggested looking at setting priority for process, but I was a bit worried about order of operations. Something in the process might have needed an update transforms before the camera updates, and we'd be back to the problem of a frame delay.

So this pattern of update everything else -> flush transforms -> scroll cameras -> flush transforms could be crucial for it to work perfectly.

@lawnjelly
Copy link
Member Author

lawnjelly commented Mar 4, 2021

Moving flush to viewport

Having a little look at the viewport. It does look possible the flush could be rigged up during idle process of the viewport, provided the priority were such it was the last thing processed. The viewport would have to register an interest in idle process, and maybe keep track of the current 2d camera (or we could simply use the existing LocalVector, and forget about current camera, as in most cases the pending scroll camera would be the current camera, and keeping track of current camera is potentially complex and error prone as the PR kobewi referenced shows).

The problem seems to be that this occurs before the timers in the SceneTree idle. If the timers move a camera, it would reintroduce the frame delay.

So although it seems deceptively simple, without evidence to the contrary, doing camera scroll flush as the very last thing before rendering seems the least likely to have order bugs.

Encapsulation

For the encapsulation, we could replace the Camera2D::flush_pending_scrolls() directly with an indirect call like:

call_group_flags(GROUP_CALL_REALTIME, "_viewports", "update_idle_final");

Or something like that and have a function update_idle_final that then calls Camera2D::flush_pending_scrolls(). But then the call_group_flags does a bunch of stuff which just isn't necessary at this point in time. It would probably be worth it if we in the future decided that it was worth tacking on other functionality at this point in the ordering. And it could be worth it if there is a problem having a direct dependency between scene tree and Camera2D.

Location

Noteworthy : The flush is called outside the root_lock. But afaik camera scroll only should change positions (actually I haven't looked in detail, it looks like it either draws the camera bound in the editor, or just calls a function in the visual server), I'm not sure what the lock is to protect against.

Also: It is after queue delete. Camera could be deleted between pending and being flushed. However the flush calls ObjectDB::get_instance so should just return NULL and ignore deleted cameras.

That said, this is literally the first time I've looked at the ordering in the scene tree idle iteration, so I can't say 'this is the right way', only that from the information I have so far it seems reasonable.

Risks

Having the default for cameras be the old style and thus the flush_pending_scrolls etc effectively be a no-op does seem protective against any major problems. Worst case, we could just say 'don't use it in such and such circumstance', or revert it if it shows problems in the (presumably last) RC. So it seems pretty low risk of being a breaking change.

scene/2d/camera_2d.h Outdated Show resolved Hide resolved
doc/classes/Camera2D.xml Outdated Show resolved Hide resolved
2d cameras currently often give a frame delay between moving the camera and see the effects being updated.

This PR adds an option to use a more optimized scroll_update (guaranteed no more than 1 per frame) which occurs after all other scene tree updates, removing the frame delay. This is exposed as a property of the camera.
@lawnjelly
Copy link
Member Author

Superceded by #46697.

@akien-mga akien-mga added this to the 3.3 milestone Apr 20, 2021
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.

4 participants