FSetProperty::InstanceSubobjects and FMapProperty::InstanceSubobjects mutate containers but don't reshash them

Development Platform & Builds
, ,
1

Hi,

Since upgrading to UE 5.5 we’ve been seeing an issue where some of our properties are being edited in UObject::PostLoadSubobjects via the codepaths in FMapProperty::InstanceSubobjects and FSetProperty::InstanceSubobjects. This is fine and the new values are correct, however the containers now fail to lookup correctly when queried since the code does not rehash them after mutating the keys.

We were able to fix this by adding

SetHelper.Rehash();

to the end of FSetProperty::InstanceSubobjects and similarly

if (bInstancedKey)

{

MapHelper.Rehash();

}

to the end of FMapProperty::InstanceSubobjects

Is this a known issue Epic is aware of?

Thanks,

Lucas

2

Steps to Reproduce
Create a TMap or TSet property where the key is a TSoftObjectPtr or TObjectPtr type.

3

Hi Lucas - your observation is correct and I’ll make the fix. I don’t know why upgrading 5.5 would make any difference here though, as nothing relevant has changed in that timeframe to those functions.

Steve

4

Thanks Steve!

Hmm I don’t know then, maybe we just got lucky with how things hashed until now?

I think there may be a similar issue with code like FPIEFixupSerializer where calling FSoftObjectPath::FixupForPIE can mutate the value in way that changes the result of GetTypeHash, that one is a tricker to fix though. It would require the FArchive to somehow look up SerializedPropertyChain to know it has to rehash a container if FixupForPIE returns true. I’m not sure the best way to do that.

-Lucas

5

Yeah, there’s no way to catch or handle all possible misuses of ‘weak’ or ‘dynamic’ key types to our associative containers other than basically removing `GetTypeHash` and `operator==` for such types to stop them being usable as keys in the first place, and that would break a lot of legacy usage. We do keep these issues in mind as we evolve our Core systems to allow better constructs.

Steve

6

The fix is in, and is visible here: https://github.com/EpicGames/UnrealEngine/commit/36cbf7480a492b141a4988876d6c3febe8c8743f

Thanks again!

Steve

7

Thanks Steve!

Yeah, I hear you on the using soft/weak object refs as keys, even beyond the issue of them becoming null and then there being duplicate keys there is also the issue of normalization since in WP multiple different paths can point to the same actor for example.

Practically though it is really useful to have TMaps where the key is a soft actor though and you can even create them in BP (which is where we’ve seen this issue)

I think making the Rehash method public on the TSet and TMap would help so people could at least call it in PostLoad() if they are doing something like that. Or to provide other containers that did work in these use cases as properties.

Thanks,

Lucas

8

Hey Lucas,

Rehashing isn’t sufficient, because you will still end up with a broken container. Multiple keys of the same (stale) value will not (cannot) be coalesced into a single key. Exposing Rehash() will likely just give users the wrong idea about how to handle stale weak keys. Whereas it makes sense to rehash after subobject instancing because the keys in that case are non-stale, unique pointer values.

Steve

9

Hi Steve,

Yeah, that’s true in the case of weak pointers but most of our problematic use cases involve soft object pointers to actors where the path changes due to PIE remapping or due to WP runtime cell paths vs editor paths, but the string paths stay unique since the actor is only inserted once.

I suppose though you could have two entries for the same actor, one with the editor path and one with the runtime path and then after canonicalizing them there are now duplicates, which you are right, is problematic.

Is it safe to use the FName of the actor as a key since I believe those are unique in WP and not remapped? There isn’t a nice already existing smart pointer type though. I would say basically all of our use cases here that are not trivial to replace are actors so maybe having a specific TSoftActorPtr vs using the generic TSoftObjectPtr that internally just used the actors FName + Level or something to make then safe to use as container keys? I guess we could make this manually by using FNames but then you lose a bunch of ergonomics in BP and also things like editor eyedropper etc.

Anyway, thanks for looking into this. I’m mostly just elaborating on our use cases incase it’s helpful for Epic’s future plans. My problem is solved for now.