Restoring Character/Controller/GameState not working in built project (UE5)

[cbQB]

...because they get different names after each reload, and SPUD simply treats them just like any other level objects. Which would be fine if the assumption below were true:

SpudState.cpp:253:

    // FNames are constant within a level
    const auto Name = SpudPropertyUtil::GetLevelActorName(Actor);

Unfortunately, that's not true.

UObjectGlobals.cpp:1891

                    if (!FPlatformProperties::HasEditorOnlyData() && GFastPathUniqueNameGeneration)
                    {
                        /*   Fast Path Name Generation
                        * A significant fraction of object creation time goes into verifying that the a chosen unique name is really unique.
                        * The idea here is to generate unique names using very high numbers and only in situations where collisions are
                        * impossible for other reasons.
                        *
                        * Rationale for uniqueness as used here.
                        * - Consoles do not save objects in general, and certainly not animation trees. So we could never load an object that would later clash.
                        * - We assume that we never load or create any object with a "name number" as large as, say, MAX_int32 / 2, other than via
                        *   HACK_FastPathUniqueNameGeneration.
                        * - After using one of these large "name numbers", we decrement the static UniqueIndex, this no two names generated this way, during the
                        *   same run, could ever clash.
                        * - We assume that we could never create anywhere near MAX_int32/2 total objects at runtime, within a single run.
                        * - We require an outer for these items, thus outers must themselves be unique. Therefore items with unique names created on the fast path
                        *   could never clash with anything with a different outer. For animation trees, these outers are never saved or loaded, thus clashes are
                        *   impossible.
                        */
                        static TAtomic<int32> UniqueIndex(MAX_int32 - 1000);
                        NameNumber = --UniqueIndex;
                    }
                    else
                    {
                        NameNumber = UpdateSuffixForNextNewObject(Parent, Class, [](int32& Index) { ++Index; });
                    }

In the Editor, both PIE and Standalone, it takes the latter path, using UpdateSuffixForNextNewObject and incrementing the counter. But in Windows built versions (UE5 at least), it takes the fast path, which uses non-repeating values.

One way to solve this is to modify Unreal, e.g. add an opt-in/out mechanism per-class for the name generator. Not complicated, but only usable by those with a home-grown engine. And not particularly more elegant than the others.

Another would be for SPUD to special-case these classes. Not very elegant.

Yet another: adding a function in ISpudObject that allows the object to override it's name, as used for saved data lookup. And an implementation of the basic naming scheme, Blueprint-usable. Functionally equivalent to allowing the developer to use the classic naming scheme. A bit more flexible, but a regular person, who doesn't care about these detail, might wonder why this is not handled by SPUD directly (see option 2), if the problem is known and the system not usable without the fix anyway.

[sinbad]

So the reason this path is taken is because both store and restore use the result of ShouldActorBeRespawnedOnRestore to determine whether it goes down the Level (FName) or Spawned (SpudGuid) naming path.

bool USpudState::ShouldActorBeRespawnedOnRestore(AActor* Actor) const
{
    return SpudPropertyUtil::IsRuntimeActor(Actor) &&
        ShouldRespawnRuntimeActor(Actor);
}

SpudPropertyUtil::IsRuntimeActor(Actor) returns true for these classes, because they're not level objects, but ShouldRespawnRuntimeActor returns false, because they're special cased as auto-instanced classes:

    switch (RespawnMode)
    {
    default:
    case ESpudRespawnMode::Default:
        // Default behaviour is to respawn everything except pawns, characters, game modes, game states
        // Those we assume are created by other init processes
        return !Actor->IsA(AGameModeBase::StaticClass()) &&
            !Actor->IsA(AGameStateBase::StaticClass()) &&
            !Actor->IsA(APawn::StaticClass()) &&
            !Actor->IsA(ACharacter::StaticClass()) &&
            !Actor->IsA(APlayerState::StaticClass());

This means that these classes have gone down the Level path which has worked until now, but probably they should be using SpudGuid as an identifier instead. In my project I've assigned constant, pre-defined Guids to characters & states on the expectation of this but didn't realise they were actually using the name.

I suspect what really should be happening is that these should be re-classified as global objects, and looked up by their SpudGuid instead. Thoughts?

[sinbad]

To be clear, these objects are enumerated as part of the PersistentLevel when storing so perhaps "Global Object" is the wrong term (that's supposed to be for other non-Actor objects). Perhaps simply automatically using a SpudGuid as the identifier in preference to the FName - either always when it's available, or specifically for these problematic cases, would be the correct choice, and leaving the actual object data in the level data for consistency. We could flag up the cases where this is a problem in the log if not provided.

[cbQB]

Yes, they are not global objects, and forcing them there would only muddy the waters.

I've implemented the name override already, it's simple (particularly since you've wrapped the name getting with a function) and it works. Should also be documented, and users warned about why it's needed. In single-player it's trivial, in multi-player a bit of care is needed. But then, people creating mp games with save-able state should be able to handle that.

Using the Guid's would be a solution, but it's more work to implement. And it's much harder to spot (or easier to miss or break) by another developer. An event implementation is a more clear indication of intent than the value of a property that is normally ignored.

[sinbad]

Hmm, good point. I was thinking the name override would be more work / more confusing as another thing to think about, but being explicit has its benefits. Especially as they're most likely to be constants anyway, unlike Guids for spawned objects.

Since you've done it already, do you want to submit a PR?

[cbQB]

I could have sworn I've done that already. One moment.

[sinbad]

I've also added a warning message in the log now when a case which should be implementing OverrideName() is detected.