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.

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

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

Open
Acren opened this issue Aug 15, 2024 · 8 comments
Assignees
Labels

Comments

@Acren
Copy link

Acren commented Aug 15, 2024

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 SRombauts self-assigned this Aug 15, 2024
@SRombauts SRombauts added the bug label Aug 15, 2024
@SRombauts
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

Acren commented Aug 15, 2024

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
Copy link
Owner

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
Copy link
Author

Acren commented Sep 3, 2024

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

@SRombautsU
Copy link
Collaborator

SRombautsU commented Oct 17, 2024

Yes I received your logs through the support team, thanks!
I couldn't dedicate time to investigate on this until now, sorry about that.

I'll see if I can provide a refactor of how the plugin handle Locks, and improve the context menu

Related: Fix lock cache excessive calls

@Acren
Copy link
Author

Acren commented Oct 19, 2024

Much thanks for that, from some initial testing it helps dramatically!

@SRombauts
Copy link
Owner

Thanks for letting me know!
I really wish I could have taken the time earlier, but we were all focused on Unite and Unity 6 launch in the past months.
Now we have Unreal 5.5 and Fab.com launch in front of us 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants