godotengine / godot-docs

Godot Engine official documentation
https://docs.godotengine.org
Other
3.95k stars 3.23k forks source link

Creating a more complete GDScript Style Guide #2997

Closed Scony closed 4 years ago

Scony commented 4 years ago

order of class definitions should be agreed and mentioned in the proper docs section.

It may be either fine-grained like e.g.:

- tools
- extends
- classnames
- enums
- consts
- signals
- exports
- onreadypubvars
- pubvars
- onreadyprvvars
- prvvars
- others

or coarse-grained like e.g.:

headers (tool, extend, classname)
definitions (enums, exports, const etc.)
publics
privates

but debate is needed

winston-yallow commented 4 years ago

I am in favor of the coarse grained order. It would allow code like this:

enum COLOR {BLUE, RED, GREEN}
export(COLOR) var outline_color

enum SHAPE {TRIANGLE, SQUARE}
export(SHAPE) var debug_shape

I think when an enum is used only for one (not multiple) exports it helps to have it in the line above the export. I feel like it is much more readable to group them by usecase rather than by type.

(just adding my point I made on discord here for reference)

TheDuriel commented 4 years ago

https://gist.github.com/TheDuriel/e34278a84ea163858d2d5e578b666dbc

NathanLovato commented 4 years ago

We've worked on a GDScript style-guide based on the GDScript styleguide from the docs this year with the team. It puts readability and accessibility first, and the information users of a given class need first. We've been using it and improving throughout the year.

Below is the quick outline. Here is the complete style guide. In short, it puts properties at the top, public before private, node dependencies before variables, and initialization functions and callbacks before other built-in callbacks:

01. extends and class_name
02. """docstring"""

03. signals
04. `onready` variable (node dependencies)
05. enums
06. constants
07. exported variables
08. public variables
09. private variables

10. optional built-in virtual `_init` method
11. built-in virtual `_ready` method
12. signal callbacks
13. remaining built-in virtual methods
14. public methods
15. private methods
16. static functions

We'd like to suggest it as:

  1. It's designed to make the code easy to read and understand what's happening, so it works well for teaching.
  2. It's the result of teamwork. We've discussed it a lot, we use it with students, got feedback from professional developers as well. We've used it in our recent beginner platformer Godot course and it doesn't seem to cause issues even for programming beginners.
  3. It's designed for teamwork but works well for individual work as well.

We discussed each decision thoroughly, so @razcore-art and I are here to clear any doubts or explain every single detail you might have questions about.

NathanLovato commented 4 years ago

In general, I'd be in favor of having polished and detailed docs for an official style guide so we can also have a precise code formatter, higher quality standards for code examples in the docs and in official demos, and help save everyone time.

I don't necessarily want our style to be the standard, I'm open to any style that people put a lot of thought into. Regardless, we're there to help write the actual documents anytime.

winston-yallow commented 4 years ago

The biggest problem I have with your styleguide is with the onready variables. I prefer to put them to the bottom directly before the first method. My reasons are:

  1. It allows me to use other variables (for example exports) to initialise the onready vars.
  2. It groups them visually closer together with _init() and _ready(). This way it is easy to see what happens on ready in one place.
  3. I see onready vars mostly as private. This way they would be grouped together with the other private vars.

What are the reasons why you propose them to follow the signals? I currently do not see many reasons for this so I would be interested in your thoughts.

winston-yallow commented 4 years ago

Another point: As enums can be used as export hints I would prefer them to be directly above the exports instead of below.

NathanLovato commented 4 years ago

Our styleguide is a living document, it's open for improvements! The enum makes sense, I'll update that, thanks.

What are the reasons why you propose them to follow the signals? I currently do not see many reasons for this so I would be interested in your thoughts.

The thing we called "onready variables" in our code are node dependencies that we cache upfront for the most part:

onready var cooldown: Timer = $Cooldown
onready var flash: Particles = $FlashEffect

Things like these. These are nodes the class depends on to function properly, so we put them at the top. A bit like in other languages, you would have imports from other modules or files.

Onready being a keyword that has several uses, I think we should rename that point to cached node dependencies.

NathanLovato commented 4 years ago

When it comes to private and public, you can use onready for both. It depends on how you want to design your APIs. We give public access to some nodes and values that need to be initialized in our code. It depends a lot on the context, but we expose properties in general over using methods to encapsulate data.

I edited the order in the list above, and fixed the style guide

razcore-rad commented 4 years ago

The biggest problem I have with your styleguide is with the onready variables. I prefer to put them to the bottom directly before the first method. My reasons are:

  1. It allows me to use other variables (for example exports) to initialise the onready vars.

As Nathan points out, we mostly use onready vars with setting up node dependencies, although not always. My recommendation (for our team) is to initialize any private vars inside the _ready() function instead of using onready for this so that we don't break onready var grouping which visually makes a bit of a mess, in my opinion. onready being used mostly for setting up these node dependencies, we decided to only use them with "public" vars since you can always do child_node.get_node("Timer") for example, which could just be accessed via child_node.timer if onready var timer: Timer = $Timer is there. The reason they're at the very top is to give a quick first-glance of the node dependencies when opening a script, but I see your point about having them above _ready() to make it easy to see what's happening. More on this below.

I'm not sure how many people know this but, this:

extends Node2D

onready var a: int = b

export var b: = 7

func _ready() -> void:
    print(a, " ", b)

"just" works, because onready is really syntax sugar for running the initialization in _ready() so you can place it before/after any other declarations, doesn't matter if it's before or after export.

  1. It groups them visually closer together with _init() and _ready(). This way it is easy to see what happens on ready in one place.

I can see this being a good argument. At the moment I'm not really convinced one way or the other: first thing after signals - for quick glance at node dependencies VS. right above _ready() to group them visually closer. I can see the merit in both of these arguments.

  1. I see onready vars mostly as private. This way they would be grouped together with the other private vars.

Yeah, I guess I don't see them this way as explained in the first part. I see them mostly as glue-code, the rest goes in _ready(). I avoid placing too much functionality inside the onready var declaration & initialization, which you'd need if referencing some functions or other private vars form the code.

What are the reasons why you propose them to follow the signals? I currently do not see many reasons for this so I would be interested in your thoughts.

Becuase we use signals mostly through code (as opposed to declaring them via UI), it's our experience that, when designing signal connections, we need to have quick visual access to them since the connections themselves don't appear anywhere in the Node panel. The Node panel is by default grouped with the Inspector panel, hidden behind a tab and we all know that people rarely change defaults. Being hidden by default means some extra work to change between Inspector <-> Node panels to visually tell the signals and the connections so it made sense for us to have them declared at the very top.

Another point: As enums can be used as export hints I would prefer them to be directly above the exports instead of below.

Yeah, that's a good point, I'm pretty sure I noticed this at some point, but I forgot to update the docs.

NathanLovato commented 4 years ago

Agreed with Razvan, and I agree with

It groups them visually closer together with _init() and _ready(). This way it is easy to see what happens on ready in one place.

I can see this being a good argument.

winston-yallow commented 4 years ago

Thank you for your detailed answers!

I did not know you could use all variables in the onready variables, even if defined below. It makes sense from a technical side. However I think it can be confusing for reading the code. The brain would be confronted with a variable that it does not know about (yet). Maybe I am just too used to other languages than gdscript as I prefer to only use variables that are already known to me (when reading it from top to bottom).

Apart from that I really like the idea of seeing onready vars like node dependencies and therefore putting them on top. I think I can get used to writing only simple node dependencies as onready and doing other var initializations in _ready(). I can see how this leads to better understandable code. For a styleguide this sounds good to me :+1:

Feniks-Gaming commented 4 years ago

I have been using @NathanLovato guide for a while now but exactly as @winston-yallow says I modified it to have onready var as last just before _ready() function I feel apart of that one change it works really well creates consistent looking code and will enhance cooperation on other projects as more people adapt to one style.

What we really need is PEP-8 of Godot as many of our developers come from many different backgrounds and there is general lack of consistency even across tutorial scene for new users furthering the divide and confusion

aaronfranke commented 4 years ago

@NathanLovato: The thing we called "onready variables" in our code are node dependencies that we cache upfront for the most part:

It would be bad to put anything in the style guide that conflict with the official demo projects. For example, they use $Whatever everywhere with no caching. If caching node references is the preferred style, it should be used in the official demo projects first.

razcore-rad commented 4 years ago

That's not how real life works. The demo projects aren't, by definition, the source of truth. We're actually in the process of updating the demos because they have such poor quality code in them. See this for example: https://github.com/GDQuest/godot-kickstarter-2019/tree/master/platformer-2d-rework. We already discussed this with Remi & Juan.

The truth is that the demos are just bad examples of code and apart form demonstrating Godot features they are in need of some better programming. And better explanations since 99% of the code isn't commented in any way.

Feniks-Gaming commented 4 years ago

I agree issue here isn't that Demo projects conflict with style but that Demo projects don't have any style at all. Style should be agreed and then added to all demo projects using random $NodeName out of a blue inside a function is just teaching people bad habits.

NathanLovato commented 4 years ago

@aaronfranke I second what @razcore-art and @Feniks-Gaming say. The official demos don't have a unified style and teach poor programming practices for the most part. They were written by different persons over the years, including back when there weren't any guidelines or peer review.

More importantly, remaking all the demos and all examples in the official docs to fit a new code style is a job for multiple developers. We couldn't expect contributors to follow the same style doing so without a reference document and before having agreed upon a style.

Also, you need peer review on the guidelines first so you don't have people discussing the smallest decision in every PR because they personally prefer another syntax.

aaronfranke commented 4 years ago

My main concern is with something becoming a "standard" that nobody follows or maybe doesn't know about because it's not used anywhere in practice, or create a "do as I say, not as I do" situation.

Overall I disagree with it not being "how real life works", standards are best defined by what works well in practice, and it's useful to see whether or not style changes actually improve the readability of a given project. Since y'all have already been using your style for GDQuest, you know it works in your own use cases, but maybe others disagree. I would recommend that you submit pull requests to the demo projects repository, and ideally get feedback on them from the community.

I agree that the demo projects not currently having a consistent style is really bad, and I'm not saying that we should base a style guide off of their current style, they just need to be improved through specific changes and eventually moved toward uniformity and improved readability. If a guide is drafted, I would suggest at least some of the demo projects be updated first, which would allow users to give feedback on specific tangible changes to projects that would be caused by such a guide.

(Side note: We really need a dedicated maintainer for the demo projects repository. Most of the projects are still optimized for Godot 3.0, and there are PRs improving the projects and making them conform to the current style guide, but right now they're just sitting there... https://github.com/godotengine/godot-proposals/issues/172)

NathanLovato commented 4 years ago

My main concern is with something becoming a "standard" that nobody follows or maybe doesn't know about because it's not used anywhere in practice.

Making people follow the same guidelines is a job for the reviewers or leads in a team. It's the same problem with every project.

Since y'all have already been using your style for GDQuest, you know it works in your own use cases, but maybe others disagree

That's why we shared them with the community and we're having a discussion here. And no, it's not about our use cases only. We didn't open sourced them from the start to make them just for ourselves.

they just need to be improved through specific changes and eventually moved toward uniformity and improved readability

Again, how do you do that without people agreeing upon said changes? Do you really find the PRs in the godot-demo-projects repository consistent and moving the demos towards uniformity?

What happened since the start of the project is that the code was all over the place in the demos and the docs until we put up the GDScript style guide: http://docs.godotengine.org/en/latest/getting_started/scripting/gdscript/gdscript_styleguide.html

The "standard" came first, then we started to have some more consistent code formatting.

If a guide is drafted, I would suggest at least some of the demo projects be updated first, which would allow users to give feedback on specific tangible changes to projects that would be caused by such a guide.

Sure, that's what we did: https://github.com/GDQuest/godot-kickstarter-2019/tree/master/platformer-2d-rework

You can see the style in action in foss demo projects for review here:

And in the projects of some community members who are unrelated to @razcore-art and me. We're the only 2 persons from GDQuest here.

The reason we don't PR to the demos directly are:

NathanLovato commented 4 years ago

As I've said above, we can detail every point in our guidelines, we've spent time discussing all of them. Then as I also said above, I don't want our guidelines to become standard, I'd like a standard like Python has PEP-8 and pythonic guidelines so that:

And to do so I'd:

  1. Write and discuss a draft for the guidelines.
  2. Use them in practice, sure, have some reference projects that people can explore and critique.
  3. Have people critique the actual guidelines, possibly one by one, contribute some more examples or counter-examples.
  4. Put a proposal doc up for more review, like the Python PEPs.

And moving forward, keep improving and modifying it as Godot changes and as people discover better ways of doing things.

We got started with 1. and 2. already, and I'm offering to do more work there to get this done, based on the community's discussion and critiques.

Then, I do want feedback from people who release actual games, from fellow developers, and guidelines that are better than what we have now.

Feniks-Gaming commented 4 years ago

I agree with @NathanLovato you can't update demos until you agree style guide to follow. Discussion here should bring some eyes to agree what style guide should look like and only there we can have a clear direction in maintaining demos and clear guide for what projects to accept and reject.

I think I it would be good idea to maybe make a video Nathan that this discussion is taking place and highlight it on twitter as you seem to have the biggest platform to do so. Comunity could then discuss here what exact style should be used and we could move forward. If you are in discussions with main developers maybe twitter update that opinions are needed here from them would also help. Here. Docs discussions aren't followed by many and small changes are fine to be done without wider involvement but big change like agreed style should see more eyes here. What do you think?

NathanLovato commented 4 years ago

Yup, we should bring people in!

I don't know if I should tweet or release a video about that because now GDQuest is relatively well-known in the Godot community, it may bring fans who would come with a bias towards our work or the opposite, or it could make some people feel like I'm trying to bias the discussion. So I'd rather let other people spread the word first, and maybe share the discussion where there's been some debate already.

NathanLovato commented 4 years ago

@akien-mga we're talking about having something like PEP8 for GDScript here, and a much more detailed style guide than what we have in the docs now, with the order of properties, methods, etc.

If you have a moment, could you give us your thoughts on the discussion, on the way to proceed, and invite devs who would be interested in participating?

NathanLovato commented 4 years ago

@Scony we've derailed the discussion, sorry for that. Do you mind if we rename this issue to say it's a discussion about having a more complete GDScript styleguide in general? Or should we open a new issue for that?

Scony commented 4 years ago

@NathanLovato yeah, feel free to set more general title.

Regarding PEP8, please note that most of the actual PEP8 may be used in Godot. And since it was discussed in a much bigger community, we should really consider starting from that.

Yet of course - we need to be careful - python is a general-purpose language and GDScript is rather a DSL for game development - that has to be taken into account.

Nevertheless, someone has to be a Thomas Jefferson of Godot and draft an initial proposal. I think PR will be a good place to discuss concrete things.

NathanLovato commented 4 years ago

I'm all for starting off PEP8 for everything that's relevant in GDScript, e.g. the details of the syntax, like where you put spaces or carriage returns, which is important to build e.g. a code formatter.

But we still have to thoroughly discuss and agree on guidelines for the order of properties, classes etc. (the topic of this issue), and other suggested practices.

You're right that PRs could help focus the discussion, as people can then discuss specific lines and points. I'll put up a PR for that. Or maybe do it in several passes to separate concerns.

Feniks-Gaming commented 4 years ago

When it comes to order of properties I think discussion so far resulted in this order correct me if I am wrong?

00. tool # optional for tool scripts
01. extends 
02. class_name
03. """docstring"""
04. signals 
05. constants
06. enums  # enums can be exported variables so should be near exported vars
07. exported variables
08. public variables
09. private variables
10.`onready` variable (node dependencies) # are initiated in _ready() so should be near it
11. optional built-in virtual `_init` method
12. built-in virtual `_ready` method
13. signal callbacks
14. remaining built-in virtual methods
15. public methods
16. private methods
17. static functions

My proposal for 03 """docstring""" longer than one line that it starts with one indent something like

Description 
    long description goes here
    if it's multiline

Here is example

1

thanks to that any long documentation can be folded away so keep code base readable to users familiar with it without need for scrolling down tones

2

razcore-rad commented 4 years ago

I think the discussion related to the docstring can be its own topic actually. Since it should indeed have its own style guideline. I haven't looked at PEP-8 for a long time now, but I don't remember the docs being discussed in that guideline. Most of the scientific/data-analysis community went with numpy's docstring styleguide which I think works pretty well: https://numpydoc.readthedocs.io/en/latest/format.html. It's human readable-first with no fancy "features". I think it was mostly made to be compatible with reST since they were using Sphinx to generate the docs. We could start with something like that. I'd personally go for something focused on Markdown rather than reST, but that's just a preference I guess.

We'll think about it, in any case, the idea we discussed at GDQuest is to have minimal to now docstrings because they tend to get out of sync with the code quite fast. It would be fine to get a workflow going where we'd complete the docstrings once the code gets into a beta stage or something like that, where we don't have breaking changes cause otherwise it's just a hassle to update it and no one wants to do it. We also believe that code should be self-documenting as much as possible so apart from an introductory docstring for the entire class we wouldn't really try to go too granular with the variables & functions. If you write declarative code more often (eg. make functions with meaningful names instead of "opaque" assignments of calculations) then it makes the code much simpler follow. This is kind of at odds with the way Godot works (preferring usage of "properties", e.g . variables and setter/gettters), but I think we can work something out. In general I'd say prefer to use meaningful functions if the computation assigned to a variable is complex enough or even if it's too "symbolical", meaning after 2 days you don't remember what the symbols meant cause we usually don't spell out stuff and instead go for abbreviations. This style of preferring functions over assignments should cut the need for docstrings drastically.

P.S.: the reason why for numpy it makes sense to get into complex docstrings like that is because it's a library so it needs proper documentation in order for users to know what the heck is going on. So this would be analogous to our demo & example projects or any other assets out there that need documentation for developers.

NathanLovato commented 4 years ago

@razcore-art There's a PEP dedicated to docstrings: PEP 257.

razcore-rad commented 4 years ago

I never studied that, but at first superficial look I much prefer numpy's style of docstrings. That PEP 257 is from 2001, before numpy's time and much of the community has adopted the numpy guide since it's much more precise. Habbit of working as a scientist of sorts :)

Feniks-Gaming commented 4 years ago

I think we should keep discussion of which style of doc strings for separate issue and we should narrow this down here to just agreeing order of properties. Otherwise we will get to distracted here. I shouldn't even start this discussion so my apologies.

Is there any opposition to this order proposed and what is rationale behind it so we can discuss different orders?

00. tool # optional for tool scripts
01. extends 
02. class_name
03. """docstring"""
04. signals 
05. constants
06. enums  # enums can be exported variables so should be near exported vars
07. exported variables
08. public variables
09. private variables
10.`onready` variable (node dependencies) # are initiated in _ready() so should be near it
11. optional built-in virtual `_init` method
12. built-in virtual `_ready` method
13. signal callbacks
14. remaining built-in virtual methods
15. public methods
16. private methods
17. static functions
razcore-rad commented 4 years ago

I think this is the order we're using, right @NathanLovato? So :+1: from me

Feniks-Gaming commented 4 years ago

There are 2 changes vs what GDQuest uses

First constants and enums switched places in order to have enums closer to export vars as enums can be exported it made sense to have them just before export variables.

Other change is bringing onready var closer to the _ready() method as they make more logical sense as stated here https://github.com/godotengine/godot-docs/issues/2997#issuecomment-568279298

NathanLovato commented 4 years ago

I'm all good with that, including the changes. I already updated our style guide the other day regarding the enums, and certainly don't mind changing the onready variables.

Scony commented 4 years ago

@Feniks-Gaming since enum may be used in e.g. const I'd prefer to keep the GDQuest order for the natural reading order.

Consider snippet from my game's Config.gd:

enum Car { CLASSIC, SEDAN, COUPE, HATCHBACK, PICKUP, TRUCK, AMBULANCE, POLICE, TAXI, GARBAGE }
# (...)

const CARS = {
    Car.CLASSIC: {
        'name': 'Classic',
        'layouts': {
            str(Color.black): 'res://vehicle-layouts/ClassicBlack.tscn',
            str(Color.red): 'res://vehicle-layouts/ClassicRed.tscn',
            str(Color.gray): 'res://vehicle-layouts/ClassicGray.tscn',
            str(Color.blue): 'res://vehicle-layouts/ClassicBlue.tscn',
            str(Color.green): 'res://vehicle-layouts/ClassicGreen.tscn',
        },
        'speed': 150,
        'price': 150,
        'tank': 100.0,
        'fuel_consumption': 0.01,
        'capacity': 20,
    },
# (...)
Feniks-Gaming commented 4 years ago

It looks like a solid argument I can't really argue with it.

ntsik commented 4 years ago

I'd make the argument that class_name should come before extends for two reasons:

  1. It's the same as most other languages (or at least ones I'm familiar with). C++ has Derived : Base, Python has Derived(Base), Java has Derived extends Base, etc.

  2. It's also consistent with the syntax for inner classes, e.g. class Derived extends Base.

Duroxxigar commented 4 years ago

If we do make a more complete standard - I'm voting against having 2 spaces between functions. There should only be 1 in my opinion. Just wanted to throw that out there. I also agree that class_name should come before extends. It is weird seeing the inherited class before the actual class.

NathanLovato commented 4 years ago

I'm voting against having 2 spaces between functions.

It's already in the gdscript guidelines since ~2+ years ago, based on Python's pep8: http://docs.godotengine.org/en/latest/getting_started/scripting/gdscript/gdscript_styleguide.html#blank-lines

Please give arguments for your choices, we don't want to fall into personal preferences there.

The case for 2 lines between functions is that it clearly distinguishes functions from one another, and from methods of an inner-class, that are separated by one line. This also distinguishes the separation between functions from the separation of logical blocks inside functions, that are also separated by one line.

As for the rest of the guidelines, it improves readability - you can understand the code's structure at a glance that way.

Feniks-Gaming commented 4 years ago

I strongly disagree. 2 spaces is a must. That is how Python and any other language that doesn't have {} does it to separate their functions. Blank line is not very readable. Especially as Nathan pointed out you already use single line to separate blocks of code inside function this would make those 2 indistinguishable and harder to read.

Duroxxigar commented 4 years ago

Code structure is all personal preference anyway. It's just what is agreed upon collectively. I know that both PEP8 and GDScript say to include 2 spaces, but that doesn't mean it can't change.

I disagree with it being more or less readable as well. There is a very clear break between two different functions. With the func keyword for GDScript and the def keyword for Python. As well as parameters and a colon. To me, it just adds a longer file to scroll through when reviewing it.

Feniks-Gaming commented 4 years ago

Of course it is but when we agree unified style we must go on something better than it's personal style.

I can be convinced that 1 line is better than 2 lines but we need some good arguments in favour of 1 line.

How many functions will you get pee script 50 maybe 100 on a larger file that's only extra 100 lines of code in several 1000s long file it's hardly much added effort especially that very rarely you will be reading entire file rather than jumping to specific functions you are reviewing. Issue is that if you scroll a little to fast it's easy to skip 1 break line and suddenly you are reading func2 thinking you are still reading func1 and it all becomes confusing. 2 lines mitigate this and reduce risk of errors. It also makes it easier to read outside of editor when code snippets are shared online on wiki, blogs and reddit.

NathanLovato commented 4 years ago

Code structure is all personal preference anyway. It's just what is agreed upon collectively.

That's contradictory. You're saying it's a personal preference, that is to say, the preference of a single individual, but at the same time agreed-upon collectively, i.e. as opposed to chosen individually.

If we discuss guidelines, that is precisely so that they are not the result of a single person's personal preferences but bring a relative consensus.

Also, if we provide arguments, it's so that we move a little more away from personal taste and towards reasoning.

Ideally, I'd love to have scientific papers on the subject to help us make the best decisions for the largest amount of users. Unfortunately, I don't know any. So we're likely going to be there with empirical evidence and experience.


I disagree with it being more or less readable as well. There is a very clear break between two different functions. With the func keyword for GDScript and the def keyword for Python. As well as parameters and a colon.

One thing I learned when I worked in UX is that the brain first processes big shapes, like bounding boxes, and that you naturally process what's in that shape as being related content.

With GDScript files being generally short, with a few functions, and code foldable, I'm not sure it makes that big of a difference when it comes to scrolling. It may make files what, 5-10% longer?

A side-by-side comparison of two long classes might be best but in the meantime, here's an example so people can compare.

One line:

extends Node
class_name StateMachine
"""
Hierarchical State machine for the player.
Initializes states and delegates engine callbacks (_physics_process, _unhandled_input) to the state.
"""

signal state_changed(previous, new)

onready var state: State = get_node(initial_state) setget set_state
onready var _state_name: = state.name

export var initial_state: = NodePath()

func _init() -> void:
    add_to_group("state_machine")

func _ready() -> void:
    connect("state_changed", self, "_on_state_changed")
    state.enter()

func _on_state_changed(previous: Node, new: Node) -> void:
    print("state changed")
    emit_signal("state_changed")

func _unhandled_input(event: InputEvent) -> void:
    state.unhandled_input(event)

func _physics_process(delta: float) -> void:
    state.physics_process(delta)

func transition_to(target_state_path: String, msg: Dictionary = {}) -> void:
    if not has_node(target_state_path):
        return

    var target_state: = get_node(target_state_path)
    assert target_state.is_composite == false

    state.exit()
    self.state = target_state
    state.enter(msg)
    Events.emit_signal("player_state_changed", state.name)

Two lines:

extends Node
class_name StateMachine
"""
Hierarchical State machine for the player.
Initializes states and delegates engine callbacks (_physics_process, _unhandled_input) to the state.
"""

signal state_changed(previous, new)

onready var state: State = get_node(initial_state) setget set_state
onready var _state_name: = state.name

export var initial_state: = NodePath()

func _init() -> void:
    add_to_group("state_machine")

func _ready() -> void:
    connect("state_changed", self, "_on_state_changed")
    state.enter()

func _on_state_changed(previous: Node, new: Node) -> void:
    print("state changed")
    emit_signal("state_changed")

func _unhandled_input(event: InputEvent) -> void:
    state.unhandled_input(event)

func _physics_process(delta: float) -> void:
    state.physics_process(delta)

func transition_to(target_state_path: String, msg: Dictionary = {}) -> void:
    if not has_node(target_state_path):
        return

    var target_state: = get_node(target_state_path)
    assert target_state.is_composite == false

    state.exit()
    self.state = target_state
    state.enter(msg)
    Events.emit_signal("player_state_changed", state.name)
Duroxxigar commented 4 years ago

Ignoring the debate about my statement being contradictory or not (as it doesn't provide much to the discussion at hand honestly) - the snippets you posted, both are readable. I wonder if there are even any actual scientific papers out there about code structure though 🤔 ...

If we discuss guidelines, that is precisely so that they are not the result of a single person's personal preferences but bring a relative consensus.

Which is exactly why I threw in my vote on something. I already know the consensus is 2 spaces.

NathanLovato commented 4 years ago

Ignoring the debate about my statement being contradictory or not (as it doesn't provide much to the discussion at hand honestly)

My bad if I misinterpreted you. I thought you were dismissing other people's arguments by saying it's all personal preference anyway. So I was answering that no, it's not a matter of personal preference, that we're trying to reach consensus, and pointed out that you wrote it yourself.

I'll let others share their thoughts. The second example, with two lines, is easier on my eyes. Not that I can't read both - it's just a little easier to see the separation.

Feniks-Gaming commented 4 years ago

As I am viewing this discussion on mobile phone 2nd example requires slightly more scrolling but it's significantly easier to read on my smaller screen. If that is of any value to discussion.

I agree with Nathan here it's not that first example is unreadable but a 2nd exame has a structure that is easy to see at glance. I could zoom far enough that I would not be able to read a font any more and I would still know just from amount of what spaces where functions ends and start. In first example that is not possible.

I am not saying that 1 line break is bad it's just that 2 lines are better with almost no drawbacks to them.

It creates a patter that is easy to recognise and as you get accustomed to 2 lines meaning end of function you notice it easier and your ability to read code becomes faster and less mistake prone.

If I was quickly scrolling down in search of a function I need 2 line break would help me find it faster and made it clearer at glance to see how long the function is that I have to deal with.

I too also would love to read some papers behind 2 line vs 1 line readability if anyone has access

Edit: I took liberty of screenshoting both spinets side by side to demonstrate length increase in my opinion it is insignificant to matter 1

YeldhamDev commented 4 years ago

I personally think that signal callbacks should be put dead last. On my personal experience, they generally are very small, with trivial coding that generally just calls other functions, and if you're programming a rather complex UI, the script will accumulate a lot of signal callbacks, that will get in the way of the actual meat of the code.

Feniks-Gaming commented 4 years ago

Main reason for callbacks on front is that many of them are connected from inside _ready() function so they may make logical sense to put near _ready() function. However it's worth discussing as when you connect callbacks from editor they are automatically added to the end of a file which would suggest that is Godots preferred place to put them in.

YeldhamDev commented 4 years ago

@Feniks-Gaming I think most people rarely connect things from script, generally only doing it in case of Nodes instantiated on the fly.

golddotasksquestions commented 4 years ago

I agree with @Duroxxigar and would vote for one space instead of two between functions. The reason is not everyone is developing on large high-res desktop screens. Two spaces is elongating the scripts, making even basic scripts with only a few functions hard to fit on a single screen, even on 17" laptop monitors. Reading and understanding the code is much easier if the user has to scroll less, provided things are still distinctively grouped, which they are.

NathanLovato commented 4 years ago

[signals] generally are very small, with trivial coding that generally just calls other functions

If you're mainly calling another function, you can directly connect to the target function, and from Godot 3.2 the script editor shows you the signal connections, on top of the icon in the scene tree that will take you to the method you connected to iirc.

I'd need to see how you end up with many small signals, as that's something we don't get in the team. Maybe there are ways to simplify your code if you're finding yourself in that situation. I find that using signals a lot tends to produce spaghetti code. We tend to use them sparingly now.

when you connect callbacks from editor they are automatically added to the end of a file

I would guess it's because it's simpler to append text to a file than to find the end of a function. You'll have to reorganize code anyway if you keep working with the code.

I think most people rarely connect things from script

I wouldn't be able to say anything about that without usage data.

All I could say is that connecting through the editor has its drawbacks. As a developer, it's more efficient to connect via code: autocompletion, staying in the script editor, you can find connections in the project with Ctrl Shift F, and Ctrl click the method you connect to jump to it.


And I'm done for this year. Time to turn off the computer. Happy new year everyone!