Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice
  2. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

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

Discussion in 'Scripting' started by litchlynn11, Oct 18, 2019.

  1. litchlynn11

    litchlynn11

    Joined:
    Oct 14, 2019
    Posts:
    3
    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 :)

    Code (CSharp):
    1. using UnityEngine;
    2. using UnityEngine.AI;
    3.  
    4. public class PlayerActions : MonoBehaviour
    5. {
    6.     bool playerIsSelected = false;
    7.     public LayerMask whatCanBeClicked;
    8.     public LayerMask whatCanBeMarked;
    9.     public GameObject marker;
    10.     GameObject PreviousMarker;
    11.     GameObject markerClone;
    12.     NavMeshAgent playerNav;
    13.     RaycastHit hitInfo;
    14.     Ray myRay;
    15.  
    16.     void Update()
    17.     {
    18.         if (Input.GetMouseButtonDown(0)) //Player movment
    19.         {
    20.             myRay = Camera.main.ScreenPointToRay(Input.mousePosition);
    21.  
    22.             if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeClicked))
    23.             {
    24.                 Debug.Log(hitInfo.collider);
    25.                 if (hitInfo.collider.gameObject.CompareTag("Player"))
    26.                 {
    27.                     playerIsSelected = true;
    28.                     playerNav = hitInfo.collider.GetComponent<NavMeshAgent>();
    29.                 }
    30.                 if (hitInfo.collider.gameObject.CompareTag("Ground") && playerIsSelected)
    31.                 {
    32.                     playerNav.SetDestination(hitInfo.collider.transform.position);
    33.                     playerIsSelected = false;
    34.                 }
    35.             }
    36.         }
    37.         if (playerIsSelected) // Movement marker
    38.         {
    39.             myRay = Camera.main.ScreenPointToRay(Input.mousePosition);
    40.             if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeMarked))
    41.             {
    42.                 if (PreviousMarker == null)
    43.                 {
    44.                     Vector3 position = hitInfo.collider.gameObject.transform.position;
    45.                     markerClone = Instantiate(marker, position, hitInfo.collider.gameObject.transform.rotation);
    46.                     PreviousMarker = hitInfo.collider.gameObject;
    47.                 }
    48.                 if (PreviousMarker != hitInfo.collider.gameObject)
    49.                 {
    50.                     Destroy(markerClone);
    51.                     PreviousMarker = null;
    52.                 }
    53.             }
    54.         }
    55.         else
    56.         {
    57.             Destroy(markerClone);
    58.             PreviousMarker = null;
    59.         }
    60.     }
    61. }
     
  2. Vectorbox

    Vectorbox

    Joined:
    Jan 27, 2014
    Posts:
    232
    It's a good idea to cache 'Camera.main' as internally it uses 'FindGameObjectsWithTag' which is a relatively 'slow' process.

    Example
    Code (csharp):
    1.  
    2. private Camera cameraReference;
    3.  
    4. void Awake() {
    5. cameraReference = Camera.main;
    6. }
    7.  
     
    Last edited: Oct 21, 2019
  3. litchlynn11

    litchlynn11

    Joined:
    Oct 14, 2019
    Posts:
    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. Vectorbox

    Vectorbox

    Joined:
    Jan 27, 2014
    Posts:
    232
    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.

    Code (csharp):
    1.  
    2.     using UnityEngine;
    3.     using UnityEngine.AI;
    4.  
    5.     public class PlayerActions : MonoBehaviour
    6.     {
    7.         private Camera cameraReference;
    8.         bool playerIsSelected = false;
    9.         public LayerMask whatCanBeClicked;
    10.         public LayerMask whatCanBeMarked;
    11.         public GameObject marker;
    12.         GameObject PreviousMarker;
    13.         GameObject markerClone;
    14.         NavMeshAgent playerNav;
    15.         RaycastHit hitInfo;
    16.         Ray myRay;
    17.      
    18.        // --
    19.        void Awake(){
    20.            cameraReference = Camera.main;
    21.        }
    22.        // --
    23.         void Update()
    24.         {
    25.             if (Input.GetMouseButtonDown(0)) //Player movment
    26.             {
    27.                 myRay = cameraReference.ScreenPointToRay(Input.mousePosition);
    28.  
    29.                 if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeClicked))
    30.                 {
    31.                     Debug.Log(hitInfo.collider);
    32.                     if (hitInfo.collider.gameObject.CompareTag("Player"))
    33.                     {
    34.                         playerIsSelected = true;
    35.                         playerNav = hitInfo.collider.GetComponent<NavMeshAgent>();
    36.                     }
    37.                     if (hitInfo.collider.gameObject.CompareTag("Ground") && playerIsSelected)
    38.                     {
    39.                         playerNav.SetDestination(hitInfo.collider.transform.position);
    40.                         playerIsSelected = false;
    41.                     }
    42.                 }
    43.             }
    44.             if (playerIsSelected) // Movement marker
    45.             {
    46.                 myRay = cameraReference.ScreenPointToRay(Input.mousePosition);
    47.                 if (Physics.Raycast(myRay, out hitInfo, 100, whatCanBeMarked))
    48.                 {
    49.                     if (PreviousMarker == null)
    50.                     {
    51.                         Vector3 position = hitInfo.collider.gameObject.transform.position;
    52.                         markerClone = Instantiate(marker, position, hitInfo.collider.gameObject.transform.rotation);
    53.                         PreviousMarker = hitInfo.collider.gameObject;
    54.                     }
    55.                     if (PreviousMarker != hitInfo.collider.gameObject)
    56.                     {
    57.                         Destroy(markerClone);
    58.                         PreviousMarker = null;
    59.                     }
    60.                 }
    61.             }
    62.             else
    63.             {
    64.                 Destroy(markerClone);
    65.                 PreviousMarker = null;
    66.             }
    67.         }
    68.     }
     
  5. csofranz

    csofranz

    Joined:
    Apr 29, 2017
    Posts:
    1,556
    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.
     
    Vectorbox likes this.