godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.43k stars 21.26k forks source link

[Physics Interpolation] Interpolated triggered from outside physics process when using multi-threaded rendering. #97175

Open drwhut opened 2 months ago

drwhut commented 2 months ago

Tested versions

Cannot reproduce in earlier versions, as in-built 3D physics interpolation was introduced in 3.5.

System information

Linux Mint 22

Issue description

Specifically when using the multi-threaded rendering model while physics interpolation is enabled, every so often this warning is generated while rigid bodies are in motion:

W 0:00:15.011   instance_set_transform: [Physics interpolation] Interpolated triggered from outside physics process: "/root/Scene/@Cube@2/CollisionShape/MeshInstance" (possibly benign).
  <C++ Source>  servers/visual/visual_server_scene.cpp:892 @ instance_set_transform()

The rigid body that generates the warning is random, and I'm pretty sure the warnings are actually rate-limited in 3.6, since in 3.5, the warnings are essentially spammed out.

I would like to keep using the multi-threaded rendering model for my project, as I need to instance meshes in a separate thread, but this warning is somewhat annoying. Is there a nice way to solve this issue, or is my only option to hide the warnings?

Steps to reproduce

  1. Open the reproduction project.
  2. Run the scene, and let physics do its thing for about 30 seconds. Since by default Godot uses single-threaded rendering, there should be no warnings generated.
  3. Stop running the scene, enter the project settings, and under Rendering > Threads > Thread Model, change to "Multi-Threaded".
  4. Run the scene again, and wait about 30 seconds again. This time, the warning should appear at regular-ish intervals.

Minimal reproduction project (MRP)

InterpolationWarning.zip

lawnjelly commented 2 months ago

I'll have a look at this properly tomorrow, but my general recommendation is not to use multithreaded rendering with physics interpolation.

I'm not convinced multithreaded offers any significant advantages for performance, and in my experience it can significantly muck up timing, and may have race issues (not a lot of people use it, so it isn't as thoroughly debugged).

Have you been able to get better performance using multithread rendering?

If you are getting warnings, then it is suggestive multithread is updating things outside of assumed sync points, which will lead to jittery behaviour (so hiding the warnings is probably not the solution).

You can turn off warnings with project setting: debug/settings/physics_interpolation/enable_warnings. You are correct it is timed to avoid spamming, so the actual object that triggers it may be somewhat random, if you have multiple objects updating outside physics tick.

drwhut commented 2 months ago

Have you been able to get better performance using multithread rendering?

I don't have it enabled necessarily for the performance gain (for my project, it barely makes a difference because of how few objects there are), but from what I remember reading in the documentation, instancing anything that renders something (like a Sprite or MeshInstance, iirc) outside of the main thread was not thread-safe, unless multi-threaded rendering was enabled. The problem is, instancing MeshInstance in a separate thread is now a necessity for my project.

If this requirement no longer applies, and I am safe to use single-threaded rendering, then I will probably do so to avoid this issue altogether, as disabling the warnings feels wrong to me :P

lawnjelly commented 2 months ago

Had a look at the MRP, and it does appear like the physics / rendering is running out of sync in multithread render mode, hence the interpolation warnings.

Essentially physics interpolation relies on things being called in a specific order:

If during multithread-render mode it is attempting to update physics objects within the render frame, then things will go wrong, best case you'd get subtle glitches in rendered transforms, as the previous and current transforms changed in and out of sync, as well as getting the warnings.

What may be worth adding on my side is a warning at startup to this effect when the combination of physics interpolation and multithread rendering is used, that the combination is not recommended and not guaranteed to work correctly. I'm not sure that adjusting multithread render syncing is worth it at this stage in 3.x lifetime.

The docs do say:

Instancing nodes that render anything in 2D or 3D (such as Sprite) is not thread-safe by default. To make rendering thread-safe, set the Rendering > Threads > Thread Model project setting to Multi-Threaded.

Note that the Multi-Threaded thread model has several known bugs, so it may not be usable in all scenarios.

I'm not aware that we've changed anything recently to make instancing these nodes thread-safe, so this advice still stands.

drwhut commented 2 months ago

Thank you for digging into this! That makes sense, but that essentially means I've boxed myself into a corner without realising, as I need the multi-threaded model, as well as physics interpolcation so that rigid bodies move smoothly on monitors that are faster than 60Hz.

I think it's definitely worth adding this to the documentation, and potentially to the tooltip for the thread model in the settings as well.

As an aside, I did notice as I was experimenting with the project settings in the MRP, that there are actually two modes for the single-thread model: safe and unsafe. I still have some hopium left in the tank, so I'd like to ask for clarification on what it means exactly by being safe vs. unsafe: what I assume is that the safe version has locks and mutexes in place in the rendering code, whereas the unsafe version does not (for performance gains), however I'm not sure at all, since I can't find anything about these modes in the documentation.

So does the "single-safe" thread model mean what I think it means, in that I can instance visual nodes outside of the main thread without the engine crashing? If so, that is 100% my course of action. Otherwise, I don't think I have any choice but to hide the warnings, unfortunately.

lawnjelly commented 2 months ago

I have never looked into this before, but it looks to me as if:

VisualServerMT has a command queue, whereas I assume regular visual server (unsafe) just runs commands as they come in.

Whether "single safe" means it is ok to create visual nodes from another thread, I really have no idea and can't confidently answer, you would probably need info from one of the ye olde maintainers like reduz (if it isn't in the docs). Whether that documentation was written before a split into single-safe and single-unsafe I don't know, and agree it is rather vague.

drwhut commented 2 months ago

That sounds promising! But yeah, I'll see if I can get extra confirmation first before committing to using it :+1:

drwhut commented 2 months ago

Just as an update, I've tried using single-safe with my project multiple times to see if it crashes, and thankfully it hasn't crashed once. That, plus the fact that when I tried to use single-unsafe it immediately crashed the game, means I'm very confident I can use the single-safe model as a workaround for this issue. :+1: