-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Add user annotations #102516
Add user annotations #102516
Conversation
36b0cb2
to
715f4d8
Compare
47573ea
to
644248f
Compare
Please try to avoid pushing multiple times in a shorty time (17 times in 2 hours) it takes up a lot of valuable runner time If you need to test things and can't test them by compiling on your own machine consider making a second branch with these changes and run CI on your own fork |
644248f
to
96678b3
Compare
96678b3
to
d1134d3
Compare
My concern with this PR approach is that Annotation is loosely coupled with core functionality. In other words, I worry about conflicts with the core's function with property_hint/usage. For example, this implementation looks like it could have additional annotations for properties with range defined by built-in or In my opinion, what the core should do in the direction of what is proposed in godotengine/godot-proposals#1316 is to improve the property_hint/usage to make it more user-accessible. For example, the current property hint “0,1,0.1,or_greater” can only be obtained as a string with dictionary from script, but if we can improve them, it could be parsed into a form that scripts can use immediately, such as Also, such things as associating one property with another is something that is already available to every users. From my experience with other people's code, there are several ways to do this, such as directly rewriting other properties with “setter” or “_validate_property()”, or doing it in a lazy process with a “dirty flag”, so there is no single way to do it. However, what is consistent here is that it is handled within a single object class and is not delegated to an external class. Considering this, it should be handled in some virtual function by adding a hint like “relation=(prop)” to property_hint, or by allowing a Probably, the loosely coupled annotation added by this PR is not sufficient to ensure the safety of the actual properties, since it cannot retrieve what the core is already doing, as mentioned above. In other words, for true safety, the user would need to know about properties that have some internal validator on the core side, and then transcribe that behavior to the your annotation class like reverse-engineering, which should be redundant. In conclusion, I think what this PR is trying to provide is a rather add-on-like solution, and I am concerned about conflicts with the core. Rather than adding a new Annotation class, it would be better to improve the property registration and information retrieval, such as adding a variation of In the process, there may be a major change to re-visit setter and |
@TokageItLab Have you looked through the proposal comments? There are some great use-cases mentioned for custom annotations that go beyond just custom On the other hand, I agree that the PR is doing a lot of things which haven't been discussed enough yet. For starters, I think it would make sense to scale the PR back a bunch and start smaller. While a user might enjoy the power of the different sub-classes to override aspects of the members you're attaching an annotation to, it's setting a lot of complex API in stone, the implications of which need to be thought through more. I personally would start with just exposing |
@RedMser I've seen a few comments on that proposal, but in my experience most of them could be handled with If there are only a few cases that cannot be handled by However, I still completely agree that we cannot easily retrieve those internal hints/usages from the outside, so I think the first thing that needs improvement is not validation by external class with additional annotations but the internal core property registration/retrieving methods. If it is done well, it would allow users to include custom annotations in their hint/usage (or some additional argument like Callable if we add it), and users could retrieve them in a better format to perform their own validation, without annotation classes. |
Sorry for the late reply in advance.
Assuming you're talking about
I don't think such a feature is really solving the same problem. See below.
I assume you're talking about this proposal (which mentioned the proposal for custom annotations): godotengine/godot-proposals#6750 Custom annotations as implemented here (with its specifications) cannot achieve this anyway (there's no code generation here!), and I don't believe that it should, nor do I believe that that's its purpose. The proposal mentioned also ended up with a different solution that does not involve custom annotations of any sort. (Suggesting that setters/getters should have a two-parameter version that takes the property name as the first argument) Outside of the proposal above, referencing other class members safely in a custom annotation isn't possible with the current implementation anyway, as the
Custom annotations at its core is just associating user-defined metadata with class members. It does not perform runtime validation. The only validation that's happening here is with the custom annotation itself - is it receiving the correct arguments, is it being used on a valid target, etc. Whether custom annotations should be able to modify their targets or not was discussed in the original proposal for custom annotations. I lean towards the idea that custom annotations should not be able to do so on by default, and only allow a limited case-by-case form of modifications on the grounds of pragmatism. This is why I let
The motivation behind custom annotations (as laid out in the original proposal) had nothing to do with extracting data from There were awkward workarounds mentioned in the thread, such as dictating special prefix combinations to encode how the variable should be serialized: Custom annotations would do away with the need of such a hack, by encoding that information in a type-safe annotation that can validate itself. For the sake of making the argument more complete, there is this one counterproposal laid out in the reply here: godotengine/godot-proposals#1316 (comment) In this reply, dalexeev suggested that a builtin annotation like Compared to a typed custom annotation approach like the one implemented here, this builtin annotation solution has almost zero compile-time validation possible (aside from being able to require the target to be a property, etc.) and requires users to remember the schema of the
As it stands only Everything else is read-only access to GDScript's member info, via target-specific
This means we have to hard-code the target restriction first, for example only allowing variables to be targeted at first and then unlocking that limitation later on in a future version. And this means that custom annotations in its first version would have no way to validate itself based on its target. (can't restrict the target's type, etc.) The first part is acceptable, but the second part would make custom annotations much less safe to use, and I think that's really unfortunate. Alternatively, as a compromise: Only
I think it'd be helpful if you could demonstrate how
Again, custom annotations are not meant to validate a property. It does not reinforce some kind of invariant on the target property on its own. It cannot run any code to say, clamp a float property within a given range. The examples shown here "enforce" that invariant by adding custom editor interfaces where the only possible operations will always result in the property satisfying the invariant. (For example, To reiterate, by itself, custom annotations do not perform any validation on the property. A custom annotation only validates itself. (user-defined code to check of the annotation is initialized with proper arguments, and if it is being used on a valid target based on its info via the |
For example, validation.mp4
Since these are already widely used methods inside the core, they may override and collapse these behaviors if the Annotation class is loosely coupled. Or Annotation class behavior will be overwritten by core behavior and become meaningless. Property information is managed in the core by a struct called Indeed, there is no way in the core to pre-validate externally set values, but if we want to add such a method, it should add a method such as Also I don't see much point in annotations that are not closely tied to the internal PropertyInfo. By making PropertyInfo user-friendly, annotation conflicts can be known in advance if the user is able to obtain numerical tolerances from the PropertyInfo information in advance; This means that unintended collapse can be avoided to some extent. For example, if the external annotation class has a float but that is internally set to Int, it means that the float is cast to Int when the user does not intend it to be. In this case, the user would need to know in advance from PropertyInfo that it will be treated as an Int in the core, and set the annotation to Int. The means to know this PropertyInfo should be what is missing from the current core; Since a dictionary of annotations not closely tied to the internal PropertyInfo can be had externally, I believe it would suffice to have it as an add-on. If your complaint is that you cannot define the
Also, functions such as adding buttons to the editor is clearly not directly related to the annotation, so it must be a separate PR. You can use the EditorInspectorPlugin, but I understand that it is cumbersome. If you really need it, you should send a new proposal to add buttons to hint and usage. Or you could send a suggestion to add a syntax sugar for EditorInspectorPlugins. This PR seems to contain a lot of stuff that has nothing to do with the essence of annotation. Please stand back and sort them out. I have shown above that it is possible to associate properties with each other using What we really need here is something that gets value information from property before instantiating the class. Property type, precision and range information are already included in the PropertyInfo. If we want to get the behavior of Instead, make users allow to read and write PropertyInfo directly easily, which is the straightforward way to do it. What is currently missing is a way to do it before class instantiation. The main point to consider here is what information is missing if PropertyInfo is to be used as annotations. |
I don't see how the To make things extra clear, here's what custom annotations are:
And here's what custom annotations are not:
And here's what custom annotations can be used for: (Note the passive form! Custom annotations don't act on their own!)
All of the above are already mentioned as use cases in the custom annotation proposal!
First of all, this is not my complaint. I made this PR because there's a proposal with over 100 thumb-ups and extensive discussion that went on for 4 years. (with core maintainers involved) There's a need for this and I'm trying to address that need. It is the responsibility of the designer of the custom annotation in question to perform the required validation logic to ensure that it is not being used in an unsupported situation.
And of course, the custom annotation cannot change how the property is serialized. I've said this before and I'll say it again, custom annotations cannot change the property's type nor do the people who wanted custom annotations in the proposal wanted to change the property's type.
That's what the proposal was about! Custom annotations! There was a proposal, and so here I am, making a PR for that proposal! I think there's a fundamental misunderstanding happening here.
The examples I had provided are just example usages. I am in no way suggesting that Godot should have a core annotation of some sort that adds buttons to editors (although that's already a thing: #96290) Consider how that PR, among many other proposals linked in the custom annotation proposal, could have been rejected from the Godot core and provided as addons, had custom annotations been a thing. With this PR even I could have made a plugin for tool buttons without modifying engine code. It would've meant not adding even more bloat to The examples are not me saying "I want my cool ToolButton syntax in Godot core." Not "I want my cool unit vector editor in Godot core." None of that. They are there to provide a case that custom annotations, as implemented here, are useful and actually usable. Once again I'll emphasize that the example custom annotations are written in GDScript as an addon, as demonstrated here: https://github.com/chocola-mint/GodotDevSandbox/tree/custom-annotations
I implore you to take a second look at what the PR actually contains, in terms of engine code. It does not add custom editors. Nowhere in the changelist is me extending
The misconception here is you believing that "custom annotations" is a feature meant to associate properties of the same class with each other. I've said this again and again and I'll say it again. That's not what they're for, and that's not what the original proposal was asking for either. If you're simply against the ergonomic feature of That said, please understand that aside from this ergonomic feature, custom annotations do not perform any validation on their targets and do not change anything about their types and whatnot. I'd love to hear feedback from other contributors as well. Pardon me for the mention, but @dalexeev maybe you could chime in seeing how you were involved in the custom annotation proposal's discussion. |
Why is the current type annotation / PropertyInfo not sufficient?
Shouldn't a way to dump PropertyInfo correctly be added to the core? Is it not sufficient to add a virtual function like
It is not the direct reason why annotation classes are required. EditorInspectorPlugin can read PropertyInfo by What is passed in the _analyze() argument of the class you created is what is retrieved from PropertyInfo and MethodInfo in the core generally. My question is that it is not clear why you need a new class to store/retrieve them. Why not just improve the methods associated with PropertyInfo and MethodInfo in the core? If there is information missing there, why not just add it? In the extreme case, the hint_string in PropertyInfo can contain anything, so if you want to have custom data for a property, you can put it all there. If this is not sufficient, then we need to find the right direction for improvement based on the reasons.
No. What I am suggesting is more generic and does not target specific things such as properties or methods. In short, I am proposing to allow the creation of add-ons that use specific symbols as declarations through some sort of register to gdscript_highlighter/parser. The annotations in this PR are hard-coded for property and method at your discretion. As mentioned above, the core has already established a method to process such information by using It is redundant to add annotation classes that have the same purpose with them ( Therefore, I argue that those annotation classes should not be added, and that this kind of information should be consolidated in PropertyInfo/MethodInfo and focus on improving methods to make them accessible to users. Well, my conclusion would be similar to what @RedMser says:
There is only one |
If the annotation does not have a member name as your case C, isn't it not much different from the following?
|
This is why this is not a good use case. As I have reminded you before, custom annotations do not and cannot validate property values. The features introduced by this PR cannot validate property values. When custom editors are needed, adding a Though even in this case, custom annotations do have a small advantage in that less keystrokes is required on the user of the custom annotation's side, and illegal values show up as compile errors. But that's besides the point. Custom annotations do not and cannot validate property values. |
If we just want to annotate that the length is 5, case B is almost sufficient.
Indeed, there is a problem with instantiation. However, I believe it is related to static member features and shouldn't be solved by adding a class like GDScriptVariableAnnotation. |
Sorry, but I think you're missing the point... What custom annotations bring to the table is way more than just the difference between static and non-static properties. And of course, static properties are not class metadata. No book on metaprogramming would consider static members of a class to be metadata. I sound like a broken record at this point so I'll ask you to re-read every use case I've mentioned so far in this thread. And, once again, custom annotations do not and cannot validate property values. Please understand. If you want them to, we can talk about the possibility of allowing them to do so, but at the moment they just can't. |
If it does nothing, then I don't see why you need a method like |
Straight from the OP: It does not do nothing. The demo videos attached even demonstrated this self-validation in action. (third one from the top) Also see sample use cases of
I'm not convinced that you've given the OP a proper read. I beg of you, please read it again. Take your time and try out the sample project with this PR. Otherwise this discussion will go nowhere. |
I wonder about the self-verification in the first place. I think it is due to the fact that AnnotationClass is an object.
Wouldn't these problems be solved by just getting a list of annotation dictionaries and targets set from ClassDB, and that's it? Then there would be no need for AnnotationClass in the first place. |
From an implementation standpoint, ClassDB only records info about C++ classes (registered with |
ClassDB can read and return the gdscript's ClassInfo parsed and stored by gdscript_parser, and the list of properties and methods are also retrieved through it. I assume it is possible to store annotations as metadata in ClassInfo in the form like |
This is just false, as far as I can see. If you don't believe me, open up You might be thinking of how Storing custom annotations in GDScript resources as non-serialized data is as far as I can see the best solution here. |
Oh sorry, that is my mistake. It is true that ScriptInstance is used for gdscript. If that is the case, I think it is possible to get the annotation list from ScriptInstance, isn't it? I am beginning to understand what my discomfort with AnnotationClass is. Perhaps it is because most of what What I have seen in Proposal is in fact such comments godotengine/godot-proposals#1316 (comment) godotengine/godot-proposals#1316 (comment) something that would be implemented as a virtual method of an object class, and I would prefer a simple implementation of iterating the dictionary list, much of Godot things is implemented that way. This does not apply to items that do not need to be listed, but annotations are the ones that should be listed IMO. |
ScriptInstance is an abstract virtual class, so GDScript implements it with GDScriptInstance, which then implements If annotation objects are stored in ScriptInstance, then CSharpScriptInstances would also have them, which would be very weird. I've mentioned this before in the OP, but
It's not "Godotic" (I made this word up) because the Think about it like this.
It's not "Godotic" in this sense but I think it's very much justified for this.
I don't think that is a good solution. Consider the merits of a strongly-typed custom annotation system. Merits which I have already demontrated in the OP:
What's wrong with iterating over a list of The implementation proposed in the comment you linked is only "simple" at a glance but is underpowered, lacks the level of UX people expect from a user-definable version of annotations, and requires changes outside the GDScript module. |
Type safety is a good thing, but even if only your annotation classes are type safe, get_property_list(), _validate_property(), etc. are not. Instead, it is better to align with other implementations. Then, if get_property_list() and _validate_property() are to be type-safe in the future, then annotation should also be type-safe. If they are similarly implemented, it will be easy to align them and rework them, but if they are disparate implementations, it will be difficult. Also it is rare, it is possible that there may be changes to structures such as PropertyInfo/MethodInfo, such cases should be assumed.
If the Annotation class could be referenced, if there are multiple Annotations with the same reference in a single script and they are iterated on, wouldn't this cause unintended duplicate processing if the user is not supposed to that those have same reference? |
As I have explained above, annotations don't exist at the same level as Consider how the query method
Thankfully, GDScriptAnnotation's implementation is entirely independent from PropertyInfo and MethodInfo. The data passed to
Every usage of a custom annotation in a single script creates an independent instance.
No duplicate processing can happen here, when calling |
It means that the arguments of |
Would they? Look at the current arguments of
I can't use |
So I suggest we should first address that issue. |
And I'm trying to tell you that PropertyInfo/MethodInfo is per-instance and we should keep treating it that way. It would also be consistent with the naming here - it's called This is a physical limitation of the parser/analyzer/compiler and it is absolutely not worth it to solve this issue to create an unnecessary dependency and get a worse API. Less is more in this case! Plus, |
I understand that annotaion should be defined for a class, not an instance. However, I still believe that the validation should be done by getting the annotation from a validation method (such as Having the validation in the Annotation class makes them two. Even if you insist that Annotation class's validation ( As I said above, for maintainability reasons we should not provide two methods of validation that can target the same thing. If we could unify all validation with the Annotation class, I would agree, but I don't think that is something that should be done at this stage of Godot 4. I can agree that we should reconsider the validation method when Godot 5 comes around. However, while in Godot 4, subclasses with |
Thank you for your work and dedication to improving GDScript! In my opinion, custom annotations are a somewhat overrated feature. Many of us think it would be cool to have them, but no one provides clear requirements for what custom annotations should or shouldn't do. What problems would you solve with them? Should annotations simply be metadata about class members (attributes/tags), support a limited set of features (like export variables, function decorators), or should they support all the capabilities of built-in annotations (modifying behavior at the language level)? Perhaps they could support a wide range of metaprogramming elements, something like macros? Let's assume the simplest scenario, where we limit annotations to metadata about class members, similar to attributes in PHP. I have the following questions regarding the concept and implementation options: 1. Do we really consider this approach important and useful? If desired, users could store metadata like this: const MEMBERS_INFO = {
login = {name = "Log In", args = ["user", "password"], returns = "token"},
logout = {name = "Log Out", args = ["token"], returns = null},
}
func login(args: Array) -> Variant: pass
func logout(args: Array) -> Variant: pass The downsides are that class members and metadata are separated, there's no compile-time validation, and there's no standardized interface, each project could implement this differently. Custom annotations could become the standard for storing class member metadata. 2. Do we really consider it necessary to declare/register custom annotations? We could allow the use of any annotations (with a prefix like @data_action("Log In", ["user", "password"], "token")
func login(args: Array) -> Variant: pass
@data_action("Log Out", ["token"])
func logout(args: Array) -> Variant: pass The downside is the lack of any compile-time validation. 3. If we want to add limited validation (existence of the annotation, allowed targets, number and types of arguments), we don't necessarily need to follow the C# approach. We could allow annotations to be declared like this: annotation @@action(name: String, args: Array = [], returns: Variant = null) targets (var, func)
@@action("Log In", ["user", "password"], "token")
func login(args: Array) -> Variant: pass
@@action("Log Out", ["token"])
func logout(args: Array) -> Variant: pass More advanced validation would require advanced compile-time evaluations, which GDScript doesn't currently support, or a tool mode for annotation classes. 4. In both the case of annotation classes and a special syntax for declaring annotations, we need to address the issue of script loading order. Before performing static analysis on any script, GDScript would need to analyze the scripts where used annotations' declarations or their classes are defined. This presents a certain complexity, given how GDScript works. Additionally, as mentioned earlier, I'm skeptical about annotation classes, as they would require executing code in the editor. If we do go down this path, I'm not sure creating multiple instances for the same annotation is a good idea. 5. There's also the question of how to retrieve this metadata about class members. There has already been heated discussion about a potential equivalent of Perhaps we shouldn't rush with this feature? Quality-of-life editor improvements (code formatting, renaming), fixes to the type system, static analysis, performance, etc., seem more prioritized and in demand. It would also be good to refactor many of the accumulated issues in the implementation (static analyzer, compiler, caching/loading system). Maybe we should first implement namespaces to avoid introducing the slightly awkward See also:
|
I've said this before, and I'll say it again. Annotations can only validate themselves, and the "validation" here is not the same thing as I've shown you how it's entirely read-only and does entirely different things. (Annotation classes cannot change anything about the class's property infos. They can't add, change, or remove existing property infos. At best it's just outputting an error message) They have fundamentally different purposes and I absolutely don't see how the user can possibly confuse them. You cannot use First of all, thank you for your time. I really appreciate it.
I'll say that it's "opinionated" instead of "overrated." In the proposal thread, there have been several different interpretations of what a custom annotation should be, from people coming from different programming languages, but no conclusion as there's been no concrete implementation so far - only specifications written on paper, basically theorycrafting. Consider this PR my effort to push the proposal closer to reality by actually offering a possible implementation, one everyone can judge for themselves if it has good usability, and if it actually solves their problems. You too are welcome to build the PR and open up the sample project to examine the pros and cons of this particular interpretation of custom annotations first-hand. 🙂
My answer to these questions, with this PR, would be:
You pointed out the downsides very well. Indeed this is where class-based custom annotations excel over per-class "metadata" as const/static properties.
The downside is a big one. For a feature called "custom annotations," it makes sense for the user to expect a level of usability akin to builtin, vanilla annotations, which feature compile-time validation of arguments as well as autocomplete. The autocomplete issue can be worked around if annotations have to be registered somehow, (like the Usability is an important factor IMO. Having type safety, autocomplete, and powerful user-defined validation of custom annotations (not (Using another issue as an example, a core solution to "conditional compilation" should provide at least similar level of usability to your gdscript-preprocessor. (Not exact implementation, usability) Excellent plugin by the way!)
I've seen this solution too in the proposal thread. The logistics of having a special The Traits PR uses a new keyword and I think it's justified there - there's no way of implementing traits as a class of course, but for custom annotations I don't think it's necessary. Or rather, I've demonstrated that it's not necessary. The KISS principle applies here I believe.
I invite you to see the advanced validation use case demonstrated in the This is already supported here.
Currently, annotation classes are not resolved until the analyzer stage, at which point they are just resolved like other class references. Which is to say, they work exactly like any other class reference.
While I can understand skepticism in regards to "executing code in the editor," I don't understand why you're against creating multiple instances for the same annotation. If anything this makes them easier to use as they can have properties and work just like any other I've demonstrated before that you can query a member's annotations and retrieve metadata from each instance, which would correspond to the arguments you used to "initialize" the custom annotation with. It's certainly way more intuitive. Sure, one could argue that this generates a bit more memory waste compared to an entirely functional programming-based approach to custom annotations, but it's infinitely more understandable. (Could you imagine if "static virtual functions" were introduced here instead of the
I think I've made my case already, when it comes to the relationship between custom annotations and PropertyInfo. Regarding
I don't think the implementation here conflicts with any of the other (all really good) proposals you have here. I'm not saying that this feature should say, be added to master for 4.5, (would be cool but I'm in no hurry) but I don't think it should be rejected outright either simply on those grounds. Regarding namespaces in particular, they won't be enough to solve the |
Even if it is read-only and only gives an error, I think the instance should read the annotation within The annotation might be owned by the class, not the instance, but since what has the error is the instance, this is not strange. Rather, it is consistent with other implementations related to property. For example, if there is
For example, if there is get_annotation() and it is possible to get the annotation from the property name, _validate_property() can be written as follows.
I think he is saying that he is skeptical of the need to always allow
validation.gd
The difference in memory cost becomes more noticeable as the number of user_names (members that only need length validation, not just user name) increases. In core c++ development, the use of dictionary data is not recommended in terms of memory usage and run-time performance. However, RefCounted is also not recommended as an alternative to a dictionary. In that case, the most commonly used type is struct. Even if we treat annotation as just metadata, if we want a certain formatting in it, we would like to treat it as a struct rather than dictionary or RefCounted (although a typed dictionary might be sufficient). So I think Struct has a higher priority than Annotation implementation. If we rush to make annotation a class, it will be difficult to change it to struct or other types. I don't know which direction struct will go, but if it has a setter, it will allow you to achieve what you want to do with _init() in the annotation class, for example:
If not, we can consider providing a setter for annotation externally, independent of struct. BTW, I believe that annotation should be simpler. If we allow types other than struct for annotation, as in the code more above (like
However, I am skeptical that validation (I am talking about validation by For example, if we need Annotation for Annotation, it should not be so. In other words, the variants in the extended your Annotation class should not have any Annotation like a Matryoshka. It is not a good idea to make everything too functional. I assume that the essence of custom annotation is just a way to give meta data about the class members. The lack of further functions should not be a major obstacle to creating games at this point. A minimal implementation of a solution for adding/retrieving meta data would be an appropriate first step for custom annotation. I suggest reading the following: |
The owner of the error is most definitely the class and not the instance. Here's why. Consider the following case: (Demontration purposes only. I know
What's causing the error in
In the case of So it follows that for
But there isn't, as this does not make sense. Annotations are a GDScript-only concept (have always been, even prior to this PR) and it makes no sense to introduce annotations as a concept into I'll explain this again:
This goes back to my point that annotations are per-class and not per-instance. If annotations are per-instance, then truly they would have very similar purposes to PropertyInfos and would be completely redundant. And we'd have to wonder why there's no But they aren't. Annotations are per-class. That's the point. Because they are per-class, errors are per-class, and end up as compilation errors and not runtime errors. (e.g., in Yes, you can use the per-class annotations with
Then this is strange too.
Surely you already understand this by now, but I'll say it again. Custom annotations do not and cannot validate properties on their own. That's not their purpose, and that's not why people wanted it in the custom annotations proposal thread either. With all due respect, I do not see how this is relevant at all to this PR. It's like arguing that
Putting aside the nonsensical use case, the memory cost is per-class. In Godot, scripts are only ever loaded once. This is a miniscule cost compared to how many times Nodes get instantiated.
I hate to bring up other languages all the time, but C# implements The memory (allocation) costs of custom annotations as an
I'd even go as far as to argue that for the amount of memory you're saving on using structs instead of classes, you're now paying additional runtime costs (for copying) for however many times you're going to read from annotations. This is a dubious optimization at the cost of usability.
It does change "dynamically" as in, the user writing the code can make mistakes. Much like how we can enter an invalid path for
Indeed, custom annotations being classes does mean that their scripts can have annotations too. I think this is a good thing - there are no new rules introduced with custom annotations. They are just like any other class, subject to the same parsing rules. This makes them very intuitive and easy for users to pick up right away. No special annotation syntax to remember, and no "limited subset of GDScript" that they need to be aware of when extending While I foresee that "using custom annotations on a script that defines a custom annotation" is not going to be a very useful use case, I don't see any value in actively preventing the possibility either. I think we can agree to disagree here.
I believe we have different ideas as to what's acceptable for this "minimal implementation." I've mentioned this before, but I'm fine with this PR only keeping Does that sound acceptable to you? Or perhaps what you really prefer is just adding a Lastly, thank you for reminding me of the best practices. I really appreciate it. 🙂 I don't have authority here, but I've made my case and I hope you can understand the thoughts that went into this design. (That's why I've spent so much time explaining every misunderstanding and addressing every concern you've had thus far) I've always acted on existing proposals, so if the decision being made here is that the proposal was fundamentally flawed/invalid - trying to solve problems that are not worth solving, then I'm more than happy to scrap this PR and stop taking up everyone's precious time. |
First, we will never close a proposal until some implementation has been done since the proposal is valid. If there is duplicate functionality or some solution is already available, it could be redirected and closed, but there is no easy way to add and retrieve metadata for members in Godot yet. The proposal author's original request should be a simple request to add some metadata for each class member. And I read that it should not be "I want to add Annotation as a class" or "I want to have a functionality like
As I said above, I disagree with the addition of In my opinion, adding a class is obviously not the"minimal implementation for the purpose". As I mentioned above, there are concerns about supporting more cases such as nested Annotation within Annotation and other performance concerns.
Probably I do not oppose it as this PR does. However, it is not required to be associated with PropertyInfo. I think it makes sense to manage annotation for each class, so if annotation is managed as a dictionary or simple list of Variants, and the usage is to retrieve it from within an instance and use it, as in If we allow plain Variants in Annotation, it means that Objects can be assigned as annotation as well. Therefore, it should become some compromise that when you assign the custom annotation class you have in mind with
|
I still don't understand why you insist that the Have we not established that annotations have always been a GDScript-exclusive concept? A method like What's wrong with something like
Were my answers regarding these concerns not satisfactory in the previous reply? Could you please tell me why?
You're suggesting that anything (any Variant, including ints, arrays, dictionaries, Objects, and any other class) can somehow be treated as an annotation. This looks and feels nothing like builtin annotations. I don't even think it's a good idea to call whatever this is "custom annotations." That'd be false advertising. But I guess it is "minimal" in that it has zero restrictions and zero protections. Just an annotation that takes a constant-expression that evaluates to a variant, and that's what matters here. And of course, no compile time safety nor autocomplete - not requiring a base class yet still looking for methods with special names to call is a bizarre Unity-ism if anything, so removing Which means the minimal implementation would just be a builtin annotation I don't like it personally, but I guess it's either this or nothing at all. If you're happy with this solution instead, I'll close this PR and somebody else can implement this feature instead. At the very least it's certainly not custom annotation anymore. |
If we think of annotations as meta for class members, that is what they will be. It will be available independently of annotations in other languages. Also, based on the concept that godot core is created by godot, it should be equally usable in c++, not just for GDScript.
As you said about this, it would be a difference in values. I say it must be prevented from a maintainability point of view.
In essence, it would be almost the same as godotengine/godot-proposals#1316 (comment) (the most popular reaction from people and has a positive response). The decorator need should be discussed separately from the meta data since even if it is not a class, adding something like a function hook to the fundamental core system reduces maintainability. However, I believe it should not be a class in the least, but should be something that adds Callable to the built-in annotations like |
Fantastic. Then this PR can be put to rest. At least something came out of this discussion/debate. For any other contributor interested, it looks like the specifications would be:
Whoever decides to take on this task, I wish you all the best. |
(Update 2025/2/14) This PR has been superceded by a different solution that doesn't actually involve the concept of "custom annotations," and so it has been closed. The author of this PR has chosen not to work on that solution, but you can find the specifications here if you are interested: #102516 (comment)
Original Post
Closes godotengine/godot-proposals#1316
This PR implements user annotations AKA custom annotations, a feature that allows users to define their own annotations with GDScript, for GDScript.
Interacting with user annotation-based custom editor properties:
godot-annotation-2.0-demo-2.mp4
Editor autocomplete for user annotations:
godot-annotation-2.0-demo-3.mp4
User-implemented compile-time validation for user annotations:
godot-annotation-2.0-demo-4.mp4
You can try out this PR with the project provided here: https://github.com/chocola-mint/GodotDevSandbox/tree/custom-annotations , which demonstrates how user annotations can be used in conjunction with EditorInspectorPlugins to achieve the results above.
Note that the examples are meant to demonstrate usage and capabilities. The user annotation
@@ToolButton
included here is -mostly- redundant with@export_tool_button
existing, for example. (Though it does allow you to embed arguments to be bound to the function as a CSV string, and can even check if the string's valid in compile time!)Specifications
GDScriptAnnotation
, with either GDScript or core C++.GDScriptVariableAnnotation
,GDScriptFunctionAnnotation
,GDScriptSignalAnnotation
,GDScriptClassAnnotation
can be extended to add annotations that can only target variables, functions, signals, and classes respectively.annotation <class name>
syntax sugar. (as suggested in the proposal thread)_init
), which defines the annotation's parameters, and cannot be a C++ virtual class.error_message
to a non-empty string inside any callback (virtual function) called by the parser. The parser will then format the error message into a parser error message._init
can be used to validate the annotation's arguments as well.GDScript<type>Annotation
classes come with_analyze
virtual functions that allow the user to read the target's info and execute logic (including validation) based on it.Vector2
._get_allow_multiple
can be overridden to specify if there can be multiple of an annotation. The default is to disallow multiple annotations.GDScriptVariableAnnotation
can additionally override_is_export_annotation
to act as an export annotation similar to@export_custom
. This is mainly for ergonomics, as otherwise for "property drawer" use cases (reasonably common IMO), users would have to write an additional@export
after their custom annotation every time they want to use it._get_property_hint
,_get_property_hint_string
,_get_property_usage
virtual functions allows the user to specify thePropertyInfo
parameters. If not implemented, the default behaviour is exactly the same as@export
.@@
googly eyesprefix.@@<user annotation class name>
GDScriptAnnotation
cannot be used as a user annotation.GDScript.get_members_with_annotations
,GDScript.get_member_annotations
, andGDScript.get_class_annotations
can be used to query user annotations from aGDScript
resource.Some notes
CSharpScript::get_public_annotations
isn't even actually implemented at the moment)System.Attribute
class, and use another built-in attributeSystem.AttributeUsage
to specify if the attribute should allow multiples, and what it is allowed to target.GDScriptAnnotation
) is not exactly a good idea in my opinion, and would result in an uglier implementation on the parser/analyzer's end.get_target_mask
virtual function._analyze
virtual function overridden by the user can have a strongly-typed interface. (instead of having to remember how to unpack aDictionary
based on the target type)GDScriptFunctionAnnotation._analyze
.@@
syntax, digraph (two-character) prefix support has been added toCodeEdit
(which previously only supported single-character prefixes)@export_navigation_flags_2d
and@export_range
? No, as those add metadata shared across languages (PropertyInfo
) and that's why both GDScript's export annotations and C#'s attributes can cause the Godot inspector to show the same UI.(Draft PR for now, usability/design feedback highly appreciated)