-
-
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
Extensions for GDScript #15639
Extensions for GDScript #15639
Conversation
16ccc84
to
111c276
Compare
I am quite impressed by this. And even more by the size of the change: it's quite smaller than I would expect to add such a feature. |
An amazing feature, too bad is too late for 3.0 :( It's easy to use, whenever you make a commit it will offer you to format the text and stash files (press S) then just commit again with the format fixed and push your changes |
111c276
to
79c5f42
Compare
I enjoy the change, but have some side-notes about it, that don't need to be actually in GDScript, but could be useful. 1.
It could look be something like in JS... Plus, extending prototypes with files could also get a method or a keyword, ex. 2. |
@Soaku My first draft for this actually had |
@poke1024 great work! I have a suggestion, instead of doing Maybe we could configure the extension like we do with singletons? |
If the use case is to have them available for the whole lifetime of the game anyways, why not do this: # class-like block of functions
extension Vector2:
func cross(v): # magnitude of 2d cross product of self with v
return x * v.y - y * v.x
extension Node2D:
func set_rotation_in_degrees(deg):
rotation = deg2rad(deg) There would be no need for making sure to add a How are conflicts between extension methods and script methods handled? Is one shadowing the other? |
@Zylann The idea is nice, but it poses technical hurdles. Would these Script methods shadow extensions: if a script attaches to a node that uses an extended class, script methods of the same name shadow the extension methods, just like they would shadow builtin script methods (on a scope resolution level, there is no difference between builtin and extension methods). EDIT: maybe you mean that |
@poke1024 yeah I meant |
So the point of those extension functions are only to add functions to the builtin types ? Honestly, I don't really see the point. If you need a custom method isn't it enough to extend the builtin type and add your own function ? Edit: or create a singleton. Honestly, I'm not sure it is worth it. IMHO, we should keep GDscript simple and moreover strictly object oriented. Everything what C# is not. |
@groud you can't inherit twice a class, you also can't inherit a class not designed to be, and you can't extend non-inheritable types (Vector3, Transform etc) |
@Zylann In that case I would rather make this possible before going for such features. |
@groud to me it looks like the same rationale behind C# extension methods. There is just no other way to go, unless creating bunches of static helper functions or making the types themselves writable somehow. Inheritance is not the same thing and won't make this kind of feature available (inheriting is about creating new types, while extension is not). |
The fact is most on proposed use cases are easily made possible with dedicated singleton. |
It's not breaking the OOP paradigm, nor is it a workaround. Extensions come in many modern OOP languages (C#, Swift, in Scala as PML), they are OOP; they are polymorphic, they don't break encapsulation. On the other hand, singletons are not that OOP at all. And using singletons for the uses cases I presented is actually old procedural programming disguising as OOP. |
I'll cite wikipedia then:
But that's not very fair, I have to admit. ^^ It's not because all other language is doing that that we should implement too. IMHO, this is a workaround, as it is explicitely said in the wikipedia page. Extension methods may be useful in three use cases:
The first one makes no sense, as GDscript is interpretated and you cannot distribute GDscript bytecode for now (unless i'm wrong on that). For the second point, Godot's design already forces you to split your code per node, so it should not be a problem in most cases. I can't argue against the last one, I guess in some specific cases it's probably true. IMHO, this complexifies the language for little gain, introducing new paradigms that can be easily worked around with the already present features. I think GDscript's most important strenght is its simplicity and ease-of-use, and such corner-case functionnalities are doing more harm than good. Anyway, I might be the only one to be conservative on that. :) |
This looks OK, but I question the real usefulness of it. In the worst case, this is also mainly a GDScript limitation, not really of other bindable languages, so making this function happen in core does not really make much sense to me. If case anyone really needed or wanted this, it should be added only within GDScript. |
Lippert referred to the static method hack they used in C#, which breaks inheritance and cannot call private methods. What I implemented in this PR on the other hand is fully OOP and completely on par with any method declared in the original class. I thought that was clear from the examples. I agree these changes should be restricted to GDScript, but at this point this is a POC as stated in the description. |
I think that this feature is awesome, as @poke1024 said it's used in many modern languages. One more example: |
@Dillybob1992 Currently extensions (e.g. Concerning the autoload question: yes, you'd just call
you can now write:
|
fa9821c
to
597c580
Compare
What will happen when a lazy loaded scene overloads a critical function for an ongoing process in another instance which is still assuming original or already changed implementation? If load order depends on user interaction will there be any inconsistent outcomes? Are there any safe guards against this scenario? |
@hubbyist overloading original methods through extensions seems dangerous, that should not happen. If there is such a conflict, then the extension is badly written, the existing one should prevail. |
I can not see how the distinction between critical original functions and safe to be extended ones can be enforced looking at the samples. Choosing which one will prevail will be responsibility of the coder as it is in javascript it seems. In this case, prototyping will hinder portability if used on built-in functions I think. I fear that code bases using different extension schemes may became sort of gdscript frameworks. Sub gdscript ecosystem conflicts may arise? I do not think this will benefit the gdscript development. Jquery may triggered some changes in javascript like querySelector functions but it caused piles of vendor specific answers and tutorials that bury the true power of the language beneath them as well. This caused the vanilla javascript reaction and so on. So I am concerned about possible impact of this feature on gdscript usability. Am I missing some point? |
For note, extensions in dynamic languages, like Ruby, involve 'monkey patching' the classes that are being extended, this means that multiple extensions can have conflicting names and it just creates whole issues with expectability. On the other hand, extensions in statically typed languages, like how C# or Scala does it, actually generate free functions that when you import in a scope then when you do something like I.E. extensions in dynamic languages are a rather inherently broken feature, where in static languages they have no overhead in both computation nor reliability and expectability (no surprises). |
@OvermindDL1 Couldn't this problem be fixed then by needing a script to explicitly import a particular extension or set of extensions into a single script file in order for them to be used? That way they have class-scope? It wouldn't be terribly difficult to have the parser swap out the interpretation of a nonexistent extension method to then scan for previously imported extensions in the class scope. I feel like there should be some way to make this possible with a good design. Unless I'm mistaken and this doesn't fix the issue you brought up about dynamic languages? |
Precisely, that's what I was implying needed to be done (should have been more clear ^.^;). However they still won't be able to be type-specific so you'd be able to call something like |
Why would that be the case? If the extension methods are indexed by base or scripted type, and you have information about what type a variable is, then you should be able to pull up an exact list of available extension methods. |
At runtime it's still all dynamically typed, so if you brought multiple identically named methods in scope then you'd have to generate a dispatch call, which is quite a bit slower than just calling it straight. If Typed GDScript becomes more embedded into the interpreter/codegen itself then those costs could be removed, but at this point it's not. |
Just for the record, this PR didn't introduce any additional dispatch calls nor any runtime overhead. It added methods to Godot's internal dispatch tables (see |
I really would love to see this feature added. Sigh @poke1024 thanks for all your work on this. |
I think it's a real shame this hasn't been merged, @poke1024 thanks for your work so far, on this and other gdscript improvements. |
Just to clarify this, since you can effectively change the signature of methods at runtime without any warning, the argument count and types cannot be guaranteed at parse-time. This means function call checks can't work in-editor. And since the idea behind typed GDScript is to catch those kind of errors in-editor, it is a bit of a problem. This could be solved by making all the extensions in a central, registered location and let the engine to the runtime changes on startup (not allowing further changes). So the parser can look there and know the changes even in-editor, being able to make the checks even in the new methods. It's also much less surprising and can detect potential issues (like conflicting names). |
Speaking of conflicting names, this is probably the biggest conceptual issue with this (I guess this is the point @OvermindDL1 was making). Example: I want to add The problem with doing this at runtime is that I may add a plugin that defines some extensions and another that makes conflicting extensions (same class, same method). I may also have some conflicting extensions in my own project. Assuming the one that is added last is the one in effect, it's not obvious in a given code which of the extensions would be called. This may give many troubles for debugging, especially if the problem comes from third-party code. |
@vnen In the current design, |
Would it work, to extend by using class_name? |
@toger5 these are different kinds of extensions. "class_name" is only going to save a script's base type / script path. Has nothing to do with saving additional methods to a base type class. |
@willnationsdev ''' class_name TestClass ... |
As said by @reduz in #15639 (comment), this should be a GDScript only thing. So for this to be merged it would need:
Since this requires a big change, this PR will be closed for now. It can be reopened if the changes are made, or you may create a new PR. You can also open a discussion about this feature in the roadmap repository. |
If @poke1024 is fine with it, I may take a crack at a second attempt for this. Think I can simplify the design a bit too. |
@willnationsdev Yes, that would be great. |
Any progress on this? Looks super-useful! |
I have a branch in my fork with some progress done on it, but I need to find out how to actually code the parser nodes which will call from the loaded script that has the content. I also need to discuss with vnen since I heard from a separate source an unconfirmed rumor about plans to add extensions to Godot in general. |
Is there any progress? It seems that the community still needs this function. #1167 |
Extension methods would be a great way to help overcome some of the constraints GDScript has. Eg not having free functions (apart from some specific cases where the language core does), and being required to contain all of your code in one file, which actually hurts OOP design (I do have an argument for this) and makes class files longer and less likely to maintain a single responsibility. A class should really have 'the minimum amount of methods' to maintain its functionality. Eg a list. You would have methods only related to constructing and maintaining the list. Other functions (map/reduce etc) should use that public interface to perform their tasks. While this is not possible in GDScript to have proper private variables, at least having the main class file only define the core objects APIs, makes them easier to read and reason about. Extensions and/or free functions shouldn't really live inside the objects core API, but GDScript currently forces you to do this, or start coming up with confusing named extension files.
It is work-around-able, but as it's more confusing than necessary, would lead lots of newer developers into writing more difficult to reason about code, the ones who would probably benefit from good practice. A massive quality of life improvement would be having these 'extension' scripts, like tool scripts. Which really would follow these rules
Alternatively, supporting free functions would go a long way to. I have noticed a few people asking for this capability, but free functions might get them most of the way, swell. |
the
However, iirc not all object instances emmit this notification before being freed, specially atomic variants, like arrays and dictionaries, they are not Objects, but also some other subclases of Object never seem to emmit it |
GDScript's core should stay as small as possible, but users want ever new custom functions that behave like built-ins. Some recent examples include #13926, #8071, #8491 as well as my own
This PR is a full POC for extensions for GDScript to let users define custom methods, renames, and method replacements that behave as if they were core functions, as partially suggested by #15586.
The following 7 examples show various use cases and aspects of what this PR is implementing.
Example 1: Extending Node2D
Installing extensions would probably normally happen at a very early point in a program, probably in the root node. In the following lines, using the new builtin function
extend_builtin
,Node2D
gets extended by the methods defined in the user scriptMyNode2D.gd
:In
MyNode2D.gd
theextension
tag needs to be present to define the script as not being a regular script deriving from a class:After calling
extend_builtin
, the functions ofMyNode2D.gd
now extend all current and future instances ofNode2D
:prints:
Example 2: Extending Object
Sometimes global utility functions that can be accessed from all scopes are useful. GDScript doesn't have globals, but with extensions you can extend Object to achieve something similar. As before:
Example 3: Extending Array
Extensions also work on all Variant types, e.g.
Array
:Somewhere else:
Example 4: Extending Vector2
Similar to extending
Array
:And using it somewhere:
Example 5: Extending String
Now
String
s everywhere know about these new functions:Example 6: Extending Engine
The proposed implementation also allows extending pseudo-static classes like
Engine
orGeometry
:Example 7: Redefining builtin methods
When extending a class method
x
that already exists, the existing methodx
will be renamed to_x
; this allows extensions to modify builtin functions. This, for example, adds a customsubstr
toString
that optionally takes only one parameter as opposed to the builtinsubstr
that always expects two:Some technical info
(1) Extensions are global and instantly affect every instance in the system
(2) All built-in classes and variant types can be extended
(3) The memory footprint of instances is not affected, as extensions are not script instances
(4) Extension scripts are not allowed to declare own variables, nor can they declare properties
(5) Extensions add zero performance overhead to unextended classes, and extremely little overhead to extended classes
(6) Extension calls into classes resolve as fast as builtin methods (via
MethodBind
)Note to (2): there's one additional null pointer check per Variant method call, everything else is done patching method hash tables on demand.