Input on whether my player movement script has any bad practices?

Unity Engine
1

So this is my script for tile-based player movement with movement markers, any input or bad practices i should fix, please let me know! Thanks in advance :slight_smile:

using UnityEngine;
using UnityEngine.AI;

public class PlayerActions : MonoBehaviour
{
    bool playerIsSelected = false;
    public LayerMask whatCanBeClicked;
    public LayerMask whatCanBeMarked;
    public GameObject marker;
    GameObject PreviousMarker;
    GameObject markerClone;
    NavMeshAgent playerNav;
    RaycastHit hitInfo;
    Ray myRay;

    void Update()
    {
        if (Input.GetMouseButtonDown(0)) //Player movment
        {
            myRay = Camera.main.ScreenPointToRay(Input.mousePosition);

            if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeClicked))
            {
                Debug.Log(hitInfo.collider);
                if (hitInfo.collider.gameObject.CompareTag("Player"))
                {
                    playerIsSelected = true;
                    playerNav = hitInfo.collider.GetComponent<NavMeshAgent>();
                }
                if (hitInfo.collider.gameObject.CompareTag("Ground") && playerIsSelected)
                {
                    playerNav.SetDestination(hitInfo.collider.transform.position);
                    playerIsSelected = false;
                }
            }
        }
        if (playerIsSelected) // Movement marker
        {
            myRay = Camera.main.ScreenPointToRay(Input.mousePosition);
            if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeMarked))
            {
                if (PreviousMarker == null)
                {
                    Vector3 position = hitInfo.collider.gameObject.transform.position;
                    markerClone = Instantiate(marker, position, hitInfo.collider.gameObject.transform.rotation);
                    PreviousMarker = hitInfo.collider.gameObject;
                }
                if (PreviousMarker != hitInfo.collider.gameObject)
                {
                    Destroy(markerClone);
                    PreviousMarker = null;
                }
            }
        }
        else
        {
            Destroy(markerClone);
            PreviousMarker = null;
        }
    }
}
2

It’s a good idea to cache ‘Camera.main’ as internally it uses ‘FindGameObjectsWithTag’ which is a relatively ‘slow’ process.

Example

private Camera cameraReference;

void Awake() {
cameraReference = Camera.main;
}
3

Is it necessary to declare the camera reference at runtime or would it have the same effect to declare it outside of any method? Thank you!

4

In practice I would opt for the ‘Awake’ method as it is called first and only once during the lifetime of the script. It is preferable to using the ‘Update’ method.

    using UnityEngine;
    using UnityEngine.AI;

    public class PlayerActions : MonoBehaviour
    {
        private Camera cameraReference;
        bool playerIsSelected = false;
        public LayerMask whatCanBeClicked;
        public LayerMask whatCanBeMarked;
        public GameObject marker;
        GameObject PreviousMarker;
        GameObject markerClone;
        NavMeshAgent playerNav;
        RaycastHit hitInfo;
        Ray myRay;
      
       // --
       void Awake(){
           cameraReference = Camera.main;
       }
       // --
        void Update()
        {
            if (Input.GetMouseButtonDown(0)) //Player movment
            {
                myRay = cameraReference.ScreenPointToRay(Input.mousePosition);

                if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeClicked))
                {
                    Debug.Log(hitInfo.collider);
                    if (hitInfo.collider.gameObject.CompareTag("Player"))
                    {
                        playerIsSelected = true;
                        playerNav = hitInfo.collider.GetComponent<NavMeshAgent>();
                    }
                    if (hitInfo.collider.gameObject.CompareTag("Ground") && playerIsSelected)
                    {
                        playerNav.SetDestination(hitInfo.collider.transform.position);
                        playerIsSelected = false;
                    }
                }
            }
            if (playerIsSelected) // Movement marker
            {
                myRay = cameraReference.ScreenPointToRay(Input.mousePosition);
                if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeMarked))
                {
                    if (PreviousMarker == null)
                    {
                        Vector3 position = hitInfo.collider.gameObject.transform.position;
                        markerClone = Instantiate(marker, position, hitInfo.collider.gameObject.transform.rotation);
                        PreviousMarker = hitInfo.collider.gameObject;
                    }
                    if (PreviousMarker != hitInfo.collider.gameObject)
                    {
                        Destroy(markerClone);
                        PreviousMarker = null;
                    }
                }
            }
            else
            {
                Destroy(markerClone);
                PreviousMarker = null;
            }
        }
    }
5

A tiny improvement: Cache the camera during OnEnable() (called shortly after Awake) and relinquish (set to null) in OnDisable(). This will cover edge cases where (for example for cut scenes) the camera main gets replaced.

1 Like