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

C#: Fix NodePaths completion error for not calling from main thread #78928

Merged
merged 1 commit into from Jul 3, 2023

Conversation

neikeq
Copy link
Contributor

@neikeq neikeq commented Jul 2, 2023

The node API we use for code completion changed and no longer allows being called from threads other than the main one.

Fixes #78913

I don't know if awaiting the next frame takes longer with low processor mode. It may be better to implement an awaitable version of CallDeferred.

The node API we use for code completion changed and no longer allows
being called from threads other than the main one.
@neikeq neikeq added this to the 4.1 milestone Jul 2, 2023
@neikeq neikeq requested a review from a team as a code owner July 2, 2023 01:50
@@ -385,9 +385,12 @@ private static async Task<Response> HandleCodeCompletionRequest(CodeCompletionRe
// However, it doesn't fix resource loading if the rest of the path is also case insensitive.
string scriptFileLocalized = FsPathUtils.LocalizePathWithCaseChecked(request.ScriptFile);

// The node API can only be called from the main thread.
await Godot.Engine.GetMainLoop().ToSignal(Godot.Engine.GetMainLoop(), "process_frame");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit hacky but we could wrap this line in an if so it only awaits when autocompleting NodePaths.

Also nit-pick, we should probably avoid calling GetMainLoop twice. And the signal name can use the exposed StringName.

Suggested change
await Godot.Engine.GetMainLoop().ToSignal(Godot.Engine.GetMainLoop(), "process_frame");
if (request.Kind == CodeCompletionRequest.CompletionKind.NodePaths)
{
var mainLoop = Godot.Engine.GetMainLoop();
await mainLoop.ToSignal(mainLoop, Godot.SceneTree.SignalName.ProcessFrame);
}

By the way, since the signal is in SceneTree and not MainLoop, this would block forever if the main loop is not a SceneTree. Although I don't think the main loop would ever change in the Godot editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want the method to block waiting for the completion. That's probably why it was using Task.Run before. So, I just decided to always await the signal, even if not needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, then maybe we could await Task.CompletedTask in the else block.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to merge. The current PR implementation should fix the issue and my previous comments can be addressed later.

@akien-mga akien-mga merged commit e044e13 into godotengine:master Jul 3, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Exception when sending code completion message for NodePath via GodotTools.IdeMessaging (C#)
3 participants