bEnablePhysicsOnDedicatedServer is handled in a way that can lead to an inconsistent state. Also, the resulting behavior might hint toward another bug in rotation handling around SpaceBases and component<->world space mappings (off-by-90’ kind of bug).
By default, physics simulation of a SkeletalMeshComponent is disabled on a dedicated server. Physics simulation can be enabled by setting the USkeletalMeshComponent::bEnablePhysicsOnDedicatedServer flag to true in, say, BeginPlay() of the owning actor.
The physics issue: The problem arises if physics simulation has been enabled for the component only using the editor interface (by checking the Simulate Physics checkmark), instead of using SkeletalMeshComponent::SetSimulatePhysics(): on a dedicated server, the skeletal mesh ends up in an inconsistent state in which it is simulating physics and receiving kinematic updates at the same time, causing it to behave erratically.
 Workaround: For your SkeletalMeshComponent, call SetSimulatePhysics(true) after(!) setting bEnablePhysicsOnDedicatedServer=true, even if you have the Simulate Physics box checked in the editor.
The reason: The problem is that the bEnablePhysicsOnDedicatedServer flag is initially set to false in the SkeletalMeshComponent constructor and is still in this default state when the FBodyInstance::InitBody() methods are called for the bodies of the skeleton. This method sees the flag as being false and consequently sets FBodyInstance::bSimulatePhysics flag to false (it seems that there is no place where to set bEnablePhysicsOnDedicatedServer in game code before this happens if one is extending from AActor?). Setting the bEnablePhysicsOnDedicatedServer flag true at a later point will enable physics simulation also on a server, but the individual FBodyInstance objects will still have their bSimulatePhysics false. This leads to inconsistent behavior during tick in USkeletalMeshComponent::UpdateKinematicBonesToPhysics(). On line 315 (“if (BodyInst->IsValidBodyInstance() && !BodyInst->IsInstanceSimulatingPhysics(true))”), execution branches now differently on a server and on a client, and a server ends up overwriting the transformation of the bodies with one taken from USkinnedMeshComponent::SpaceBases.
The rotation issue: What happens on a server in this case is that the SkeletalMeshComponent simulates physics correctly, but in addition, the entire skeleton becomes rotated by 90’ on each tick due to the translation overwrite coming from SpaceBases. This might be just a side effect of the inconsistent state, but it might also suggest that the loop from physics to SpaceBases and back to physics might have a bug in rotation handling.
[edit: fixed the class name in the last paragraph]
Does this commit fix this issue by defaulting the bEnablePhysicsOnDedicatedServer to true (or a project-wide override)?
@piinecone: Interesting! I didn’t try it, but it seems that it should resolve the issue, as long as the flag is set in the editor: it has its intended value from the beginning and the bodies would be initialized properly in InitBody(). But if one sets it in code instead of in the editor, then the problem probably still persists.
In general, I feel that this flag is handled in a way that is unnecessarily prone to errors: the flag has both transient effects during every tick and non-transient effects during initialization. At the same time, users are permitted (and lead to, by the preceding comment block?) to change it directly, thus getting the former but subtly missing the latter.
@oricohen: While you are at it, maybe you could do something to protect others a bit from stepping into this pitfall? Maybe just add a warning to the preceding comment block or something, in case that you are not planning to fully refactor this thing?
ps. I played around with it some more and just realized that in fact, bEnablePhysicsOnDedicatedServer does not even disable physics for the skeleton on the physx level! I’ll post another bug report about that in a moment.
@hiili: I totally didn’t recognize the non-transient initialization effects of this. I was purely looking at it from the perspective of getting physics simulating in a standalone dedicated server environment. Also, I can’t let @oricohen take the blame for not including a warning in the comment – that’s my mistake!
Thanks for all this info, I’ll take another pass at this when I get a chance.
@piinecone: Oh, I definitely didn’t mean to blame anyone: bugs happen, and neither of you could have known about this one lurking beneath the surface! I just meant that now when it is known, it would be good that someone with access to the code adds a warning to the comment block (if not going to refactor it).
I was looking into this, but was unable to hit your exact case. InitBody should be getting called after properties in the editor have been properly set (i.e. bEnablePhysicsOnDedicatedServer)
Is the issue that you’re setting this flag in code, but at this point it’s too late and InitBody has already been called?
I agree that this is not ideal. I think the best solution would be to add a function to call which would ensure the BodyInstance is modified. As it’s setup right now bEnablePhysicsOnDedicatedServer is setup as more of a read only data that should not be modified at runtime.
I’m a bit hesitant about introducing this new function though as I’m not sure what other bugs this would expose. As it is right now the code is not expecting this to be changed at runtime. Can you think of a reason why you’d want to turn this on and off only on a dedicated server? In general we either want it on or not.
For some reason I missed this update on this thread and noticed it only now. Thanks for taking a look at this issue, and sorry for the delay!
Yes, the issue is that I was setting this flag in code, but at this point it’s too late and InitBody has already been called.
I completely agree with you that there is probably no reason to touch the actual code around this. Instead, I think that it would be nice to save others from falling into the same pitfall by just simply adding a warning to the preceding comment block? As you say, the code is clearly not expecting this to be changed at runtime, but there is no mentioning of this in the comments at the moment.
I’ve put a feature request in for adding a comment of that fashion to the source so that people can be warned about this functionality. For your reference, the number is UE-20439.
Have a nice day!