From 9491f63c2d9466b28c058b3bce927d268e88ceeb Mon Sep 17 00:00:00 2001 From: Angel Pons Date: Sat, 6 Apr 2024 21:55:00 +0200 Subject: [PATCH 1/3] Harden AbstractInstanceManager globals for PIE At least in the modding environment, trying to use abstract instances would crash the editor quite often, and compiling blueprints that use abstract instances would always crash the editor. Looks like the crashes happen because `StaticManager_PIE` is a global variable that stores pointers to UObjects, but is not an `UPROPERTY`. When the GC kills those UObjects, the pointers in the global variable become dangling pointers without warning. Make abstract instance managers clean up global state when destroyed, and use `IsValid()` to avoid returning UObjects that are about to get killed (as this will result in unpredictable undefined behaviour). Tested in PIE in the modding environment, this allows seeing abstract instances in the editor previews (blueprint and level editor) as well as in PIE worlds. No crashes observed. FIXME: It is likely that crimes against UE were committed in here... TODO: There seems to be a bug somewhere that makes abstract instances in editor worlds get duplicated for some reason, but it does not seem to cause issues? NOTE: This also adapts the non-editor code path accordingly, as these problems could also occur there. However, these changes have not been validated. Signed-off-by: Angel Pons --- .../AbstractInstance.Build.cs | 5 +- .../Private/AbstractInstance.cpp | 11 +- .../Private/AbstractInstanceManager.cpp | 101 +++++++++++++++--- .../Public/AbstractInstanceManager.h | 10 +- 4 files changed, 110 insertions(+), 17 deletions(-) diff --git a/Plugins/AbstractInstance/Source/AbstractInstance/AbstractInstance.Build.cs b/Plugins/AbstractInstance/Source/AbstractInstance/AbstractInstance.Build.cs index f512376495..6bcfb0304e 100644 --- a/Plugins/AbstractInstance/Source/AbstractInstance/AbstractInstance.Build.cs +++ b/Plugins/AbstractInstance/Source/AbstractInstance/AbstractInstance.Build.cs @@ -31,7 +31,6 @@ public AbstractInstance(ReadOnlyTargetRules Target) : base(Target) // ... add other public dependencies that you statically link with here ... } ); - PrivateDependencyModuleNames.AddRange( new string[] @@ -44,6 +43,10 @@ public AbstractInstance(ReadOnlyTargetRules Target) : base(Target) } ); + if (Target.bBuildEditor) + { + PrivateDependencyModuleNames.Add("UnrealEd"); + } DynamicallyLoadedModuleNames.AddRange( new string[] diff --git a/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstance.cpp b/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstance.cpp index 144e6273d0..a9652a9d50 100644 --- a/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstance.cpp +++ b/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstance.cpp @@ -2,11 +2,20 @@ #include "AbstractInstance.h" +#if AI_PIE +#include "AbstractInstanceManager.h" +#include "Editor.h" +#endif + #define LOCTEXT_NAMESPACE "FAbstractInstanceModule" void FAbstractInstanceModule::StartupModule() { - // This code will execute after your module is loaded into memory; the exact timing is specified in the .uplugin file per-module +#if AI_PIE + FEditorDelegates::BeginPIE.AddStatic(&AAbstractInstanceManager::BeginPIE); + FEditorDelegates::PostPIEStarted.AddStatic(&AAbstractInstanceManager::PostPIEStarted); + FWorldDelegates::OnWorldCleanup.AddStatic(&AAbstractInstanceManager::OnWorldCleanup); +#endif } void FAbstractInstanceModule::ShutdownModule() diff --git a/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstanceManager.cpp b/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstanceManager.cpp index 58b66634f2..a63a3774fb 100644 --- a/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstanceManager.cpp +++ b/Plugins/AbstractInstance/Source/AbstractInstance/Private/AbstractInstanceManager.cpp @@ -10,6 +10,11 @@ #include "Engine/World.h" #include "GameFramework/PlayerController.h" +#if AI_PIE +#include "EngineUtils.h" +#include "Editor.h" +#endif + static TAutoConsoleVariable CVarAllowLazySpawning( TEXT("lightweightinstances.AllowLazySpawn"), 0, @@ -57,7 +62,8 @@ constexpr int32 NumCollisionInstances = 500; #if !AI_PIE AAbstractInstanceManager* AAbstractInstanceManager::StaticManager = nullptr; #else -TMap AAbstractInstanceManager::StaticManager_PIE; +TMap, TObjectPtr> AAbstractInstanceManager::StaticManager_PIE; +static FRWLock StaticRWLock; #endif // Sets default values @@ -68,29 +74,90 @@ AAbstractInstanceManager::AAbstractInstanceManager() RootComponent = CreateDefaultSubobject(TEXT("Root")); RootComponent->SetMobility( EComponentMobility::Static ); + + if (!this->HasAnyFlags(RF_ClassDefaultObject | RF_ArchetypeObject)) { + this->SetFlags(RF_Transient | RF_NonPIEDuplicateTransient); + } +} + +/* FIXME(Th3): this terribleness feels like paranoia, is it even needed? */ +void AAbstractInstanceManager::Destroyed() +{ + Super::Destroyed(); +#if AI_PIE + const UWorld* World = GetWorld(); + EditorCheck(World); + FRWScopeLock Lock(StaticRWLock, FRWScopeLockType::SLT_Write); + if (StaticManager_PIE.Contains(World) && StaticManager_PIE[World] == this) { + StaticManager_PIE.Remove(World); + } +#endif +} + +#if AI_PIE +void AAbstractInstanceManager::OnWorldCleanup(UWorld* World, bool bSessionEnded, bool bCleanupResources) +{ + StaticManager_PIE.Remove(World); +} + +static AAbstractInstanceManager* GetEditorInstanceManager() +{ + UWorld* EditorWorld = GetValid(GEditor->EditorWorld); + if (!EditorWorld) { + EditorWorld = GetValid(GWorld.GetReference()); + } + return EditorWorld ? AAbstractInstanceManager::GetInstanceManager(EditorWorld) : nullptr; +} + +void AAbstractInstanceManager::PostPIEStarted(bool bIsSimulating) +{ + if (AAbstractInstanceManager* Manager = GetEditorInstanceManager()) { + Manager->SetFlags(RF_Transient | RF_NonPIEDuplicateTransient); + } } +void AAbstractInstanceManager::BeginPIE(bool bIsSimulating) +{ + if (AAbstractInstanceManager* Manager = GetEditorInstanceManager()) { + Manager->ClearFlags(RF_Transient | RF_NonPIEDuplicateTransient); + } +} + +static AAbstractInstanceManager* GetExistingInstanceManager(UWorld* World) +{ + for (TActorIterator It(World, AAbstractInstanceManager::StaticClass(), EActorIteratorFlags::AllActors); It; ++It) { + return *It; + } + return nullptr; +} +#endif + AAbstractInstanceManager* AAbstractInstanceManager::GetInstanceManager( UObject* WorldContext ) { + UWorld* World = WorldContext->GetWorld(); #if !AI_PIE - if(!AAbstractInstanceManager::StaticManager) - { - AAbstractInstanceManager::StaticManager = WorldContext->GetWorld()->SpawnActor< AAbstractInstanceManager >(); + if (IsValid(AAbstractInstanceManager::StaticManager)) { + return AAbstractInstanceManager::StaticManager; } - + AAbstractInstanceManager::StaticManager = World->SpawnActor(); return AAbstractInstanceManager::StaticManager; #else - if (StaticManager_PIE.Contains(WorldContext->GetWorld())) - { - return StaticManager_PIE[WorldContext->GetWorld()]; + /* FIXME(Th3): why do we maintain a map if we're iterating over actors anyway */ + if (AAbstractInstanceManager* ExistingManager = GetExistingInstanceManager(World)) { + return ExistingManager; } - else - { - auto NewManager = WorldContext->GetWorld()->SpawnActor< AAbstractInstanceManager >(); - StaticManager_PIE.Add(WorldContext->GetWorld(),NewManager); - + FRWScopeLock Lock(StaticRWLock, FRWScopeLockType::SLT_Write); + if (StaticManager_PIE.Contains(World)) { + if (IsValid(StaticManager_PIE[World])) { + return StaticManager_PIE[World]; + } + StaticManager_PIE.Remove(World); + } + if (AAbstractInstanceManager* NewManager = World->SpawnActor()) { + StaticManager_PIE.Add(World, NewManager); return NewManager; } + return nullptr; #endif } @@ -100,6 +167,7 @@ AAbstractInstanceManager* AAbstractInstanceManager::GetInstanceManager() return AAbstractInstanceManager::StaticManager; #else // TODO implement. + EditorCheck(false); return nullptr; #endif } @@ -592,7 +660,12 @@ void AAbstractInstanceManager::EndPlay( const EEndPlayReason::Type EndPlayReason #if !AI_PIE AAbstractInstanceManager::StaticManager = nullptr; #else - AAbstractInstanceManager::StaticManager_PIE.Empty(); + FRWScopeLock Lock(StaticRWLock, FRWScopeLockType::SLT_Write); + const UWorld* World = GetWorld(); + EditorCheck(World); + if (StaticManager_PIE.Contains(World) && StaticManager_PIE[World] == this) { + StaticManager_PIE.Remove(World); + } #endif } diff --git a/Plugins/AbstractInstance/Source/AbstractInstance/Public/AbstractInstanceManager.h b/Plugins/AbstractInstance/Source/AbstractInstance/Public/AbstractInstanceManager.h index b2d44510f1..b30c1891e4 100644 --- a/Plugins/AbstractInstance/Source/AbstractInstance/Public/AbstractInstanceManager.h +++ b/Plugins/AbstractInstance/Source/AbstractInstance/Public/AbstractInstanceManager.h @@ -92,6 +92,8 @@ class ABSTRACTINSTANCE_API AAbstractInstanceManager : public AActor static AAbstractInstanceManager* GetInstanceManager( UObject* WorldContext ); static AAbstractInstanceManager* GetInstanceManager(); + virtual void Destroyed() override; + virtual void Tick(float DeltaSeconds) override; static void SetInstancedStatic( AActor* OwnerActor, const FTransform& ActorTransform, const UAbstractInstanceDataObject* InstanceData, TArray& OutHandle, bool bInitializeHidden = false ); @@ -170,9 +172,15 @@ class ABSTRACTINSTANCE_API AAbstractInstanceManager : public AActor #if !AI_PIE static AAbstractInstanceManager* StaticManager; #else // PIE support - static TMap StaticManager_PIE; + static TMap, TObjectPtr> StaticManager_PIE; #endif static FName BuildUniqueName( const FInstanceData& InstanceData ); static FName BuildUniqueName( const UInstancedStaticMeshComponent* MeshComponent ); +public: +#if AI_PIE + static void OnWorldCleanup(UWorld* World, bool bSessionEnded, bool bCleanupResources); + static void BeginPIE(bool bIsSimulating); + static void PostPIEStarted(bool bIsSimulating); +#endif }; From 7e04ba7b8948cd809fb19fe0b7df3403e24d862d Mon Sep 17 00:00:00 2001 From: Angel Pons Date: Mon, 1 Jul 2024 17:56:30 +0200 Subject: [PATCH 2/3] FGBuildable.cpp: Improve implementation Be a bit more paranoid when handling abstract instances to avoid NULL pointer dereferencing. Checking whether `this` is NULL gets optimised out by most compilers (a NULL `this` is undefined behaviour), so drop the checks as they are misleading. Define the `LogBuilding` log category to log a single error, but also to allow using some inline functions defined in base game headers. If the log category is not defined, using said inline functions causes a build-time error. Signed-off-by: Angel Pons --- .../Private/Buildables/FGBuildable.cpp | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/Source/FactoryGame/Private/Buildables/FGBuildable.cpp b/Source/FactoryGame/Private/Buildables/FGBuildable.cpp index e8542423e3..363b1e338c 100644 --- a/Source/FactoryGame/Private/Buildables/FGBuildable.cpp +++ b/Source/FactoryGame/Private/Buildables/FGBuildable.cpp @@ -6,6 +6,8 @@ #include "Net/UnrealNetwork.h" #include "AbstractInstanceManager.h" +DEFINE_LOG_CATEGORY(LogBuilding); + #if WITH_EDITOR void AFGBuildable::PostEditChangeProperty(FPropertyChangedEvent& PropertyChangedEvent) { Super::PostEditChangeProperty(PropertyChangedEvent); @@ -44,25 +46,33 @@ TArray AFGBuildable::GetActorLightweightInstanceData_Imple return TArray(); } void AFGBuildable::SetupInstances_Native(bool bInitializeHidden) { - if (this && GetWorld() && IsValid(mInstanceDataCDO)) { + if (GetWorld() && IsValid(mInstanceDataCDO)) { for (FInstanceData InstanceData : mInstanceDataCDO->GetInstanceData()) { if (IsValid(InstanceData.StaticMesh) && !InstanceData.OverridenMaterials.Contains(nullptr)) { - FInstanceHandle* Handle; - AAbstractInstanceManager::SetInstanceFromDataStatic(this, FTransform(), InstanceData, Handle, bInitializeHidden); - mInstanceHandles.Add(Handle); + FInstanceHandle* Handle = nullptr; + AAbstractInstanceManager::SetInstanceFromDataStatic(this, this->GetActorTransform(), InstanceData, Handle, bInitializeHidden); + if (Handle && Handle->IsValid()) { + mInstanceHandles.Add(Handle); + } else { + UE_LOG(LogBuilding, Warning, TEXT("[Th3] Invalid Handle for %s mesh %s at %s"), + *this->GetPathName(), + InstanceData.StaticMesh ? *InstanceData.StaticMesh->GetName() : TEXT("nullptr"), + *InstanceData.RelativeTransform.ToString()); + } } } } } void AFGBuildable::RemoveInstances_Native() { - if (this && GetWorld()) { - AAbstractInstanceManager* Manager = AAbstractInstanceManager::GetInstanceManager(GetWorld()); - for (FInstanceHandle* InstanceHandle : mInstanceHandles) { - if (InstanceHandle->IsInstanced()) { - Manager->RemoveInstance(InstanceHandle); - } - } - mInstanceHandles.Empty(); + if (UWorld* World = GetWorld()) { + if (AAbstractInstanceManager* Manager = AAbstractInstanceManager::GetInstanceManager(World)) { + for (FInstanceHandle* InstanceHandle : mInstanceHandles) { + if (InstanceHandle->IsInstanced()) { + Manager->RemoveInstance(InstanceHandle); + } + } + } + mInstanceHandles.Empty(); } } From 7dcb1d62601d6adca1ae81c0fdedab80e6e8bfab Mon Sep 17 00:00:00 2001 From: Angel Pons Date: Mon, 1 Jul 2024 18:07:57 +0200 Subject: [PATCH 3/3] FGBuildable.cpp: Fix root component mobility Looking at the game's binaries, the root component of buildables has to default to static mobility. Otherwise, pre-placed buildings do not work properly in the editor. The pre-placed buildings in ExampleLevel should no longer be stuck at 0 (which is inside the starting cube), but the level needs to be resaved. Signed-off-by: Angel Pons --- Source/FactoryGame/Private/Buildables/FGBuildable.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/Source/FactoryGame/Private/Buildables/FGBuildable.cpp b/Source/FactoryGame/Private/Buildables/FGBuildable.cpp index 363b1e338c..3c422c6edd 100644 --- a/Source/FactoryGame/Private/Buildables/FGBuildable.cpp +++ b/Source/FactoryGame/Private/Buildables/FGBuildable.cpp @@ -175,6 +175,7 @@ AFGBuildable::AFGBuildable(const FObjectInitializer& ObjectInitializer) : Super( this->NetDormancy = ENetDormancy::DORM_Initial; this->NetCullDistanceSquared = 5625000000.0; this->RootComponent = CreateDefaultSubobject(TEXT("RootComponent")); + this->RootComponent->SetMobility(EComponentMobility::Static); } void AFGBuildable::Serialize(FArchive& ar){ Super::Serialize(ar); } void AFGBuildable::PostLoad(){ Super::PostLoad(); }