Increment of 0 by 5 equals 5 sometimes 10. Is this a race condition problem? How can I solve this?

[DevKaiUnreal]

Hi,

i play in editor, pickup an actor by overlapping colliders and pressing E (Ehanced input system - ETriggerEvent::Started). The actor stores an int count = 5; which is added to my inventory. The inventory count is now incremented sometimes by 5 sometimes by 10 randomly even though the function that modifies the value is only hit once with the process attached in Rider. The following UE_LOG is only just executed a single time and look either like this,

Slot.Count += StorageInfo.Count; // Slot.Count == 5, StorageInfo.Count == 5

or like this,
Slot.Count += StorageInfo.Count; // Slot.Count == 10, StorageInfo.Count == 5

AddToSlot(...) is the only function that adds to the Count:

void USlotStorage::AddToSlot(FSlot& Slot, FStorageInfo& StorageInfo)
{
	Slot.Count += StorageInfo.Count;	
	Slot.Count = FMath::Clamp(Slot.Count, 0, Slot.Max);
	UE_LOG(LogTemp, Warning, TEXT("%s -> %d - %d"), *FString(__FUNCTION__), Slot.Count, StorageInfo.Count);
	StorageInfo.Update(Slot.Count, Slot.Index, Slot.Max, true);
}

USlotStorage::AddToSlot is called polymorphic from here:

UStorage* Storage = GetStorage(Pickup->StorageName);
	
if(IsValid(Storage))
{
	Storage->Add(Pickup, StorageInfo);
}

The Storage is owned by the inventory, which is alive the whole time, like this:

UPROPERTY() TObjectPtr<USlotStorage> Storage1;

Any suggestions how to solve the issue?

Thanks in advance!

[hedron (qkSuna)]

It’s a bit difficult to assess because I don’t have a very good picture of the actual inventory system. Namely, I don’t know what FStorageInfo / FSlot look like and I don’t know the function bodies of Add / TryAddToEmptySlot / Try AddToUsedSlot.

If you can add those that might help diagnose the issue a bit; however, the one thing that I did notice in your log screenshots is that the the 5 - 5 and 10 - 5 logs take different code paths. When it’s 5 - 5 you are using TryAddToEmptySlot, when it’s 10 - 5 you are using TryAddToUsedSlot. From the source code I’ve seen, this seems correct, since when it’s 10 - 5 you’re adding 5 to a slot that already has 5 items. Assuming that holds true whenever the issue occurs, the first two places I would check:

1. Make sure TryAddToUsedSlot isn’t doing any accidental state mutation.
2. Make sure the storage isn’t somehow maintaining its state between play sessions. If you are dynamically creating that USlotStorage at runtime, (e.g., in PostInitializeComponents or BeginPlay), then you should consider marking the UPROPERTY with Transient. By default all UPROPERTY fields are included in serialization, which isn’t something you usually want for dynamically constructed UObjects.

[DevKaiUnreal]

Hi,

first of all thank you for your help and sorry for my late answer! I actually was kind of blind and didn’t noticed that it was taking diffent code paths. It’s supposed to first call TryAddToUsedSlot and if that fails to call TryAddToEmptySlot, but never both.

It seems to be due to saving data beyond play sessions, because it’s predictably alternating between calling the two functions mentioned above from session to session.

I actually create the UStorageSlot statically in the contructor and initialize its Count in BeginPlay(...). This is the code…

UInventoryComponent::UInventoryComponent()
{
...
	ShotgunShells = CreateDefaultSubobject<USlotStorage>("ShotgunShells");
}

void UInventoryComponent::BeginPlay()
{
	Super::BeginPlay();
...
	ShotgunShells->Init(this, InventoryLayout.ShotgunShells);
}

void USlotStorage::Init(UInventoryComponent* AInventoryComponent, TArray<int> SlotCapacities)
{
	InventoryComponent = AInventoryComponent;
	for (int i = 0; i < SlotCapacities.Num(); ++i)
	{
		FSlot Slot;
		Slot.Index = i;
		Slot.Class = nullptr;
		Slot.Count = 0;
		Slot.Max = SlotCapacities[i];
		Slots.Add(Slot);
	}
}

FStorageInfo

USTRUCT(BlueprintType)
struct FStorageInfo
{
	GENERATED_BODY()

	FStorageInfo() {};
	FStorageInfo(EStorageName StorageName, TObjectPtr<UTexture2D> Icon = nullptr, bool Consumable = true, int Count = 0);
	void Update(int ACount, int AIndex, int AMax, bool ASuccess);
	// When Pickup is added to an unknown Slot
	
	UPROPERTY()	EStorageName StorageName;
	UPROPERTY()	TObjectPtr<UTexture2D> Icon;
	UPROPERTY()	bool Consumable = true;	
	UPROPERTY()	int Count = 0;
	UPROPERTY()	int Index = 0;
	UPROPERTY()	int Max = 0;
	UPROPERTY()	bool Success = false;	
};

FSlot

USTRUCT()
struct FSlot
{
	GENERATED_BODY()
	int Index;
	TSubclassOf<APickup> Class;
	int Count;
	int Max;
};

Add(…)

void USlotStorage::Add(APickup* Pickup, FStorageInfo& StorageInfo)
{
	UE_LOG(LogTemp, Warning, TEXT("%s -> %d"), *FString(__FUNCTION__), StorageInfo.Count);
	TryAddToUsedSlot(Pickup, StorageInfo);
	if(!StorageInfo.Success)
	{
		TryAddToEmptySlot(Pickup, StorageInfo);
	}
	if(StorageInfo.Success)
	{
		Pickup->Destroy();
	}	
}

TryAddToUsedSlot(…)

void USlotStorage::TryAddToUsedSlot(APickup* Pickup, FStorageInfo& StorageInfo)
{
	for (FSlot& Slot : Slots)
	{
		// If there is already an item of that type in a Slot that is not full: Add the item
		if(Slot.Class == Pickup->GetClass())
		{
			if(Slot.Count < Slot.Max)
			{
				UE_LOG(LogTemp, Warning, TEXT("%s -> %d - %d"), *FString(__FUNCTION__), Slot.Count, StorageInfo.Count);
				AddToSlot(Slot, StorageInfo);
				return;
			}
		}
	}
}

TryAddToEmptySlot(…)

void USlotStorage::TryAddToEmptySlot(APickup* Pickup, FStorageInfo& StorageInfo)
{
	for (FSlot& Slot : Slots)
	{
		if(Slot.Count == 0)
		{
			Slot.Class = Pickup->GetClass();
			UE_LOG(LogTemp, Warning, TEXT("%s -> %d - %d"), *FString(__FUNCTION__), Slot.Count, StorageInfo.Count);
			AddToSlot(Slot, StorageInfo);
			return;
		}
	}
}

GetStorage(…)

UStorage* UInventoryComponent::GetStorage(EStorageName StorageName)
{
	switch (StorageName)
	{
	case EStorageName::ShotgunShells:
		return ShotgunShells;
       // ...

class THETHIRDPERSONGAME_API UInventoryComponent : public UActorComponent
{
...
private:
    UPROPERTY() TObjectPtr<USlotStorage> ShotgunShells;

[DevKaiUnreal]

The solution was to:

1. Make everything UPROPERTY(Transient).
2. Initialize UStorage in BeginPlay() with NewObject<USlotStorage>(this).

But why has this to be initialized dynamically inside BeginPlay() if it does not access any Blueprint data? Wouldn’t it load faster at runtime if it is initialized at compile time?

UCLASS(ClassGroup=(Custom), meta=(BlueprintSpawnableComponent))
class THETHIRDPERSONGAME_API UInventoryComponent : public UActorComponent
{
	GENERATED_BODY()
// ...
private:
	UPROPERTY(Transient) TObjectPtr<USlotStorage> ShotgunShells;

USTRUCT(BlueprintType)
struct FStorageInfo
{
	GENERATED_BODY()
// ...
	UPROPERTY(Transient)	int Count = 0;

void UInventoryComponent::BeginPlay()
{
	Super::BeginPlay();	
// ...
	ShotgunShells = NewObject<USlotStorage>(this);
	ShotgunShells->Init(this, InventoryLayout.ShotgunShells);
}

[hedron (qkSuna)]

Hey, glad you got it working.

As for the question about initialization: it’s rather complicated, but let me explain a couple of things that might help.

The first thing is that compile time initialization requires that all data is known at compile time; this is rarely feasible in a majority of situations. Even if you put default values in a default constructor in C++, that still is going to require runtime initialization, and this initialization would occur when the constructor is invoked. In most scenarios, some runtime initialization cost is going to exist. Whether or not that cost is acceptable is going to depend on your particular context and will require measurements and analysis via profilers.

Secondly, because USlotStorage is a UObject, it must be dynamically allocated on the heap since it is going to be traced in Unreal’s garbage collection system. This is why you can’t, for instance, have a member field of type USlotStorage (instead of USlotStorage*), because that would embed it directly in the owning class’s memory and would escape the call to NewObject that properly registers it with garbage collection system. In any situation, if you are using a UObject, it is going to need to be initialized via NewObject at runtime.

Now, in your situation you won’t have lost any performance; CreateDefaultSubobject is also going to end up calling NewObject (or an equivalent) for the target as well. But it’s not appropriate for things that are not serialized (transient); it’s really just meant for components and subobjects that are integral to the “structure” of the UObject; things that are meant to be visible in the Blueprint visual component hierarchy, and thus must be serialized.

[DevKaiUnreal]

Ah I could have known that its allocated on the heap i.e. dynamically, meaning it is not created at runtime. It’s always a pointer to the UObject and it’s always created with NewObject().

Thanks a lot for making the effort to read plenty of code and for finding a solution to my problem! Have great weekend!