- Home /
C# - foreach problem
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;
}
}
}
Probably you need on 2nd foreach to check if it has the Enemy component and then access it as you do in first one.
if (collider != null && collider.GetComponent<Enemy>() /*!= null*/)
Agreed, and you might think about making a class-scope List and adding the slowed objects to that ins$$anonymous$$d of using the original array. As long as its cached and you just call Clear on it ins$$anonymous$$d of re-making the list, it's not a real performance hit.
You might consider making .Slow on the Enemy component just create a new coroutine locally and handle its own reset. This way seems convoluted to me.
That's a very good idea. Having, say, a List<Collider> EnemyColliders
and adding in it all colliders that were found to be tagged "Enemy" in the first foreach loop, would make things simpler, since in the second foreach loop he would iterate though the EnemyColliders
ins$$anonymous$$d of the resetCol
array.
So, the there would be no need check in the second foreach
collider.GetComponent<Enemy>() != null
, and also if there is a significant amount of non-Enemy colliders returned from the OverlapSphere
call, there would be some performance increase, since the second foreach
would iterate through a smaller number of elements.
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.
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!
@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.
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 $$anonymous$$e, 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 ;)
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.
This I$$anonymous$$HO 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
.
Enough methods like that on Enemy and speed would not need to be a public variable at all.
Quite right! I just did a quick copy/paste for the example... didn't give any thought to that.
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)
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.
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?
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.
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 ins$$anonymous$$d 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 @$$anonymous$$axGuernseyIII will be more efficient?
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...
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 @$$anonymous$$axGuernseyIII 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 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...
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.
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 ins$$anonymous$$d....
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 ;)