search

ephread / inkgd

Implementation of inkle's Ink in pure GDScript for Godot, with editor support.
MIT License
305 stars 33 forks source link

Added template and autoload on plugin activation #28

Closed greencloversguy closed 2 years ago

greencloversguy commented 3 years ago

Checklist for this pull request

Description

The plugin now will add the "Ink Template" template to the project the plugin is activated in. It will also create the "__InkRuntime" autoload. I have done some small tests, I would like to do some more but I haven't used GUT before, I need to familiarise myself with it first. However I have tested that the template is generated and that it works, and I've tested that the autoload is added and removed when the plugin is activated.

For the template, I tried to simplify the "ink_runner.gd" script so the code could act more as something to be built on, This meant removing the choice_container and references. Now the script will run the exported ink file and print it into the console, with the first option being chosen each time. I think this will make it easier for new users to get used to how to use the code before moving onto the new code.

This is actually the first code-based pull request I've done, so I'd appreciate any feedback.

One thing to note, a lot of files as recorded as changed, however I think this is because it converted spaces into tabs due to my editor settings. Let me know if there's a set way to handle this in future.

ephread commented 3 years ago

Fantastic, thanks for creating the PR!

The plugin now will add the "Ink Template" template to the project the plugin is activated in. It will also create the "__InkRuntime" autoload.

We briefly touched the topic on Discord, but we didn't talk about the purpose of autoloading __InkRuntime (I'm not opposed to it, I'm just trying to wrap my head around it). Is it a first step to get runtime support in the editor? Or do you have something else in mind?

However I have tested that the template is generated and that it works, and I've tested that the autoload is added and removed when the plugin is activated.

I'm not too sure how to go about it, I don't think GUT can test editor stuff. A couple of tests can probably be created to check that the template works, though. This can be done later in a separate PR.

For the template, I tried to simplify the "ink_runner.gd" script so the code could act more as something to be built on, This meant removing the choice_container and references. Now the script will run the exported ink file and print it into the console, with the first option being chosen each time. I think this will make it easier for new users to get used to how to use the code before moving onto the new code.

Good call 👍

This is actually the first code-based pull request I've done, so I'd appreciate any feedback.

No worries! I glanced at the PR and I'm going to comment on a few things, but I'll give it a full review in the coming week. I might be a bit nitpicky, so bear with me!

One thing to note, a lot of files as recorded as changed, however I think this is because it converted spaces into tabs due to my editor settings. Let me know if there's a set way to handle this in future.

I've been pondering converting the codebase to tabs since that's what GDScript projects tend to use. I can't remember what I used spaces in the first place, but it will have to be reverted as part of this PR to keep the codebase as consistent as possible. Then we'll do a sweep one day to convert everything.

Anyway great stuff.

greencloversguy commented 3 years ago

Thanks for the feedback. I'll have a look through and update the code. I'll respond to code comments, but here I'll cover one of the points about the Autoload.

We briefly touched the topic on Discord, but we didn't talk about the purpose of autoloading __InkRuntime (I'm not opposed to it, I'm just trying to wrap my head around it). Is it a first step to get runtime support in the editor? Or do you have something else in mind?

So this method does NOT add InkRuntime to the editor, so it's not about runtime support. The code I've added has made it so that just by activating the plugin, the InkRuntime singleton is added with Autoload without the user having to do it themselves, like is suggested here. This means the user doesn't need to use extra code or add the Singleton with Autoload themselves, making a better experience.

ephread commented 3 years ago

Thanks for the feedback.

No worries, thanks for contributing and bearing with my comments. 😅

So this method does NOT add InkRuntime to the editor, so it's not about runtime support. The code I've added has made it so that just by activating the plugin, the InkRuntime singleton is added with Autoload without the user having to do it themselves, like is suggested here. This means the user doesn't need to use extra code or add the Singleton with Autoload themselves, making a better experience.

Okay, perfect. I hadn't realised that add_autoload_singleton actually added an entry in the project's Autoloads. I thought it only worked within the context of the editor. Thanks for the clarification!

ephread commented 3 years ago

@greencloversguy do you want me to continue the review? I'm asking because the PR is a draft now, but I'm requested to review. Let me know!

ephread commented 3 years ago

@greencloversguy FYI I've changed the target branch, so you won't get tons of conflicts. I updated GUT and turned spaces into tabs (great suggestion by the way) in the main branch. Feel free to change the target branch back to main and fix those conflicts if you want to, otherwise I'll make the final merge after this PR is approved.

greencloversguy commented 3 years ago

Heya. Again, I need to get some time to get my head around some of the Github workflow and terms. I've set it as a draft because I wanted to make sure some of the changes, such as adding in the new template, were OK first, as I added another commit.

ephread commented 3 years ago

Hey @greencloversguy! Are you still working on this, do you need me to do anything? 🙂

greencloversguy commented 3 years ago

Hello @ephread, there are still some things I want to add to it, but I think that this version is ready to be merged at the moment. Sorry for the delay, but I'm sure you understand!

ephread commented 2 years ago

Wow, didn't see this PR go from draft to ready for review eight months ago. I'm merging this and will clean up the conflicts in a separate PR. Thanks for your work!