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

Add Input Action and NodePath completion #102

Merged
merged 22 commits into from
Jun 28, 2023

Conversation

jsbeckr
Copy link
Contributor

@jsbeckr jsbeckr commented Jun 25, 2023

I implemented input action and NodePath completion by using https://github.com/godotengine/godot/tree/master/modules/mono/editor/GodotTools/GodotTools.IdeMessaging (it's MIT). I had to copy the code from the Godot repo, because the nuget version is outdated. But that would be just a temporary thing, because Godot plans to publish proper nuget packages for communicating with the Godot IDE in the future. Once that's done we would integrate them and remove the GodotTools.IdeMessaging code and assembly from the plugin.

TODO

  • Account for script placement in scene tree
  • Indicator about the status of the connection to the Godot IDE

Possible Improvements:

  • Refactor ResourcePath completion to use the IdeMessaging as well
  • (maybe) Implement ScenePaths completion
  • (maybe) Implement PlayRequests / OpenFileRequests via IdeMessaging

NodePath Completion

Screen.Recording.2023-06-25.at.19.36.59.mov

Input Action Completion

Screen.Recording.2023-06-25.at.19.37.34.mov

P.S.: I signed the contributor agreement.

@jsbeckr
Copy link
Contributor Author

jsbeckr commented Jun 25, 2023

Closes #86

@jsbeckr jsbeckr marked this pull request as draft June 25, 2023 21:09
@jsbeckr
Copy link
Contributor Author

jsbeckr commented Jun 26, 2023

NodePath completion should work now exactly how it works in VS Code and Godot Editor. It queries against the currently open scene and accounts for relative script location in the scene tree.

@jsbeckr jsbeckr marked this pull request as ready for review June 26, 2023 07:10
@van800
Copy link
Collaborator

van800 commented Jun 26, 2023

Thank you! I will take a look shortly.
Mentioning @gipsond as he might have considerations.

@van800
Copy link
Collaborator

van800 commented Jun 26, 2023

What are options for adding automated tests for this completion?
It can only be tested with GodotEditor running, right?

@van800 van800 self-requested a review June 26, 2023 09:47
@jsbeckr
Copy link
Contributor Author

jsbeckr commented Jun 26, 2023

What are options for adding automated tests for this completion? It can only be tested with GodotEditor running, right?

I was thinking about that as well. And you are correct the Godot Editor has to run in order to accept connections from Rider. We could mock the Editor part but that would only accomplish testing the mock implementation imho. The only part that would be interesting to test would be that the correct methods are called in GodotMessagingClient depending on the caret position in a C# script. But tbh I have no clue how to implement that... I would love to see a bit more love for the Rider/Resharper SDK documentation in general :D

@van800
Copy link
Collaborator

van800 commented Jun 26, 2023

What are options for adding automated tests for this completion? It can only be tested with GodotEditor running, right?

I was thinking about that as well. And you are correct the Godot Editor has to run in order to accept connections from Rider. We could mock the Editor part but that would only accomplish testing the mock implementation imho. The only part that would be interesting to test would be that the correct methods are called in GodotMessagingClient depending on the caret position in a C# script. But tbh I have no clue how to implement that... I would love to see a bit more love for the Rider/Resharper SDK documentation in general :D

I would be happy even without those tests for now. I just wanted to clarify.

@van800 van800 self-requested a review June 26, 2023 10:59
also avoid creation GodotMessagingClient for non-Godot projects
@Intrivus
Copy link

Wow, this is so cool

@van800 van800 self-requested a review June 28, 2023 07:19
@van800
Copy link
Collaborator

van800 commented Jun 28, 2023

@jsbeckr Is there anything else, which you want to accomplish within this PR?

@jsbeckr
Copy link
Contributor Author

jsbeckr commented Jun 28, 2023

@jsbeckr Is there anything else, which you want to accomplish within this PR?

Nope! I am super happy with this! 👍

@Intrivus
Copy link

Closes #86

👍

@van800 van800 merged commit 6bf4a82 into JetBrains:net232 Jun 28, 2023
2 checks passed
@van800
Copy link
Collaborator

van800 commented Jun 28, 2023

@jsbeckr some of your commits were from a different git user, I have fixed that and force-pushed.

@van800
Copy link
Collaborator

van800 commented Jun 28, 2023

Plugin is published - 2023.2.121.

@geowarin
Copy link

geowarin commented Jun 28, 2023

Great job guys, I love this!

Testing this with the master version of godot (rider EAP 6) has mixed result:

Sometines, When I open a C# script from Godot, it seems that rider can't connect to godot and I get a null reference when asking for a completion (cannot reproduce right now, I will send the stacktrace when I get a new one).

I could not find the icon indicating the status of the connection to Godot...

This error goes away if I restart rider.


Input completions work great 👍


However, for NodePath completions I get errors in the Godot console:

Caller thread can't call this function in this node (/root/@EditorNode@17639/@Control@697/@Panel@698/@VBoxContainer@706/@HSplitContainer@709/@HSplitContainer@717/@HSplitContainer@725/@VBoxContainer@726/@VSplitContainer@728/@VSplitContainer@753/@VBoxContainer@754/@PanelContainer@799/MainScreen/@CanvasItemEditor@10089/@VSplitContainer@9914/@HSplitContainer@9916/@HSplitContainer@9918/@Control@9919/@SubViewportContainer@9920/@SubViewport@9921/TrollSlasher). Use call_deferred() or call_thread_group() instead.

This is probably linked to the new Threading model in godot 4.1 (see godot#78149)

Did any of you test this with the 4.1 RC version of Godot?

@van800
Copy link
Collaborator

van800 commented Jun 28, 2023

I was using Godot 4.0.3

@jsbeckr
Copy link
Contributor Author

jsbeckr commented Jun 28, 2023

The icon is in the top right next to the build and run icon. Maybe it's only visible with Next UI enabled?

The exception that you see should be an internal problem of Godot. We just connect via TCP and send a message. I'm not not at home right now so it's hard to test for me. Would be a shame if it's not working for 4.1.

@geowarin
Copy link

geowarin commented Jun 28, 2023

Yes I see the icon, I was expecting it to be on the bottom panel for some reason 😄

Might be a godot bug, the new threading model has been causing a few issues here and there, maybe the interaction with the IDE messaging has been undetected before.

@van800
Copy link
Collaborator

van800 commented Jun 28, 2023

The connection indicator should work in both new/old UI. I have tried that.

@jsbeckr
Copy link
Contributor Author

jsbeckr commented Jun 28, 2023

I guess we have to open a bug for that. Indeed NodePath completion does not work in 4.1 right now. Curiously enough it's not Godot (I think). During development of this feature I build the plugin to use it in my current Rider 2023.1.3 installation.

With that version I can still get NodePath completion. So in my opinion we introduced the problem during the refactoring (but that only affects 4.1 for whatever reason) or it has something to do with the Rider EAP 2023.2 in combination with Godot 4.1. Which also sounds weird. ¯_(ツ)_/¯

@van800
Copy link
Collaborator

van800 commented Feb 27, 2024

Tried it today again with Godot 4.2.1 and Rider 2023.3.3. It works fine. Make sure, GodotEditor is running and eyes of the Godot icon in the Rider toolbar is opened.
image

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

Successfully merging this pull request may close these issues.

None yet

5 participants