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

A proposal for reducing the need for @Register annotations #606

Open
MartinHaeusler opened this issue Apr 10, 2024 · 31 comments
Open

A proposal for reducing the need for @Register annotations #606

MartinHaeusler opened this issue Apr 10, 2024 · 31 comments
Labels

Comments

@MartinHaeusler
Copy link
Contributor

Introduction and motivation

I've been working with the kotlin API quite a lot in the last couple of weeks. While I'm very happy with it in general, one of the papercuts is the @Register family of annotations (@RegisterClass, @RegisterFunction and @RegisterProperty).

Imagine the following scenario:

@RegisterClass
class MyButton : Button(){

    override fun _onReady(){
        GD.print("hello world!")
    }

}

This will never print anything. Why? Because _onReady needs to be annotated with @RegisterFunction. I've lost count of how many times I was pulling my hair why some action wouldn't trigger in my game, and in 90% of all cases it was because I was missing an annotation. The information is also redundant: we're overriding a function of the Godot API. Of course we want Godot to "see" it, why wouldn't we?

To reduce this pain, and just get rid of a pitfall, I propose the following set of rules where the Godot gradle plugin should automatically register classes, functions and properties, no matter if they're annotated or not.

Rules for auto-registration

  1. Every class which directly or indirectly inherits from godot.Node (and is not abstract) should be treated as if it was annotated by @RegisterClass.

It prevents anyone from forgetting it, and there is no use case for a subclass of godot.Node that isn't usable in the editor. Conversely, with this rule, I would argue that the explicit @RegisterClass annotation can be deprecated and marked for removal. It is always a mistake to annotate a class with @RegisterClass if it doesn't inherit directly or indirectly from godot.Node as Godot will not know what to do with it.

  1. Every function which is an override of a function of godot.Node or its subclasses in the godot namespace should be treated as if it was annotated by @RegisterFunction.

For example, override fun _onReady(){} should be enough, there's no need for @RegisterFunction. There is no use case for overriding a Godot-defined API that shouldn't be visible to Godot.

  1. Every property which is annotated as @Export should also be treated as if it was annotated by @RegisterProperty.

There is no use case where we want to export a property to the Godot editor and hide it at the same time. The inverse however is not true: a property which is marked as @RegisterProperty should not necessarily show up in the editor, which justifies the distinction between @Export and @RegisterProperty.

Implementation considerations

All three rules are detectable with reasonable effort (both on the gradle plugin authors side as well as in terms of runtime overhead) via JVM reflection. No rule requires method bodies to be inspected, therefore all rules can be evaluated via the reflection API alone; no source code scanning is required. The most expensive part will be scanning the user project for classes, but this already needs to happen in the existing gradle plugin to find the annotated classes.

I would also be willing to help out with the implementation of these rules if needed, as long as I can code it in Java or Kotlin. I have a fair amount of experience with JVM reflection from my day job.

@CedNaru
Copy link
Member

CedNaru commented Apr 10, 2024

I agree with .2 and .3.
.1 is a more delicate matter because you actually don't always want to register a child class of a Godot type to the Engine. It often happens that I "skip" a class between the original Godot type and the actual final script, and this class is not necessarily abstract.
I don't want to bloat the public Godot inheritance tree with those intermediate classes.

On top of that, we actually have an argument in this annotation to give a different name to the Script than the one in Kotlin. Even if that argument is left empty most of the time, I'd rather have it consistent and at least keep the @RegisterClass as a mandatory annotation.

@MartinHaeusler
Copy link
Contributor Author

@CedNaru regarding the class rule, I see several alternatives:

  • If those "intermediate classes" which you don't want to expose don't need to be instantiated on the JVM side, you could declare them as abstract (even though they may not have any abstract members) to avoid the generation of their godot binding files.
  • If you need to instantiate these classes (or just dislike the notion of an abstract class with no abstract members) then I would argue it would be better to have an explicit @HideInGodot (or @RegisterClass(false)) annotation that actively prevents the generation of their godot binding files. This may seem like exchanging one annotation for another, but please consider that the case where the user will actually want the bindings is far more common. Also, it makes these cases easier to spot, while the absence of a @RegisterClass may appear like an oversight.

In my own project, I currently have ~30 registered classes, and none of them would require hiding.

@chippmann
Copy link
Contributor

TLDR; I'm one of the maintainers who's strongly in favor of explicit registration. However I do see I'm definitely the minority and i do see the painpoints. Some things need to be considered. My proposed alternative is at the bottom of this post.

I also object 1 because of the following reason. I use class delegation quite a lot. Thus, these delegates should not be registered and should never be used nor seen by godot. Delegates cannot be abstract so that will not work.

interface DelegationInterface {
    @RegisterProperty
    var blubb: String
}

class DelegatedInvisibleToGodot: DelegationInterface {
    override var blubb: String = "some delegated property"
}

@RegisterClass
class SeenByGodot: Node3D, DelegationInterface by DelegatedInvisibleToGodot()

I would not want to break that.

For the other points: I personally find this approach quite intransparent.
Given the following class:

class Blubb: Node3D {
    val cannotBeRegisteredAsItsReadOnly: String

    var canBeRegistered: String

    fun cannotBeRegisteredAsContainsNonRegistrableTypes(): SomeOtherThirdPartyDependencyClassUnknownToGodot {

    }

    fun canBeRegistered(): String {

    }
}

Now from just looking at the class in a github review (or even in the editor) We cannot clearly see anymore what is registered and what not and why.
We also cannot just add IDE checks and helpers for that as what would we display? A warning is too obtrusive. Inlay hints could be blocked by other inlay hints, And gutter icons could be annoying. Also not all have our IDE plugin installed.
And what about reviews on github or gitlab?

These are the main reasons I'm more or less against a implicit registration approach.

However, as it's not the first time being discussed and as we have different opinions even in the maintainer team, I definitely see that I'm the minority here and that's fine. Thus my proposed solution is two fold;

My alternative proposal (or extension to the initial proposal)

As I do not want to get rid of explicit registration as IMO it just allows for more fine grained control i suggest that:

By default we:

  • register "everything" by default (more or less as suggested by the proposal)
  • if something should not be registered we provide a "invisible to godot" annotation
  • we document when what is registered
  • we add optional debug logging to the compiler plugin which prints to the console which members of a class were not registered and why. This log can be activated by either a gradle config or through command line parameters like gradlew build -PverboseRegistration=true or something like that

As for the explicit registration we:

  • allow a config in the gradle plugin in which one can add source sets or package paths which should be explicitly registered like today
  • maybe even add a general switch which allows that

As for the maintanance work: The entry gen would not need to be touched at all. It is already completely separated from the gathering of the classes and members which need to be registered.
For the symbol processor, some simple if else should do the trick.
So IMO there should not be too much maintanace burden to support both implicit and explicit registration.

One caveat to the implicit registration regardless of the approach:
KSP works most efficiently on annotations AFAIK. I need to investigate how we can gather all registered members and register them when they do not have a annotation on it. Especially on incremental compilations.

@chippmann
Copy link
Contributor

As a side note which was already mentioned on discord: We do not use reflection at all, nor should we.
We're using KSP a kotlin compiler plugin to analyze the source code at compile time and gather the information we need to generate the registration source code and trigger a second compilation round (handled by KSP)

@gabryon99
Copy link
Contributor

Personally, I prefer having an implicit class registration as well, but only for those classes inheriting from Godot ones. As @MartinHaeusler mentioned, most of the time, while writing new scripts, I forget to register a default Godot function.

Having implicit class registration to Godot types would result in a better user experience, similar to the one provided by GDScript and other bindings (such as JavaScript, check the link below). But, at the same time, I understand the problem with classes that do need to be registered, such as utility/intermediate classes for instance. In this case, @chippmann's option, on adding an "invisible to Godot" annotation would be a good fit.

Other bindings stick to the current system, annotating classes with the @RegisteClass annotation, see Swift and Rust (in the latter I think it is more of a constraint).

I think we are forgetting to mention Godot signals. Dealing with signals' callbacks functions is painful sometimes. For example, if I have the following code:

class Box : Aread3D() {
    @RegisterFunction
    override fun _ready() {
        areaEntered.connect(this, Box::onAreaEnter)
    }

    fun onAreaEnter(area: Area3D) = ...
}

I spend some time to understand why the function is not invoked, just to find out that it wasn't annotated. This could be solved during the KSP phase (correct me I am wrong), trying to trigger a warning. I understand that the solution is not that trivial, considering that a signal can be connected from other classes too.

Resources:

@chippmann
Copy link
Contributor

chippmann commented Apr 11, 2024

For user defined signals we already have an IDE check for that case. But i think we just forgot the api defined signals.
Do you mind opening a separate issue for that @gabryon99 ?

Regarding ksp checks; that would indeed be a nice addition. One problem with it is though that the entry gen does not know anything related to ksp and there the compile time checks lie at the moment.
Additional ksp based checks would be good to have though

@MartinHaeusler
Copy link
Contributor Author

MartinHaeusler commented Apr 11, 2024

I think we are forgetting to mention Godot signals. Dealing with signals' callbacks functions is painful sometimes.
...
I spend some time to understand why the function is not invoked, just to find out that it wasn't annotated. This could be solved during the KSP phase (correct me I am wrong), trying to trigger a warning. I understand that the solution is not that trivial, considering that a signal can be connected from other classes too.

I fully agree that it would be ideal for methods which are used as signal receivers should be auto-registered. I'm just not sure how feasible it is to detect these cases in KSP. If you plug in the method reference directly into the connect function, then it's fine, but as far as I know, in Kotlin you can also do this:

val ref = if(Random.nextDouble() > 0.5){
    MyClass::someMethod
} else {
   MyClass::otherMethod
}

this.someSignal.connect(this, ref)

So which method do we auto-annotate? someMethod or otherMethod?

While I would love to see auto-registration for signal handlers, I think it's just not doable. But if someone can prove me wrong, I promise I wont be sad about it :)

@piiertho
Copy link
Member

piiertho commented Apr 11, 2024

TLDR; I'm one of the maintainers who's strongly in favor of explicit registration. However I do see I'm definitely the minority and i do see the painpoints. Some things need to be considered. My proposed alternative is at the bottom of this post.

I also object 1 because of the following reason. I use class delegation quite a lot. Thus, these delegates should not be registered and should never be used nor seen by godot. Delegates cannot be abstract so that will not work.

interface DelegationInterface {
    @RegisterProperty
    var blubb: String
}

class DelegatedInvisibleToGodot: DelegationInterface {
    override var blubb: String = "some delegated property"
}

@RegisterClass
class SeenByGodot: Node3D, DelegationInterface by DelegatedInvisibleToGodot()

I would not want to break that.

For the other points: I personally find this approach quite intransparent. Given the following class:

class Blubb: Node3D {
    val cannotBeRegisteredAsItsReadOnly: String

    var canBeRegistered: String

    fun cannotBeRegisteredAsContainsNonRegistrableTypes(): SomeOtherThirdPartyDependencyClassUnknownToGodot {

    }

    fun canBeRegistered(): String {

    }
}

Now from just looking at the class in a github review (or even in the editor) We cannot clearly see anymore what is registered and what not and why. We also cannot just add IDE checks and helpers for that as what would we display? A warning is too obtrusive. Inlay hints could be blocked by other inlay hints, And gutter icons could be annoying. Also not all have our IDE plugin installed. And what about reviews on github or gitlab?

These are the main reasons I'm more or less against a implicit registration approach.

However, as it's not the first time being discussed and as we have different opinions even in the maintainer team, I definitely see that I'm the minority here and that's fine. Thus my proposed solution is two fold;

My alternative proposal (or extension to the initial proposal)

As I do not want to get rid of explicit registration as IMO it just allows for more fine grained control i suggest that:

By default we:

  • register "everything" by default (more or less as suggested by the proposal)
  • if something should not be registered we provide a "invisible to godot" annotation
  • we document when what is registered
  • we add optional debug logging to the compiler plugin which prints to the console which members of a class were not registered and why. This log can be activated by either a gradle config or through command line parameters like gradlew build -PverboseRegistration=true or something like that

As for the explicit registration we:

  • allow a config in the gradle plugin in which one can add source sets or package paths which should be explicitly registered like today
  • maybe even add a general switch which allows that

As for the maintanance work: The entry gen would not need to be touched at all. It is already completely separated from the gathering of the classes and members which need to be registered. For the symbol processor, some simple if else should do the trick. So IMO there should not be too much maintanace burden to support both implicit and explicit registration.

One caveat to the implicit registration regardless of the approach: KSP works most efficiently on annotations AFAIK. I need to investigate how we can gather all registered members and register them when they do not have a annotation on it. Especially on incremental compilations.

I don't think you're in minority.
I really don't like implicit things in programming, it can leads to unexpected side effects (remember cpp auto conversions that leads to explicit keyword for single arg constructor). Currently trouble is when user forget to register, but if we have implicit registration we will have opposite: troubles because everything is implicitly registered.
Regarding class registration, @CedNaru and I often use classes that inherits godot types, but we don't register those classes because we don't need them to be exposed to engine, those are here only to provide some custom behaviour on JVM side, and those classes are not abstract.
IMHO setting up IDE warning is a better way: This does not imply implicit flow (that can be hard to debug and maintain), and still provide information that a method should be registered.
For auto class registration I'm strongly against since IMO defining a class that inherits from a godot type and exposing it to engine are two seperate things. You need to extends a godot type to register a class, but you don't need to register a class to extends a godot type.

@CedNaru
Copy link
Member

CedNaru commented Apr 11, 2024

Regarding marking class that inherit Godot type as abstract so they are not registered:
This wouldn't work. We don't support the feature yet, but Godot allows registering Abstract scripts too now. So by the logic of implicit registration, even abstract scripts would be registered.

My idea is to keep @RegisterClass mandatory but have an option to use explicit or implicit registration. Either as a gradle plugin parameter or as a parameter of the @RegisterClass annotation itself.

I'm not fond of having a @'HideX' annotation. For me it wouldn't remove the bloat, the scripts I make often have a lot of properties and function that are not supposed to be exposed. The end result would just to remove existing annotations from scripts, and add them to the members that didn't have any before.

I don't mind implicit features in the first place. I am just cautious of them when they have side effect. Let's take the implicit C++ constructor example. This one is tricky because it causes automatic conversation of an object, which can cause a lot of issues regarding memory allocation, ownership and many others.
Kotlin on the other side is a language that mostly favors explicit operations, but you still have implicit features like declaring the type of variable. If a Kotlin variable type can be deduced from context, then you don't need to write it yourself and then they call "inference".

I think we can put several rules in place that also falls in the category of inference. The case of registering method is a good inference to me:
If owning class is registered to Godot and the method override a function from the Godot API, then register automatically. It's an excellent heuristic and won't break anything in the Godot project even if registered by "mistake" (and if I fail to see how you can wrongly register it in that case).

@chippmann
Copy link
Contributor

What i meant by being the minority, is that many users already complained about that fact.
While i strongly share your opinion @piiertho i still think having a more lenient default might be beneficial.

I like the idea of @CedNaru:
Do a @RegisterClass annotation which is mandatory and solve the rest through ksp and the IDE plugin where possible and have a gradle option which optionally enforces strict registration. I think that would be the best of both worlds.

@piiertho
Copy link
Member

Maybe this "all register" feature can be done by an external plugin, like the kotlin all open plugin.

@chippmann
Copy link
Contributor

chippmann commented Apr 11, 2024

hmm interesting idea indeed. Pluggable entry registrators. I kinda like that idea. This could be developed separately and independently from our main project. We would just need a check in the gradle plugin if such a gradle plugin is applied and if not, apply our own "strict" entry generation.
And because we already separated the entry gen from the symbol processor, this new "lenient" plugin can just use our entry gen module for the actual registration. Like we intended with the entry gen rewrite for different jvm language. Just that in this case it's not a different language, but a more lenient registrator

Just the IDE checks would still be a problem

@CedNaru
Copy link
Member

CedNaru commented Apr 11, 2024

By the way, if we go along with a more implicit option.
Shouldn't we take the opportunity to rename the main annotation to something like @GodotScript instead of @RegisterClass, it makes it more obvious it's related to Godot. And will allow having a @GodotExtension if we eventually decide to support extensions at the same time as scripts.

@CedNaru
Copy link
Member

CedNaru commented Apr 11, 2024

I'm not up for that feature being an external plugin. It seems core enough for me. And like the Cedric said, maintaining it is just a matter of a few 'ifs', so I don't think we need to add a whole plugin system just for that. (Even if it doesn't exclude to eventually allows for plugin, but even there I don't think this feature should be kept outside).

@chippmann
Copy link
Contributor

I'm not up for that feature being an external plugin. It seems core enough for me. And like the Cedric said, maintaining it is just a matter of a few 'ifs', so I don't think we need to add a whole plugin system just for that. (Even if it doesn't exclude to eventually allows for plugin, but even there I don't think this feature should be kept outside).

Fair. We could still develop it as a separate plugin but just have it applied by default. Would give use the option to properly test our separation and find possible problems with it before anyone comes and tries to support scala or whatever. And it would serve as a "project documentation" for advanced use cases and users.

@chippmann
Copy link
Contributor

And by writing "separate" i mean as a separate module in our project. Not necessarily as a separate project itself.

@MartinHaeusler
Copy link
Contributor Author

Oh boy, I didn't expect this to cause such a disturbance in the force. Looks like I've opened Pandoras Box, I'm sorry 😅

It seems that the main point of disagreement is the @RegisterClass annotation (at least that's the most debated one). I would like to offer a change of perspective on this one. It's been stated that @RegisterClass is explicit and therefore a good thing. I would argue that it's not really explicit, because there are two sides to the medal. If a class has no annotation, the gradle plugin implicitly makes a choice whether to generate a binding for it or not. To be truly explicit, we would have to annotate both cases (register or not) equally. In the current situation, when a class pops up in a pull request review, and it extends godot.Node in one way or another, and it is not annotated with @RegisterClass, a reviewer may call that out as an oversight. The fact whether this was intentional or not is implicit. To fix this, we would need a @HideInGodot annotation (the name is a placeholder).

So what is being discussed here really isn't about explicit or implicit configuration. We're primarily discussing what is the default when it comes to registering a class or not. With the current @RegisterClass solution, the implicit rule is:

Every class which is not annotated with @RegisterClass will not receive a binding.

So we default to "no binding". If we switch to @HideInGodot, the implicit rule is:

Every class which directly or indirectly inherits from godot.Node should receive a binding, unless it is abstract or annotated with @HideInGodot.

This approach favors "binding by default".

With the implicit vs. explicit debate out of the way, I think we can ask the real question: what's the default for class binding generation? On paper, it's a coin toss; one solution is as good as the other. However, if we consider how frequent each case is (classes inheriting from godot.Node that need bindings vs. classes that don't), then I think we can agree that the number of classes that require bindings by far exceeds the other. From my perspective, the default therefore should be to generate a binding by default for every class that extends godot.Node, and allow the programmer to opt out. The worst case is that we generate too many bindings and clutter up the editor. In the current situation, the worst case is that the programmer's code simply won't do anything for no apparent reason (until they check the docs).

Finally, I would like to get back to the philosophy of Godot, to be as user-friendly as possible. And I think it doesn't get much easier than:

class MyButton : Button(){

    override fun _pressed(){
        GD.print("Hello World!")
    }

}

Making this example actually work (without further code adjustments) is a good goal to strive for in my opinion.

@chippmann
Copy link
Contributor

Before we get lost in the details on how something like this could look like, I first wanted to verify that a plugin approach would work (as for me personally, the explicit registration has to work no matter if it's the default or could be added optionally). So I quickly threw together a quick test and it works:
2024-04-12T14:09:23,978409767+02:00

So my suggestion for now would be to make such a pluggable entry generator easier to use (atm it's hacked together in my test by outright disabling our default ksp configuration).
After that is done, it will be a lot easier to implement these individual variants of registration.

If you're fine with that, i would proceed with it and submit a PR (as i find it a good idea no matter the outcome of this discussion).


Regarding this discussion:
With my little test here i discovered that we need a way to "hide" stuff from godot, no matter what. For example we do not support constructor overloading yet and we need a way of hiding "kotlin only" constructors from godot.

Also generic functions in registered classes oppose problems i rather not deal with in the entry gen. There are just so many cases which we would need to cover.

Another approach could be to rely on kotlin's visibility modifiers. So if something is public we register it and if it's internal or private, we don't. But that would maybe restrict some usage scenarios in multi module builds.

But in general the argumentation of @MartinHaeusler is good and something i could stand behind for a default configuration. But as mentioned; only if there are other solutions available and with my test, i think we see that these are possible.

The only problem i see with multiple registration solutions is the IDE plugin checks. For the first implementation I would suggest we only support the default case. And once we support FIR, my suggestion would be see if we could make the IDE checks pluggable as well (like that the entry generator implementation used, could provide the IDE checks and the IDE plugin would load these on demand whenever the entry generator implementation would change).
But that is a thing for the distant future. For now it would also mean that the IDE checks would reduce in size and complexity i think which would improve maintainability as no one of us has a lot of expertise with IDE plugins and PSI.
And one could even argue that the explicit registration will be an advanced feature which does not necessitate IDE helper checks "at launch". Because atm we just had them to help people on common problems as it's "something new" which we do differently.


TLDR;
I'd like to make a PR to make the entry gen fully pluggable.
I'd like to implement 2 versions: explicit and the default "all-open" approach.
I think we need the hide annotation (or use kotlin visibility)

Oh boy, I didn't expect this to cause such a disturbance in the force. Looks like I've opened Pandoras Box, I'm sorry 😅

Don't worry. It's just something we discussed internally quite a lot and debated over it.
It's just that this issue gave the push to discuss it (hopefully) for the last time and find a good solution because clearly it's a pain point for people as this topic comes up not for the first time.

@CedNaru
Copy link
Member

CedNaru commented Apr 12, 2024

Sorry, I couldn't understand your explanation about the current situation being implicit already. We wish for some side effect (here registering a class) so we have some syntax for it, so in my mind it's explicit. If you wish for nothing, then you write nothing.
Or do you mean that "wanting nothing" (not registering the class) also has to be explicit ?

Also just want to nitpick on something, scripts are not for Node and its children only. Scripts are an Object level feature. So anything inheriting Object can be a Script. I know it might be confusing because 99% of the time we attach scripts directly to nodes in the scenes but you can perfectly create lower level scripts and instantiate them from code (or even for resource files too)

@chippmann
Copy link
Contributor

chippmann commented Apr 12, 2024

After some discussions in our internal discord channel it seems i misunderstood the direction this discussion was taking, so I want to rephrase my opinion and suggestion:

  • I suggest to make the entry gen pluggable like i described earlier (it's still useful for other jvm based languages if one want's to add third party support for them or write custom registrars)
  • For me the auto registration makes sense in these cases:
    • classes extending godot.Object (although I'm also fine with requiring a registration annotation in this case)
    • overridden functions from the godot api
    • signals (they are only useful in godot anyways so they need to be registered)
    • auto register property if there is a @Export annotation
  • auto registration does not make sense/is feasible in these cases:
    • user created properties and functions
    • signal callbacks (not feasible and not that big of an issue once we have custom callables again)

I want to apologize for the confusion i might have caused with the "all open" approach ^^

@MartinHaeusler
Copy link
Contributor Author

Sorry, I couldn't understand your explanation about the current situation being implicit already. We wish for some side effect (here registering a class) so we have some syntax for it, so in my mind it's explicit. If you wish for nothing, then you write nothing. Or do you mean that "wanting nothing" (not registering the class) also has to be explicit ?

The gradle plugin at some point has to decide: generate a class binding, yes or no. The current solution is to explicitly mark the "yes" side with an annotation, while the "no" side implicitly follows (the default is "no binding"). I'm just saying that this argument works both ways: if we mark the "no" side explicitly and default to "yes", we are just as explicit as before - we merely swapped the default around. In my experience, the vast majority of classes that extend godot.Object (not godot.Node, as I've learned today) will require a binding. So generating a binding by default (an explicitly annotating the classes which should not receive one) would save effort and would stop people from falling into this trap.

I'm not sure how severe the consequences of generating binding for classes that don't need one really are. Assuming the gradle plugin would generate bindings by default, and someone forgets to add the @HideInGodot annotation to some classes - what is the worst thing that could happen? Is it only about what's shown in the editor, or does it also substantially affect build times, bundle size, memory usage at runtime, etc.? Can it get completely out of hand?

@chippmann
Copy link
Contributor

It would but not by an amount i would consider a technical "limitation" or a problem. Apart from the editor being possibly bloated with unwanted classes i do not see a technical issue.
Registration only happens once at startup (in the editor every time you build) and on incremental builds only the registrars change for which the class has changed.

So to me the discussion in this regard is mainly convention.

@MartinHaeusler
Copy link
Contributor Author

  • For me the auto registration makes sense in these cases:
    • overridden functions from the godot api
    • signals (they are only useful in godot anyways so they need to be registered)
    • auto register property if there is a @Export annotation
  • auto registration does not make sense/is feasible in these cases:
    • user created properties and functions
    • signal callbacks (not feasible and not that big of an issue once we have custom callables again)

Totally in agreement here about all of those.

@Frontrider
Copy link

From this I'd have the following idea:
Add an "autoregister" flag to the annotation that registers the class, and set it to true by default.

If this "autoregister" flag is true, then the following gets registered:

  • default no arg constructor, if available (throw error if no suitable candidate was found, and none was set manually)
  • overridden methods from godot (eg_ready)
  • IF we can catch it, then any methods needed by signals
  • public properties
  • maybe public methods
    (internal methods and properties should be excluded, I think that is a nice solution for that)

IF this flag is false, then the current behavior persists, so you can keep any existing code just by updating this. This one setting is also a very good piece of documentation, as now the annotation reads as "I registered this to Godot, but I register everything by hand here, so I should watch out".

I'd favor this flag over having extra annotations for exposing/hiding things from godot, because it's easier to understand when you look at the code. You know the one registration annotation, every other config attribute is a parameter of this that you can trace with intelisense.

This is objectively less powerful than having explicit excludes, but also easier to manage in my opinion. Instead of juggling 2 separate models (exclude from the auto registry, or only register manually) it's either the "simple auto registry" or handpick what should be seen by Godot. There is value in excludes, but I think a bit of tedium when you want to be specific is better than possibly increasing mental load.

The only shaky part with this "automatic or explicit" approach is what classes to register, and that is why I'd have the class registration annotation as the only mandatory piece. Only register marked classes, but for those register as much as possible. This is also a very small load for the developer, as it is just 1 annotation for each class instead of one for every individual item that Godot needs to see.

I think this is a good compromise between workload and clarity, together with that "register everything" plugin as an option.

@MartinHaeusler
Copy link
Contributor Author

@Frontrider It's an interesting idea and I like it; but I think it needs some refining.

Adding all public properties of a class to the godot binding can be dangerous. For instance, some of my classes have public properties which contain Kotlin callbacks, hash maps and other Kotlin things; Godot will not be able to handle those. The fix for this would be to limit it to only properties which Godot can understand (this list is limited and can be hard-coded as far as I know). Same for methods; if any parameter or return type is of a type that is incompatible with Godot, there's no point in creating a binding for it as it is guaranteed to fail.

Why would you register constructors? As far as I know, all godot.Object descendants are not supposed to have any constructors and only use the default one?

I'm also not sure about Kotlin properties which are not actually backed by a field:

var myProperty: String
 get() {
     return "foo"
 }
set(newValue){
   GD.print(newValue)
} 

Are the godot binding able to deal with such constructions? What about val properties, surely they will cause some hickups when a user attempts to change them in the editor? I don't know because it has never been relevant, but if we start auto-registering all public properties, this is a point of concern...

Another caveat is that as soon as you require one field to be hidden in the editor, you suddenly need to annotate the entire class manually as there are no exclusion annotations. I guess you could also just make that field private, but even if you're adding a getter method, the method will be auto-registered. Maybe this approach would benefit from exclusion annotations for properties and methods?

@CedNaru
Copy link
Member

CedNaru commented Apr 12, 2024

Custom getters and setters are currently supported, but of course, their types has to be Godot-compatible .
We don't support "val" yet but Godot recently added a read-only flag for properties so it should be possible.

Regarding constructors, we already support non-empty constructors anyway. It has limitations but it's there (All Godot base types have 0 arg constructors, but scripts are perfectly allowed to have constructors with arguments. GDscript supports this too).

We have different opinions in the maintainer team regarding implicit vs explicit, but we all agree that we don't want an "automatically register anything that is Godot compatible" feature. Properties and methods not originally belonging to the base Godot type will still require an explicit annotation, whatever the outcome of this proposal is.
The reason is because of the complexity of implementing such a feature that we have to maintain afterward. In its current state, the symbol processor is already quite complex, and we are searching for ways to make it simple. Anything we do to register scripts members also has to be done a second time in the IDE plugin.
And of course we think it's dangerous to have everything public to Godot by default. Like mentionned before, we very often have Kotlin scripts with many methods and properties we don't want to expose to Godot but still wish to be used by the rest of the Kotlin code (so they can't be private either and internal would only work within the same module).

@Frontrider
Copy link

Adding all public properties of a class to the godot binding can be dangerous. For instance, some of my classes have public properties which contain Kotlin callbacks, hash maps and other Kotlin things; Godot will not be able to handle those. The fix for this would be to limit it to only properties which Godot can understand (this list is limited and can be hard-coded as far as I know). Same for methods; if any parameter or return type is of a type that is incompatible with Godot, there's no point in creating a binding for it as it is guaranteed to fail.

That is reasonable.

We don't support "val" yet but Godot recently added a read-only flag for properties so it should be possible.

Good to know, but I usually don't see it as a problem, as you can try to convert it to a getter.

We have different opinions in the maintainer team regarding implicit vs explicit, but we all agree that we don't want an "automatically register anything that is Godot compatible" feature. Properties and methods not originally belonging to the base Godot type will still require an explicit annotation, whatever the outcome of this proposal is.

That is also a good compromise, as the main problem is that stuff belonging to godot still needs to be handled manually.

I did paint in broad strokes there.

@chippmann
Copy link
Contributor

chippmann commented Apr 12, 2024

So i guess we could agree on the following rule set then?:

Auto registration (if the class is manually registered):

  • overridden functions from the godot api
  • signals (they are only useful in godot anyways so they need to be registered)
  • auto register property if there is a @export annotation

manual registration:

  • classes
  • user created properties and functions
  • signal callbacks (not feasible and not that big of an issue once we have custom callables again)

@MartinHaeusler regarding the classes; is there a strong preference or would you also be happy with our idea here?

@piiertho are you also happy with this rule set given that you shared the same strict opinion as i did/am?

@MartinHaeusler
Copy link
Contributor Author

@chippmann I think it's a good first step and definitely a big improvement over the current situation. It also allows you to "test the waters" and see how people react to the change. Maybe we can talk about classes again in the future; let's try this first, I think it will help quite a lot.

@CedNaru
Copy link
Member

CedNaru commented Apr 13, 2024

By the way, should we take the opportunity to rename some annotations?
Like RegisterClass could become GodotScript, for the sake of being clear this is related to Godot.

@Frontrider
Copy link

By the way, should we take the opportunity to rename some annotations? Like RegisterClass could become GodotScript, for the sake of being clear this is related to Godot.

Yeah, and that also describes the behavior it will have with GDExtension.

@chippmann, yeah to me that looks like a good start, and may be a good solution to land on.
Adding auto registration to @export is one that can't really be contested imo, as while the godot api allows them to be separate (I think that is how you described it), it does not make sense otherwise and removes some of that noise from the code.

Another caveat is that as soon as you require one field to be hidden in the editor, you suddenly need to annotate the entire class manually as there are no exclusion annotations. I guess you could also just make that field private, but even if you're adding a getter method, the method will be auto-registered. Maybe this approach would benefit from exclusion annotations for properties and methods?

I was thinking about that. I agree with you, but I opted for a simpler ruleset first. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants