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

Physics interpolation - Fix multithreaded rendering #59875

Closed
wants to merge 1 commit into from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Apr 4, 2022

Fixes physics interpolation ticking and pre-draw not working in multithreaded rendering mode.

Fixes #59736

I moved the interpolation tick and frame updates to use the regular VisualServer macros, as discussed in the PR meeting. This worked fine in game, but I noticed a problem - the regular macros call DISPLAY_CHANGED and thus force a continuous screen redraw, particularly in the editor, as they are called on every physics tick and every frame.

So rather than go back to the more verbose previous approach, I've simply added two new BIND1 type macros which don't call DISPLAY_CHANGED. This seems to work and prevents the editor constantly updating.

I've done some research and debugging and worked out that in multithreaded mode it calls both the FUNC1 type macros and the BIND1 type macros. This explains why the PR works in both single threaded and multithreaded mode and confirms it is the correct fix.

I've also added a _physics_interpolation_enabled check in scene_tree.cpp before it calls the update functions, as they are currently only used for physics interpolation, just as an extra safety measure to ensure we aren't doing any extra processing when interpolation is switched off. If we end up using these functions for things in addition to the physics interpolation then we can remove this from the if check.

@lawnjelly lawnjelly added this to the 3.5 milestone Apr 4, 2022
@lawnjelly lawnjelly marked this pull request as ready for review April 4, 2022 14:36
@lawnjelly lawnjelly requested a review from a team as a code owner April 4, 2022 14:36
@akien-mga
Copy link
Member

See also #47232 which wants to fully remove draw_pending.

@lawnjelly
Copy link
Member Author

lawnjelly commented Apr 4, 2022

I don't know enough about the draw_pending to say whether removing it generally as per #47232 is a good idea, but it doesn't seem to be useful for the interpolation. Provided it is a FIFO queue as suggested in that PR, then this should be fine for the interpolation afaik, all it needs to do is update at the correct point within the tick / frame.

(These tick and pre-draw calls are only used by the interpolation BTW, I gave them more general names just in case we ended up wanting to use them for something else additionally in future).

@lawnjelly lawnjelly marked this pull request as draft April 5, 2022 12:43
@lawnjelly lawnjelly force-pushed the interpolation_fix_mt branch from b5a4468 to 901135e Compare April 5, 2022 16:31
@lawnjelly lawnjelly marked this pull request as ready for review April 5, 2022 16:35
@lawnjelly lawnjelly marked this pull request as draft April 6, 2022 06:15
@lawnjelly lawnjelly changed the title Physics interpolation - Fix multithreaded rendering [WIP] Physics interpolation - Fix multithreaded rendering Apr 6, 2022
@lawnjelly lawnjelly force-pushed the interpolation_fix_mt branch from 901135e to efe4cb8 Compare April 6, 2022 06:58
@lawnjelly lawnjelly changed the title [WIP] Physics interpolation - Fix multithreaded rendering Physics interpolation - Fix multithreaded rendering Apr 7, 2022
@lawnjelly lawnjelly marked this pull request as ready for review April 7, 2022 10:18
Fixes physics interpolation ticking and pre-draw not working in multithreaded rendering mode.
@lawnjelly lawnjelly force-pushed the interpolation_fix_mt branch from efe4cb8 to b611878 Compare April 11, 2022 08:06
@lawnjelly lawnjelly requested a review from a team as a code owner April 11, 2022 08:06
@@ -149,7 +153,6 @@ class VisualServerRaster : public VisualServer {
#define BINDBASE VSG::storage

/* TEXTURE API */

Copy link
Member

Choose a reason for hiding this comment

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

It would be better to keep this separation, like done for other section comments.

@lawnjelly
Copy link
Member Author

lawnjelly commented Apr 11, 2022

After talking with @reduz we decided to go back to the original approach I was using of having the interpolation data global, rather than associated with Scenarios. The association with Scenarios was done part way through the interpolation PR, in response to feedback and @jordo's request to have the on / off switch associated with the SceneTree, but it turns out you can only have one scene tree, so we can keep the interpolation data global.

This always made more sense to me as 99% of users just want to turn interpolation globally on or off, rather than deal with some special case.

This shouldn't be too involved to do and should make some aspects a little simpler in the VisualServer, and I'll do at the same time as this PR, in fact I'll probably end up just renaming this PR and adding the extra changes. 👍

@lawnjelly lawnjelly marked this pull request as draft April 11, 2022 10:50
@jordo
Copy link
Contributor

jordo commented Apr 11, 2022

After talking with @reduz we decided to go back to the original approach I was using of having the interpolation data global, rather than associated with Scenarios. The association with Scenarios was done part way through the interpolation PR, in response to feedback and @jordo's request to have the on / off switch associated with the SceneTree, but it turns out you can only have one scene tree, so we can keep the interpolation data global.

@reduz @lawnjelly We use multiple scene-trees for processing network rollback re-simulations. We also do this on a separate thread, asynchronously, to off-load this work to a background worker until a re-sync point. This is a typical method to run rollback simulation code needed to re-simulate & synchronize say a past server state on network clients to re-synchronize them, see below for video (second thread has different scene tree):

scene-tree.mov

The reason this is typically done in a separate thread is because this work can be performance intensive and load can sometimes be volatile. If your game ticks at 60hz, and your client's latency is something like say 150ms, then your re-simulations can typically involve simulating 8-10 frames/ticks at a time, which depending on game, can take a significant amount of cpu time.

In the video above, the re-simulation thread maintains a separate scene-tree, and waits for re-simulation jobs to be posted by the network layer. It runs them (re-simulates), then re-synchronizes with the main thread and posts finished jobs.

We need and use multiple scene trees.

You want resimulation code to run 100% on the exact same code paths that you would normally run. In idomatic Godot, this means Instancing nodes (some nodes contain physics bodies for example), adding them to others, adding to scene tree, removing nodes that get 'destroyed', etc. So when you are re-simulating game simulation, you actually just want to run the same code, not have special paths for anything as it leads to errors. For example, when re-simulating, all a developer's typical 'onready, _ready, _enter, _exit, code, etc can all run exactly the same in the re-simulation as what's currently happening on the main thread (wherever execution on main may be at the time).

We also tie this scene-tree to a different Rasterizer/VisualServer (yes in the same process space)... Since the re-simulation has no visual properties, we can link a dummy rasterizer to this thread and separate scene-tree. As the re-simulation scene-tree doesn't actually draw anything, we give it a dummy visual server.

In the techniques I describe above, it was particularly difficult to get working with the Godot code-base mostly due to design philosophies around global state & existing singleton patterns. 1) Singleton is a global-state design pattern - it has some pros, but also has cons as well. As you can imagine, VisualServer::get_instance, and it's interface is almost everywhere, so it's implementation is particularly difficult to inject as a dependency into any godot subsystem. We had to work around this so that a dummy implementation of VisualServer::get_instance could be registered per-thread, which gave us a dummy rasterizer to use on our resimulation thread, while main thread VS/rasterizer actually issues draw commands etc.

All this to say, is that generally global state in software design is not great for extend-ability, dependency injection/testing, and flexibility for future unknown scenarios. I do understand the 99% of users today sentiment, however it also can be limiting not to build and architect/design the codebase for the future.

To be clear, I'm not advocating for a new feature or anything, I'm advocating for a way to not to make this state global in the application's process space. That would really be appreciated and good design. Perhaps this state should be associated with the VisualServer impl then, instead of something that is process space global. But logically, the interpolation state needed for interpolant system to do it's job's between visual frames is very much associated with a scene-tree, so logically that's the area I would store interpolation state. Whether or not that's called a Scenario or something else perhaps is a implementation detail but I would really encourage this to not be something that is process space global.

@lawnjelly
Copy link
Member Author

Superseded by #60147.

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.

3 participants