johnfn / ts2gd

💥 Compile TypeScript to GDScript for Godot
200 stars 14 forks source link

Constructor should generate _init function instead of _ready in main file class #71

Open adamuso opened 2 years ago

adamuso commented 2 years ago

According to Godot docs main file classes can also have _init function which acts as constructor and would better match TS constructor. Currently for main classes constructor is always generated as _ready() and constructor parameters are removed. In Godot main class with parameters is valid but cannot be attached to a node, because node always instantiate class without parameters, arguments with default values still works.

Here is some code that currently is not working:

// src/BaseNode.ts

export class BaseNode extends Node {
  constructor(nodeName: string, otherValue: int) {
    super()
    print("I'm a base node and I cannot be attached to a Node in Godot because I have parameters")
    print("My name is: " + nodeName + " and other value: " + otherValue)
  }
}
// src/DerivedNode.ts

import { BaseNode } from "./BaseNode" 

class DerivedNode extends BaseNode {
  constructor() {
    super("DerivedNode", 1337)
    print("I'm a derived node and I can be attached to a Node in Godot")
  }
}
# compiled/BaseNode.gd
extends Node
class_name BaseNode

func _ready(): 
  print("I'm a base node and I cannot be attached to a Node in Godot because I have parameters")
  print("My name is: " + nodeName + " and other value: " + otherValue)

# compiled/DerivedNode.gd

extends BaseNode
class_name DerivedNode

var BaseNode = load("res://scripts/compiled/BaseNode.gd")

func _ready(): 
  print("I'm a derived node and I can be attached to a Node in Godot")
johnfn commented 2 years ago

First comment: currently you can work around this by making an _init() method explicitly - though I know that's somewhat unsatisfying.

This is actually a really tricky one. I think that 50% of people will expect constructor to be _ready (e.g. people with TS background would expect this, and Godot noobs), and the other 50% will expect _init() (people with a heavy Godot background would expect this).

I can see a couple of solutions here, none of them are totally ideal:

There's also a 4th solution: only use constructors, and separate out the code that must go into _init() (just the super() call?) into an _init method and put the rest in _ready. I don't know enough about the semantics of _init() to say if this is actually possible though (are there other things that need to be separated out?), and it also might be confusing. On the other hand, this solution intrigues me, because it seems like it might square the circle here.

Thoughts?

adamuso commented 2 years ago

Just emit _init() for the constructor, and make people write _ready() explicitly for the ready method. This is probably the most correct thing, and we get to take advantage of TS's type checking on constructors, which is nice. Unfortunately, this breaks get_node in the constructor, which is a very natural thing to want to write.

I agree that this one is the most correct, but also I do not really like the fact that get_node doesn't work. I would go with this solution if no better comes to mind.

Emit _ready() for the constructor and make people write _init() explicitly for the init method. This is probably more in line with what people expect, and they won't run into unpleasant surprises when properties aren't ready in their constructor. On the other hand, it's annoying for the reasons that you listed.

For me it's too confusing that TS constructor does not generates a _init() in GDScript which is a method that is essentially a constructor.

Disallow constructors entirely. Make people explicitly write _ready() and _init(). This is probably the most correct solution, but it comes with a rather large drawback that since TS can't statically determine that either of these functions are constructor functions, you'll get a bunch of errors about "property not initialized in constructor" for all properties in your class without default values.

This one will be hard because constructor in TS is something natural and I don't think people will like that they cannot declare constructors, also I think this can break new expressions: new MyClass(...).

There's also a 4th solution: only use constructors, and separate out the code that must go into _init() (just the super() call?) into an _init method and put the rest in _ready. I don't know enough about the semantics of _init() to say if this is actually possible though (are there other things that need to be separated out?), and it also might be confusing. On the other hand, this solution intrigues me, because it seems like it might square the circle here.

The magic solution that makes everyone happy, maybe. I think it would work and this would allow for using get_node in constructors. One thing that we need to take into account that in TS you can add some code before super() call. In my opinion everything behind super() (including it) should go into _init() and the rest after super() should be moved to _ready(). There is only one problem (which maybe is not really a problem), that _init() gets called exactly only once (when node is created) and _ready() can be called multiple times (every time when node is ready on scene).

One more thought. When a class does not extends Node or anything related to it then generating _ready() is useless because it will never be invoked.