godotengine / godot-vscode-plugin

Godot development tools for VSCode
MIT License
1.52k stars 149 forks source link

Implement headless LSP mode #488

Closed ryanabx closed 10 months ago

ryanabx commented 1 year ago

Implements https://github.com/godotengine/godot-proposals/issues/7558.

The code adds a new config property, runAtStartup, which is an enum:

none - Default value, the normal behavior users of the plugin expect. editor - Automatically opens the editor whenever the extension would normally start the connection to the GDScript language server lsp3, lsp4 - Automatically opens the language server whenever the extension would normally start the connection to the GDScript language server.

The code also adds a button to the error when a GDScript language server cannot be found, called "Run Automatically". Pressing this button will provide the options shown above to the user with an explanation of what they are, then restart the connection process.

DaelonSuzuka commented 11 months ago

That's... actually a great idea, thanks for the PR!

I'll try to make some time to review it this weekend.

ryanabx commented 11 months ago

That's... actually a great idea, thanks for the PR!

I'll try to make some time to review it this weekend.

Oh, hey! Long time no see, I've been following your work in #452, and eagerly awaiting that pr hehe

DaelonSuzuka commented 11 months ago

Random thought, does the editor have a command line argument to specify the LSP port? If it does, then we can solve some issues that I was considering intractable.

And yeah, the debugger PR has been a huge pain. I'm hoping I make some progress this weekend.

ryanabx commented 11 months ago

Random thought, does the editor have a command line argument to specify the LSP port? If it does, then we can solve some issues that I was considering intractable.

If not, I can see about making a PR to do that.

And yeah, the debugger PR has been a huge pain. I'm hoping I make some progress this weekend.

Good luck!

DaelonSuzuka commented 11 months ago

Damn, there isn't a command line argument for picking the LSP port. If there was, it would let us solve a BUNCH of issues with the LSP. Currently, it's not really great to have multiple projects open because the extension has no way to know which editor instance it's talking to. This obviously causes a bunch of problems with errors and completions.

If we could specify the port via CLI, then we could skip trying to connect to an open editor at all, and just launch our own headless Godot process with the LSP on a random high port. Each open workspace could get its own LSP that we know is looking at the correct project. It would also mean that if when the LSP crashes, it wouldn't impact the user's open editor, and the extension could just kill and restart it.

This would also let us sidestep the whole 6005/6008 issue, except maybe as a fallback for there not being a Godot path set or something.

If not, I can see about making a PR to do that.

@ryanabx I have a couple ideas for terrible hacks that I want to test before we try getting engine changes committed. I'd strongly prefer to implement this in a way that works with what users already have, instead of forcing them to update.

That said, it's probably an easy item to implement and I don't see why they'd object to it, so don't like me dissuade you from starting.

While you're digging around in there, try taking a look at the LSP itself. There are a number of known issues with the LSP that I haven't been able to work around from the client side, and it would help a lot of people if we could start to make progress on those.

Calinou commented 11 months ago

Damn, there isn't a command line argument for picking the LSP port.

We (or you) can add that to the editor :slightly_smiling_face:

ryanabx commented 11 months ago

Damn, there isn't a command line argument for picking the LSP port.

We (or you) can add that to the editor 🙂

Doing you one better, we can probably cherry pick it back to 3.x as well, correct?

Calinou commented 11 months ago

Doing you one better, we can probably cherry pick it back to 3.x as well, correct?

Sure :slightly_smiling_face:

atirut-w commented 11 months ago

Or we can read the editor configs?

ryanabx commented 11 months ago

Or we can read the editor configs?

We want to manually specify the language server port, regardless of editor setting. This makes it easier for us to connect to the language server without requiring the user to know what port it's running on (the default ports vary from 3.x to 4.x)

atirut-w commented 11 months ago

Right, and I had a minor brain fart there, too. Settings that are at their default values are not saved, right?

ryanabx commented 11 months ago

Right, and I had a minor brain fart there, too. Settings that are at their default values are not saved, right?

Ah, I see what you mean now, you're saying we should read it from the user directory, or wherever editor settings are saved on the disk.

Default values aren't saved in the editor settings, no. And I'd be hesitant to rely on the user settings anyways. Ideally the only input we want from the user is the path to the godot exe.

DaelonSuzuka commented 11 months ago

I just built a prototype that writes to the editor settings to try and bypass needing a command line flag for the LSP port. This idea was roughly:

Unfortunately, when I restored the original editor settings, the headless Godot noticed the file change, reloaded its settings, and changed its LSP port back to the original. That makes this a dead end.

Another idea was to provide a fake editor settings file to each headless instance, but there's no way to do that either. I don't understand how there's no flag for that because it seems like it would be instrumental in testing the damn editor, but here we are.

ryanabx commented 11 months ago

Unfortunately, when I restored the original editor settings, the headless Godot noticed the file change, reloaded its settings, and changed its LSP port back to the original. That makes this a dead end.

Yeah, I noticed that in the code earlier today. It'll emit a NOTIFICATION_SETTINGS_CHANGED or something like that and the language server hooks into that to check if the port or host changed.

Another idea was to provide a fake editor settings file to each headless instance, but there's no way to do that either. I don't understand how there's no flag for that because it seems like it would be instrumental in testing the damn editor, but here we are.

That's an idea. I was thinking about a command line argument to specify an arbitrary number of overrides to the editor settings but that might be something to consider. It might be easier to specify a path for a separate editor settings file.

EDIT: The alternate editor file should probably be a list of overrides to the general user settings. The reason for this is that some people have certain editor settings changed that they'd want to keep. For instance, I changed my editor's text_editor/completion/idle_parse_delay to 1.0 seconds instead of 2.0

turbohz commented 11 months ago

Hi.

I have a setup like this working, and works well. I have a Godot Server with some modifications, one of which implements a "--lsp-port" command line argument.

I'm not very experienced using C++, and I didn't think many people was interested on such feature, so I never tried to get this upstreamed.

Anyway, if anyone is interested, take a look. https://github.com/turbohz/godot/commit/ad0252752f137c1f6df01194f3c03aac5b88c8f5

(This is for Godot 3.x)

DaelonSuzuka commented 11 months ago

EDIT: The alternate editor file should probably be a list of overrides to the general user settings.

From a general purpose/API perspective, there's no real excuse to not have both behaviors (replace and override) available if the user wants them, but for this LSP situation I agree that we just need an additional layer of override.

ryanabx commented 11 months ago

Hi.

I have a setup like this working, and works well. I have a Godot Server with some modifications, one of which implements a "--lsp-port" command line argument.

I'm not very experienced using C++, and I didn't think many people was interested on such feature, so I never tried to get this upstreamed.

Anyway, if anyone is interested, take a look.

Hey! I'll definitely be looking this over and I'll see about making a PR.

I had an in-progress branch on my own godot fork and couldn't think of the best way to override the lsp port, I didn't think about using a static property!

DaelonSuzuka commented 11 months ago

I made some changes to the user-facing API and simplified the new setting to a bool.

I also refactored the internals to cut down the number of code paths and to not use open_workspace_with_editor() or run_editor(), which are entirely too complicated for what they do.

Things left to do:

ryanabx commented 11 months ago

I made some changes to the user-facing API and simplified the new setting to a bool.

I also refactored the internals to cut down the number of code paths and to not use open_workspace_with_editor() or run_editor(), which are entirely too complicated for what they do.

Thanks! I'll look over those changes later, at the gym right now :p

Are you active on the godot contributors chat? If not you should be!

DaelonSuzuka commented 11 months ago

its_working.anakin.gif

Random port headless LSP is working using godot master branch.

The "can't connect" dialog and resulting workflow is the major thing left to do. I want the extension to correctly and as seamlessly as possibly attempt to launch its own unique headless LSP(requires --lsp-port), then fallback to the open editor, then fallback to launching a non-unique headless LSP(current and older versions of Godot). I believe there's a path that does the right thing in almost every situation, including checking both default port values.

@Calinou what's the process for making sure https://github.com/godotengine/godot/pull/81844 gets cherry picked to 3.x?

proof of life:

firefox_tFkwhZKfDB

ryanabx commented 11 months ago

@Calinou what's the process for making sure https://github.com/godotengine/godot/pull/81844 gets cherry picked to 3.x?

proof of life:

firefox_tFkwhZKfDB

Nice work! If it helps, I created a cherry pick PR for the feature in 3.x just now: http://github.com/godotengine/godot/pull/82025

DaelonSuzuka commented 11 months ago

Nice work! If it helps, I created a cherry pick PR for the feature in 3.x just now: https://github.com/godotengine/godot/pull/82025

Awesome, thank you! I just wanted to make sure it didn't get forgotten.

DaelonSuzuka commented 10 months ago

Headless LSP mode works. It has an error handling workflow that will hopefully prevent trying to launch a headless LSP with a wrong/old version of Godot specified in editorPath.godot3/4.

I also snuck in a fix for #373 (bad formatting in hovers), and a feature improvement to allow right clicking on type-hinted variables and opening the godot docs for them. (these were both LSP-adjacent)