-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Movement Overhaul #2073
base: master
Are you sure you want to change the base?
Movement Overhaul #2073
Conversation
9578635
to
35ada97
Compare
Vehicle positions are more resilient, and the Cyclops interpolation has been fixed, LocalPlayer in the Cyclops has been partially fixed but Y axis movement is still an issue |
I've no idea what the syncing mechanism behind the nitrox is. But hope this reliable networking model from valve might help a bit. https://developer.valvesoftware.com/wiki/Source_Multiplayer_Networking |
We use https://github.com/RevenantX/LiteNetLib which can do NAT hole punching and is very reliable so far, for us. But we're looking into using Steam P2P as alternative for users unable to port forward or behind a dynamic IP (which works poorly for hosting game servers). |
@ywgATustcbbs assuming i'm understanding the reason for your suggestion properly... for the LocalPlayer its position is not actually updated by the server, its more the LocalPlayer simulates its own position and the positions of other Entities that they have acquired a Simulation lock for, in this instance the Remote player's movement is fine, but the LocalPlayer as controlled by the game's code, is experiencing issues with movement in relation to the Cyclops |
The article is about how valve's source engine processes different players' lag, entity interpolation and syncing game world with server. It's a general and reliable model used by valve's fps games such as cs:go. That's why i think it might be useful in solving client/server movement syncing issues of nitrox. But as described by killzoms. Subnautica might have a different system that processes player movement which can't be taken over by nitrox |
We can technically replace the system the game uses for player movement entirely, although it might be easier to identify where the bug originates from and fix that |
008169b
to
66b15e3
Compare
66b15e3
to
d83877d
Compare
eaa489a
to
9903845
Compare
this is now ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this pr is full of code smells, convention breaks and no automatic cleaning/intending/etc. of the src files
NitroxClient/Communication/Packets/Processors/BasicMovementProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxClient/Communication/Packets/Processors/BasicMovementProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxClient/Communication/Packets/Processors/SimulationOwnershipResponseProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxPatcher/Patches/Dynamic/FMOD_CustomEmitter_Start_Patch.cs
Outdated
Show resolved
Hide resolved
NitroxPatcher/Patches/Dynamic/FreezeRigidbodyWhenFar_FixedUpdate_Patch.cs
Outdated
Show resolved
Hide resolved
NitroxPatcher/Patches/Dynamic/FreezeRigidbodyWhenFar_FixedUpdate_Patch.cs
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/VehicleMovementPacketProcessor.cs
Outdated
Show resolved
Hide resolved
5e80c8c
to
eacf930
Compare
NitroxClient/GameLogic/Spawning/WorldEntities/VehicleWorldEntitySpawner.cs
Show resolved
Hide resolved
NitroxPatcher/Patches/Dynamic/FMOD_CustomEmitter_Start_Patch.cs
Outdated
Show resolved
Hide resolved
Nitrox.Test/Client/Communication/MultiplayerSession/ConnectionState/DisconnectedStateTests.cs
Show resolved
Hide resolved
800df78
to
02dde6d
Compare
{ | ||
public static readonly MethodInfo TARGET_METHOD = Reflect.Method((FreezeRigidbodyWhenFar t) => t.FixedUpdate()); | ||
|
||
public static IEnumerable<CodeInstruction> Transpiler(MethodBase original, IEnumerable<CodeInstruction> instructions, ILGenerator generator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible I would not do manual Transpilers with foreach bc readability is vastly reduced compared to our established methods.
Still pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments for now, but i've still got to figure out the new Transpiler methods and hope it doesnt make the readability worse than it already is considering that this has some special cases...
NitroxClient/Communication/Packets/Processors/BasicMovementProcessor.cs
Outdated
Show resolved
Hide resolved
@@ -56,7 +56,7 @@ private void StartVehicleUndocking(VehicleUndocking packet, GameObject vehicleGo | |||
vehicle.mainAnimator.SetBool("player_in", true); | |||
playerInstance.Attach(vehicle.playerPosition.transform); | |||
// It can happen that the player turns in circles around himself in the vehicle. This stops it. | |||
playerInstance.RigidBody.angularVelocity = Vector3.zero; | |||
// playerInstance.RigidBody.angularVelocity = Vector3.zero; // May not happen anymore needs verifying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the verifying say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotta get a test session going to repeatedly test this as judging by the original comment it doesnt seem to happen often?
NitroxModel-Subnautica/DataStructures/GameLogic/ExoSuitMovementData.cs
Outdated
Show resolved
Hide resolved
New Commit for requested changes. |
Co-authored-by: tornac1234 <[email protected]>
82eb608
to
beab7d5
Compare
Rebased branch and fixed null ref |
[TestInitialize] | ||
public void TestInitialize() | ||
{ | ||
packetReceiver = new PacketReceiver(); | ||
loadedCell = new AbsoluteEntityCell(loadedActionPosition, CELL_LEVEL); | ||
visibleCells.Add(loadedCell); | ||
ClientAutoFacRegistrar registrar = new ClientAutoFacRegistrar(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method can be completely removed.
using NitroxModel.Packets; | ||
|
||
namespace NitroxClient.Communication; | ||
|
||
[TestClass] | ||
public class DeferredPacketReceiverTest | ||
{ | ||
private readonly VisibleCells visibleCells = new(); | ||
private PacketReceiver packetReceiver; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private PacketReceiver packetReceiver; | |
private readonly PacketReceiver packetReceiver = new(); |
Broadcasting = broadcasting; | ||
if (Receiving && broadcasting) | ||
{ | ||
Receiving = !broadcasting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is always false
Receiving = receiving; | ||
if (Broadcasting && receiving) | ||
{ | ||
Broadcasting = !receiving; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is always false
public event Action AfterUpdate = () => { }; | ||
public event Action AfterFixedUpdate = () => { }; | ||
|
||
private float timeSinceLastBroadcast; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never used
if (!NitroxEntity.TryGetObjectFrom(id, out GameObject gameObject)) | ||
{ | ||
return false; | ||
} | ||
|
||
if (gameObject.TryGetComponent(out mc) && mc) | ||
{ | ||
movementControllersById.Add(id, mc); | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!NitroxEntity.TryGetObjectFrom(id, out GameObject gameObject)) | |
{ | |
return false; | |
} | |
if (gameObject.TryGetComponent(out mc) && mc) | |
{ | |
movementControllersById.Add(id, mc); | |
return true; | |
} | |
if (NitroxEntity.TryGetObjectFrom(id, out GameObject gameObject) && | |
gameObject.TryGetComponent(out mc) && mc) | |
{ | |
movementControllersById.Add(id, mc); | |
return true; | |
} |
packetSender = NitroxServiceLocator.LocateService<IPacketSender>(); | ||
simulationOwnership = NitroxServiceLocator.LocateService<SimulationOwnership>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packetSender = NitroxServiceLocator.LocateService<IPacketSender>(); | |
simulationOwnership = NitroxServiceLocator.LocateService<SimulationOwnership>(); | |
packetSender = this.Resolve<IPacketSender>(); | |
simulationOwnership = this.Resolve<SimulationOwnership>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasnt aware we had an Extension for that. Neat.
rigidbody = GetComponent<Rigidbody>(); | ||
if (!TryGetComponent(out NitroxEntity nitroxEntity)) | ||
{ | ||
NitroxEntity.GenerateNewId(gameObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really generate a new id here? To me it seems more like a log and destroy spot
TargetPosition = transform.position; | ||
TargetRotation = transform.rotation; | ||
|
||
packetSender.Send(new BasicMovement(entity.Id, transform.position.ToDto(), transform.rotation.ToDto())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
packetSender.Send(new BasicMovement(entity.Id, transform.position.ToDto(), transform.rotation.ToDto())); | |
packetSender.Send(new BasicMovement(entity.Id, TargetPosition.ToDto(), TargetRotation.ToDto())); |
using System.Collections.Generic; | ||
using NitroxClient.Communication.Abstract; | ||
using NitroxClient.GameLogic; | ||
using NitroxModel.Core; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed with changes to Awake()
{ | ||
private static readonly Dictionary<NitroxId, MultiplayerMovementController> movementControllersById = new(); | ||
|
||
public float TimeScalar { get; set; } = 1f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose, though I figured there may be some instances we could change the TimeScalar say for example when Subnautica's TimeScalar changes
Movements of Vehicles easily Desynced
Game's Player.cs changes Rigidbody Interpolation on Cyclops when entering
Game's Player is not relative to the Cyclops when inside, except when piloting
Cyclops Movement is still jagged from the perspective of players not piloting the Cyclops, I am unable to figure out a fix for this issue and it still affects the current Nitrox as well. More accurately this issue only appears to affect the look of the environment outside of the Cyclops.
Fix Bugs
Bugs noticed for other PRs