Fix KinematicBody3D having a duplicate locked_axis variable

[aaronfranke]

Simply put, KinematicBody3D had two different locked_axis variables, which caused incorrect behavior any time they were out of sync. It's a much better idea to only have one variable to track this (like RigidBody3D does), so I deleted the duplicate one.

This fixes #25798 in master, and is the master version of #45176.

[AndreaCatania]

#45176 (comment)

[madmiraal]

#25798 was fixed with #38852, but never backported to 3.2; so this change is not required.

Personally, I think keeping the locked_axis variable in KinematicBody3D is the correct approach. This is the only place it is used. It is not used by the PhysicsServer3D so there is no need to store it there, and, as @AndreaCatania points out, "the physics server API are generally not so fast especially if compared to locked_axis & (1 << i)". The only reason for storing it in PhysicsServer3D would be to keep it consistent with RigidBody3D. The other thing to bear in mind is that the PhysicsServer3D should only be available in _physics_process(). Currently, it doesn't matter, because (in 3D) the physics server doesn't run in its own thread, but we shouldn't be relying on it.

Ultimately, the real problem is the lack of proper syncing between the server and the scene, but that shouldn't be the reason for bypassing that line of separation.

[aaronfranke]

@madmiraal Then perhaps we should delete the variable in PhysicsServer3D if it's not used there. Either way seems fine to me, but it doesn't make sense to have two variables for the same thing.

I was aware of #38852 when I made this PR (removing the extra code in set_axis_lock is part of this PR), but I don't like that fix at all. I very much disagree with #38852 being backported, there are better fixes available (which is why I opened these PRs). We shouldn't have two variables for the same thing, it's simply a bad idea.

[madmiraal]

We shouldn't have two variables for the same thing, it's simply a bad idea.

The scene-server architecture of Godot requires it. The architecture allows changes to scenes (user interaction) and changes within the server to run in separate threads. Changes are then synced separately. Currently, there is no multi-threading in 3D and the multi-threading in 2D has issues; so it's generally not used. Therefore, it appears as if there is no need to have multiple copies, but that's not actually the case.

This need for separation becomes more obvious, when you consider the more complex scenario of multi-user networked game, where the server runs separately from the players. As I said before:

Ultimately, the real problem is the lack of proper syncing between the server and the scene, but that shouldn't be the reason for bypassing that line of separation.

[pouleyKetchoupp]

@madmiraal There's no reason to keep two separate variable that are not in sync. RigidBody3D and KinematicBody3D share a variable within the physics server, so the best approach is to keep only the one there.

If you keep the two variables, calling PhysicsServer.body_set_axis_lock for a KinematicBody3D is still not going to work, and we'll have another bug on our hands. So we need to choose the solution that keeps everything in sync here.

The performance hit of these calls to the physics server is negligible compared to the cost of a KinematicBody3D update. Choosing poor design to fix unknown performance issues is not a good idea. An actual issue needs to be identified before finding a solution for it.

[lawnjelly]

Forgive me as I'm not really very familiar with the physics, but unless there were good reason to do otherwise I'd tend to agree with @madmiraal . Godot is chocked full of this paradigm, 2 copies of variables, one client side and one on the server, and as I understand it this is the multithread paradigm.

Once you violate it you may be at risk of stalls, or desynchronization, depending on how the queue is implemented. This probably needs @reduz to have a look at and confirm one way or the other.

That said I'm more familiar with visual server, and there, pretty much all the runtime functions are void (one way) aside from creating RIDs which are handled separately, and debug functions which aren't intended to be used generally. The physics server does seem to have more functions available which return values, so maybe the situation is different.

[pouleyKetchoupp]

Yes, after #45852 there's a risk of stalling when in multithreaded mode so I agree it's a good reason to limit the number of calls to the physics server.

For now, porting #38852 to 3.3 is probably the best approach.

[pouleyKetchoupp]

@aaronfranke Sorry I was probably not clear enough, but I've changed my mind after comments from @madmiraal and @lawnjelly, so the easiest for now is to make a simpler fix like in #38852 even it's not ideal for synchronization between the node and the server state.

[aaronfranke]

@pouleyKetchoupp Thanks for the feedback. This PR is for master so I closed it, but I updated #45176.