Why does my foreach loop crash where a for loop won't?

Development Programming & Scripting
1

I’m working on a function that iterates through a list of quests the player has, checks if each quest has been completed, and if it has been a) give the player that quest’s rewards, then b) remove said quest from the list:

void EvaluateQuest(UQuestBase* quest) {
	if (quest->IsComplete()) {
		//Add reward
		int tempInt = quest->reward.money;
		GEngine->AddOnScreenDebugMessage(-1, 5.f, FColor::Red, FString::Printf(TEXT("Quest completed! Reward: %i dollars"), tempInt));
		questList.Remove(quest);
	}

}

Because the second part of evaluating a quest involves removing finished quests from a list currently being iterated through, the following code compiles and runs but will not consistently process every quest in the list (as the list’s size changes during the loop, rendering list.Num() inaccurate):

for (int i = 0; i < questList.Num(); i++) {
	EvaluateQuest(questList[i]);
}

To solve this, I thought it would be easier to just explicitly say “For each element in questList, do some stuff”:

for (UQuestBase* quest : questList)
{
	EvaluateQuest(quest);
}

This also compiles, but unreal crashes every time it runs- am I doing something obviously wrong here? I assume it’s related to the fact that EvaluateQuest() can change the size of an array that is actively being iterated through- is it just a fundamentally bad idea to modify the array at the same time I evaluate something inside of it?

2

This is traditionally solved by simply iterating backwards from the last element to the first to preserve index integrity, like so:

for (int i = questList.Num() - 1; i >= 0 ; i--)
{
        if (questList[i]->IsComplete())
        {
                // your game logic

                EvaluateQuest.RemoveAt(i)
        }
}

Work it out on a piece of paper to visualize why this works if you need to, it’s equally important to understand why it works.

is it just a fundamentally bad idea to
modify the array at the same time I
evaluate something inside of it?

It is certainly a bad idea to blindly iterate over the elements while removing them conditionally regardless of which “for” syntax is used. Foreach and for are functionally equivalent, the crash occurs because of foreach’s internal implementation.

In this thread Epic’s dev explains several techniques you can use to handle this situation include Lambda predicates. But remember that the other methods discussed in that thread (like passing a lambda predicate to RemoveAll) won’t work for your case because your remove operations are coming from an outer loop which is also iterating over the array!

In fact, your original code already uses the lambda predicate method. How? Because questList.Remove(quest) internally calls questList.RemoveAll(…) with a lambda predicate on “quest”!

–

BTW there’s a more important issue with the original code:

You should definitely prefer using questList.RemoveAt(index) instead of calling questList.Remove(item) whenever you already know the index of an array element you want to remove!.

Remember that calling Remove(item) forces TArray to manually iterate through each element looking for the right index; a waste of cycles, because you already know what the index is! Always be conscious of what functions you’re calling and how they work internally when you’re dealing with large arrays or indeed, any array!

Suggested reading: Optimizing TArray Usage for Performance - Unreal Engine

3

Wow, this is a way more in-depth question than I thought it was, thank you so much for the detailed explanation and reading material!! I’m having a lot of fun reading the TArray guide now, I wonder if that means that my brain is getting weird :slight_smile: