Skip to content
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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

killzoms
Copy link
Collaborator

@killzoms killzoms commented Aug 16, 2023

  • 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

    • Cyclops jumps in some direction when swapping SimulationOwnership
    • Seamoth and Prawn Suit are still jagged, can be fixed unlike the Cyclops which has been mitigated
    • Seamoth has gained yeeting capabilities

Bugs noticed for other PRs

  • Items held in player's hands have Colliders enabled for remote players
  • Beds and Benches lock players in place after/during usage

@killzoms
Copy link
Collaborator Author

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

@ywgATustcbbs
Copy link

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

@Measurity
Copy link
Collaborator

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).

@killzoms
Copy link
Collaborator Author

@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

@ywgATustcbbs
Copy link

ywgATustcbbs commented Aug 19, 2023

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).

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

@killzoms
Copy link
Collaborator Author

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

@killzoms killzoms changed the title Movement Overhaul and EntitySlot Spawning Movement Overhaul Nov 19, 2023
@killzoms killzoms force-pushed the playerStretching branch 2 times, most recently from eaa489a to 9903845 Compare November 19, 2023 16:38
@killzoms killzoms marked this pull request as ready for review November 19, 2023 17:04
@killzoms
Copy link
Collaborator Author

this is now ready for review

Copy link
Member

@Jannify Jannify left a 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/MonoBehaviours/NitroxEntity.cs Outdated Show resolved Hide resolved
NitroxPatcher/Main.cs Show resolved Hide resolved
NitroxModel/Packets/BasicMovementPacket.cs Outdated Show resolved Hide resolved
NitroxClient/ClientAutoFacRegistrar.cs Show resolved Hide resolved
NitroxClient/GameLogic/LocalPlayer.cs Outdated Show resolved Hide resolved
NitroxClient/GameLogic/LocalPlayer.cs Outdated Show resolved Hide resolved
NitroxClient/GameLogic/LocalPlayer.cs Outdated Show resolved Hide resolved
@killzoms killzoms force-pushed the playerStretching branch 2 times, most recently from 800df78 to 02dde6d Compare November 23, 2023 15:33
{
public static readonly MethodInfo TARGET_METHOD = Reflect.Method((FreezeRigidbodyWhenFar t) => t.FixedUpdate());

public static IEnumerable<CodeInstruction> Transpiler(MethodBase original, IEnumerable<CodeInstruction> instructions, ILGenerator generator)
Copy link
Member

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

Copy link
Collaborator Author

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...

NitroxPatcher/Main.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
Copy link
Member

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?

Copy link
Collaborator Author

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?

@killzoms
Copy link
Collaborator Author

New Commit for requested changes.

@killzoms
Copy link
Collaborator Author

Rebased branch and fixed null ref

@tornac1234 tornac1234 added the Area: player Related to player character actions label Dec 23, 2023
Comment on lines 15 to 20
[TestInitialize]
public void TestInitialize()
{
packetReceiver = new PacketReceiver();
loadedCell = new AbsoluteEntityCell(loadedActionPosition, CELL_LEVEL);
visibleCells.Add(loadedCell);
ClientAutoFacRegistrar registrar = new ClientAutoFacRegistrar();
}
Copy link
Member

@Jannify Jannify Jan 3, 2024

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private PacketReceiver packetReceiver;
private readonly PacketReceiver packetReceiver = new();

Broadcasting = broadcasting;
if (Receiving && broadcasting)
{
Receiving = !broadcasting;
Copy link
Member

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;
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never used

Comment on lines +66 to +75
if (!NitroxEntity.TryGetObjectFrom(id, out GameObject gameObject))
{
return false;
}

if (gameObject.TryGetComponent(out mc) && mc)
{
movementControllersById.Add(id, mc);
return true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;
}

Comment on lines +98 to +99
packetSender = NitroxServiceLocator.LocateService<IPacketSender>();
simulationOwnership = NitroxServiceLocator.LocateService<SimulationOwnership>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
packetSender = NitroxServiceLocator.LocateService<IPacketSender>();
simulationOwnership = NitroxServiceLocator.LocateService<SimulationOwnership>();
packetSender = this.Resolve<IPacketSender>();
simulationOwnership = this.Resolve<SimulationOwnership>();

Copy link
Collaborator Author

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);
Copy link
Member

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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor:

Suggested change
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;
Copy link
Member

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;
Copy link
Member

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?

Copy link
Collaborator Author

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

@Measurity Measurity added this to the 1.8 milestone Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: player Related to player character actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants