Request: Change behavior when saving BP with components missing redirectors

Development Programming & Scripting
, ,
1

Hi,

We’ve encountered the following scenario several times over the years and feel that perhaps the default behavior of the engine should take it into account in order to prevent data loss.

  1. Someone renames a c++ component class that’s constructed/assigned in a c++ actor class that has some number of child BPs, but forgets to check in a redirector immediately.
  2. Someone (else) syncs and saves one of the child blueprints. Because there’s no redirector and the component class is not found, it silently fails and treats it as nullptr (which shouldn’t be possible!) causing the component to be serialized into the uasset as null.
  3. At this point, adding the redirector won’t return that child BP to a valid state (even if the parent class is resaved with a valid redirected component) because it’s no longer inheriting the property. Instead, it’s deserializing the nullptr from the uasset as though it was manually overridden by the user (which, again, is not something that’s normally possible to do).
    1. This can be detected in FObjectProperty::PostSerializeObjectItem as (ObjectValue == nullptr && CurrentValue != nullptr) which is a perfectly valid case for a normal UPROPERTY pointing to an asset that has been manually cleared in a child BP, but should never be a valid case for a component UPROPERTY that was constructed/assigned in the native class.

I would expect the engine to either hit a check() and/or notify the user attempting to save in step 2 that the asset could not be saved due to an invalid native component class (and probably be a cook failure too, so it can be caught early rather than at runtime with a null pointer exception). This could save a lot of grief and debugging later on.

Thanks.

[Attachment Removed]

2

Hello! I was able to reproduce the nulled subobject via redirectors. For sure, it would be best to catch these as soon as possible.

When a blueprint loads and a subobject class isn’t found, the following message is printed to log from LinkerLoad.cpp:

FString OuterName = Export.OuterIndex.IsNull() ? LinkerRoot->GetFullName() : GetFullImpExpName(Export.OuterIndex);
FString ClassName = GetClassName(Export.ThisIndex).ToString();
UE_CLOG(Export.ObjectFlags & EObjectFlags::RF_Public, LogLinker, Warning, TEXT("Unable to load %s with outer %s because its class (%s) does not exist"), *Export.ObjectName.ToString(), *OuterName, *ClassName);
return nullptr;

You can consider modifying the engine code there to be more disruptive:

  • FMessageDialog for a user popup that could inform them to blame someone
  • Or a Fatal log. Personally I recommend against that because it would block non-technical team members

I also wrote an editor validator that will popup an error when saving a blueprint that has a nulled subobject that clearly stems from corruption. It uses reasoning similar to yours, about a UPROPERTY clearly shouldn’t be null if the native constructor created a default subobject for it. This validator will show the popup on save (since validators run on any asset save), but wouldn’t prevent the save. If your studio uses Epic’s SubmitTool, you can prevent people from checking in assets that fail validators though!

[Image Removed]

#include "EditorValidator_NulledSubobjects.h"
#include "Misc/DataValidation.h"
#include "UObject/FieldIterator.h"
 
bool UEditorValidator_NulledSubobjects::CanValidateAsset_Implementation(const FAssetData& InAssetData, UObject* InObject, FDataValidationContext& InContext) const
{
	return InObject != nullptr && InObject->IsA<UBlueprint>();
}
 
EDataValidationResult UEditorValidator_NulledSubobjects::ValidateLoadedAsset_Implementation(const FAssetData& InAssetData, UObject* InAsset, FDataValidationContext& Context)
{
	// Valid, until invalid
	EDataValidationResult Result = EDataValidationResult::Valid;
 
	if (InAsset->IsA<UBlueprint>())
	{
		UBlueprint* BP = Cast<UBlueprint>(InAsset);
 
		// We will use reflection to check value of UPROPERTY pointers.
 
		// The blueprint generated class's default object contains the blueprint's saved values.
		UClass* BPGC = BP->GeneratedClass;
		check(BPGC);
		check(!BPGC->IsNative());
		UObject* BPCDO = BPGC->GetDefaultObject();
		check(BPCDO);
		
		// The native parent class's default object's subobjects can be checked to see if the
		// native constructor defines default subobjects. This is to minimize false hits.
		// Find the native class by walking up the class hierarchy until a native class is found.
		UClass* NativeSuperClass = BPGC;
		do
		{
			NativeSuperClass = NativeSuperClass->GetSuperClass();
		} while (!NativeSuperClass->IsNative());
		check(NativeSuperClass);
		UObject* NativeCDO = NativeSuperClass->GetDefaultObject();
		check(NativeCDO);
 
		// Iterate all of the native class's object pointer UPROPERTIES
		for (TFieldIterator<FObjectProperty> PropertyIt(NativeSuperClass); PropertyIt; ++PropertyIt)
		{
			const FObjectProperty* Prop = *PropertyIt;
			const FString PropClass = Prop->GetClass()->GetName();
			const FString PropName = Prop->GetName();
			UObject* NativeCDO_Subobject = Prop->GetObjectPtrPropertyValue_InContainer(NativeCDO);
			UObject* BPCDO_Subobject = Prop->GetObjectPtrPropertyValue_InContainer(BPCDO);
 
			// If the native CDO has a default subobject, but the blueprint CDO doesn't, it's likely a subobject being incorrectly nulled.
			if (NativeCDO_Subobject && NativeCDO_Subobject->HasAnyFlags(EObjectFlags::RF_DefaultSubObject) && BPCDO_Subobject == nullptr)
			{
				const FString ErrorMessage = FString::Printf(TEXT("%s has an invalid subobject for property '%s'. It's null but expected non-null. Missing a class redirector?"), *InAsset->GetName(), *PropName);
				Context.AddMessage(InAssetData, EMessageSeverity::Type::Error, FText::FromString(ErrorMessage));
 
				Result = EDataValidationResult::Invalid;
			}
		}
	}
 
	return EDataValidationResult::Invalid;
}

I understand that ideally, you wouldn’t want the asset to save at all when a subobject clearly is null incorrectly. I’m not sure off the top of my head if EditorValidators can be used to prevent save, or if we have other ways to prevent saves under project specific conditions. I’ll ask around and respond back later.

[Attachment Removed]

3

Hello again, a colleague pointed me to the following method of custom save-prevention logic:

FSavePackageSettings& SavePackageSettings = FSavePackageSettings::GetDefaultSettings();
SavePackageSettings.AddExternalExportValidation(...);

I’ve adapted the validator snippet from above to use that system. Add the following snippet to any module start-up function that runs just once:

#include "UObject/FieldIterator.h"
#include "Misc/UObjectToken.h"
 
void FMyEditorModule::StartupModule()
{
	FSavePackageSettings::GetDefaultSettings().AddExternalExportValidation(
		[](const FExportsValidationContext& Context) -> ESavePackageResult
		{
			bool bSuccess = true;
			for (UObject* Obj : Context.Exports)
			{
				if (Obj->IsA<UBlueprint>())
				{
					UBlueprint* BP = Cast<UBlueprint>(Obj);
 
					// We will use reflection to check value of UPROPERTY pointers.
 
					// The blueprint generated class's default object contains the blueprint's saved values.
					UClass* BPGC = BP->GeneratedClass;
					check(BPGC);
					check(!BPGC->IsNative());
					UObject* BPCDO = BPGC->GetDefaultObject();
					check(BPCDO);
 
					// The native parent class's default object's subobjects can be checked to see if the
					// native constructor defines default subobjects. This is to minimize false hits.
					// Find the native class by walking up the class hierarchy until a native class is found.
					UClass* NativeSuperClass = BPGC;
					do
					{
						NativeSuperClass = NativeSuperClass->GetSuperClass();
					} while (!NativeSuperClass->IsNative());
					check(NativeSuperClass);
					UObject* NativeCDO = NativeSuperClass->GetDefaultObject();
					check(NativeCDO);
 
					// Iterate all of the native class's object pointer UPROPERTIES
					for (TFieldIterator<FObjectProperty> PropertyIt(NativeSuperClass); PropertyIt; ++PropertyIt)
					{
						const FObjectProperty* Prop = *PropertyIt;
						const FString PropClass = Prop->GetClass()->GetName();
						const FString PropName = Prop->GetName();
						UObject* NativeCDO_Subobject = Prop->GetObjectPtrPropertyValue_InContainer(NativeCDO);
						UObject* BPCDO_Subobject = Prop->GetObjectPtrPropertyValue_InContainer(BPCDO);
 
						// If the native CDO has a default subobject, but the blueprint CDO doesn't, it's likely a subobject being incorrectly nulled.
						if (NativeCDO_Subobject && NativeCDO_Subobject->HasAnyFlags(EObjectFlags::RF_DefaultSubObject) && BPCDO_Subobject == nullptr)
						{
							const FString ErrorMessage = FString::Printf(TEXT("%s has an invalid subobject for property '%s'. It's null but expected non-null. Missing a class redirector?"), *Obj->GetName(), *PropName);
							FMessageLog MessageLog("MyLog");
							MessageLog.Error()
								->AddToken(FTextToken::Create(FText::FromString(ErrorMessage)))
								->AddToken(FUObjectToken::Create(BP));
							MessageLog.Open(EMessageSeverity::Error);
 
							bSuccess = false;
						}
					}
				}
			}
			return bSuccess ? ESavePackageResult::Success : ESavePackageResult::ValidatorError;
		}
	);
}

Hopefully that suits your needs!

[Attachment Removed]

4

Hey Andrew, I have a final update.

“I feel like it would really be a good idea to add this functionality directly to the engine out-of-the-box. By the time someone comes across this post it may be too late to easily revert the affected assets.”

I see your point for sure, since data loss from a missing class is a problem most teams will run into at some point. Our team is averse to increasing the warning’s visibility, because of potential disruptiveness to a large number of existing projects.

I do like the idea myself, so had been working on a snippet. Discussing is ongoing and I don’t think this will make it into the engine by default, but use it if you want, in addition to the validator snippet from before:

UObject* FLinkerLoad::CreateExport( int32 Index )
{
	...
		FString OuterName = Export.OuterIndex.IsNull() ? LinkerRoot->GetFullName() : GetFullImpExpName(Export.OuterIndex);
		FString ClassName = GetClassName(Export.ThisIndex).ToString();
		UE_CLOG(Export.ObjectFlags & EObjectFlags::RF_Public, LogLinker, Warning, TEXT("Unable to load %s with outer %s because its class (%s) does not exist"), *Export.ObjectName.ToString(), *OuterName, *ClassName);
 
// ENGINE MOD MODIFICATION: Show missing class to users in message log to mitigate data loss occurrence, and hint at how to fix the missing class.
#if WITH_EDITOR
		
		if (Export.ObjectFlags & EObjectFlags::RF_Public)
		{
			TWeakObjectPtr<UPackage> PackageToLog = CurrentLoadContext->SerializedObject ? CurrentLoadContext->SerializedObject->GetPackage() : nullptr;
			const FName PackageName = PackageToLog.IsValid() ? PackageToLog->GetFName() : NAME_None;
			const FString PackageShortName = FPackageName::GetShortName(PackageName);
			FFormatNamedArguments ErrorArguments;
			ErrorArguments.Add(TEXT("ObjectName"), FText::FromName(Export.ObjectName));
			ErrorArguments.Add(TEXT("OuterName"), FText::FromString(OuterName));
			ErrorArguments.Add(TEXT("ClassName"), FText::FromString(ClassName));
			ErrorArguments.Add(TEXT("PackageName"), FText::FromName(PackageName));
			ErrorArguments.Add(TEXT("PackageShortName"), FText::FromString(PackageShortName));
		
			const FText ErrorText1 = FText::Format(NSLOCTEXT("Core", "LinkerLoad_ExportMissingClass_Report", "Unable to load object ({ObjectName}) in {PackageShortName} because its class ({ClassName}) does not exist."), ErrorArguments);
			const FText ErrorText2 = FText::Format(NSLOCTEXT("Core", "LinkerLoad_ExportMissingClass_ResaveWarning", "Saving {PackageShortName} will cause the previously saved object ({ObjectName}) of class ({ClassName}) to be lost.\nIf the class was renamed, add a ClassRedirect in DefaultEngine.ini [CoreRedirects]. If ({ObjectName}) is no longer needed, resave the asset.\nThe class might not have been checked into source control. It may be in a module that isn't loaded yet.\nIf the class was intentionally deleted, you can resave the asset to clean up the reference.\n"), ErrorArguments);
		
			// Display to message log. Scheduling this on main thread since we might be on the async loading thread
			// and this will interact with Slate.
			Async(EAsyncExecution::TaskGraphMainThread, [PackageToLog, ErrorText1, ErrorText2]() {
				FMessageLog MessageLog("LoadErrors");
				MessageLog.Error(ErrorText1)
					->AddToken(FUObjectToken::Create(PackageToLog.Get()));
				MessageLog.Warning(ErrorText2)
					->AddToken(FURLToken::Create(FString::Printf(TEXT("https://dev.epicgames.com/documentation/en-us/unreal-engine/core-redirects-in-unreal-engine")), LOCTEXT("LinkerLoad_CoreRedirectDocs", "Documentation: CoreRedirects")));
				MessageLog.Open(EMessageSeverity::Error);
				});
		}
#endif
// END ENGINE MODIFICATION
	...
}

[Attachment Removed]

5

Thanks, that validator is a great idea. I’ll probably go ahead and add that to our project, though given how easy it is to not be aware of this issue, I feel like it would really be a good idea to add this functionality directly to the engine out-of-the-box. By the time someone comes across this post it may be too late to easily revert the affected assets.

[Attachment Removed]

6

Ah excellent, I’ll give that a try, thanks!

[Attachment Removed]

7

Output of the snippet:

[Image Removed]

I realized we discussed multiple ways of warnings on load, validation on save, and validation to prevent save. The final status is:

  • Popup warnings on load: doesn’t look like we’ll change default engine behavior for this. But a LinkerLoad.cpp tweak gets you the above warning which perhaps can mitigate a lot of cases.
  • Validation on save and validation to prevent save: won’t be on by default, but I’m keeping it in mind for an opt-in plugin, since the engine hooks are already there.

Internally, we run a Horde cook job after every commit and that LinkerLoad log warning will automatically cause a backout and blame someone. That’s how we catch the missing redirector or missing BP class before someone else can take the asset and save it with data loss.

[Attachment Removed]