sinbad / SPUD

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

Arrays/Maps of TObjectPtr in UE 5.2 #56

Closed munkeysocks closed 1 year ago

munkeysocks commented 1 year ago

So I am using SPUD. I have a UObject with is my Inventory. In this object is an array of UObjects called ItemSlots. When I try and save this Uobject i get this error with SPUD. FArchive does not support FObjectPtr serialization. Use FArchiveUObject instead. Does the plugin also save TSoftObjectPtr's and TSoftClassPtr's?

munkeysocks commented 1 year ago

I think it has something to do with TObjectPtr<>'s being used which is now the standard in UE5.

munkeysocks commented 1 year ago

Im using 5.2 also just encase.

munkeysocks commented 1 year ago

We need some way to serialize and deserialize TObjectPtr's in UE5 for this to work properly.

munkeysocks commented 1 year ago

Now if do not use TObjectPtr properties and use the old Uobject* i now get this error.

UnrealEditor_??????_Win64_DebugGame!FMemoryArchive::operator<<() [C:\UE_5.2\Engine\Source\Runtime\Core\Public\Serialization\MemoryArchive.h:56]

It appears to happen in the Spud Property Utilities class.

sinbad commented 1 year ago

This used to work so maybe something changed in 5.2. I'm not using 5.2 yet (it's barely been out for a week) so you'll either have to experiment yourself or wait until I have time to look at it.

munkeysocks commented 1 year ago

Thanks for the reply. Yeah sure. Ill just wait for your next awesome update :)

thomas-oconnell commented 1 year ago

Just for curiosity's sake - were you able to properly store this UObject in 5.1, or is this your first time testing it (and it's currently causing issues in 5.2)?

munkeysocks commented 1 year ago

Upgraded to 5.2 other day and it broke for some reason. I did see that something was deprecated in the test part of the plugin which just needed a quick change to the new macro which the error provided but yeah this does not work on 5.2. I mean the object in question is just a UObject with an array of UObjects inside of it. Also the error that came up said that TObjectPtr's which is the new UE5 standard could not be serialized with FArchive and FArchiveUObject had to be used. No idea what the problem is beyond that,

munkeysocks commented 1 year ago

Only thing I can think of is that the UObjects in the array do have a TMap on them. Could this be causing the issue? Are TMaps supported?

thomas-oconnell commented 1 year ago

I had to bypass some issues with TMap storage of structs by using JSON conversions. I stored a TMap of strings as a SaveGame variable and wrote to it on SpudPreStore_Implementation, and then loaded it and filled it back into the original TMap in SpudPostRestore_Implementation

For SpudPreStore_Implementation:


        StringMap.Empty();
    for (const TPair<FGameplayTag, FPayload>& pair : PayloadMap)
    {
        FString OutString;
        FJsonObjectConverter::UStructToJsonObjectString(pair.Value, OutString);

        StringMap.Add(pair.Key, OutString);
    }

For SpudPostRestore_Implementation:

        for (const TPair<FGameplayTag, FString>& pair : StringMap)
    {
        FPayload OutPayload;
        FJsonObjectConverter::JsonObjectStringToUStruct(pair.Value, &OutPayload, 0, 0);

        PayloadMap.Add(pair.Key, OutPayload);
    }

Where StringMap is a SaveGame variable, and the PayloadMap stores structs, each struct storing AActor pointer references, etc.

This isn't a 1-1 representation of what you're dealing with, and I'm still in 5.1, but this type of solution (converting to something using FStrings via JSON conversion) may work. I don't know if JSON and JSONUtilities have also been changed in 5.2, rendering the above unhelpful.

"Json", "JsonUtilities" would have to be added to your project's Build.cs file, too

sinbad commented 1 year ago

I added built-in TMap support about 6 months ago but it sounds like the mechanism that used has changed in 5.2. Hopefully there's an alternative but I'll need to take a better look at it.

thomas-oconnell commented 1 year ago

I added built-in TMap support about 6 months ago but it sounds like the mechanism that used has changed in 5.2. Hopefully there's an alternative but I'll need to take a better look at it.

Absolutely, saving TMaps has been extremely reliable in 99% of my use-cases (thank you for adding this!). The only time it's given me an error (where I resorted to the above workaround) was when I had a TMap that stored a struct ExampleStruct that then stored an actor reference within it. Without the workaround in that case, I run into a similar issue to OP's second issue upon saving, an exception at:

FMemoryArchive::operator<<(UObject *&) MemoryArchive.h:58

I might be missing a built-in working solution, and I might also be handling it incorrectly to begin with. The above showed a similar carryover between 5.1 and 5.2, so I'm hopeful that it might help narrow things down a bit

sinbad commented 1 year ago

OK I can confirm that it's specifically a problem with Arrays of TObjectPtr. We didn't used to support arrays of UObject but added support via a wrapped FRecord a little while back, using FProperty::SerializeBinProperty. Unfortunately from 5.2 this no longer work for arrays of TObjectPtr, annoyingly.

Sadly it's several layers down in UE code that it throws this error so I can't just switch to calling FArchiveUObject, I'll need to figure out if there's an alternate path now. I really wish they wouldn't break stuff like this.

sinbad commented 1 year ago

Bleh, I can't make this work anymore. The trouble is that FArchiveUObject is a new subclass of FArchive, but SPUD completely relies on the fact that we can exchange an FMemoryArchive for an FArchive. We can no longer do that because the class hierarchy has been split with this new subclass FArchiveUObject. What a pain in the arse.

I've experimented with trying to wrap it using FArchiveUObjectFromStructuredArchive but I can't get it to work.

I've had to just re-impose the limitation on Arrays/Maps of custom objects - you just can't use them with TObjectPtrs unless/until we find a way to make this work again. SPUD now detects this and reports an unsupported type. Raw UObject pointers continue to work fine.

I've no idea what they're trying to achieve with this FArchive hierarchy split, I can only assume it's because TObjectPtrs can support lazy-loading. Until there's a way to make it work with FMemoryArchive again this will be unsupported.

Part of the reason SPUD mostly uses custom serialization was to avoid this kind of thing, but I decided to add a fallback for these cases which used the UE core serialization in a wrapped record, but I'm regretting it now. You lose control of things too much and then stuff like this happens.

sinbad commented 1 year ago

Ref: https://github.com/sinbad/SPUD/commit/17c75688815df9c6ea84458e0207e119a9f938eb

munkeysocks commented 1 year ago

Is there anyway to steal the virtual FArchive& operator<<(UObject*& Obj) override; virtual FArchive& operator<<(FLazyObjectPtr& Value) override; virtual FArchive& operator<<(FObjectPtr& Value) override; virtual FArchive& operator<<(FSoftObjectPtr& Value) override; virtual FArchive& operator<<(FSoftObjectPath& Value) override; virtual FArchive& operator<<(FWeakObjectPtr& Value) override; from the FArchiveUObject class and use those somewhere to help?

munkeysocks commented 1 year ago

I ponder if we added those to your chunkedarchive and then somehow used the chunkedarchive in this bit of the prop utility class it might fix the issue. RegisterProperty(Property, PrefixID, ClassDef, PropertyOffsets, Meta, Out); FBinaryArchiveFormatter Fmt(Out); FStructuredArchive Ar(Fmt);
const auto RootSlot = Ar.Open(); Property->SerializeBinProperty(RootSlot, const_cast<void*>(ContainerPtr)); Ar.Close(); bUpdateOK = true;

sinbad commented 1 year ago

Is there anyway to steal the virtual FArchive& operator<<

Possibly by implementing an FMemoryArchive subclass that adds back the methods that are needed from FArchiveUObject. That would need to be used everywhere and would need a lot more testing though.

munkeysocks commented 1 year ago

Also look up the class FObjectWriter this seems to be new and very interesting.

munkeysocks commented 1 year ago

No idea if it matters but FBinaryArchiveFormatter can take in both FArchive and FArchiveUObject

munkeysocks commented 1 year ago

Which may help to modify this code. RegisterProperty(Property, PrefixID, ClassDef, PropertyOffsets, Meta, Out); FBinaryArchiveFormatter Fmt(Out); FStructuredArchive Ar(Fmt); const auto RootSlot = Ar.Open(); Property->SerializeBinProperty(RootSlot, const_cast<void*>(ContainerPtr)); Ar.Close(); bUpdateOK = true;

sinbad commented 1 year ago

Which may help to modify this code

It doesn't I'm afraid. The issue is that the archive itself is a FMemoryArchive, a subclass of FArchive. The problem is they added FArchiveUObject as a new subclass of FArchive, making it a sibling of FMemoryArchive, and require the archive to be that sibling type. We need both the functionality of FArchiveUObject and FMemoryArchive at the same time, either one on its own is not enough. There is an FArchiveProxy but that's not going to help either, since whatever its proxying has to support both these sets of features. Everything was fine when FArchive was the only interface needed, but FArchiveUObject really screws up the utility of the rest of the hierarchy. It's a really weird choice they've made with this and I'm not sure what the intended approach is, without digging into the core code a lot more than I have time for right now. Hopefully either someone else can figure out what Epic were thinking with this, or I'll have some more time soon.

sinbad commented 1 year ago

The primary way I can see to fix it is to add the FArchiveUObject overrides to FMemoryArchive, but if that's the case I can't understand why they haven't done this themselves, because anyone using FMemoryArchive with TObjectPtr and the standard serialization route would hit this very same problem, so why haven't they addressed it? Maybe they have, and I just can't see it yet.

sinbad commented 1 year ago

(Basically I don't want to do a half-assed fix that just gets broken again in 5.3, I need to understand what their thinking is)

munkeysocks commented 1 year ago

Managed to make any progress on this at all @sinbad ?

sinbad commented 1 year ago

No, haven't had time to look at it properly yet.

sinbad commented 1 year ago

I've had to implement custom FSpudMemoryReader and FSpudmemoryWriter, tests with arrays of TObjectPtr now pass.

munkeysocks commented 1 year ago

So they all work above board? No bugs or anything so far?

munkeysocks commented 1 year ago

If so, you sir are a god and my saviour!!!!

sinbad commented 1 year ago

I believe so. I'm short of time right now but this passed the test that the old code failed and uses an approach I found in the engine code so hopefully is correct.

brpendle-ti commented 11 months ago

I think I found a bug with the updated implementation mentioned on 6/19/2023. Sinbad, you mention the tests are now passing. Inside FTestBasicAllTypes::RunTest, it makes a SaveObj, saves it, then restores into LoadedObj, then deep compares SaveObj and LoadedObj and passes. LoadedObj.TObjectPtrArray[0] will serialize and end up pointing at the SaveObj .TObjectPtrArray[0] Ubject. So it passes the test, because it's comparing the same object to itself. If you save this out to an archive, and split the loading into a different test, LoadedObj.TObjectPtrArray[0] will get serialized and be nullptr and the test will fail.