3
\$\begingroup\$

I have been working on my game for quite a while. one of its key features is cross-platform local multiplayer for example 2 players on 1 keyboard and some people on the controller.

However after finishing my script which took me a whole day to get it running just the way I want it to work. it works but is very inefficient there is very noticeable input lag when selecting players. and sometimes the input is skipped altogether.

I suspect the main cause to be the Update function and the last 3 functions in it (AlternateInput, assignplayerInput, StartGame)

here is the first iteration of my Working but slow code

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.UI;
using System.Linq;

public class InputManger : MonoBehaviour
{
    public List<string> playerOrderString = new List<string>() { "notSelected", "notSelected", "notSelected", "notSelected" };
    public bool resetPlayerSelect;
    public string repFor = "K";
    public Image startScreenImage;
    public float fadeTime = 1.0f;
    void Update()
    {
        PlayerHasBeenSelected();
        ResetInput();
        AlternateInput();
        assignPlayerInput();
        StartGame();


    }
    void StartGame()
    {
        if (playerOrderString[0] != "notSelected")
        {
            if (Input.GetButtonDown("Start" + playerOrderString[0]))
            {

                for (int i = 0; i < playerOrderString.Count; i++)
                {
                    if (playerOrderString[i] == "notSelected")
                    {
                        playerOrderString.RemoveAt(i);
                        i = 0;
                    }
                }
                playerOrderString.RemoveAll(item => item == null);
                GameObject.FindWithTag("GameController").GetComponent<GameManager>().PlayerOrder = playerOrderString.ToArray();
                GameObject.FindWithTag("GameController").GetComponent<GameManager>().FadeTime = fadeTime;
                GameObject.FindWithTag("GameController").GetComponent<GameManager>().enabled = true;
                StartCoroutine(FadeOutStartScreen(fadeTime, 0.0f));
                return;
            }
        }
        else
        {
            return;
        }
    }
    IEnumerator FadeOutStartScreen(float fadeTime, float fadeToValue)
    {
        float alpha = startScreenImage.color.a;
        for (float t = 0.0f; t < 1.0f; t += Time.deltaTime / fadeTime)
        {
            Color newColor = new Color(1, 1, 1, Mathf.Lerp(alpha, fadeToValue, t));
            startScreenImage.color = newColor;
            yield return null;
        }
    }
    void ResetInput()
    {
        if (resetPlayerSelect)
        {

            playerOrderString = new List<string>() { "notSelected", "notSelected", "notSelected", "notSelected" };
            resetPlayerSelect = false;
            StartCoroutine(FadeOutStartScreen(fadeTime, 1.0f));
        }
    }
    void AlternateInput()
    {
        if (repFor == "K")
            repFor = "C";
        else
            repFor = "K";
    }
    void assignPlayerInput()
    {
        int checkForHowManayInput;
        if (repFor == "K")
            checkForHowManayInput = 2;
        else
            checkForHowManayInput = 4;
        for (int unAssignedPlayer = 0; unAssignedPlayer < playerOrderString.Count; unAssignedPlayer++)
        {
            if (playerOrderString[unAssignedPlayer] == "notSelected")
            {
                for (int keyboardInput = 0; keyboardInput < checkForHowManayInput; keyboardInput++)
                {
                    string playerOrderVale = repFor + (keyboardInput + 1);
                    string playerThisTime = "Fire1" + playerOrderVale;
                    if ((Input.GetButtonDown(playerThisTime)) && (playerOrderString[unAssignedPlayer] == "notSelected"))
                    {
                        if (CheckRepetedValue(playerThisTime))
                        {
                            return;
                        }
                        else
                        {
                            playerOrderString[unAssignedPlayer] = playerOrderVale;
                            return;
                        }
                    }
                }

            }
        }
    }
    bool CheckRepetedValue(string playerThisTime)
    {
        for (int i = 0; i < playerOrderString.Count; i++)
        {
            if (playerThisTime == ("Fire1" + playerOrderString[i]))
            {
                return true;
            }
        }
        return false;
    }
    public Text InitilizeText;
    void PlayerHasBeenSelected()
    {
        if (playerOrderString[0] != "notSelected")
        {
            InitilizeText.text = "";
            for (int i = 0; i < playerOrderString.Count; i++)
            {
                InitilizeText.text = InitilizeText.text + "\nPlayer " + (i + 1) + " is " + playerOrderString[i];
            }
        }
    }
}

I think the biggest reasons for the slow code would be the nested for loops in assignPlayerInput();

I will explain a bit more about how my code works

In unity input manager I have set up controls for 6 types of controllers which follow the pattern of thisInputK1, thisInputK2, thisInputC1... and so on where K denotes keyboard input and C denotes Controller Input.

The numbers after that denote the different sets of keys the input requests, for example, VecticalK1 is the same as VecticalK2 but K1 uses WASD while K2 uses arrow keys same is for the controller but they use separate controllers

\$\endgroup\$
2
\$\begingroup\$

Return statement demystified

You have 2 return statement in the following function, and both are excessive.

void StartGame()
{
    if (playerOrderString[0] != "notSelected")
    {
        if (Input.GetButtonDown("Start" + playerOrderString[0]))
        {
            // .. code
            return;
        }
    }
    else
    {
        return;
    }
}

The first one sits in a nested if-statement. When you walk the code, leaving the if-statement, there is no other code being called in this method, so the method returns anyway.

void StartGame()
{
    if (playerOrderString[0] != "notSelected")
    {
        if (Input.GetButtonDown("Start" + playerOrderString[0]))
        {
            // .. code
        }
    }
    else
    {
        return;
    }
}

The second one is the only statement in an if-statement. The if-statement itself is a terminal (no other code follows this block in your method). The entire if-statement can be omitted.

void StartGame()
{
    if (playerOrderString[0] != "notSelected")
    {
        if (Input.GetButtonDown("Start" + playerOrderString[0]))
        {
            // .. code
        }
    }
}

We could still go further from here. I would love to get rid of the nested if-statement. We could invert the outer if-statement. We have introcuded a new return statement, but this one makes sense. We actually want to exit early here.

void StartGame()
{
    if (playerOrderString[0] == "notSelected")
    {
        return;
    }

    if (Input.GetButtonDown("Start" + playerOrderString[0]))
    {
        // .. code
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ In case you wonder why all the properties are fields. It's unity3d ailment. It doesn't work well with properties... so I've heard. This pattern is common to all unity3d questions. \$\endgroup\$ – t3chb0t Jul 25 at 15:37
  • 1
    \$\begingroup\$ I read your answer and have understood about where I am using the return; excessively and have changed it and removed the 2 return statements and the else statement. However, I don't understand the second part in which you say "The second one sits in an empty if-statement, and again not followed with any statements after leaving this block. The entire if-statement can be omitted." as if (Input.GetButtonDown("Start" + playerOrderString[0])) is nested in a for loop and after the for loop, there is still some code to be run. \$\endgroup\$ – FlamesWillBurst Jul 25 at 15:43
0
\$\begingroup\$

I have managed to solve the problem on my own. the problem was int the AssignPlayerInput() and the AlternateInput(). let me explain what was happening

each frame the value of repFor was either "K" or "C" after which the AssignPlayerInput() would use that value to check input the frame which meant if I tried to press a K type input and a C type input only one of those would get detected depending on value of repFor So I changed my code so it would check for both K and C in a single Frame.

here is my latest code and AssignPlayerInput() is called every frame

void AssignPlayerInput()
    {
        for (int unAssignedPlayer = 0; unAssignedPlayer < playerOrderString.Count; unAssignedPlayer++)
        {
            if (playerOrderString[unAssignedPlayer] == "notSelected")
            {

                CheckInputFor("K", 2, unAssignedPlayer);
                CheckInputFor("C", 4, unAssignedPlayer);
            }
        }
    }
    void CheckInputFor(string repFor, int times, int unAssignedPlayer)
    {
        for (int input = 0; input < times; input++)
        {
            string playerOrderVale = repFor + (input + 1);
            string playerThisTime = "Fire1" + playerOrderVale;
            if ((Input.GetButtonDown(playerThisTime)) && (playerOrderString[unAssignedPlayer] == "notSelected"))
            {
                if (CheckRepetedValue(playerThisTime))
                {
                    return;
                }
                else
                {
                    playerOrderString[unAssignedPlayer] = playerOrderVale;
                    return;
                }
            }
        }
    }

`
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.