sinbad / SPUD

Steve's Persistent Unreal Data library
MIT License
300 stars 44 forks source link

Saved pointers in a non-Actor class cannot be restored #28

Closed error454 closed 2 years ago

error454 commented 2 years ago

I was implementing a checkpoint system where I store the last checkpoint encountered in my GameInstance:

UPROPERTY(SaveGame)
ACheckPoint* LastCheckPoint;

The first thing I noticed was that, when loading, LastCheckPoint was not getting restored. From this code, I see that as a non-AActor there won't be a valid level defined to find references:

https://github.com/sinbad/SPUD/blob/373585d2ceeea2e4e33262e079d8212c0ead1b0d/Source/SPUD/Private/SpudPropertyUtil.cpp#L855

As a quick test, I tried making the below change (I can see an edge case here for actors contained in streaming levels):

ULevel* Level = RootObject->GetWorld()->GetCurrentLevel();

Now LastCheckPoint gets restored, but, is immediately GC'd. It's valid and legit in my GameInstance's SpudPostRestore, where I set a breakpoint when value changes and subsequently see TFastReferenceCollector doing its job and garbage collecting LastCheckPoint before the level finishes loading.

I thought that perhaps the order of restoration might matter, so tried shuffling level loading so it occurred before restoring data, however this did not help: https://github.com/sinbad/SPUD/blob/db0ac4932ad0402d3445323898802d7c6ac184f8/Source/SPUD/Private/SpudSubsystem.cpp#L481

Any ideas why we might be losing a reference count when restoring data from a save? I do note that according to the log output, this class is taking the fast path.

sinbad commented 2 years ago

On the losing reference count thing, I don't know - it can't be that SPUD holds the only reference, your level would do as well (and would be the ultimate owner), so maybe the level is unloading again or there's some other sequence going on.

On a more general note, I think it's a really bad idea to hold level actor references in global objects, since you can't guarantee that the level is loaded, and when it is, that reference will potentially be stopping the level actor unloading. It's for this reason that (hard) cross-references are only allowed between actors in the same level in UE (I've recently discovered that TSoftObjectPtr<> works well for references between streaming levels, but have not tried making any of these saved properties yet, I suspect it won't work (and all my sublevel cross-references are static).

Personally I'd store a checkpoint ID rather than a hard actor reference for this kind of thing. Either a GUID or a human readable manually assigned ID associated with your checkpoint actor would do it.

error454 commented 2 years ago

On a more general note, I think it's a really bad idea to hold level actor references in global objects

Gah, wow you are so right. I am using soft object pointers literally everywhere else to avoid reference creep and prevent destruction blocking, so the real mystery is why I threw away all common sense to try to force this solution. This is the textbook scenario where you want to avoid a hard reference and yet here I am, lost in the trees 🤣.

Thank you for this, I switched to a GUID system and it worked effortlessly.

error454 commented 2 years ago

I've recently discovered that TSoftObjectPtr<> works well for references between streaming levels, but have not tried making any of these saved properties yet, I suspect it won't work

It seems like this would work since you're already doing a 2-pass restore method. If everything retains the same name, it should all just line up.