Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash on updating workspace using "Sync" from global source control menu #46

Merged
merged 6 commits into from
Nov 8, 2022
Merged

Fix crash on updating workspace using "Sync" from global source control menu #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e6d3e7
Remove the InConcurrency unused arguments from all commands
SRombautsU Oct 28, 2022
43dfb76
Apply a potential crash fix for Garbage Collection on Package Reloads…
SRombautsU Oct 28, 2022
06f9745
Add a "SyncAll" custom operation to hold the list of UpdatedFiles
SRombautsU Oct 28, 2022
2093974
Move code to a new PlasticSourceControlUtils::RunSync() and implement…
SRombautsU Oct 28, 2022
1c1349b
Use the list of updated files to reload only what is needed
SRombautsU Oct 28, 2022
f70895b
Re-enable the "Sync/Update" action of the source control menu
SRombautsU Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Source/PlasticSourceControl/Private/PlasticSourceControlConsole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ void FPlasticSourceControlConsole::ExecutePlasticConsoleCommand(const TArray<FSt
const FString Command = a_args[0];
TArray<FString> Parameters = a_args;
Parameters.RemoveAt(0);
PlasticSourceControlUtils::RunCommand(Command, Parameters, TArray<FString>(), EConcurrency::Synchronous, Results, Errors);
PlasticSourceControlUtils::RunCommand(Command, Parameters, TArray<FString>(), Results, Errors);
if (Results.Len() > 200) // RunCommand() already log all command results up to 200 characters (limit to avoid long wall of text like XML)
{
UE_LOG(LogSourceControl, Log, TEXT("Output:\n%s"), *Results);
Expand Down
103 changes: 67 additions & 36 deletions Source/PlasticSourceControl/Private/PlasticSourceControlMenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,51 +137,51 @@ TArray<FString> FPlasticSourceControlMenu::ListAllPackages()
return PackageNames;
}

/// Unkink all loaded packages to allow to update them
// Unkink all loaded packages to allow to update them
// Note: Extracted from AssetViewUtils::SyncPathsFromSourceControl()
TArray<UPackage*> FPlasticSourceControlMenu::UnlinkPackages(const TArray<FString>& InPackageNames)
{
// Form a list of loaded packages to unlink...
TArray<UPackage*> LoadedPackages;

// Inspired from ContentBrowserUtils::SyncPathsFromSourceControl()
if (InPackageNames.Num() > 0)
LoadedPackages.Reserve(InPackageNames.Num());
for (const FString& PackageName : InPackageNames)
{
// Form a list of loaded packages to reload...
LoadedPackages.Reserve(InPackageNames.Num());
for (const FString& PackageName : InPackageNames)
// NOTE: this will only find packages loaded in memory
if (UPackage* Package = FindPackage(nullptr, *PackageName))
{
UPackage* Package = FindPackage(nullptr, *PackageName);
if (Package)
{
LoadedPackages.Emplace(Package);
LoadedPackages.Emplace(Package);

// Detach the linkers of any loaded packages so that SCC can overwrite the files...
if (!Package->IsFullyLoaded())
{
FlushAsyncLoading();
Package->FullyLoad();
}
ResetLoaders(Package);
// Detach the linkers of any loaded packages so that SCC can overwrite the files...
if (!Package->IsFullyLoaded())
{
FlushAsyncLoading();
Package->FullyLoad();
}
ResetLoaders(Package);
}
UE_LOG(LogSourceControl, Log, TEXT("Reseted Loader for %d Packages"), LoadedPackages.Num());
// else not loaded in memory, nothing to unlink nor reload!
}
UE_LOG(LogSourceControl, Log, TEXT("Reseted Loader for %d Packages"), LoadedPackages.Num());

return LoadedPackages;
}

// Hot-Reload all packages after they have been updated
// Note: Extracted from AssetViewUtils::SyncPathsFromSourceControl()
void FPlasticSourceControlMenu::ReloadPackages(TArray<UPackage*>& InPackagesToReload)
{
UE_LOG(LogSourceControl, Log, TEXT("Reloading %d Packages..."), InPackagesToReload.Num());

// Syncing may have deleted some packages, so we need to unload those rather than re-load them...
TArray<UPackage*> PackagesToUnload;
// Note: we will store the package using weak pointers here otherwise we might have garbage collection issues after the ReloadPackages call
TArray<TWeakObjectPtr<UPackage>> PackagesToUnload;
InPackagesToReload.RemoveAll([&](UPackage* InPackage) -> bool
{
const FString PackageExtension = InPackage->ContainsMap() ? FPackageName::GetMapPackageExtension() : FPackageName::GetAssetPackageExtension();
const FString PackageFilename = FPackageName::LongPackageNameToFilename(InPackage->GetName(), PackageExtension);
if (!FPaths::FileExists(PackageFilename))
{
PackagesToUnload.Emplace(InPackage);
PackagesToUnload.Emplace(MakeWeakObjectPtr(InPackage));
return true; // remove package
}
return false; // keep package
Expand All @@ -191,7 +191,16 @@ void FPlasticSourceControlMenu::ReloadPackages(TArray<UPackage*>& InPackagesToRe
UPackageTools::ReloadPackages(InPackagesToReload);

// Unload any deleted packages...
UPackageTools::UnloadPackages(PackagesToUnload);
TArray<UPackage*> PackageRawPtrsToUnload;
for (TWeakObjectPtr<UPackage>& PackageToUnload : PackagesToUnload)
{
if (PackageToUnload.IsValid())
{
PackageRawPtrsToUnload.Emplace(PackageToUnload.Get());
}
}

UPackageTools::UnloadPackages(PackageRawPtrsToUnload);
}

void FPlasticSourceControlMenu::SyncProjectClicked()
Expand All @@ -202,11 +211,11 @@ void FPlasticSourceControlMenu::SyncProjectClicked()
if (bSaved)
{
// Find and Unlink all packages in Content directory to allow to update them
PackagesToReload = UnlinkPackages(ListAllPackages());
UnlinkedPackages = UnlinkPackages(ListAllPackages());

// Launch a "Sync" operation
// Launch a custom "SyncAll" operation
FPlasticSourceControlProvider& Provider = FPlasticSourceControlModule::Get().GetProvider();
TSharedRef<FSync, ESPMode::ThreadSafe> SyncOperation = ISourceControlOperation::Create<FSync>();
TSharedRef<FPlasticSyncAll, ESPMode::ThreadSafe> SyncOperation = ISourceControlOperation::Create<FPlasticSyncAll>();
const ECommandResult::Type Result = Provider.Execute(SyncOperation, TArray<FString>(), EConcurrency::Asynchronous, FSourceControlOperationComplete::CreateRaw(this, &FPlasticSourceControlMenu::OnSourceControlOperationComplete));
if (Result == ECommandResult::Succeeded)
{
Expand All @@ -215,9 +224,8 @@ void FPlasticSourceControlMenu::SyncProjectClicked()
}
else
{
// Report failure with a notification and Reload all packages
// Report failure with a notification (but nothing need to be reloaded since no local change is expected)
DisplayFailureNotification(SyncOperation->GetName());
ReloadPackages(PackagesToReload);
}
}
else
Expand Down Expand Up @@ -277,7 +285,7 @@ void FPlasticSourceControlMenu::RevertAllClicked()
if (bSaved)
{
// Find and Unlink all packages in Content directory to allow to update them
PackagesToReload = UnlinkPackages(ListAllPackages());
UnlinkedPackages = UnlinkPackages(ListAllPackages());

// Launch a "RevertAll" Operation
FPlasticSourceControlProvider& Provider = FPlasticSourceControlModule::Get().GetProvider();
Expand All @@ -292,9 +300,8 @@ void FPlasticSourceControlMenu::RevertAllClicked()
}
else
{
// Report failure with a notification and Reload all packages
// Report failure with a notification (but nothing need to be reloaded since no local change is expected)
DisplayFailureNotification(RevertAllOperation->GetName());
ReloadPackages(PackagesToReload);
}
}
else
Expand Down Expand Up @@ -448,11 +455,37 @@ void FPlasticSourceControlMenu::OnSourceControlOperationComplete(const FSourceCo
{
RemoveInProgressNotification();

if ((InOperation->GetName() == "Sync") || (InOperation->GetName() == "RevertAll"))
if (InOperation->GetName() == "SyncAll")
{
// Reload packages that where unlinked at the beginning of the Sync operation
TSharedRef<FPlasticSyncAll, ESPMode::ThreadSafe> Operation = StaticCastSharedRef<FPlasticSyncAll>(InOperation);

TArray<UPackage*> PackagesToReload;
PackagesToReload.Reserve(Operation->UpdatedFiles.Num());
for (const FString& FilePath : Operation->UpdatedFiles)
{
FString PackageName;
FString FailureReason;
if (FPackageName::TryConvertFilenameToLongPackageName(FilePath, PackageName, &FailureReason))
{
// NOTE: this will only find packages loaded in memory
if (UPackage* Package = FindPackage(nullptr, *PackageName))
{
PackagesToReload.Emplace(Package);
UE_LOG(LogSourceControl, Log, TEXT("Reload: %s"), *PackageName);
}
}
// else, it means the file is not an asset from the Content/ folder (eg config, source code, anything else)
}

// Reload packages that where updated by the Sync operation
ReloadPackages(PackagesToReload);
}
else if (InOperation->GetName() == "RevertAll")
{
// Reload packages that where unlinked at the beginning of the Sync operation
// TODO: PackagesToReload should be filled by the source control operation itself, like for the update above
ReloadPackages(UnlinkedPackages);
}

// Report result with a notification
if (InResult == ECommandResult::Succeeded)
Expand All @@ -476,17 +509,15 @@ void FPlasticSourceControlMenu::AddMenuExtension(FToolMenuSection& Menu)
"PlasticSync",
#endif
LOCTEXT("PlasticSync", "Sync/Update Workspace"),
// TODO: temporarily disabled since it tries to reload the whole Content, which crashes the Editor
LOCTEXT("PlasticSyncTooltip", "[Disabled/crashing] Update all files in the workspace to the latest version."),
LOCTEXT("PlasticSyncTooltip", "Update the workspace to the latest changeset of the branch, and reload all affected assets."),
#if ENGINE_MAJOR_VERSION == 5 && ENGINE_MINOR_VERSION >= 1
FSlateIcon(FAppStyle::GetAppStyleSetName(), "SourceControl.Actions.Sync"),
#else
FSlateIcon(FEditorStyle::GetStyleSetName(), "SourceControl.Actions.Sync"),
#endif
FUIAction(
FExecuteAction::CreateRaw(this, &FPlasticSourceControlMenu::SyncProjectClicked),
// TODO: temporarily disabled since it tries to reload the whole Content, which crashes the Editor
FCanExecuteAction::CreateRaw(this, &FPlasticSourceControlMenu::False)
FCanExecuteAction()
)
);

Expand Down
4 changes: 2 additions & 2 deletions Source/PlasticSourceControl/Private/PlasticSourceControlMenu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ class FPlasticSourceControlMenu
FDelegateHandle ViewMenuExtenderHandle;
#endif

/** Loaded packages to reload after a Sync or Revert operation */
TArray<UPackage*> PackagesToReload;
/** Loaded packages we unlinked, to reload after a Sync or Revert operation */
TArray<UPackage*> UnlinkedPackages;

/** Current source control operation from extended menu if any */
TWeakPtr<class SNotificationItem> OperationInProgressNotification;
Expand Down