Why does this compile, but crashes the engine?

KVogler · December 29, 2016, 1:42am

Hi,

Im slowly, very slowly, but surely getting warm with c++, finallly
However, here is something that I cant get my head around:

I have a class that looks like this:


UCLASS()
class CODE_API UStaffManager : public UObject
{
	GENERATED_BODY()

private:
	TArray<UStaffUnit*> RegularUnits;  
	UStaffUnit WaitingRoom;             

public:
	UStaffManager();
	~UStaffManager();

 	TArray<UStaffUnit*>& GetRegularUnits() { return RegularUnits; }
	
	UStaffUnit& GetWaitingRoom() { return WaitingRoom; }
};

It compiles, but crashes the engine with an error like

The culprit seems to be the WaitingRoom declaration. At least, without it, it compiles and the engine does not crash…
But I dont get what the difference is. Except for the member types, its the same way of declaration.

Any help is appreciated.
Thanks

Cheers,
Klaus

ScottMichaud · December 29, 2016, 2:13am

Two issues.

You want to have WaitingRoom be a pointer to a UStaffUnit object.


UStaffUnit* WaitingRoom

It’s trying to store the whole UStaffUnit in the class otherwise, which UE4 doesn’t like. It likes to handle the pool of memory that all UObjects are spawned into, and only give your classes a pointer to where it is.

Which leads into the second problem.

Because UE4 wants to handle memory management of UObjects, you will also need to add a UPROPERTY() macro above it. This adds a bit of code so UE4’s garbage collector can see if a UObject is still in use by someone. Without the UPROPERTY() macro, the garbage collector will not count UStaffManagers as needing the UStaffUnit that WaitingRoom points to. In other words? UE4 will just delete it out from under you.

So you should have, instead:



UCLASS()
class CODE_API UStaffManager : public UObject
{
	GENERATED_BODY()

private:
	UPROPERTY() //I'd put one here, too, just to be safe.
	TArray<UStaffUnit*> RegularUnits;
	
	UPROPERTY() //Prevents GC from deleting the UStaffUnit pointed at by WaitingRoom until it's nullified here (and everywhere else).
	UStaffUnit* WaitingRoom;

public:
	UStaffManager();
	~UStaffManager();

 	TArray<UStaffUnit*>& GetRegularUnits() { return RegularUnits; }
	
	UStaffUnit& GetWaitingRoom() { return WaitingRoom; }
};

If you need to spawn a UStaffUnit, then do it in a game thread function (such as the constructor, beginplay, or whatever) and store the returned pointer in a pointer slot that has a UPROPERTY() macro over it.



//In some method, like the constructor, beginplay, tick, one that you create and call, or whatever.
//It needs to be on the main (game) thread, though, but you would know if it isn't (because you'd need to intentionally spawn the FRunnable or whatever).
WaitingRoom = NewObject<UStaffUnit>(this, UStaffUnit::StaticClass());

Alternatively, if you actually did want an object to be stored in UStaffManager, and it doesn’t need the benefits of being a UObject, you could use a UStruct or a standard C++ object. Those can be directly held in a UCLASS header file. If you made it a USTRUCT, it would need to be called FStaffUnit, though. I’m pretty sure spawning it as a UObject and storing a pointer with a UPROPERTY() is what you want to do, though.

Your code compiles because it’s valid C++, even though UE4 rejects it for UObject-derived classes.

KVogler · December 29, 2016, 2:56am

Thanks for the reply

I tried and it gives me on this line:


UStaffUnit& GetWaitingRoom() { return WaitingRoom; }

the follwing compiler errors:

Missing “*” in Expected a pointer type
A reference of type “USTaffUnit &” cannot be initialized with a value of “UStaffUnit *”

But why does it work for the array? I declared no pointer, just “TArray<UStaffUnit*>” instead of “TArray<UStaffUnit*>*****”

I have UPROPETY macros I just removed them in the example for readability.

That is done in the StaffManager constructor.


UStaffManager::UStaffManager()
	:Super()
{
        UStaffUnit* WaitingRoom= NewObject<UStaffUnit>();  // would be needed since WaitingRoom* is a mere pointer?

	for (int i = 0; i < 6; i++)
	{
		UStaffUnit* Newunit = NewObject<UStaffUnit>();

		RegularUnits.Add(Newunit);
	}

 	RegularUnits[0]->Initialize(EStaffUnitTypes::Combat);
 	RegularUnits[1]->Initialize(EStaffUnitTypes::Development);
 	RegularUnits[2]->Initialize(EStaffUnitTypes::Construction);
 	RegularUnits[3]->Initialize(EStaffUnitTypes::Support);
 	RegularUnits[4]->Initialize(EStaffUnitTypes::Intel);
 	RegularUnits[5]->Initialize(EStaffUnitTypes::Medical);
}

It should populate the array with instances of UStaffUnit and then run the Initialize functon on them…

Well here is what the Staff manager is supposed to do:

So what is the best way to declare these things.
The reason why the regular units (for which a skill exist for staff members) are in an array as opposed to state them sxplicitly is that I need to iterate over them programatically…

ScottMichaud · December 29, 2016, 3:11am

Okay so there’s a few questions here.

First, it doesn’t complain with TArray because TArray isn’t a UObject. That can be stored within a header without issue (just like a UStruct, a primitive type, or an object that is not derived from UObject can).

Second, your UStaffManager constructor should look like this.



UStaffManager::UStaffManager()
	:Super()
{
        WaitingRoom = NewObject<UStaffUnit>();  //The header file already said WaitingRoom is a UStaffUnit*

	for (int i = 0; i < 6; i++)
	{
		UStaffUnit* Newunit = NewObject<UStaffUnit>();

		RegularUnits.Add(Newunit);
	}

 	RegularUnits[0]->Initialize(EStaffUnitTypes::Combat);
 	RegularUnits[1]->Initialize(EStaffUnitTypes::Development);
 	RegularUnits[2]->Initialize(EStaffUnitTypes::Construction);
 	RegularUnits[3]->Initialize(EStaffUnitTypes::Support);
 	RegularUnits[4]->Initialize(EStaffUnitTypes::Intel);
 	RegularUnits[5]->Initialize(EStaffUnitTypes::Medical);
}

Third, I was wrong about UStaffManager’s header file. It should look like this:



UCLASS()
class CODE_API UStaffManager : public UObject
{
	GENERATED_BODY()

private:
	UPROPERTY() //I'd put one here, too, just to be safe.
	TArray<UStaffUnit*> RegularUnits;
	
	UPROPERTY() //Prevents GC from deleting the UStaffUnit pointed at by WaitingRoom until it's nullified here (and everywhere else).
	UStaffUnit* WaitingRoom;

public:
	UStaffManager();
	~UStaffManager();

 	TArray<UStaffUnit*>& GetRegularUnits() { return RegularUnits; }
	
	**UStaffUnit*** GetWaitingRoom() { return WaitingRoom; } //You want to return the pointer to WaitingRoom, not a reference to it.
	//I think you could also do the below instead of the above if you really need to pass a reference, not a pointer. I could be wrong, though. Rusty on this.
	**UStaffUnit&** GetWaitingRoom() { return *WaitingRoom; }
};

KVogler · December 29, 2016, 3:37am

Thanks. Now it compiles again

Hmm. I thought a reference is a “pointer that cant be changed”. In this case, the instance of the waiting room to which the pointer points to will never change. There will never be a different instance of waiting room, or the regualr unit array for that matter.
So I thought references are the way to go. I know that I need pointers to the regaular staff unit instances since arrays cannot store refernces but only pointers.

Is there any reason pointers should be used for the waiting room (and also the sickbay, brig, etc) ?

ScottMichaud · December 29, 2016, 7:09am

Yeah… I know my limits. I’ll leave this to someone who is better in C++.

Edit: Oh wait, never mind. I know when you’d want to return a pointer instead of a reference: if the method could possibly return NULL. For instance, getting the closest actor of a given type – there might not be one of those actors in the level. In this case, if WaitingRoom is deleted, calling UStaffUnit& GetWaitingRoom() would dereference (the already deleted) WaitingRoom to return a UStaffUnit&… which would be undefined behaviour, likely crashing / access violation.

However, if you return a pointer, then you’ll just pass along a NULL pointer that could be checked by the receiver and dealt with.

KVogler · December 29, 2016, 9:50am

So is checking == nullptr the same as using IsValid() ? I thought IsValid() checks references if an object exists at the other end… Or does IsValid() operates (only) on pointers?

BrUnO_XaVIeR · December 29, 2016, 9:55am

The real reason it crashes is you declare a destructor, but don’t implement it;
If you don’t have very specific needs, let the GENERATED_BODY() macro deal with UObject destructors for you instead.
UEditor will create and destroy your class object several times for checksums when editor is loading, a problematic constructor or destructor will cause problems or even crash the editor when the UObject Garbage Collector system is triggered.

KVogler · December 29, 2016, 10:10am

Ah. That is good to know.
Currently, the destructor is just an empty stub. I thought it would be irrelevant.
Do I need to destroy my created UStaffUnit instances in the destructor of the staff manager, or are they automatically destroyed when the staff manager gets destroyed and thus no pointers exist anymore?
I also didnt know that those objects are actually created during editor load even if they are not “used” yet…

BrUnO_XaVIeR · December 29, 2016, 11:10am

You don’t have to do anything (usually) to destroy a UObject memory footprint.
The Garbage Collector will get hid of it as soon as all pointers or references to those objects are cleared; so when you destroy the owning UObject the TArray<UStaffUnit> RegularUnits* members are destroyed too, unless something else holds pointers to them elsewhere. If you want to really make sure that they are destroyed when you need them to be, then override your UObject’s BeginDestroy() function and call RegularUnits.Empty() in there.

The editor creates “CDO”, Class Default Object, for every single UObject class your project has. You can access those within Editor code or within game code even though they aren’t instantiated anywhere into your game world.

KVogler · December 29, 2016, 1:19pm

Is it possible to exclude object instances from garbage collection?
Usually I take good care of my pointers (Delphi has no garbage collection) to avoid memory leaks. In UE4 I need to keep my objects alive.
Would it be more performant if I dont rely on GC and destroy my objcest myself.
Im not used to GC and find it a bit odd having another entity mess with “my” pointers

Curious: What purpose do they serve?

BrUnO_XaVIeR · December 29, 2016, 3:28pm

CDO does exist because the Editor and many of its tools have to access UObjects in a variety of ways even when your game isn’t running and UWorld doesn’t exist.
It is also apparently used for a faster process of object instancing when you create an AActor instance of the UObject you declare; it helps the engine not have to call object constructor every time a new instance is needed, among other things…

When I was passing certification for my UFSM plugin, the Epic engineer responsible for the C++ review told me that in case I don’t want my objects cleared by GC and clear an array of objects by myself, I should at least then use TSharedPrt<> or/and TWeakPtr<> instead of UPROPERTY() macro for the objects that I want to manage myself.
Ultimately I ended up using them just for Editor extensions for objects that aren’t child of UObjects to avoid confusion, within UObjects I didn’t really use those for anything serious just yet;
But I suppose if you do TArray< TSharedPtr<UStaffUnit> > RegularUnits; that may work for your puposes since they will never be garbage collected and are much safer to prevent memory leaks than regular C pointers.

Keep in mind however that TSharedPtr or TSharedRef will not be tracked by the reflection system (UProperty system) so they won’t show up in Detail Panels and you cannot declare as UPROPERTY() either.

KVogler · December 29, 2016, 5:13pm

For that purpose one could wrap it in yet another class that maps those members to reflection conform counterparts and exposes them…(?)

BrUnO_XaVIeR · December 29, 2016, 6:12pm

Yes, depending on what one needs, could wrap them in an UStruct too which then can be declared as UProperty();
I personally use UStructs a lot for this kind of things.