sinbad / SPUD

Steve's Persistent Unreal Data library
MIT License
328 stars 47 forks source link

Race Condition With Streamed Out Level Auto Saves #80

Open dyanikoglu opened 1 month ago

dyanikoglu commented 1 month ago

Hi. This is kind of complex issue I faced with auto saves for streamed out levels.

While testing the feature, I noticed dynamically spawned actors in sublevels were failing to save their data into SPUD while their level is streaming out. After extensive debugging, I noticed it's because plugin tries to save data of those actors "after" level is streamed out, so actors have a null UWorld reference during the operation.

The root cause was, all of ISpudObject and ISpudObjectCallback interface functions were failing to execute for those actors during the stream out save process. I noticed AActor::ProcessEvent function is responsible with initiating interface calls, and it has a check for validity of actor's world:

UWorld* MyWorld = GetWorld();
if( ((MyWorld && (MyWorld->AreActorsInitialized() || bAllowScriptExecution)) || HasAnyFlags(RF_ClassDefaultObject)) && !IsGarbageCollecting() )
{
#if !UE_BUILD_SHIPPING
        if (!ProcessEventDelegate.IsBound() || !ProcessEventDelegate.Execute(this, Function, Parameters))
        {
            Super::ProcessEvent(Function, Parameters);
        }
#else
        Super::ProcessEvent(Function, Parameters);
#endif
}

So, all interface calls were just being ignored for Spud because of that.

As solution, I honestly didn't like what was done in https://github.com/sinbad/SPUD/pull/45 to monitor load/unload state of streamed levels in tick function of spud subsystem. LevelStreamingDelegates.cpp from engine source already has two good callbacks for SpudSubsystem to bind itself:

TMulticastDelegate<void(UWorld*, const ULevelStreaming*, ULevel* LoadedLevel)> FLevelStreamingDelegates::OnLevelBeginMakingVisible;
TMulticastDelegate<void(UWorld*, const ULevelStreaming*, ULevel* LoadedLevel)> FLevelStreamingDelegates::OnLevelBeginMakingInvisible;

After cleaning out MonitoredStreamingLevels stuff from subsystem and binding into those callbacks fixed the issue for me. Now, spud saves state of actors first, then the level streams out.

I might open a pull request, but I'm not able to test this change for WorldPartition at the moment, and not sure if this change breaks the compatibility.

Anyways, wanted to create this issue to give that problem some visibility.

dyanikoglu commented 1 month ago

One small note about the new delegates, they're also used in LevelStreamingPersistence plugin of Unreal

https://github.com/EpicGames/UnrealEngine/blob/c830445187784f1269f43b56f095493a27d5a636/Engine/Plugins/Runtime/LevelStreamingPersistence/Source/LevelStreamingPersistence/Private/LevelStreamingPersistenceManager.cpp#L174

dyanikoglu commented 1 month ago

I pushed a commit including this fix along with other project specific changes here: https://github.com/ShadowfallStudios/SPUD

Probably not going to open a PR until I can verify this works with world partition.

sinbad commented 1 month ago

IIRC the original World Partition code (a PR itself) was like it is because of a lack of good delegate options at the time, but that was a while ago so perhaps these were added since. Look forward to a PR that does it better!