-
Notifications
You must be signed in to change notification settings - Fork 45
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
Comments
I agree with .2 and .3. 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. |
@CedNaru regarding the class rule, I see several alternatives:
In my own project, I currently have ~30 registered classes, and none of them would require hiding. |
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. 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. 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:
As for the explicit registration we:
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. One caveat to the implicit registration regardless of the approach: |
As a side note which was already mentioned on discord: We do not use reflection at all, nor should we. |
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 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: |
For user defined signals we already have an IDE check for that case. But i think we just forgot the api defined signals. 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. |
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 val ref = if(Random.nextDouble() > 0.5){
MyClass::someMethod
} else {
MyClass::otherMethod
}
this.someSignal.connect(this, ref) So which method do we auto-annotate? 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 :) |
I don't think you're in minority. |
Regarding marking class that inherit Godot type as abstract so they are not 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. 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: |
What i meant by being the minority, is that many users already complained about that fact. I like the idea of @CedNaru: |
Maybe this "all register" feature can be done by an external plugin, like the kotlin all open plugin. |
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. Just the IDE checks would still be a problem |
By the way, if we go along with a more implicit option. |
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. |
And by writing "separate" i mean as a separate module in our project. Not necessarily as a separate project itself. |
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 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
So we default to "no binding". If we switch to
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 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. |
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: 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). 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: 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). TLDR;
Don't worry. It's just something we discussed internally quite a lot and debated over it. |
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. Also just want to nitpick on something, scripts are not for |
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 want to apologize for the confusion i might have caused with the "all open" approach ^^ |
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 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 |
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. So to me the discussion in this regard is mainly convention. |
Totally in agreement here about all of those. |
From this I'd have the following idea: If this "autoregister" flag is true, then the following gets registered:
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. |
@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 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 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? |
Custom getters and setters are currently supported, but of course, their types has to be Godot-compatible . 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. |
That is reasonable.
Good to know, but I usually don't see it as a problem, as you can try to convert it to a getter.
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. |
So i guess we could agree on the following rule set then?: Auto registration (if the class is manually registered):
manual registration:
@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? |
@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. |
By the way, should we take the opportunity to rename some annotations? |
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.
I was thinking about that. I agree with you, but I opted for a simpler ruleset first. 😅 |
Update on that matter as we are going to star the rework. Planned changes:
Here an example of what it would look like in practice: @GodotScript
class MyScript: Node() {
val mySignal by signal0() // Signals are automatically registered.
@GodotMember
var text = "MyScript" // Appears in GDScript but not in inspector
@Export
var number = 0 // Visible in inspector
override fun _ready() {
// Automatically registered because overriding a Godot method.
}
@GodotMember
fun customMethod(){
// User defined method so it needs to be explicitly registered
}
} |
Personally think the |
We are split on that one. Either we rename |
I would recommend going with the former option since the class is marked with Rarely is anyone going around showing their code without actually explaining what their code does. Even if you have a name conflict you may as well resolve it with the full qualifier name. |
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:
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
class
which directly or indirectly inherits fromgodot.Node
(and is notabstract
) should be treated as if it was annotated by@RegisterClass
.override
of a function ofgodot.Node
or its subclasses in thegodot
namespace should be treated as if it was annotated by@RegisterFunction
.@Export
should also be treated as if it was annotated by@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.
The text was updated successfully, but these errors were encountered: