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

Open and run the project in the Godot editor. #40

Closed
wants to merge 1 commit into from

Conversation

francogarcia
Copy link
Contributor

Hello, @NathanLovato,
How are you?

A long time ago, I was waiting for LSP support in Godot to update my Emacs major
mode. LSP support has recently been added to 3.2; I was about to work on it,
when I found your new mode mentioned in
emacs-lsp/lsp-mode#1377.

You have submitted your package to Melpa as well. Thanks for your work and well
done!

From what I can tell, your mode is almost at parity with mine. Some of your open
issues are features that I had implemented long ago; I would be glad to share
them with you.

Please check if the PR fulfills these requirements:

  • The commit message follows our guidelines.
  • For bug fixes and features:
    • You tested the changes.
    • You updated the docs or changelog.

I have not read the code style guidelines yet.
Also, I had implemented the changes before seeing this pull request template, so I have not updated the changelog.

Related issue (if applicable): Closes #11 and #13

What kind of change does this PR introduce?

This pull request allows Emacs to run scenes and scripts in Godot. It should
close Issues #11 and #13.

Does this PR introduce a breaking change?

It does not change the original API; therefore, I suppose not.

New feature or change

What is the current behavior?

The programmer manually starts an instance of Godot engine to run her/his
code.

What is the new behavior?

The main features are the interactive functions added to the gdscript-mode-map
to start an instance of Godot engine and do something with it (open the editor,
run a scene, or run a script).

Other information

As I am Evil user, the keybinds probably do not make any sense. Feel free to
change them (or tell me how you would prefer them). For instance, a group for
the commands could be an improvement.

Currently, you have to close the instance after it is running. It could be
changed to close this instance automatically before running a new one.

However, there may be a better alternative as I have not check LSP in detail
yet. If it allows to send commands to control the Godot editor, a better
approach could be keeping an instance running at all times, then just tell Godot
to re-run the code whenever it is desired.

Regarding Issue #6, I have a repository of snippets available at
https://github.com/francogarcia/yasnippet-godot-gdscript. If you are
interested, I can update them.

Best regards,
Franco

@NathanLovato
Copy link
Collaborator

Your contribution is much appreciated!

Note for contributions like these, if your PR solves a specific issue, you don't have to detail your changes too much. Especially if the code is self-explanatory.

I wouldn't put that code in 'gdscript-utils, as it has a specific purpose. How about a new package named gdscript-godot, a place where we could put all godot-related commands and code?

Regarding the keystrokes, I'm also using evil so I don't know what to use. We can leave it as-is for now and ask emacs users to make changes.

Regarding code style, the style guide we use is the one required for MELPA.

Mainly, in lisp, -- means private. Interactive functions are public, so they should never have two hyphens. For instance, gdscript--run-godot-editor -> gdscript-run-godot-editor.

I would also rename the user commands so they have the same prefix, making them easier to find with M-x: gdscript-run-godot-editor -> gdscript-godot-run-editor

I can take care of the style adjustments and naming if you want.

I'll just test your code later.

@NathanLovato
Copy link
Collaborator

Regarding Issue #6, I have a repository of snippets available at https://github.com/francogarcia/yasnippet-godot-gdscript. If you are interested, I can update them.

Nice! I actually have some here as well that support conditional type hints.

I haven't made a public package for them yet because with the language server, the workflow and which snippets are useful changes a bit. Except for boilerplate code, I don't get to use them that much.

Taking a quick look at them:

  • Some snippets are now not necessary with the language server support for GDScript (it already works with a nightly build of Godot 3.2.2, it's coming in the next 3.2.x release).
  • Some follow a specific workflow or code style, like class, but also scene_load.

So for now I'd leave this on hold. We could maybe write down a list of snippets that have proven most useful working on Godot projects in #6, and create a package with these? What do you think?

@NathanLovato
Copy link
Collaborator

I've started reviewing and working on the changes - there are quite a few style changes required and linting errors.
I'll merge the PR manually and let you know when your features are in.

@francogarcia
Copy link
Contributor Author

Hello, @NathanLovato,
How are you?

I apologize, I have not seem your message in time.

I have followed the previous comments and updated the commit. Please consider using the updated version if you can.

I have just noticed I have to rebase it against master first, though.

Best regads
Franco

@NathanLovato
Copy link
Collaborator

Done, merged manually in beac587

Thanks for your contribution! You can see the code style changes and refactor in fa26dd5

@tavurth
Copy link
Contributor

tavurth commented Apr 28, 2020

This PR is super helpful! Thank you @francogarcia!

Have found it to be very useful in spawning another instance of a game to test multiplayer side-by-side.

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.

Open and run the project in the Godot editor
3 participants