Search Unity

  1. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

Question How Could I Simplify My Code and Make It More Efficient?

Discussion in 'Scripting' started by Bl00dyFish, Nov 24, 2023.

  1. Bl00dyFish

    Bl00dyFish

    Joined:
    Mar 12, 2022
    Posts:
    77
    Hello,

    I've been experimenting with 2D Enemy AI movement, and I got something I'm pretty happy with. However, there are just a lot of checks (whether they are next to something, should jump, etc.) and my code is just very long. I'm worried it might cause problems later on.

    I don't know if simple AI scripts are typically very long, but I was just wondering if there was anything I could simplify down.

    Just for background, I used scriptable objects to determine the move sets for the enemies. That way I can make different types of enemies with just one script.

    Anyways, here's the code:

    Code (CSharp):
    1. using System.Collections;
    2. using System.Collections.Generic;
    3. using UnityEngine;
    4.  
    5. public class Enemy : MonoBehaviour
    6. {
    7.     public List<AIMoveEvent> moveEvents;
    8.     string chosenMoveName;
    9.  
    10.     int AIMoveEvent_Number;
    11.  
    12.     public Rigidbody2D rb;
    13.  
    14.     public float health;
    15.     public float speed;
    16.     public bool isGrounded;
    17.     public bool isOnTopOfAnotherEnemy;
    18.  
    19.     bool AIMoveCoroutine_isRunning;
    20.     RaycastHit2D hitDown, hitDownLeft, hitDownRight;
    21.     RaycastHit2D hitRight;
    22.     RaycastHit2D hitLeft;
    23.  
    24.     // Start is called before the first frame update
    25.     void Start()
    26.     {
    27.         rb = GetComponent<Rigidbody2D>();
    28.         StartCoroutine("AIMove_Event");
    29.  
    30.     }
    31.  
    32.     // Update is called once per frame
    33.     void Update()
    34.     {
    35.         GroundedChecks();
    36.  
    37.  
    38.  
    39.         hitDown = Physics2D.Raycast(new Vector2(transform.position.x, transform.position.y - 0.7f), Vector2.down, 0.3f);
    40.         Debug.DrawRay(new Vector2(transform.position.x, transform.position.y - 0.7f), Vector2.down, Color.green);
    41.        
    42.         hitDownRight = Physics2D.Raycast(new Vector2(transform.position.x + 0.2f, transform.position.y - 0.7f), Vector2.down, 0.3f);
    43.         Debug.DrawRay(new Vector2(transform.position.x + 0.2f, transform.position.y - 0.7f), Vector2.down, Color.green);
    44.        
    45.         hitDownLeft = Physics2D.Raycast(new Vector2(transform.position.x - 0.2f, transform.position.y - 0.7f), Vector2.down, 0.3f);
    46.         Debug.DrawRay(new Vector2(transform.position.x - 0.2f, transform.position.y - 0.7f), Vector2.down, Color.green);
    47.  
    48.         hitRight = Physics2D.Raycast(new Vector2(transform.position.x, transform.position.y + 0.7f), Vector2.right, 0.5f);
    49.         Debug.DrawRay(new Vector2(transform.position.x + 0.7f, transform.position.y), Vector2.right, Color.red);
    50.      
    51.         hitLeft = Physics2D.Raycast(new Vector2(transform.position.x - 0.7f, transform.position.y), Vector2.left, 0.5f);
    52.         Debug.DrawRay(new Vector2(transform.position.x - 0.7f, transform.position.y), Vector2.left, Color.red);
    53.  
    54.      
    55.        
    56.        
    57.         AIMoveEvent chosenMove = moveEvents[AIMoveEvent_Number];
    58.         chosenMoveName = chosenMove.name;
    59.         print(chosenMove);
    60.      
    61.         if(chosenMoveName == "MoveLeft")
    62.         {
    63.             MoveLeft();
    64.         }
    65.      
    66.         if(chosenMoveName == "MoveRight")
    67.         {
    68.             MoveRight();
    69.         }
    70.    
    71.         if(health <= 0)
    72.         {
    73.             Destroy(gameObject);
    74.         }
    75.    
    76.     }
    77.  
    78.     void Jump()
    79.     {
    80.         if (!AIMoveCoroutine_isRunning)
    81.         {
    82.             StartCoroutine("AIMove_Event");
    83.         }
    84.  
    85.         if (!isOnTopOfAnotherEnemy)
    86.         {
    87.             StopCoroutine("AIMove_Event");
    88.             AIMoveCoroutine_isRunning = false;
    89.             rb.AddForce(Vector2.up * 2.5f, ForceMode2D.Impulse);
    90.         }
    91.  
    92.     }
    93.  
    94.     void MoveLeft()
    95.     {
    96.         if (!AIMoveCoroutine_isRunning)
    97.         {
    98.             StartCoroutine("AIMove_Event");
    99.         }
    100.  
    101.         if (hitLeft.collider != null && isGrounded)
    102.         {
    103.             int choice = Random.Range(1, 3);
    104.             if (choice == 1 && !isOnTopOfAnotherEnemy )
    105.             {
    106.                 StopCoroutine("AIMove_Event");
    107.                 AIMoveCoroutine_isRunning = false;
    108.                 Jump();
    109.             }
    110.             if (choice == 2 || isOnTopOfAnotherEnemy)
    111.             {
    112.                 StopCoroutine("AIMove_Event");
    113.                 AIMoveCoroutine_isRunning = false;
    114.                 chosenMoveName = "MoveRight";
    115.             }
    116.         }
    117.  
    118.         else if (hitLeft.collider != null && !isGrounded)
    119.         {
    120.             chosenMoveName = "MoveRight";
    121.         }
    122.         else if (hitLeft.collider != null && hitLeft.collider.CompareTag("Enemy"))
    123.         {
    124.             int choice = Random.Range(1, 3);
    125.            
    126.             if (choice == 1)
    127.             {
    128.                 StopCoroutine("AIMove_Event");
    129.                 AIMoveCoroutine_isRunning = false;
    130.                 Jump();
    131.             }
    132.             if (choice == 2)
    133.             {
    134.                 StopCoroutine("AIMove_Event");
    135.                 AIMoveCoroutine_isRunning = false;
    136.                 chosenMoveName = "MoveRight";
    137.             }
    138.  
    139.         }
    140.  
    141.         else
    142.         {
    143.             transform.Translate(Vector2.left * speed * Time.deltaTime);
    144.         }
    145.  
    146.  
    147.     }
    148.  
    149.     void MoveRight()
    150.     {
    151.         if (!AIMoveCoroutine_isRunning)
    152.         {
    153.             StartCoroutine("AIMove_Event");
    154.         }
    155.  
    156.         if (hitRight.collider != null && isGrounded)
    157.         {
    158.             int choice = Random.Range(1, 3);
    159.  
    160.             if(choice == 1 && !isOnTopOfAnotherEnemy)
    161.             {
    162.                 StopCoroutine("AIMove_Event");
    163.                 AIMoveCoroutine_isRunning = false;
    164.                 Jump();
    165.             }
    166.             if(choice == 2 || isOnTopOfAnotherEnemy)
    167.             {
    168.                 StopCoroutine("AIMove_Event");
    169.                 AIMoveCoroutine_isRunning = false;
    170.                 chosenMoveName = "MoveLeft";
    171.             }
    172.         }
    173.        
    174.         else if(hitRight.collider != null && !isGrounded)
    175.         {
    176.             chosenMoveName = "MoveLeft";
    177.         }
    178.         else if(hitRight.collider != null && hitRight.collider.CompareTag("Enemy"))
    179.         {
    180.            
    181.             int choice = Random.Range(1, 3);
    182.  
    183.             if (choice == 1)
    184.             {
    185.                 StopCoroutine("AIMove_Event");
    186.                 AIMoveCoroutine_isRunning = false;
    187.                 Jump();
    188.             }
    189.             if (choice == 2)
    190.             {
    191.                 StopCoroutine("AIMove_Event");
    192.                 AIMoveCoroutine_isRunning = false;
    193.                 chosenMoveName = "MoveLeft";
    194.             }
    195.         }
    196.         else
    197.         {
    198.              transform.Translate(Vector2.right * speed * Time.deltaTime);
    199.         }
    200.     }
    201.  
    202.  
    203.     IEnumerator AIMove_Event()
    204.     {
    205.         yield return new WaitForEndOfFrame();
    206.         AIMoveCoroutine_isRunning = true;
    207.         StopCoroutine("AIMove_Timer");
    208.  
    209.         AIMoveEvent_Number = Random.Range(0, moveEvents.Capacity);
    210.         print(AIMoveEvent_Number);
    211.  
    212.         StartCoroutine("AIMove_Timer");
    213.  
    214.     }
    215.    
    216.     IEnumerator AIMove_Timer()
    217.     {
    218.         yield return new WaitForEndOfFrame();
    219.  
    220.         int timerValue = Random.Range(1, 5);
    221.         yield return new WaitForSeconds(timerValue);
    222.  
    223.         StopCoroutine("AIMove_Event");
    224.         AIMoveCoroutine_isRunning = false;
    225.         StartCoroutine("AIMove_Event");
    226.  
    227.     }
    228.  
    229.     void GroundedChecks()
    230.     {
    231.  
    232.         if (hitDown.collider != null)
    233.         {
    234.             isGrounded = true;
    235.  
    236.             if (!hitDown.collider.gameObject.CompareTag("Enemy"))
    237.             {
    238.                 isOnTopOfAnotherEnemy = false;
    239.             }
    240.             if (hitDown.collider.gameObject.CompareTag("Enemy"))
    241.             {
    242.                 isOnTopOfAnotherEnemy = true;
    243.             }
    244.  
    245.         }
    246.         if (hitDownRight.collider != null)
    247.         {
    248.             isGrounded = true;
    249.  
    250.             if (!hitDownRight.collider.gameObject.CompareTag("Enemy"))
    251.             {
    252.                 isOnTopOfAnotherEnemy = false;
    253.             }
    254.             if (hitDownRight.collider.gameObject.CompareTag("Enemy"))
    255.             {
    256.                 isOnTopOfAnotherEnemy = true;
    257.             }
    258.  
    259.         }
    260.         if (hitDownLeft.collider != null)
    261.         {
    262.             isGrounded = true;
    263.  
    264.             if (!hitDownLeft.collider.gameObject.CompareTag("Enemy"))
    265.             {
    266.                 isOnTopOfAnotherEnemy = false;
    267.             }
    268.             if (hitDownLeft.collider.gameObject.CompareTag("Enemy"))
    269.             {
    270.                 isOnTopOfAnotherEnemy = true;
    271.             }
    272.  
    273.         }
    274.  
    275.         else
    276.         {
    277.             isGrounded = false;
    278.             isOnTopOfAnotherEnemy = false;
    279.        
    280.         }
    281.        
    282.         if (isOnTopOfAnotherEnemy)
    283.         {
    284.        
    285.             int choice = Random.Range(1, 3);
    286.  
    287.             if (choice == 1)
    288.             {
    289.                 chosenMoveName = "MoveRight";
    290.  
    291.             }
    292.             if (choice == 2)
    293.             {
    294.                 chosenMoveName = "MoveLeft";
    295.             }
    296.  
    297.         }
    298.     }
    299.  
    300.     private void OnTriggerEnter2D(Collider2D collision)
    301.     {
    302.         if(collision.name == "GroundPoundTrigger")
    303.         {
    304.             health = health - 5;
    305.         }
    306.     }
    307. }
    Thanks in advance for any feedback!
     
  2. gilley033

    gilley033

    Joined:
    Jul 10, 2012
    Posts:
    1,174
    Whether this code would be problematic is really dependent on how many enemies you expect to have in the scene running this logic. If it's just a few I doubt there will be any issues. However, here are some notes on things I could see improved:
    1. There is no reason to create a new WaitForEndOfFrame intance everytime you wish to yield until the end of frame. Just create one instance in Awake or Start and use for all of those yields.
    2. Personally, I would try to encapsulate the movement logic (MoveLeft/MoveRight) in the AIMoveEvent classes themselves. Create a base abstract AIMoveEvent class with an abstract Move method, then create child classes that implement the Move method in whatever direction you want. This will eliminate the chosenMove.name string comparison, as you can just call Move and it will perform the correct type of movement.
    3. It would take too much to fully understand the purpose of the of the AIMove_Event and AIMove_Timer methods and the reason for calling StartCoroutine/StopCoroutine so often, but if I had to guess I'd say you could find a more efficient way of achieving whatever you are trying to achieve here. Again though, it probably won't be an issue unless you have many enemies. Just keep in mind that every time you call StartCoroutine, a bit of garbage is generated.
    This certainly isn't everything that could be "improved." In general, people will probably advise you to not worry too much about improving working code unless you see it become a problem (via profiling). The reason is, you can end up spending a bunch of time on something that really was never an issue to begin with.

    However, personally I think if you are a beginner it doesn't hurt to gather this type of feedback so that you can write more efficient code from the start. Good luck!
     
    MelvMay likes this.
  3. neginfinity

    neginfinity

    Joined:
    Jan 27, 2013
    Posts:
    13,499
    Some things I noticed.

    Code (csharp):
    1.  
    2. StartCoroutine("AIMove_Event");
    3.  
    Do not start coroutines using their name. If you rename the function, things will break with no warning.

    See example here: https://docs.unity3d.com/ScriptReference/MonoBehaviour.StartCoroutine.html
    And use StartCoroutine(AiMove_Event()).

    Code (csharp):
    1.  
    2. if(chosenMoveName == "MoveLeft")
    3.  
    Use switch-case. Additionally consider storing functiosn being called within the event itself. See System.Func, System.Action, idea of delegates.

    Code (csharp):
    1.  
    2.        hitDown = Physics2D.Raycast(new Vector2(transform.position.x, transform.position.y - 0.7f), Vector2.down, 0.3f);
    3.         Debug.DrawRay(new Vector2(transform.position.x, transform.position.y - 0.7f), Vector2.down, Color.green);
    4.  
    This pattern repeats several times, and can be probably turned into a function with parameters.

    Code (csharp):
    1.  
    2.  
    3.         if (hitLeft.collider != null && isGrounded)
    4.         {
    5.             int choice = Random.Range(1, 3);
    6.             if (choice == 1 && !isOnTopOfAnotherEnemy )
    7.  
    EVERY if/else branch has "hitCollider != null" in it, meaning it can be moved into outer scope, something like "if (hitLeft.collider == null) return".

    Code for left and right movement is nearly identical, meaning it should be refactored into a single function with parameters.

    Code (csharp):
    1.  
    2.             if (!hitDown.collider.gameObject.CompareTag("Enemy"))
    3.             {
    4.                 isOnTopOfAnotherEnemy = false;
    5.             }
    6.             if (hitDown.collider.gameObject.CompareTag("Enemy"))
    7.             {
    8.                 isOnTopOfAnotherEnemy = true;
    9.             }
    10.  
    11.  
    Replace with:
    isOnTopOfAnotherEnemy = hitDown.collider.gameObject.CompareTag("Enemy")


    Code (csharp):
    1.  
    2.         else
    3.         {
    4.             isGrounded = false;
    5.             isOnTopOfAnotherEnemy = false;
    6.        
    7.         }
    8.  
    9.  
    You can remove this else completely, just set the values BEFORE if/else branch.
    Code (csharp):
    1.  
    2.             isGrounded = false;
    3.             isOnTopOfAnotherEnemy = false;
    4.            if (...)
    5.  
    With previous fix you can remove the whole if/else tree.

    Code (csharp):
    1. if(collision.name == "GroundPoundTrigger")
    2.  
    If you forget to name collider correctly, the code will break. You WILL forget to name the collider correctly.

    Instead of checking tags or comparing names, you can create component for enemy and world colliders and check for their presence with GetComponent.
     
    SisusCo and Ryiah like this.
  4. halley

    halley

    Joined:
    Aug 26, 2013
    Posts:
    2,298
    @neginfinity has a bunch of good tips.

    But number zero on the list is always: get it to work first, and then measure your performance with the profiler. Always work on the #1 worst piece of code first, and then when it's no longer #1, work on the new #1. Every moment you spend trying to make the #16th worst piece of code faster is your time wasted.
     
    Ryiah likes this.
  5. zulo3d

    zulo3d

    Joined:
    Feb 18, 2023
    Posts:
    775
    Instead of having two almost identical functions that move left and right you could just have a single function that moves left or right. You could also drop the use of coroutines as they're not really necessary.

    Use AddForce to move your enemy left or right and if the rb.velocity drops then you can assume it has bumped into something and reverse its move direction. This will save you from having to do all those collision checks.

    You should be able to squish the whole thing into about 50 lines of code.
     
    Last edited: Nov 24, 2023
  6. CodeSmile

    CodeSmile

    Joined:
    Apr 10, 2014
    Posts:
    5,129
    The simple fix for that is to use the nameof expression:

    StartCoroutine(nameof(AIMove_Event));

    But I would generally advice against using several coroutines. It just gets complicated to manage which coroutine is running and which isn't. A simple statemachine can be implemented with switch or if/else constructs.

    choice == 1

    This is non-expressive code and a real maintenance headache. A month from now will you be able to remember what choice 12 is? Use an enum for that:

    choice == AIState.Attacking

    DRY principle:
    Code (CSharp):
    1. if(hitRight.collider != null && isGrounded)
    2. ..
    3. else if(hitRight.collider != null && !isGrounded)
    4. ..
    5. else if(hitRight.collider != null && hitRight.collider.CompareTag("Enemy"))
    6. ..
    Don't repeat checking for null in every condition! Not only will this run the check unnecessarily often, it also makes the code harder to read.

    In addition, avoid explicitly writing opposite / negated conditions. Did you notice that the above if/else if/else construct will NEVER execute the third block? ;)

    Here's the simplified form of the conditions:
    Code (CSharp):
    1. if(isGrounded)
    2. ..
    3. else if(!isGrounded)
    4. ..
    5. else if (something else)
    6. ..
    7.  
    You see it now?

    You'll spot this IMMEDIATELY in the corrected version:
    Code (CSharp):
    1. if(isGrounded)
    2. ..
    3. else // if we get here, isGrounded is DEFINITELY false
    4. ..
    5.  
    That is why you should NEVER ever write code that checks one condition, and then else if's on the negated condition.

    This also tells me that you haven't tested your code. There's a seemingly important portion of your code that NEVER executes and you did not notice that. How come?

    Best practice programming rule NUMBER ONE: test your code! ALL OF IT! ;)
    (well at least what's logically important, you needn't test every error condition)


    Lastly, about your IsGrounded checks .. well here's how I would write it:

    Code (CSharp):
    1. private void GroundedChecks()
    2. {
    3.     var downColliderValid = hitDown.collider != null;
    4.     var rightColliderValid = hitDownRight.collider != null;
    5.     var leftColliderValid = hitDownLeft.collider != null;
    6.     isGrounded = downColliderValid || rightColliderValid || leftColliderValid;
    7.     isOnTopOfAnotherEnemy = downColliderValid && hitDown.collider.CompareTag("Enemy") ||
    8.                             rightColliderValid && hitDownRight.collider.CompareTag("Enemy") ||
    9.                             leftColliderValid && hitDownLeft.collider.CompareTag("Enemy");
    10. }
    Notice that the collider has the CompareTag method too since it is declared in the Component class which both Collider and GameObject inherit from.
     
    SisusCo likes this.
  7. _geo__

    _geo__

    Joined:
    Feb 26, 2014
    Posts:
    1,256
    Not 100% sure I got all your logic right but this is a rewrite with some comments :)

    Code (CSharp):
    1.  
    2. using System.Collections;
    3. using System.Collections.Generic;
    4. using UnityEngine;
    5.  
    6. // Added to make my IDE happy.
    7. public class AIMoveEvent
    8. {
    9.     public string name;
    10. }
    11.  
    12. public class Enemy : MonoBehaviour
    13. {
    14.     // Not a fan of strings, but if you use them then please only define them in one spot.
    15.     // It's just too easy to mess those up
    16.     public const string MoveLeftName = "MoveLeft";
    17.     public const string MoveRightName = "MoveRight";
    18.     public const string EnemyTag = "Enemy";
    19.     public const string GroundPoundTrigger = "GroundPoundTrigger";
    20.  
    21.     public List<AIMoveEvent> moveEvents;
    22.  
    23.     // You are mixing naming converntions. Use _ or camel case. Using both is confusing.
    24.     // Also it's a convention among programmes to call this an INDEX, not number (nitpick).
    25.     // Also some of your members start with lower case (health, speed, ...) but this starts upper case.
    26.     int AIMoveEvent_Number;
    27.  
    28.     // I'd rather store reference to the current move than a copy of the string.
    29.     // Scratch that: How about full normalization and access the elements directly.
    30.     // Another improvement would be that by updating "AIMoveEvent_Number" you automatically
    31.     // "update" the chosenMove. You can NOT not update it.
    32.     AIMoveEvent chosenMove => moveEvents[AIMoveEvent_Number];
    33.  
    34.     public Rigidbody2D rb;
    35.     public float health;
    36.     public float speed;
    37.     public bool isGrounded;
    38.     public bool isOnTopOfAnotherEnemy;
    39.  
    40.     // You may want to change this if performance is hit by it (I doubt it).
    41.     public bool isMoving => AIMoveCoroutine != null;
    42.  
    43.     // Instead of using an extra bool you could store a reference to the coroutine.
    44.     // It has the added benefit that you can easily stop it anytime and you can not
    45.     // forget to update isRunning.
    46.     // bool AIMoveCoroutine_isRunning;
    47.     Coroutine AIMoveCoroutine;
    48.     RaycastHit2D hitDown, hitDownLeft, hitDownRight;
    49.     RaycastHit2D hitRight;
    50.     RaycastHit2D hitLeft;
    51.  
    52.     void Start()
    53.     {
    54.         rb = GetComponent<Rigidbody2D>();
    55.         startMovement();
    56.     }
    57.  
    58.     // Update is called once per frame
    59.     void Update()
    60.     {
    61.         GroundedChecks();
    62.         Move();
    63.  
    64.         if (health <= 0)
    65.         {
    66.             Destroy(gameObject);
    67.         }
    68.     }
    69.  
    70.     private void Move()
    71.     {
    72.         // DRY Principle ([D]on't [R]epeat [Y]ourself). Make repeating code into a method.
    73.         // That way it's also easier to grasp what is going on (helps with debugging).
    74.         hitDown = checkDirection(startX: 0f, startY: -0.7f, direction: Vector2.down, distance: 0.3f, debug: true, debugColor: Color.green);
    75.         hitDownRight = checkDirection(startX: 0.2f, startY: -0.7f, direction: Vector2.down, distance: 0.3f, debug: true, debugColor: Color.green);
    76.         hitDownLeft = checkDirection(startX: -0.2f, startY: -0.7f, direction: Vector2.down, distance: 0.3f, debug: true, debugColor: Color.green);
    77.         hitRight = checkDirection(startX: 0.7f, startY: 0f, direction: Vector2.right, distance: 0.5f, debug: true, debugColor: Color.red);
    78.         hitLeft = checkDirection(startX: 0.7f, startY: 0f, direction: Vector2.left, distance: 0.5f, debug: true, debugColor: Color.red);
    79.  
    80.         print(chosenMove.name);
    81.  
    82.         // Don't use strings. How about an enum or a reference to the move object itself.
    83.         // A switch might be nice here, though not sure.
    84.         if (chosenMove.name == MoveLeftName)
    85.         {
    86.             MoveLeft(); // MoveLeft() can change the chosenMove. That might be unexpected for future you.
    87.                         // Maybe consider making this a return value.
    88.         }
    89.  
    90.         if (chosenMove.name == MoveRightName)
    91.         {
    92.             MoveRight(); // MoveRight() can also change the choseMove
    93.         }
    94.     }
    95.  
    96.     private RaycastHit2D checkDirection(float startX, float startY, Vector2 direction, float distance, bool debug, Color debugColor)
    97.     {
    98. #if UNITY_EDITOR
    99.         if (debug)
    100.         {
    101.             Debug.DrawRay(new Vector2(transform.position.x + startX, transform.position.y + startY), direction, debugColor);
    102.         }
    103. #endif
    104.  
    105.         return Physics2D.Raycast(new Vector2(transform.position.x + startX, transform.position.y + startY), direction, distance);
    106.     }
    107.  
    108.     void Jump()
    109.     {
    110.         startMovement();
    111.  
    112.         if (!isOnTopOfAnotherEnemy)
    113.         {
    114.             stopMovement();
    115.             rb.AddForce(Vector2.up * 2.5f, ForceMode2D.Impulse);
    116.         }
    117.  
    118.     }
    119.  
    120.     private void startMovement()
    121.     {
    122.         if (isMoving)
    123.             return;
    124.  
    125.         if (AIMoveCoroutine == null)
    126.         {
    127.             // No string needed. You can use AIMove_Event() here.
    128.             AIMoveCoroutine = StartCoroutine(AIMove_Event());
    129.         }
    130.     }
    131.  
    132.     private void stopMovement()
    133.     {
    134.         if (!isMoving)
    135.             return;
    136.  
    137.         if (AIMoveCoroutine != null)
    138.         {
    139.             StopCoroutine(AIMoveCoroutine);
    140.         }
    141.         AIMoveCoroutine = null;
    142.     }
    143.  
    144.     void MoveLeft()
    145.     {
    146.         Move(getMoveEventIndexByName(MoveLeftName), hitLeft, Vector2.left);
    147.     }
    148.  
    149.     void MoveRight()
    150.     {
    151.         Move(getMoveEventIndexByName(MoveRightName), hitRight, Vector2.right);
    152.     }
    153.  
    154.     void Move(int movementIndex, RaycastHit2D hit, Vector2 defaultMovement)
    155.     {
    156.         startMovement();
    157.  
    158.         if (hit.collider != null)
    159.         {
    160.             if (isGrounded)
    161.             {
    162.                 bool choice = getRandomBool();
    163.                 if (choice && !isOnTopOfAnotherEnemy)
    164.                 {
    165.                     stopMovement();
    166.                     Jump();
    167.                 }
    168.                 if (!choice || isOnTopOfAnotherEnemy)
    169.                 {
    170.                     stopMovement();
    171.                     AIMoveEvent_Number = movementIndex;
    172.                 }
    173.             }
    174.             else
    175.             {
    176.                 AIMoveEvent_Number = movementIndex;
    177.             }
    178.             // In your origianl code this would never be executed!
    179.             // Not sure where it belongs.
    180.             /*
    181.             else if (hit.collider.CompareTag(EnemyTag))
    182.             {
    183.                 bool choice = getRandomBool();
    184.                 if (choice)
    185.                 {
    186.                     stopMovement();
    187.                     Jump();
    188.                 }
    189.                 else
    190.                 {
    191.                     stopMovement();
    192.                     AIMoveEvent_Number = movementIndex;
    193.                 }
    194.             }
    195.             */
    196.         }
    197.    
    198.         // Not sure if this should always be executed.
    199.         // Your if-else constructs in the move methods are definitely faulty.
    200.         transform.Translate(defaultMovement * speed * Time.deltaTime);
    201.     }
    202.  
    203.     private static bool getRandomBool()
    204.     {
    205.         return Random.Range(0, 2) == 1;
    206.     }
    207.  
    208.     // This you could replace if needed due to performance but I doubt it will ever be necessary.
    209.     private int getMoveEventIndexByName(string name)
    210.     {
    211.         for (int i = 0; i < moveEvents.Count; i++)
    212.         {
    213.             if (moveEvents[i].name == name)
    214.                 return i;
    215.         }
    216.  
    217.         return -1;
    218.     }
    219.  
    220.     IEnumerator AIMove_Event()
    221.     {
    222.         yield return new WaitForEndOfFrame();
    223.  
    224.         // U sure you want to use CAPACITY here not COUNT?!
    225.         // Using CAPACITY might give you and IndexOutOfBoundsException when used.
    226.         // AIMoveEvent_Number = Random.Range(0, moveEvents.Capacity);
    227.         AIMoveEvent_Number = Random.Range(0, moveEvents.Count);
    228.         print(AIMoveEvent_Number);
    229.  
    230.         yield return new WaitForEndOfFrame();
    231.  
    232.         int timerValue = Random.Range(1, 5);
    233.         yield return new WaitForSeconds(timerValue);
    234.  
    235.         // Repeating coroutine. Maybe think about mocing this to Update().
    236.         // Coroutines are a costly way of doing this.
    237.         stopMovement();
    238.         startMovement();
    239.     }
    240.  
    241.     private void GroundedChecks()
    242.     {
    243.         // By courtesy of CodeSmile
    244.         var downColliderValid = hitDown.collider != null;
    245.         var rightColliderValid = hitDownRight.collider != null;
    246.         var leftColliderValid = hitDownLeft.collider != null;
    247.         isGrounded = downColliderValid || rightColliderValid || leftColliderValid;
    248.         isOnTopOfAnotherEnemy = downColliderValid && hitDown.collider.CompareTag(EnemyTag) ||
    249.                                 rightColliderValid && hitDownRight.collider.CompareTag(EnemyTag) ||
    250.                                 leftColliderValid && hitDownLeft.collider.CompareTag(EnemyTag);
    251.  
    252.         if (isOnTopOfAnotherEnemy)
    253.         {
    254.             bool moveRight = getRandomBool();
    255.             if (moveRight)
    256.             {
    257.                 AIMoveEvent_Number = getMoveEventIndexByName(MoveRightName);
    258.             }
    259.             else
    260.             {
    261.                 AIMoveEvent_Number = getMoveEventIndexByName(MoveLeftName);
    262.             }
    263.         }
    264.     }
    265.  
    266.     private void OnTriggerEnter2D(Collider2D collision)
    267.     {
    268.         if (collision.CompareTag(GroundPoundTrigger))
    269.         {
    270.             ChangeHealthBy(-5);
    271.         }
    272.     }
    273.  
    274.     private void ChangeHealthBy(float delta)
    275.     {
    276.         health = Mathf.Max(0, health + delta); // Often you don't want health to become negative.
    277.                                            // Not sure, just something I like to do.
    278.     }
    279. }
    280.  
    281.  
    One of the recurring things I noticed is that you have some information redundancy in there. Eliminating those will make your code much easier to maintain (see my comments in the code).

    Your Move...() if else constructs are definitely faulty (some code is never executed).

    Your coroutine nesting was unnecessary (if I interpreted what you want to do correctly).
     
    Last edited: Nov 25, 2023
  8. zulo3d

    zulo3d

    Joined:
    Feb 18, 2023
    Posts:
    775
    Code (CSharp):
    1. using UnityEngine;
    2. public class Enemy : MonoBehaviour
    3. {
    4.     Rigidbody2D rb;
    5.     public float health;
    6.     Vector2 moveDirection=Vector2.right;
    7.  
    8.     void Start()
    9.     {
    10.         rb=GetComponent<Rigidbody2D>();
    11.         Physics2D.queriesStartInColliders=false;
    12.     }
    13.  
    14.     void FixedUpdate()
    15.     {
    16.         if (rb.velocity.magnitude<1 && Physics2D.CircleCast(transform.position,0.4f,Vector2.down,1.0f) && Physics2D.CircleCast(transform.position,0.4f,moveDirection,1.0f))  // are we grounded and have hit a wall?
    17.         {
    18.             if (Random.Range(0,2)>0)
    19.                 rb.AddForce(Vector2.up*20,ForceMode2D.Impulse);  // sometimes jump
    20.             else
    21.                 moveDirection=-moveDirection;   // sometimes change direction
    22.         }
    23.         else
    24.             rb.AddForce(moveDirection*30f);    // move left or right
    25.  
    26.         if (health<=0)
    27.             Destroy(gameObject);
    28.     }
    29.    
    30.     void OnCollisionEnter2D(Collision2D collision)
    31.     {
    32.         if (collision.gameObject.name=="GroundPoundTrigger")
    33.             health=health-5;
    34.     }
    35. }
     
    Last edited: Nov 25, 2023
  9. SisusCo

    SisusCo

    Joined:
    Jan 29, 2019
    Posts:
    1,266
    There's no reason to have both movement and health handling in the same component. Extracting all the health related stuff into its own component would improve readability on the whole.

    In general long scripts aren't necessarily bad, if they
    1. hide the complexity of the implementation details and are simple for other scripts to work with in code,
    2. have good cohesion (everything inside the classes feel like they belong together; they don't have multiple unrelated responsibilities),
    3. and can be extended without making modifications to the existing code (or probably will not have to be extended at all).
    It's when you have many classes tightly intertwined together in a spaghetti code mess, or a really long and complicated class that needs to be modified often, that bugs tend to become a big problem.