sinbad / SPUD

Steve's Persistent Unreal Data library
MIT License
308 stars 45 forks source link

Hard references to asset objects are saved incorrectly #16

Closed rfsheffer closed 2 years ago

rfsheffer commented 2 years ago

Hi again

Declaring a hard object reference in c++ or blueprint for example:

UPROPERTY(EditAnywhere, SaveGame, BlueprintReadWrite) class USoundBase* HardAudioAsset;

Does not store or restore properly. They are not transient UObjects but they are treated as such. If you have a case of an object property that is null at the start but gets set to something (like an audio asset) and saved, it will save the class ID off:

/Script/Engine.SoundWave

And on restore create a new transient SoundWave and fill in the property. It should instead store only the asset path and restore that. In my own code I am checking if the object is an asset using IsAsset() and instead storing off the asset path and a special class ID to instead use StaticLoadObject against the asset path to fill in the ObjectProperty.

Cheers Ryan

sinbad commented 2 years ago

The trouble is this isn't just for assets, its for every UObject, which can be constructed myriad ways. So the rule is; if you're happy for it just to be constructed automatically, leave it, but if you need it constructed a certain way, sub to the pre-restore hook and construct it yourself (from an asset, if you like). I don't really want to get into asset loading.

sinbad commented 2 years ago

Alternatively, if you've found an elegant way to do it, feel free to submit a PR and I'll consider it :)

rfsheffer commented 2 years ago

So my issue with this is the save/restore will do something odd with properties that are hard references to assets. It will set the asset path to an object that would never have been normally created dynamically (or at least not if it wasn't setup correctly). The ramifications of this are kinda undefined in my mind, the engine actually threw a couple of asserts for some assets, and I imagine if you tried to use them it could crash because they are not initialized properly. I feel that the way Unreal serializes and loads assets needs to be the same in your solution or the inconsistency will just cause confusion. Any old developer tagging properties in blueprint needs to understand the new rules imposed by SPUD which in my experience the knowledge wouldn't have been passed and ultimately leads to engineering investigation time.

Understanding if something is an asset is pretty straightforward, Epic have a function for that; Obj->IsAsset() which I would guess works at identifying static assets as opposed to dynamic UObjects. There just needs to be a special case to deal with those accordingly.

I will submit a PR of the additions I have made once I get everything sorted out. :)

Cheers Ryan

sinbad commented 2 years ago

One of my issues is I fail to really see why you'd have hard references to assets in save game data. It just seems like a really odd thing to change & persist dynamically. I'm trying to keep SPUD as focussed and simple as possible for dynamic game data. My main reason for not using Epic's serialisation is that it's super complicated because it's designed to handle all the editor serialisation as well. My view is that you can do basically anything with derived data in pre/post-load, so best to keep your save games simpler and not dependent on things like direct links to assets, which could change over time. If it's about types of object, collectibles etc, better to have a data table that abstracts that than to link it directly to assets, IMO.

rfsheffer commented 2 years ago

Alright then I would suggest ignoring IsAsset objects in the store process. That seems to fit your complexity limit and prevents naughty things from happening with those types of objects. As for a case where swapping a hard asset reference might occur, depending on a games logic an AI might swap a commonly played sound asset. They might for example switch to an alert state and a consistent beeping sound changes to an alert beeping sound. As events like "BecameAlert" and "BecameIdle" occur the sound asset changes. No need for a branch in your beep sound timer event.

sinbad commented 2 years ago

Again, I think saving an asset link in that case is the wrong approach. Really the state is "NPC is alert"; in the post-load you should then turn that into whatever derived data you need. Storing X individual states derived from that, and especially direct links to assets, is fragile. What if later, you associate some other audio/visual change to the NPC based on their alert state? It just doesn't make sense to store derived, fragile data like that - you should be storing just the core state.

rfsheffer commented 2 years ago

I am more for the "set it and forget it" approach where I expect the thing to return to the state I left it when I saved it, and if the state was not restored properly, fix the issue in one place and solve all of the bugs associated at once "Kill many birds with one stone". Additional restore code only adds complexity but is needed in certain cases, but keep it as simple as possible. No skin off my back tho, the additions will be made in my own fork.

Cheers