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

Improve the editor template workflow for users and devs #53957

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

fabriceci
Copy link
Contributor

@fabriceci fabriceci commented Oct 18, 2021

Co-Authored-By: jmb462 jmb462@gmail.com

This PR attempts to improve/rewrite the script template workflow to make the usage as easy/smooth as possible.

Closes godotengine/godot-proposals#3365

fix #36605
fix #28799
fix #44534
fix #46436
close godotengine/godot-proposals#1899

1_before-after

What are the main changes?

  • Built-in templates are in external file instead of hard-coded c++, they are no more mixed with user templates and can be updated
  • Templates are now linked to a node, when you create a script for a node, you will only see the templates for this node, or one of its parents.
  • Simplification and cleaning.

How it can be used by core dev

The original motivation was to provide a more relevant template for CharacterBody nodes in order to :
    - help to avoid the common error we see in issues (like forget to assign the result of move and slide in 3.x)
    - help to make the transition in 4.0 (act like a tutorial)
    - fast prototyping, you have a moving character in few seconds

default.template.mp4

How it works?

The template folders have the following structure

2_structure

To add a template simply create a file in the correct folder (template_path/node_type/file.ext), that's it.

By default:

  • the name of the template file will be the name of the file
  • the description will be empty
  • the space indent will be 4
  • the template will not be use as default

You can customize the template info with metas at the begning of your templates. Metas starts with the single-line comment syntax of your language:

# meta-name: Basic movement
# meta-description: Classic movement for gravity games (platformer, ...)
# meta-default: true
# meta-space-indent: 4

Example of description:

Screenshot 2021-10-18 at 14 40 43

It works exactly the same for built-in and user templates

Notes

  • When you create an editor plugin in c# you will have the basic structure for a plugin that I found in the doc. However there is a warning that pop up for now (I don't know why as I don't use c# & there was nothing before)
  • I wonder if the project template folder should not be fixed like the add-ons one (not changeable in the editor), to avoid the ifdef in gdscript.cpp

@fabriceci fabriceci added this to the 4.0 milestone Oct 18, 2021
@fabriceci fabriceci requested a review from a team October 18, 2021 12:56
@fabriceci fabriceci requested review from a team as code owners October 18, 2021 12:56
@Calinou
Copy link
Member

Calinou commented Oct 18, 2021

the space indent will be 4

As per the GDScript style guide, indentation should use tabs by default. Or is this about templates being written with spaces instead of tabs (to convert from spaces to tabs)?

Official script templates should be indented with tabs, still.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

@YuriSizov
Copy link
Contributor

YuriSizov commented Oct 18, 2021

Not sure from the changes if you handle this or not, but I think templates at the root of the template folder (like now) can be handled as Object or Node templates for backward compatibility.

PS. Also, did you call the two files basic_mouv on purpose? Looks like a typo, and we better use full qualified names in the engine anyway.

@fabriceci
Copy link
Contributor Author

As per the GDScript style guide, indentation should use tabs by default. Or is this about templates being written with spaces instead of tabs (to convert from spaces to tabs)? Official script templates should be indented with tabs, still.

The parser will replace the tab and 4 spaces by _TS_ (previously %TS%) by default. If the user want to use 2 spaces indent in the templates; he can pass the information in the parser with this meta. The result will look like this:

Screenshot 2021-10-18 at 15 26 00

In the end, the result of the indentation will be the one defined in the editor's configuration:

Screenshot 2021-10-18 at 15 23 37

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.

So is the replacement syntax changed from %BASE% to _BASE_? Is there a reason to use _ over %? It seems to me that _ could be conflicting with valid scripts.

modules/mono/editor_templates/plugin.cs Outdated Show resolved Hide resolved
modules/mono/editor_templates/plugin.cs Outdated Show resolved Hide resolved
@fabriceci
Copy link
Contributor Author

fabriceci commented Oct 18, 2021

So is the replacement syntax changed from %BASE% to _BASE_? Is there a reason to use _ over %? It seems to me that _ could be conflicting with valid scripts.

@raulsntos Yes, if you use % %syntax you will have errors in your editor (godot or external) when you open/edit the template as this syntax is not allowed in the language:

Godot:
Screenshot 2021-10-18 at 18 18 40
VS Code:
Screenshot 2021-10-17 at 11 37 20

To avoid this I changed the syntax to a valid one. As this syntax only concerns templates, I don't think is an issue, it seems to me the lesser evil.

Screenshot 2021-10-17 at 11 37 32

@raulsntos
Copy link
Member

raulsntos commented Oct 18, 2021

Good enough for me, I guess the chances of someone wanting to use a world like _CLASS_ in the output of the template are low.

@fabriceci
Copy link
Contributor Author

I thought about doubling the underscore at one point __BASE__, then I thought that one was enough. But if anyone has a better idea, I'll take it.

@jmb462
Copy link
Contributor

jmb462 commented Oct 18, 2021

Not sure from the changes if you handle this or not, but I think templates at the root of the template folder (like now) can be handled as Object or Node templates for backward compatibility.

I'm working with @fabriceci on this PR.
Following your comments, I've started implementing this, but I realized that it brings more problems than it solves.

  1. If we consider old templates in old root template directory as Object derivated, they will be shown for all node even if they are not suitable to that node.

The new system ensure that proposed templates can be applied for this node and should not include non existing methods for the node selected.

  1. Old template uses % delimited tags which need to be translated.

  2. I think very few people are using the old user template. Those could really easily convert their templates ( % => _ and add # meta information if needed) and put it in folders that is suitable for their templates.

@YuriSizov
Copy link
Contributor

YuriSizov commented Oct 18, 2021

If we consider old templates in old root template directory as Object derivated, they will be shown for all node even if they are not suitable to that node.

They are suitable for all nodes though, just not all objects. Which is why I mentioned both. But since the syntax for placeholders was changed, I guess this is already a compatibility breaking change.

@jmb462
Copy link
Contributor

jmb462 commented Oct 18, 2021

They are suitable for all nodes though, just not all objects. Which is why I mentioned both. But since the syntax for placeholders was changed, I guess this is already a compatibility breaking change.

What I mean is if someone wrote a template suitable for all Control nodes (template include method calls from Control class) with the old system in the old root directory.

With the new system, we put it in a Control subfolder and it won't be proposed for Node2D or Node3D derivated nodes.

If this template is left in the root template directory and considered to be suitable for all Object, it will kill one of the avdandage of the new system (if a template is proposed, it is suitable).

@aaronfranke aaronfranke self-requested a review October 18, 2021 19:28
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Amazing work!

I've left some comments about the physics templates and the general API in core, I'm not very familiar with specific script implementations.

core/object/script_language.h Outdated Show resolved Hide resolved
core/object/script_language.h Outdated Show resolved Hide resolved
core/object/script_language.h Outdated Show resolved Hide resolved
core/object/script_language.h Outdated Show resolved Hide resolved
core/object/script_language.cpp Outdated Show resolved Hide resolved
@fabriceci fabriceci requested review from a team as code owners October 21, 2021 16:02
@KoBeWi
Copy link
Member

KoBeWi commented Jan 1, 2022

The dialog feels really cluttered now. I'd add separators here:
image
Also the Template dropdown could get disabled instead of disappearing when unchecking the "Use Template".

Also² you could add this template for EditorScript:

# meta-description: Basic editor script template
@tool
extends EditorScript

func _run():
    # Called when the script is executed (using File -> Run in Script Editor).
    pass

Other than that all looks fine now (still need to check the code).

@jmb462 jmb462 force-pushed the new-template-workflow branch 5 times, most recently from 4b9a84f to 51e9047 Compare January 2, 2022 12:32
@jmb462
Copy link
Contributor

jmb462 commented Jan 2, 2022

The dialog has been reorganized. The template part is now more compact.
The dropdown doesn't disapear any more.
EditorScript template has been added for GDScript and C#.

dialog

// Update last used dictionaries
if (is_using_templates && !parent_name->get_text().begins_with("\"res:")) {
if (sinfo.origin == ScriptLanguage::TemplateLocation::TEMPLATE_PROJECT) {
// Save the last used template for this node into the project dictionnary.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Save the last used template for this node into the project dictionnary.
// Save the last used template for this node into the project dictionary.

Misspelled dictionary. Same below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed. Was a mix-up with the french spelling...

@KoBeWi
Copy link
Member

KoBeWi commented Jan 2, 2022

image
Disabled is redundant here; I see it's disabled.
It should say "Empty" or "No Template".

I get errors every time I open script create dialog:
image

@jmb462
Copy link
Contributor

jmb462 commented Jan 2, 2022

image Disabled is redundant here; I see it's disabled. It should say "Empty" or "No Template".

I get errors every time I open script create dialog.

Fixed. A test was missing before the first creation of the dictionary.

Co-Authored-By: jmb462 <jmb462@gmail.com>
@akien-mga akien-mga merged commit 98b3ba1 into godotengine:master Jan 3, 2022
@akien-mga
Copy link
Member

Thanks!

jynus added a commit to jynus/godot-docs that referenced this pull request Jul 23, 2022
The script templates page wasn't updated after godotengine/godot#53957
The templates are now per node type and folder organization has changed.

A few behaviours and configuration have also changed. Documenting based
on current editor behaviour and source code HEAD.

Fixes godotengine#5887
jynus added a commit to jynus/godot-docs that referenced this pull request Jul 24, 2022
The script templates page wasn't updated after godotengine/godot#53957
The templates are now per node type and folder organization has changed.

A few behaviours and configuration have also changed. Documenting based
on current editor behaviour and source code HEAD.

Fixes godotengine#5887
@fabriceci fabriceci deleted the new-template-workflow branch August 26, 2022 14:32
rsubtil pushed a commit to rsubtil/godot-docs that referenced this pull request Apr 11, 2023
The script templates page wasn't updated after godotengine/godot#53957
The templates are now per node type and folder organization has changed.

A few behaviours and configuration have also changed. Documenting based
on current editor behaviour and source code HEAD.

Fixes godotengine#5887
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants