C# - foreach problem

Question

The problem is that I'm creating here an array of Colliders, to get acces to script "Enemy" attached to them and execute function which slow object. After I save all colliders in class variable resetCol. Then I wait for 1.2f and want to reset speed of each object inside this array. So problem is, that when some object is destroyed before executing last part of this code (under "yield return new WaitForSeconds(1.2f);") - unity show me an error. As - there is no reference "NullReferenceException" and it's ok, because in array appears empty space. So Idk how to fix it, because inside foreach I'm checking for "collider != null", so it shouldn't take empty space inside array, isn't it?

So, basically, the problem is - am I checking empty statement inside array wrongly?

I probably misspoke something (have not good enough English knowledge), so ask - I'll try to explain.

 Collider[] resetCol;
 public float duration;
 public float slowerSlow;

 IEnumerator GrowingSphere()
     {
         float r = 0;
         float distance = 0;
         while (r < duration)
         {
             Collider[] colliders = Physics.OverlapSphere(transform.position, distance);
             r++;
             distance += .074f;
             foreach (Collider collider in colliders)
             {
                 if (collider.tag == "Enemy")
                 {
                     collider.GetComponent<Enemy>().Slow(slowerSlow);
                 }
             }
             yield return new WaitForFixedUpdate();
             resetCol = colliders;
         }
         yield return new WaitForSeconds(1.2f);
         foreach (Collider collider in resetCol)
         {
             if (collider != null)
             {
                 collider.GetComponent<Enemy>().speed = collider.GetComponent<Enemy>().startSpeed;
             }
         }
     }

Answer 1

Best Answer

Answer by MaxGuernseyIII · Jan 20, 2018 at 08:50 AM

You need to decouple when the decision to do something is made from when it actually happens. That's a command pattern type problem.

I would use something like Invoke to make the speed be reset after 1.2 seconds. I believe that scheduled invoke's aren't executed if an object was destroyed in the intervening time. For instance, you could modify Enemy to have a method like SetSpeedAfterDelay that uses invoke to defer setting.

That assumes that you have access to the Enemy script. If not, you'd have to do a little more work.

In any event, I would get rid of the second foreach altogether and just schedule a command in the initial foreach.

2 Show 7 · Share

ADiSiN · Jan 20, 2018 at 02:00 PM 2

Share

That's brilliant, that's really much more easier and clear in my opinion than those a mess of codes that I've created. I've tried it out and it's works good! x) Thank you!

pako ADiSiN · Jan 20, 2018 at 02:19 PM 1

Share

@ADiSiN since you tried this and it works well,I think it's more fair and helpful to other that I unaccept my answer (that you've marked best) and accept this one.

ADiSiN pako · Jan 20, 2018 at 02:32 PM 1

Share

Well, both your answers is very helpful, your answer show why there was a problem in my certain situation and how to solve it, and this answer gave another view on whole construction and how to solve the problem by changing the way of thinking. However, I've chosen yours as the best, because if other people will search for same problem as mine, then your answer will be more common for them, cos it's much more often happens that people forget to check statements. Anyway, both your answers very helpful, so ok ;)

MaxGuernseyIII ADiSiN · Jan 20, 2018 at 08:56 PM 1

Share

I'm glad you found the advice helpful. It's an application of patterns thinking.

Encapsulate that which varies.

In this case the delay and ultimate execution of the command.

pako · Jan 20, 2018 at 02:15 PM 1

Share

This IMHO is probably the best answer (I'm saying this even after the OP accepted my answer as best), especially if it had included some example code. In fact, I think that not-having example code is what prevents this answer from standing out!

So, I'll add some explanatory example code:

Enemy script:

 public class Enemy {
 
 public float speed;
 public float startSpeed;

 float slowerSlow;
 
 public void Slow(float newspeed)
 
 //existing code e.g. speed = newSpeed;
 
 //additional code
 if (newspeed = slowerSlow){
 Invoke("ResetSpeed", 1.2f);
 }

 }
 
 void ResetSpeed(){
 
 speed = startSpeed;
 
 }

If you do this you can remove the code line 24-31 form GrowingSphere.

MaxGuernseyIII pako · Jan 20, 2018 at 09:00 PM 0

Share

Enough methods like that on Enemy and speed would not need to be a public variable at all.

pako MaxGuernseyIII · Jan 20, 2018 at 10:04 PM 1

Share

Quite right! I just did a quick copy/paste for the example... didn't give any thought to that.

Answer 2

Answer by pako · Jan 20, 2018 at 01:30 PM

@ADiSiN in your first foreach statement you selectively call collider.GetComponent<Enemy>().Slow(slowerSlow); to those colliders tagged "Enemy". If you have found the need to do that, it means that you suspect, or may have found, that the the OverlapSphere method returns some other colliders that are not tagged "Enemy".

And apparently, this is why your getting the NullReferenceException, not because the code inside the second foreach tries to execute a statement when there's no collider, but because a collider instance does not have an Enemy Component. So, collider.GetComponent<Enemy>() returns null, and your are trying to set the speed of null to the startSpeed of null.

So, the solution is what @PizzaPie suggested, i.e. to check if the collider has an Enemy component:

 if (collider != null && collider.GetComponent<Enemy>() != null)

3 Show 6 · Share

Bunny83 · Jan 20, 2018 at 01:49 PM 2

Share

Yep, i guess that's the problem. Though a better layout of the second foreach loop would be:

 foreach (Collider collider in resetCol)
 {
      if (collider == null)
         continue;
     Enemy enemy = collider.GetComponent<Enemy>();
     if (enemy == null)
         continue;
     enemy.speed = enemy.startSpeed;
 }

Also code like the last line should be generally avoided, especially since there is already a "Slow" method on the Enemy it would be better to have a method "ResetSpeed()" in the enemy which you call inplace of enemy.speed = enemy.startSpeed;. If various different components can directly change the state of other components you quickly will loose track of who is changing what and when. That's the main point of using encapsulation. You don't have to stick to the OOP principles like they are absolute laws but more like good practises. The execution flow of methods can be followed more easily when debugging.

ransomink Bunny83 · Jan 20, 2018 at 02:08 PM 0

Share

Question: If the collider is picked up by OverlapSphere, why check if it's null? I understand the Enemy component being the culprit but it has to have a Collider on it to be in the array in the first place, right?

pako ransomink · Jan 20, 2018 at 02:25 PM 1

Share

Yes, I thought about that too. However, the 1.2s delay may be enough time for some of the colliders picked up originally by OverlapSphere to have been destroyed, and will now by null inside the array.

ADiSiN · Jan 20, 2018 at 01:56 PM 2

Share

Oh, yeah! Thank you so much, there was the problem, I wasn't pay attention that I'm creating an array of all colliders and not only colliders with certain tag. Your suggestion, as well as @PizzaPie suggestion solved it. Funny thing, that I thought about that but instead of 2 checkings I made only 1 "collider.GetComponent() != null" and it still showed me an error because I can't check for component if there is no object. Thank you x) I see your comment, so make as suggested @MaxGuernseyIII will be more efficient?

ransomink ADiSiN · Jan 20, 2018 at 02:12 PM 0

Share

If you're searching for the enemy tag then those game objects are expected to have an Enemy component on them, like @Bunny83 mentioned. I think the issue is making sure the game objects tagged "enemy" have the Enemy component attached from the inspector. So check that they do...

I think checking if the collider is null will not help because the collider was already found, hence, why it's in the Collider array...

ADiSiN ransomink · Jan 20, 2018 at 02:23 PM 1

Share

Checking if the collider is null is neccessary only in my certain situation, when I assign class array to loop array and then I wait 1.2f to make changes in it. So, there is problem - if within these 1.2f my object will be destroyed, then in my array will be hole with an empty object (I may not use correct words, sorry for that, but hope you understand). So, if not check if collider is null, then it'll throw an error, that I don't have reference, but still trying to acces to component. However, I found @MaxGuernseyIII great solution and removed second array, so now all works great. But the problem was that I didn't check for an object and an component on him as well.

Answer 3

Answer by ransomink · Jan 20, 2018 at 10:48 AM

On which line is the null reference exception? That'll tell us what variable to look for. It seems resetCol could be null because you say the array is empty. This is because resetCol must have the same length as colliders, if it doesn't, then not all or any of the collided game objects will copy to the array. You have to set the length of the array first then copy it over

 resetCol = new Collider[ colliders.Length ];
 resetCol = colliders;

Also, use if (collider.CompareTag( "Enemy" ) ) instead of .tag. It is much more efficient...

0 Show 4 · Share

Bunny83 · Jan 20, 2018 at 01:38 PM 2

Share

Your first part is nonsense. An array is a reference type. Assigning one array to another variables just makes both variables reference the same array. Your code would create a new empty array, assign it to resetCol and then you overwrite the the reference with the reference to the array stored in colliders. This will cause your newly created array to be garbage collected.

ransomink Bunny83 · Jan 20, 2018 at 02:04 PM 0

Share

Yeesh, you could have easily said, "This will not work as your method only creates an empty array and overwrites it causing the new array to create garbage." or something. Sounds a bit harsh to someone trying to help. I'm used to OverlapSphereNonAlloc where you have to initialize the array with a set number to make sure you grab all possible colliders. Guess I shouldn't help people at 5 am and sleep instead....

MaxGuernseyIII ransomink · Jan 20, 2018 at 09:05 PM 0

Share

I think @Bunny83 didn't mean "gibberish" I think @Bunny83 meant "could safely be optimized to a no-op". Maybe.

ADiSiN · Jan 20, 2018 at 01:57 PM 0

Share

Thank you for answer, but no, as said @Bunny83 there isn't problem with my lenght, however, thank you for information about CompareTag. I didn't know it ;)

C# - foreach problem

3 Replies

Your answer

Welcome to Unity Answers

Follow this Question

Related Questions