Search Unity

Adding "Attacker" objects to list detected by collision reseting list count to 0

Discussion in 'Scripting' started by SprayNpraY, Aug 26, 2019.

  1. SprayNpraY

    SprayNpraY

    Joined:
    Oct 2, 2014
    Posts:
    89
    Hi I'm very confused to why this is happening after several tests and google searches I can't find an answer. But basically following a Udemy 2D unity game dev tutorial but adding some of my own features hense why Im here and basically.

    I'm wanting to detect "Attackers" that are going to a particular type of "ShieldTower" and for each attacker that is collided to be added to a list so it can be checked periodically by an enumeration to detect if any attackers are still attacking it to control an animator.

    Code (CSharp):
    1. using System.Collections;
    2. using System.Collections.Generic;
    3. using UnityEngine;
    4.  
    5. public class ShieldTower : MonoBehaviour {
    6.     Animator animator;
    7.  
    8.     private List<Attacker> colliders;
    9.     // Use this for initialization
    10.  
    11.     void Start () {
    12.         animator = GetComponent<Animator>();
    13.         colliders = new List<Attacker>();
    14.         StartCoroutine(CheckIfBeingAttacked());
    15.        
    16.  
    17.     }
    18.  
    19.     private void OnTriggerEnter2D(Collider2D collision)
    20.     {
    21.         Attacker attacker = collision.gameObject.GetComponent<Attacker>();
    22.         Debug.Log("Oncollision stay being called");
    23.         if (attacker)
    24.         {
    25.             animator.SetBool("isBeingAttacked", true);
    26.             colliders.Add(attacker);
    27.             Debug.Log(colliders.Count);
    28.             foreach (var attackers in colliders)
    29.             {
    30.                 Debug.Log("Attacker name: " + attackers.name);
    31.             }
    32.             Debug.Log("Detecting attacker");
    33.  
    34.         }
    35.      
    36.     }
    37.  
    38.  
    39.  
    40.     public void NoLongerBeingAttacked()
    41.     {
    42.         animator.SetBool("isBeingAttacked", false);
    43.         Debug.Log("Not being attacked called");
    44.     }
    45.  
    46.     IEnumerator CheckIfBeingAttacked()
    47.     {
    48.         while (true)
    49.         {
    50.             yield return new WaitForSeconds(1f);
    51.             Debug.Log("Checking if being attacked called");
    52.            
    53.  
    54.             for (int i = 0; i < colliders.Count; i++)
    55.             {
    56.                 Debug.Log("For loop called for: " + i + "time");
    57.                 if (colliders[i] == null)
    58.                 {
    59.                     Debug.Log("Removing null object from list");
    60.                     colliders.Remove(colliders[i]);
    61.                 }
    62.                 else if (colliders[i] != null)
    63.                 {
    64.                     colliders.Remove(colliders[i]);
    65.                     break;
    66.                 }
    67.                 NoLongerBeingAttacked();
    68.             }  
    69.                
    70.            
    71.  
    72.              
    73.             }
    74.         }
    75.      
    76.     }
    In the debug logs it keeps saying the list count is at 1 even if multiple attackers have reached the tower. I have tested with breakpoints in visual studio and it appears that whenever a new attacker is colliding with the collider on the tower it is replacing or resetting the list count and replacing it with the new attacker that has collided hence why the count is remaining at 1.

    The attackers that are colliding with the tower are being spawned from the same point and are from the same prefab.

    Does anyone understand what is causing this issue?

    Thanks
     
  2. Kurt-Dekker

    Kurt-Dekker

    Joined:
    Mar 16, 2013
    Posts:
    4,244
    Your logic in line 57 through 66 (listing above) says basically:

    "If the collider is null, remove it, else if the collider is NOT null, remove it."

    I am guessing the issue lies somewhere near there. :)
     
  3. Antistone

    Antistone

    Joined:
    Feb 22, 2014
    Posts:
    1,258
    Also, removing objects from a list while you are iterating over that list can mess up your iteration. Looks to me like when your loop removes something from the list, it also effectively skips the next thing in the iteration.

    Imagine you have list {A, B, C, D}

    When i == 1, you are looking at B. Suppose you remove B.

    On the next loop iteration, i == 2, and your list is now {A, C, D}, which means you are now looking at D. You skipped C.
     
    SprayNpraY and Kurt-Dekker like this.
  4. Antistone

    Antistone

    Joined:
    Feb 22, 2014
    Posts:
    1,258
    ALSO also, I notice that "NoLongerBeingAttacked" gets called for each null entry in your list, even if there are also non-null entries. So it could get called multiple times in a single check, it could get called even if some of the attackers in the list are still valid, and it won't get called if the list is entirely empty. I'm guessing that's not working like you intended either.
     
  5. SprayNpraY

    SprayNpraY

    Joined:
    Oct 2, 2014
    Posts:
    89
    Thanks for the help I've made some changes and tried to figure out some solutions but theres so many things not working like I'd expect Im lost and thinking my code is a mess. Heres the changes I've made below hopefully you can see what I'm doing wrong.


    Code (CSharp):
    1. private void OnTriggerEnter2D(Collider2D collision)
    2.     {
    3.         Attacker attacker = collision.gameObject.GetComponent<Attacker>();
    4.         //Collider2D collider2D = collision.GetComponent<Collider2D>();
    5.         Debug.Log("Oncollision stay being called");
    6.         if (attacker)
    7.         {
    8.             animator.SetBool("isBeingAttacked", true);
    9.             isBeingAttacked = true;
    10.             attackerList.Add(attacker);
    11.             Debug.Log(attackerList.Count);
    12.         }
    13.      
    14.     }
    15.  
    16.  
    17.  
    18.     public void NoLongerBeingAttacked()
    19.     {
    20.         animator.SetBool("isBeingAttacked", false);
    21.         isBeingAttacked = false;
    22.         Debug.Log("Not being attacked called");
    23.     }
    24.  
    25.     IEnumerator CheckIfBeingAttacked()
    26.     {
    27.         while (true)
    28.         {
    29.             yield return new WaitForSeconds(1f);
    30.             Debug.Log("Checking if being attacked called");
    31.             attackerList.RemoveAll(x => x == null);
    32.             for (int i = 0; i < attackerList.Count; i++)
    33.             {
    34.                 Debug.Log("For loop called for: " + i + "time");
    35.                 Debug.Log("Name of attacker in list: " + attackerList[i]);
    36.                 if (attackerList[i] != null)
    37.                 {
    38.                     Debug.Log("Detecting element not null in list");
    39.                     isBeingAttacked = true;
    40.                    goto BeingAttacked;
    41.                 }
    42.                              
    43.             }
    44.             isBeingAttacked = false;
    45.  
    46.             if (!isBeingAttacked)
    47.             {
    48.                 NoLongerBeingAttacked();
    49.             }
    50.  
    51.         BeingAttacked:
    52.             break;
    53.         }
    54.     }
    The if statement:
    Code (CSharp):
    1.   if (attackerList[i] != null)
    2.                 {
    3.                     Debug.Log("Detecting element not null in list");
    4.                     isBeingAttacked = true;
    5.                    goto BeingAttacked;
    6.                 }
    Is never being called also the debuglog at the top of for the forloop:
    Code (CSharp):
    1.   Debug.Log("Name of attacker in list: " + attackerList[i]);
    is never called either which makes 0 sense to me as its at the top of the forloop

    Im using it to try and figure out if the attacker object is destroyed if it counts as null or empty in the list but it isn't being called.
     
  6. Antistone

    Antistone

    Joined:
    Feb 22, 2014
    Posts:
    1,258
    Using goto is almost always a bad idea, and I'm pretty sure your use of it here isn't doing what you think it's doing.

    How about you replace that entire loop section with something like:
    Code (CSharp):
    1. attackerList.RemoveAll(x => x == null);
    2. isBeingAttacked = (attackerList.Count > 0);
    3. if (!isBeingAttacked)
    4. {
    5.     NoLongerBeingAttacked();
    6. }
    Looks to me like the net result of your current goto is going to be that you break from the outer loop (where you check the attacker list once per second). That wouldn't explain your Debug.Log statements never being called, but it would explain them being called only once and then never again.

    If you're sure that they're not being called at all (even once), that implies that either you never started your coroutine or the attacker list never contains a non-null entry (the null entries are all being removed before you get to the loop).
     
    SprayNpraY likes this.
  7. SprayNpraY

    SprayNpraY

    Joined:
    Oct 2, 2014
    Posts:
    89
    Thanks Antistone I don't fully understand what goto even does I just came across it when I was trying to figure a way of skipping from a part of a loop to somewhere else.

    Your code has fixed it and is much simpler thanks again.
     
  8. CurtisMcGill

    CurtisMcGill

    Joined:
    Aug 7, 2012
    Posts:
    66
    The syntax goto is like a jump in coding which will control the flow of the program.

    You create a label <no spaces> and end with a colon :
    BeingAttacked:

    Code (CSharp):
    1. for (i=0;i<1000;i++)
    2. {
    3.   if (condition X) goto label
    4. }
    5.  
    6. label:
    7. debug.log("exited for loop);