Multiplayer Peer-To-Peer position interpolation

Question

I have coded this movement interpolation for Unity3d using Photon.

OnPhotonSerializeView gets called at the tickrate.

Any kind of criticism/improvement suggestions are welcome. I tried to make the code as plain as possible because I don't see this as something that should be over-engineered because it is rather algorithmic in nature.

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
public class PlayerInterpolation : MonoBehaviour {
 Queue<Vector3> positions = new Queue<Vector3>();
 Queue<Quaternion> rotations = new Queue<Quaternion>();
 Queue<int> frameNumbers = new Queue<int>();
 static int amountBuffered = 1;
 static float oneTickTime = 1.0f / Settings.tickRate;
 PhotonView photonView;
 int localSentFrameNumber = 0;
 int lastReceivedFrameNumber = -1;
 int lastFrameNumberMovedTowards = 0;
 int expectedExecuteFrameNumber = 0; //An estimation of what frame number should be executing at this time.
 float expectedExecuteFrameNumberLastExecutedTime = 0;
 Vector3 moveTowards;
 float moveTowardsSpeed = 1.0f;
 bool moveInProgress = false;
 void Start() {
 photonView = gameObject.GetComponent<PhotonView>();
 moveTowards = transform.position;
 }
 void Update() {
 if (photonView.isMine) return;
 transform.position = Vector3.MoveTowards(transform.position, moveTowards, moveTowardsSpeed * Time.deltaTime);
 if (Mathf.Abs(Vector3.Distance(transform.position, moveTowards)) < 0.001f) {
 moveInProgress = false;
 } else {
 moveInProgress = true;
 }
 if (expectedExecuteFrameNumberLastExecutedTime + oneTickTime < Time.realtimeSinceStartup) {
 expectedExecuteFrameNumber++;
 expectedExecuteFrameNumberLastExecutedTime = Time.realtimeSinceStartup;
 }
 if (frameNumbers.Count > 0 && ((frameNumbers.Peek() + amountBuffered <= lastReceivedFrameNumber) || frameNumbers.Peek() <= expectedExecuteFrameNumber - amountBuffered)) {
 if (lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered) {
 int trimUntil = lastReceivedFrameNumber - amountBuffered;
 while (frameNumbers.Peek() < trimUntil) {
 positions.Dequeue();
 rotations.Dequeue();
 frameNumbers.Dequeue();
 }
 }
 Vector3 position = positions.Dequeue();
 Quaternion rotation = rotations.Dequeue();
 int frameNumber = frameNumbers.Dequeue();
 if (moveInProgress == true) {
 transform.position = moveTowards;
 }
 float distance = Vector3.Distance(transform.position, position);
 moveTowards = position;
 moveTowardsSpeed = distance * (frameNumber - lastFrameNumberMovedTowards) / oneTickTime;
 lastFrameNumberMovedTowards = frameNumber;
 expectedExecuteFrameNumber = frameNumber;
 expectedExecuteFrameNumberLastExecutedTime = Time.realtimeSinceStartup;
 }
 }
 void OnPhotonSerializeView(PhotonStream stream, PhotonMessageInfo info) {
 if (stream.isWriting) {
 stream.SendNext(transform.position);
 stream.SendNext(transform.rotation);
 stream.SendNext(localSentFrameNumber++);
 } else {
 positions.Enqueue((Vector3)stream.ReceiveNext());
 rotations.Enqueue((Quaternion)stream.ReceiveNext());
 int frameNumber = (int)stream.ReceiveNext();
 frameNumbers.Enqueue(frameNumber);
 lastReceivedFrameNumber = frameNumber;
 }
 }
}

Answer 1

I recommend you always write access modifiers for class members and for classes itself. It will make code more clear and understandable. So instead of

Queue<Vector3> positions = new Queue<Vector3>();
Queue<Quaternion> rotations = new Queue<Quaternion>();
...
void Start() { ... }

write

private Queue<Vector3> positions = new Queue<Vector3>();
private Queue<Quaternion> rotations = new Queue<Quaternion>();
...
private void Start() { ... }

Also these queues are not supposed to be changed so mark them with readonly keyword:

private readonly Queue<Vector3> positions = new Queue<Vector3>();
...

---

It is a matter of taste but it is more common for C# to place opening curly braces on the next line:

private void Start()
{
 photonView = gameObject.GetComponent<PhotonView>();
 moveTowards = transform.position;
}

---

This code

if (Mathf.Abs(Vector3.Distance(transform.position, moveTowards)) < 0.001f) {
 moveInProgress = false;
} else {
 moveInProgress = true;
}

can be simplified to

moveInProgress = Mathf.Abs(Vector3.Distance(transform.position, moveTowards)) >= 0.001f;

Also it would be better to declare 0.001f as named constant like

private const float DistanceEpsilon = 0.001f;

---

You should definitely rewrite these conditions:

if (frameNumbers.Count > 0 && ((frameNumbers.Peek() + amountBuffered <= lastReceivedFrameNumber) || frameNumbers.Peek() <= expectedExecuteFrameNumber - amountBuffered)) {
 if (lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered)

First of all, you use frameNumbers.Peek() multiple times here so store the result of it to variable:

var firstFrameNumber = frameNumbers.Peek();

I recommend to use Any method for checking a collection on emptiness:

frameNumbers.Any()

instead of

frameNumbers.Count > 0

Thanks @Nikita B for pointing out that frameNumbers.Any() should be done before frameNumbers.Peek(). In your case you can just return from the method if there are no elements in the frameNumbers:

if (!frameNumbers.Any())
 return;

These conditions

frameNumbers.Peek() + amountBuffered <= lastReceivedFrameNumber
lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered

can be written in one form:

lastReceivedFrameNumber - frameNumbers.Peek() > amountBuffered
lastReceivedFrameNumber - frameNumbers.Peek() >= amountBuffered

Now we see that we can store lastReceivedFrameNumber - frameNumbers.Peek() to variable since this expression is used multiple times. So the final form of conditions above will be

if (!frameNumbers.Any())
 return;
var firstFrameNumber = frameNumbers.Peek();
var frameNumbersDifference = lastReceivedFrameNumber - firstFrameNumber;
if (frameNumbersDifference >= amountBuffered ||
 firstFrameNumber <= expectedExecuteFrameNumber - amountBuffered)
{
 if (frameNumbersDifference > amountBuffered)

Also I recommend to extract all subconditions to methods with appropriate names. It will make your code much more easy to read and understand.

---

Small note: you don't need to write == true or == false when checking value of bool variable. This

if (moveInProgress == true) {

should be rewritten as

if (moveInProgress)
{

• Heslacher: Rgearding .Any() vs. Count > 0 I prefer the later because it only checks the Count property. If we talk about the extension method Count() I prefer .Any() like you.
• Maxim: @Heslacher Do you use Count > 0 to save a couple of microseconds or this expression is more clear for you? As for me I use Any for every IEnumerable to make my code consistent between different applications and for easy replacement of a collection with another class, and it looks prettier for me :) But I never wrote performance critical apps where Any can increase time of execution of some code.
• Nikita B: frameNumbers.Peek() will throw if Queue is empty. You should checkframeNumbers.Any() first, and only then peek.
• Majiick: Hey, OP here. Can't login into that account because it says that a user with that email already exists which is this account... But anyway. I'd just like to say, thank you very much for all of the comments!
• Majiick: Ah there, managed to merge my accounts and accepted your answer.

Answer 2

One small note...

static int amountBuffered = 1;
static float oneTickTime = 1.0f / Settings.tickRate;

If you define contants then you should decorate them either with the const keyword or the readonly one. By doing so you clearly inform that this value naver changes and you also cannot overwrite it by accident.

Overall your code isn't bad. I find you use very good and meaningful names, early returns, not too much nesting. Just a few more helper variables like @Maxim suggested and it's fine.