Physics running on separate thread with max fps enabled causes the physics server to be synchronized every frame when a collision happens

HexagonNico commented 3 months ago

Tested versions

Tested in v4.3.beta3.official [82cedc83c]
Does not happen in beta 2

System information

Manjaro Linux - Godot v4.3.beta3 - Forward+

Issue description

When physics/2d/run_on_separate_thread or physics/3d/run_on_separate_thread is enabled and application/run/max_fps is set to any value higher than zero in the project settings, moving a CharacterBody2D or a CharacterBody3D with move_and_slide will cause this warning to be printed in the console every time it collides with something:

Call to body_get_collision_layer causing PhysicsServer2D synchronizations on every frame. This significantly affects performance.

This does not happen if physics/2d/run_on_separate_thread is set to false and application/run/max_fps is set to something higher than zero or if application/run/max_fps is set to zero and physics/2d/run_on_separate_thread is set to true.

The warning is not printed in v4.3 beta 2.

Steps to reproduce

Go the Project Settings
- Set Application -> Run -> Max fps to 60 (or any other number)
- Set Physics -> 2D -> Run on separate thread and Physics -> 3D -> Run on separate thread to true.
Create a scene with a CharacterBody2D or CharacterBody3D as root and add a collision shape.
- Create a script attached to move the body using move_and_slide. This also happens with the built-in template.
- Create a scene that contains the character that was just created and any other collider.

Running the project now will cause the warning to be printed every time there is a collision.

Minimal reproduction project (MRP)

issure-reproduction-project.zip

clayjohn commented 3 months ago

I suspect this is a valid error unfortunately. Due to a bug, these sync errors weren't being reported until https://github.com/godotengine/godot/pull/93367 (merged in Beta 3).

The error checks if a function is causing excessive synchronizations between the physics thread and the main thread. I can see that it is possible for move_and_slide() to call body_get_collision_layer.

When you set max_fps, you end up running the physics tick multiple times per frame, which results in body_get_collision_layer being called more times in a frame which triggers the error.

If I am correct, this new error message has identified an area where Godot's physics system is not functioning correctly. Normal use of move_and_slide should not require syncing the physics thread to the main thread

I suspect that the original regression is from https://github.com/godotengine/godot/pull/52889 and we just never caught it since we didn't have proper debugging tools

I suggest that for 4.3, we just disable this error tracking and then re-enable it in 4.4 with a goal of finding and fixing all the issues it catches

HexagonNico commented 3 months ago

Maybe it should be specified in the documentation for those two options that they shouldn't be used together until the issue is resolved?

clayjohn commented 3 months ago

Maybe it should be specified in the documentation for those two options that they shouldn't be used together until the issue is resolved?

I think it is fine to use them together for now as it is no worse than it has been for the last 3 years. The combination of those two things is just triggering the error. The underlying problem is there whenever someone uses move_and_slide(), it just doesn't always trigger our error detection logic on its own.

Wolve-3DTech commented 3 months ago

Yesterday i've ported my project from 4.3 beta2 to 4.3 beta3 and i've ran into the same issue.

rburing commented 3 months ago

The issue is here (as also analyzed by @clayjohn above) in CharacterBody3D::_set_platform_data: https://github.com/godotengine/godot/blob/97b8ad1af0f2b4a216f6f1263bef4fbc69e56c7b/scene/3d/physics/character_body_3d.cpp#L609

This is used for the character body "platform floor and wall layers" feature:

https://github.com/godotengine/godot/blob/97b8ad1af0f2b4a216f6f1263bef4fbc69e56c7b/scene/3d/physics/character_body_3d.cpp#L51-L58

One way around the problem would be to add collider_collision_layer to the PhysicsServer3D::MotionCollision struct, so that the first mentioned line can be replaced by platform_layer = p_collision.collider_collision_layer. I don't think other engines have this in their collision/hit struct, and to me it feels out of place to have it there. Doing this would be further complicated by the fact that the MotionCollision struct is exposed to GDExtension (as PhysicsServer3DExtensionMotionCollision), which tries to maintain forward-compatibility. The compatibility code would be hacky: add PhysicsServer3DExtensionMotionCollision2, add PhysicsServer3DExtensionMotionResult2, add body_test_motion2 in PhysicsServer3DExtension; inside CharacterBody3D, check if the physics server is from a GDExtension and overrides body_test_motion; if so, keep the legacy code path that triggers the valid warning from this issue. (And the same for 2D.)

As an alternative I thought collision_layer could be added to PhysicsDirectBodyState3D, but it seems this would not help in the threaded case (but I'm not an expert on threading):

https://github.com/godotengine/godot/blob/97b8ad1af0f2b4a216f6f1263bef4fbc69e56c7b/servers/physics_server_3d_wrap_mt.h#L263-L267

I don't know how else to solve it while keeping the "platform floor and wall layers" feature intact. I wonder how much this feature is used in the wild (i.e. with platform_floor_layers and platform_wall_layers set to non-default values). Could it be replaced by platform floor and wall node groups? Would that help? I guess it would be more annoying to set up.

cc @fabriceci

akien-mga commented 3 months ago

The regression / warning spam is mitigated for 4.3 with #94279.

Keeping the issue open for the 4.4 milestone to address the underlying issue properly (which is not a regression).

godotengine / godot