Returning a class from an array sometimes results in nullptr

Development Programming & Scripting C++
1

Hi! So I have recently started getting crashes regularily due to memory access violation and have finally traced it back to the culprit. But I cannot figure out how it actually happens and how to fix it.

I have a list of actor classes that can be spawned at random.
I have logged and confirmed that the contents of the list are the classes as expected.

I use a randrange to pick which item in the array gets spawned:

UClass* UWSSpawnItems::ItemToDrop()
{
	UClass* SpawnClass;
	int SpawnRoll;
	int ListIndex = 0;
	bool bRollFound = false;

	SpawnRoll = FMath::RandRange(1, DroptableTotal);

	for (int i = 0; i < DropTable.Num() && !bRollFound; i++)
	{
		if (SpawnRoll <= DropTable[i].DropRollValue)
		{
			ListIndex = i;
			bRollFound = true;
		}
	}
   SpawnClass = DropTable[ListIndex].ItemClass;

   return SpawnClass;

}

I’ve even initialed the ListIndex to 0 so that even if there was an issue with how I’m picking the index to use, it will always return one of the items. However, it doesn’t. It works for a while, but eventually I get a nullptr in return, causing a crash. Usually after about 30 seconds of play or so.

The strange part is that I use the exact same method to spawn enemies without any issues whatsoever. The only real difference is that the item drop functionality is contained in an independent object class, while the enemy spawn functionality is within my gamemode.

Do perhaps plain objects behave differently than I realize in a way that could trigger this? Could garbage collection be emptying my list?

2

Yes! While I haven’t been able to test too thorougly it does indeed seem like the object it was contained in was the issue, I simply made the pointer to it an UPROPERTY, and now the references seem stable.

Could someone confirm and explain this?

3

Yes, if you don’t have the UPROPERTY markup on your member and it’s a UObject/AActor then garbage collection may get rid of that object.

There are other ways to manage it (with different function overrides), but using the UPROPERTY macro is the easiest and generally preferred method to prevent objects from being gc’d.

1 Like
4

I’ve tested it some more, and I am still occationally getting a nullptr, here’s an example of how the above function is used:

        AWSPickup* Drop;
		DropLocation.Z = 130;
		TSubclassOf<AWSPickup> SpawnClass = ItemSpawner->ItemToDrop(false);
		FActorSpawnParameters SpawnParams;
		SpawnParams.SpawnCollisionHandlingOverride = ESpawnActorCollisionHandlingMethod::AlwaysSpawn;
		if (SpawnClass)
		{
			Drop = GetWorld()->SpawnActor<AWSPickup>(*SpawnClass, DropLocation, FRotator(0, 0, 0), SpawnParams);
			if (Drop)
				Drop->ProxSphere->SetSphereRadius(64.0f);

Sometimes Drop is a Nullptr, I’m not sure if it’s because it can’t find the class and therefore not spawning, or if sometimes the spawning itself fails despite setting it to spawn regardless of collision. Any suggestions on what to be on the lookout for?
The list of classes are only children of AWSPickup, so the SpawnActor shouldn’t fail in that regard.

5

The only thing I can think of is that you’re somehow assigning a class to SpawnClass that doesn’t actually derive from AWSPickup. I’m not sure if the if (SpawnClass) would catch that or not. I do know that you can initialize a TSubclassOf so that it looks like it’s set to something in the debugger but will return nullptr because it’s not a proper match. Why it doesn’t do that on assignment, I have no idea.

Other than that, you’d likely have to debug SpawnActor and see what it’s deciding to do (or not do).

6

Yeah, I’m absolutely certain that there are only classes that derive from AWSPickup.

I guess I’ll just keep trying and see if I can figure something out if it keeps happening.
I haven’t had it happen for a while again, it might have been a compiler fluke, it seems to sometimes miss changes in my files.

7

Yeah if i do a lot of code changes or add more or delete a bunch. i have to recompile all so that all the new stuff will take effect and get compiled up. If i just use build sometimes it will not compile all the new stuff up or just part of it and i get some strange results, until i recompile all.

1 Like
8

If you have an empty DropTable anywhere you’ll be returning nullptr. Consider adding a check(), or manual check with logging to help you catch any instances of these.

If your SpawnRoll is greater than all of the drop table itemsDropRollValue then you will also be returning nullptr.

A few nit picky code review comments:
Consider caching the value of DropTable.Num() outside of the for loop rather than calling that unnecessarily every time through the loop (the compiler MIGHT make a similar optimization only if it can inline and optimize).
Since bRollFound isn’t used outside of the loop, you could consider removing it in favor of using a break statement inside the if statement.
Forward declaring all variables at the beginning of the function has a “c smell”. Modern C++ practices tend to prefer to declare it closer to where it is used.
Since you don’t use the index; you could replace the loop with a range-for syntax:
for (const REPLACE_WITH_YOUR_TYPE& DropTableItem : DropTable)
Then, in the body of the if statement in the loop, you can just “return DropTableItem.ItemClass;”
Of course, outside the loop, if you didn’t return, you’d probably want a:
check(false); // we should not get here; we didn’t find anything!
return nullptr;

If you put:
check(DropTable.Num() > 0);
before the loop, and you hit the check after the loop then you know that DropRollValue’s in that drop table aren’t setup properly.

These are all nit picky changes. But might help you get a handle on the issue if you haven’t already found the culprit.

2 Likes
9

Cool, I really love in depth code reviews like that, it really helps me get better!

I only have a single drop table array that is created at beginplay then never modified. Logging confirms contains the classes I want, but I’ll have it log a confirmation right before its content is used to make sure it doesn’t still get emptied or somehow manipulated at some point.

SpawnRoll and and ItemsDropRollValue are created in unison from the same source variables, so they should always match as far as I can tell.

void UWSSpawnItems::Init()
{
	if (!ItemLibrary)
	{
		ItemLibrary = UObjectLibrary::CreateLibrary(TSubclassOf<AWSPickup>(), true, GIsEditor);
		ItemLibrary->AddToRoot();
	}
	ItemLibrary->LoadAssetDataFromPath(TEXT("/Game/TwinStick/Gameplay/Pickups"));
	//GEngine->AddOnScreenDebugMessage(-1, 5.f, FColor::Orange, FString::FromInt(ItemLibrary->GetAssetDataCount()));

	if (!ItemLibrary->IsLibraryFullyLoaded())
		ItemLibrary->LoadAssetsFromAssetData();

	ItemLibrary->GetAssetDataList(ItemAssets);
	//GEngine->AddOnScreenDebugMessage(-1, 5.f, FColor::Orange, FString::FromInt(ItemAssets.Num()));

	int assetNum = ItemAssets.Num();

	for (int i = 0; i < assetNum; i++)
	{
		UClass* ClassToAdd;
		FString iBP;

		iBP = ItemAssets[i].GetPackage()->GetFName().ToString();

		//GEngine->AddOnScreenDebugMessage(-1, 50.f, FColor::Orange, iBP);

		UObject* ItemActor = StaticLoadObject(UObject::StaticClass(), NULL, *iBP);

		//Get class to use for spawning
		UBlueprint* GeneratedBP = Cast<UBlueprint>(ItemActor);
		ClassToAdd = GeneratedBP->GeneratedClass;

		//Add class to struct
		FDropTableItem iItem;
		iItem.ItemClass = ClassToAdd;

		//GEngine->AddOnScreenDebugMessage(-1, 50.f, FColor::Green, ClassToAdd->GetFName().ToString());
		AWSPickup* iActor = Cast<AWSPickup>(ClassToAdd->GetDefaultObject(true));


		DroptableTotal += iActor->DropRate;
		SelltableTotal += iActor->SellChance;

		//Set struct's other values.
		//if item has a drop chance of 0, assigning to roll result 0 will make sure it never drops, as rolls start at 1
		//Droptable and selltable total is used for max value to roll for.
		if (iActor->DropRate == 0)
			iItem.DropRollValue = 0;
		else
			iItem.DropRollValue = DroptableTotal;
		
		//Sell rolls start at 1, so items at 0 never drops
		if (iActor->SellChance == 0)
			iItem.SellRollValue = 0;
		else
			iItem.SellRollValue = SelltableTotal;
		
		//add struct to array
		DropTable.Add(iItem);
	}

	dropNum = DropTable.Num();

	//GEngine->AddOnScreenDebugMessage(-1, 5.f, FColor::Green, FString::FromInt(DropTable.Num()));

	//Sales can only be initiated after table has been completed, otherwise we will crash.
	bDropsReady = true;
}

Basically the idea is, say i have 2 items, one has a “DropRate” of 10, the other of 40. These get added together to generate a total value of 50 to roll. We roll and get 35. The first item has a roll value of 10, so it does not drop. We move on to the next item which as a roll value of 50, it drops as the roll was within the range of this and the last item’s values. The droptabletotal and roll values are added together sequentially through the iterations, so no comparison should be needed other than “if less than do x, if not, move on to next”.

The rand number should never be higher than the highest roll value, even with items with 0 value as they still add 0 to the rand max.

But of course, I might have missed something.