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

Movements overhaul #2189

Merged
merged 20 commits into from
Nov 12, 2024
Merged

Conversation

tornac1234
Copy link
Collaborator

@tornac1234 tornac1234 commented Oct 30, 2024

Reason for PR 🤔:

Current state of vehicles movements is really poor, both for regular play, and for testing the brand new #2106.
+ the network optimization behind movements is currently really poor and mostly random
Latest rework of movements (after modpocalypse) messed up movements sync, and it was never that suitable for Nitrox.

Benefits of the PR 🤩:

Will drastically improve movements for players and vehicles:

  • Only 30 packets/second are now required per tracked unit (players and vehicles) which is a lot better than it used to be (it had even poorer visible performance)
  • Packets will be lighter than before
  • New system is adaptable to poorer connections (there are two settings for the automatically adjusted latency system)
  • While having different latency between players might cause a bit of frustration (e.g being hit on another player screen while they're still safe on their own screen), it is the best compromise between performance and playability.

Tasks for myself 🍙🍜🍣 (I kept the best ones 😋):

  • Reimplement channels in LiteNetLib (we always had a setting in packets but we never actually fed it to LiteNetLib)
  • Create a new dedicated MovementReplicator MonoBehaviour responsible for reproducing players and vehicles movements, while automatically taking variable latency into account
  • Deprecate our old system (delete the old packets and MultiplayerCyclops (etc) behaviours)
  • Make some classes inheriting from MovementReplicator for the Cyclops, Seamoth and Exosuit to have their animations working accordingly with the replicated movement.
  • Have players be set to at the right position when they're driving a vehicle (vehicles packets' might require at least one more field or two for steering wheel and throttle data (can probably be reduced to a byte for the angles, and a bool for appliedThrottle), but as long as we know it's a cyclops, we can hardcode the relative localposition to avoid sending that data

Tasks assigned to @Jannify 🍕🍔🍟 (thanks for proposing your help 😀):

  • Finally make use of VehicleOnPilotModeChanged packet to be the trigger of a remote player entering/exitting a vehicle (and no longer have a very weird logic like the current Vehicles.UpdateVehiclePosition)
  • Ensure remote players remain in place during the driving (that I supposedly fixed with cyclops movement PR)
  • Rework docking and undocking accordingly to the new movement system (a trigger must be sent to start the animations, during which remote players will ignore incoming data for the vehicles involved)

Translations to be added to Weblate 📜🌐:

  • NitroxBandwidthSettings: Bandwidth Settings
  • NitroxHigherForUnstable_Tooltip: Give a higher value for more unstable connections
  • NitroxSettingsLatencyUpdatePeriod: Latency Update Period (s)
  • NitroxSettingsSafetyLatencyMargin: Safety Latency Margin (ms)

Context images for the translations:
image

@tornac1234 tornac1234 added Type: bug Area: vehicles Related to vehicles (seamoth, cyclops, prawn) Complexity: hard Requires deep understanding and/or hard to setup and verify Area: player Related to player character actions Type: enhancement labels Nov 1, 2024
@tornac1234
Copy link
Collaborator Author

tornac1234 commented Nov 3, 2024

This PR is now ready for both tests and reviews :)

@dartasen
Copy link
Member

dartasen commented Nov 10, 2024

Translations string has been added to Weblate, modified the format a bit so it matches with existing keys :

Nitrox_Settings_Bandwidth Bandwidth Settings
Nitrox_Settings_HigherForUnstable_Tooltip Give a higher value for unstable connections
Nitrox_Settings_LatencyUpdatePeriod Latency Update Period (s)
Nitrox_Settings_SafetyLatencyMargin Safety Latency Margin (ms)

@dartasen dartasen mentioned this pull request Nov 11, 2024
8 tasks
@tornac1234 tornac1234 added this to the 1.8 milestone Nov 11, 2024
… being deleted when the vehicle is gone, being too far from the piloting chair, destroying a cyclop's objects would bug for players outside the cyclops, remote player wrong rotation in bases
@dartasen dartasen merged commit c0f346a into SubnauticaNitrox:master Nov 12, 2024
Copy link
Collaborator

@Coding-Hen Coding-Hen left a comment

Choose a reason for hiding this comment

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

Typical merged it a few minutes before I finished the review I was doing. Sorry for being slow but thought I would still comment them. Good code!

@@ -111,10 +111,11 @@ public void ProcessUpdate(TimeChange packet)

latestRegistrationTime = DateTimeOffset.FromUnixTimeMilliseconds(packet.UpdateTime);
latestRegisteredTime = packet.CurrentTime;


// We don't want to have a big DeltaTime when processing a time skip so we calculate it beforehands
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// We don't want to have a big DeltaTime when processing a time skip so we calculate it beforehands
// We don't want to have a big DeltaTime when processing a time skip so we calculate it beforehand


public void Update()
{
float currentTime = (float)this.Resolve<TimeManager>().RealTimeElapsed;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this to be cast to float or can we have it as it is for the evaluation of the if?


if (latency > maxAllowedLatency)
{
maxAllowedLatency = latency + SafetyLatencyMargin;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This calculation seems strange to me, we are saying are we higher than the max allowed and then resetting that. It feels like if it's the max allowed we shouldn't go past this. Interested in others thoughts

}
}

float occurrenceTime = time + INTERPOLATION_TIME + maxAllowedLatency;
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this won't this calculation be different for everyone and so mean each client has them out of sync?

if (simulationOwnershipData.GetPlayerForLock(movementData.Id) != player)
{
Log.WarnOnce($"Player {player.Name} tried updating {movementData.Id}'s position but they don't have the lock on it");
// TODO: In the future, add "packet.Data.RemoveAt(i);" and "continue;" to prevent those abnormal situations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this not be an easy thing to do now? Why wait?

killzoms pushed a commit to killzoms/Nitrox that referenced this pull request Dec 24, 2024
* Changed movement replicator's buffer to be a LinkedList (very good optimization), added a setting to adjust movement latency

* Huge improvements to get a smooth movements for seamoths and exosuits with reduced packet load

* Reimplement channels into LiteNetLib

* First attempt at implementing a variable latency adaptor

* Fix channels (again), add two settings for better control of movements latency managing, and files cleanup

* Deprecate the current vehicle animation system and replace it by a new one integrated in the new movement system

* Fix seamoth enter sound playing all over the map, cyclops driver not being set correctly

* Restore driver state of other players when connecting to a server

* Fix remotely driven exosuits' animations

* Fix a possible divide by zero from TimeManager.DeltaTime

* Refactor docking code to bring it up to new standard

* Decouple docking patches from Vehicles.cs

* Supress MovementReplicator during (un)docking

* Fix docking processor

* Few corrections for docking and simulation ownership

* Refactors and little corrections here and there

* Add a movement broadcast rate limiter

* Fix bugs: not being able to drive a vehicle, rate limiter entries not being deleted when the vehicle is gone, being too far from the piloting chair, destroying a cyclop's objects would bug for players outside the cyclops, remote player wrong rotation in bases

---------

Co-authored-by: Jannify <[email protected]>
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 Area: vehicles Related to vehicles (seamoth, cyclops, prawn) Complexity: hard Requires deep understanding and/or hard to setup and verify Type: bug Type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants