Blueprint reinstancing may retain references to old objects after duplication

Development Platform & Builds
, ,
1

After updating from Unreal 5.4 to 5.5.4 we are encountering a blueprint reinstancing / object duplication issue.

This may possibly (but not necessarily) be related to this other issue discussed on Reddit (https://www.reddit.com/r/unrealengine/comments/1hof3mp/comment/m497n2o/) and may be something that surfaced after a specific commit (https://github.com/EpicGames/UnrealEngine/commit/c43bd3c5a09f4dc6e2e859dc34868ef9619a7e9c), although the exact point in the code where the failure happens does not seem to have been touched by that change.

Steps to reproduce

Our setup involves an automated test that compiles all blueprints in the project. Among them we have:

  1. A widget blueprint inheriting directly from UScrollBox.
  2. A widget blueprint inheriting from UCommonUserWidget which uses (1) and has content inside the ScrollBox.

The issue happens if we use the automated test to perform the following steps in order:

A. Compile blueprint 1

B. Compile blueprint 2

C. Compile blueprint 1

D. Compile blueprint 2

The core issue happens in step (C), when the ScrollBox instance of (1) that is contained in (2) is duplicated for reinstancing. The Slots array of the original instance of (1) contains only one item, the slot object. A duplicate of the slot object is created, referencing the new instance of (1) as its Outer. However, the Slots array of the new instance of (1) still retains references to the original slot object.

[Image Removed]Then, in step (D), recompiling creates a duplicate of the WidgetTree. The algorithm now fails to create a complete new copy, because the Outer of the slot object in the Slots array of the ScollBox is incorrect, thus the slot and all its children in the new WidgetTree are the same as the old WidgetTree.

During the validation of the duplicate WidgetTree, we hit this assertion in UWidgetBlueprint::ValidateGeneratedClass:

Ensure condition failed: Widget->GetOuter() == WidgetTree

Potential point of failure

The point of failure in step (C) seems to be in FBlueprintCompileReinstancer::CopyPropertiesForUnrelatedObjects. While copying the properties of the old “REINST_” object of the ScrollBox to the new instance, the process fails to use the OldToNewInstanceMap, which correctly contains the mapping from the old slot object to the newly created slot object.

I tracked the behavior further down in UEngine::CopyPropertiesForUnrelatedObjects.

Because Params.bReplaceInternalReferenceUponRead is false, Params.OptionalReplacementMappings is ignored when creating the serialization Reader. Thus the old objects are populated into the NewObject during deserialization.

Later, FFindInstancedReferenceSubobjectHelper::Duplicate does nothing to correct the situation (and I’m not sure that this is the purpose of the function either), because the old and new slot objects (array items) are present inside the Params.OptionalReplacementMappings that is passed to the function, and are considered to have been already handled.

Tenative fix just for demonstration

A tentative fix that corrects the situation for us is to set Params.bReplaceInternalReferenceUponRead unconditionally inside FBlueprintCompileReinstancer::CopyPropertiesForUnrelatedObjects.

[Image Removed]However, I’m not familiar enough with this code to understand the side effects or implications of this change, and there may be a more correct fix. We look forward to receiving advice on this issue!

2

[mention removed]​ I looked up similar cases on EPS and tested this seemingly unrelated fix, that was in fact unrelated. There have been several changes to blueprint compilation in the meantime, but I’m not sure which ones I could try to backport to 5.5, maybe you can point me in the right direction?

3

Great find. We really want to land a fix here.

Historically I believe these references were fixed up by this logic in the case of widgets:

for (UObject* NewInstance : ComponentsOnNewObject) { if (int32* pOldInstanceIndex = OldInstanceMap.Find(NewInstance->GetPathName(NewObject))) { // Restore modified properties into the new instance FInstancedObjectRecord& Record = SavedInstances[*pOldInstanceIndex]; FObjectReaderWithReplacement Reader(NewInstance, Record.SavedProperties, true, true, OptionalReferenceReplacementMap, OptionalClassReferenceReplacementMap); UE::Engine::Private::DuplicateEditInlineSubObjects(Record.OldInstance, NewInstance, ReferenceReplacementMap, EditInlineSubobjectsOfComponents, Params.OptionalReplacementMappings); } }Can you confirm that is no longer running in light of the refactor you linked to (Ben Z’s change)? If you have assets that demonstrate the issue please do attach them to this thread. Your fix is probably safe, but it’s including a UClass* mapping that is only useful when there are old class mappings, and so may change behavior when classes are regenerated in place as is the case with blueprint compilation.

4

Thank you for the quick reply!

I confirm that ComponentsOnNewObject is empty when the code above runs. The same goes for OldInstanceMap, as the original ScrollBoxSlot is excluded during CollectAllSubobjects( OldObject, Components ) earlier in the function.

I attached a minimal test case in Unreal 5.5.4:

1. Open ScrollBoxBlueprint and UserWidgetBlueprint in the asset editor

2. Click Compile on ScrollBoxBlueprint (this is where the issue occurs)

3. Click Compile on UserWidgetBlueprint (this is where the ensure call fails)

In our project, I set a conditional breakpoint inside UObjectBase::LowLevelRename for (NamePrivate.Number == 1234) to find out where the ScrollBox blueprint instance was renamed to “REINST_”, then step through the rest of the reinstancing code. This should probably also work in the test case above.

Hope this helps in finding a proper fix!

5

[mention removed]​ Any updates on your investigation of the test project I posted earlier?

I’ve been running my temporary fix for the past two weeks, and even though it may not be the full solution, it didn’t seem to have noticeable side effects.

Regarding “it’s including a UClass* mapping that is only useful when there are old class mappings, and so may change behavior when classes are regenerated in place”, if you’re referring to OptionalOldToNewClassMappings, this is set in the same cases as it was before so I don’t expect that part of the behavior to change.

6

[mention removed]​ I’ve now tested this on our build of Unreal 5.6.0, and it is affected by the same bug.

Do you have an estimate for when we could expect a fix to be identified? I’m trying to understand whether we should land my temporary fix for everyone at the company, or just wait a few more days for an official one.

The temporary fix looks safe on the surface level, because doing more object replacements earlier in UEngine::CopyPropertiesForUnrelatedObjects seems strictly better than doing less of them. However, I don’t know enough to rule out side effects entirely, or how to test for them.

7

Super interesting bug, sorry for leaving you hanging. I’ve found one possible fix and will discuss it further after the break:

// Ensure that our archetype instance mapping is up to date, as this will be used for reference replacement below. Note that this // might be populated by BuildDSO() initially to contain instance mapping pairs for which the new instance won't be valid until we // run through each of the PreCreateSubObjectsForReinstantiation() and subsequent CopyPropertiesForUnrelatedObjects() steps above. for (const TPair<UObject*, UObject*>& Pair : OldToNewInstanceMap) { if (UObject** NewArchetypeMapping = OldArchetypeToNewArchetype.Find(Pair.Key)) { *NewArchetypeMapping = Pair.Value; } else // make sure we map the old to new archetypes so that cycles can be fixed up in the final replacement pass { OldArchetypeToNewArchetype.Add(Pair.Key, Pair.Value); } }>However, I don’t know enough to rule out side effects entirely, or how to test for them.

Earlier replacement while often helpful, can make it difficult (bordering on impossible) for user code to do something sensible when there are cycles. For this reason I generally try to solve the hard case (graphs with cycles) rather than encouraging approaches that only work in simple graphs without cycles. We’ve introduced a precreate step that is meant to break cycles, but I don’t think it’s totally backward compatible and accepting of the imperative nature of user code.

8

Thank you [mention removed]​ for the update, coincidentally I’ve also been away from work and back today!

I just tested your fix of updating the archetype mapping and I confirm it solves the issue for us, we’ll integrate it in our build.

9

Commit is here if that’s helpful:

https://github.com/EpicGames/UnrealEngine/commit/5025b6d3b4cf7698c026b2d7facd614dc5c2e9f6

This will be part of the 5.7 binary release.

10

Hi Epic team, is there a plan to get this fixed for UE5.5?

I tried to manually apply the UE5.6 fix, but it is quite difficult as there are a lot of dependent changes that don’t exist in 5.5.

In the meantime, we will be using the Params.bReplaceInternalReferenceUponRead fix that [Content removed] suggested.

11

Thanks for the bump - still investigating.

12

[mention removed]​ Thank you! Might be worth backporting to 5.5 and 5.6 as a stability fix for other users as well?

Both versions are affected by the bug, while 5.4 used to work correctly for us.

13

There is no 5.6.2 build planned at the moment. I will forward your request to the release team.

14

Cheers, thank you so much for the top quality support!

15

We are no longer updating 5.5 and there is no planned 5.6.2 release. The fix will be in 5.7. Great job finding a work around!