SRombauts / UEPlasticPlugin

Plastic SCM Plugin for Unreal Engine
http://srombauts.github.io/UEPlasticPlugin
147 stars 29 forks source link

Plugin hangs the editor for long periods while generating context menu entries #144

Open Acren opened 3 weeks ago

Acren commented 3 weeks ago

Hey there,

We're currently using 1.11.0 in UE 5.4, and I've been noticing that when I hover an asset in the context menu the first time the editor will hang for 30-60 seconds.

It looks like in this case the plugin is waiting on cm commands synchronously.

FScopeLock::FScopeLock(FWindowsCriticalSection *) ScopeLock.h:39
PlasticSourceControlShell::RunCommand(const FString &, const TArray<…> &, const TArray<…> &, FString &, FString &) PlasticSourceControlShell.cpp:394
PlasticSourceControlUtils::RunCommand(const FString &, const TArray<…> &, const TArray<…> &, TArray<…> &, TArray<…> &) PlasticSourceControlUtils.cpp:52
PlasticSourceControlUtils::RunListLocks(const FPlasticSourceControlProvider &, const bool, TArray<…> &) PlasticSourceControlUtils.cpp:534
PlasticSourceControlUtils::GetLocksForWorkingBranch(const FPlasticSourceControlProvider &, const TArray<…> &) PlasticSourceControlUtils.cpp:558
FPlasticSourceControlMenu::GeneratePlasticAssetContextMenu(FMenuBuilder &, TArray<…>) PlasticSourceControlMenu.cpp:195

This can be quite frustrating when hovering something locks the editor for so long. Would it be possible to make this async?

Thanks, Sam

SRombauts commented 3 weeks ago

Hi @Acren, thanks for the report.

I actually also found myself this issue quite recently; it's indeed the UI waiting for the status of locks! After digging I found out that it's a common issue across all source control providers including Perforce: the menu wait for SCC status, calling the serveur! At that time I couldn't find a way to populate a submenu asynchronously in Unreal, but perhaps I missed the proper way?

Independandlty from fixing the UI, potential fixes could be;

  1. to make sure that the lock status are always in the cache before hand
  2. if not, either making sure it never take so long (more on that on a following message)
  3. or relying on a default behaviour that doesn't require the lock status (not disabling entries in the menu)

In any case, it means that Unreal expect the provider to answer quickly; for sure in my own testing the issue wasn't as bad as yours, as I was only seeing sub second latencies. It means that I may have missed an important use case;

SRombauts commented 3 weeks ago

Would you be able to send me the Unreal logs of a minimal session showing the issue? Ideally I could also make use of the logs from the CLI (cm.exe) https://github.com/SRombauts/UEPlasticPlugin#enable-debug-logs

If you prefer, creating an official support ticket might be better for confidentiality, and help us sort the priorities Unity Version Control support.

Many thanks, Sébastien

Acren commented 3 weeks ago

Thanks @SRombauts, I'll gather those logs and create a ticket referencing this thread.

As for making the menus async, I noticed that there were some generic source control submenus in the editor that are async already. Check out AddAsyncMenuEntry in AssetSourceControlContextMenu.cpp, looks like it has a bespoke approach using custom widgets to show a loading state until it's done.

https://github.com/EpicGames/UnrealEngine/blob/575944b129b9d2f1a30424accdf026712e93bbd2/Engine/Plugins/Editor/AssetManagerEditor/Source/AssetManagerEditor/Private/AssetSourceControlContextMenu.cpp#L96-L119

Would be better if the editor had a unified way to approach it, but maybe you could do something similar?

SRombauts commented 3 weeks ago

Thanks, that was the kind of magic I was looking for; I'll have to take a closer look and replicate the logic, that wouldn't be the first time copy past is needed. In the longer run this should perhaps be added to the UI framework I believe.

Acren commented 1 week ago

Just to follow up on this, I did send through the logs on a ticket, did you get those?