Closed xsellier closed 1 year ago
Seems to be related to: https://github.com/godotengine/godot/issues/40288
Shouldn't it be _set
? I don't recall the engine expecting set
to be overridable
I expect to override any engine's function if I want to do it. Or at least the editor should tell me that I cannot override such functions and explain why.
Another solution would be, having an empty object which hasn't had any functions but can be extended as a Singleton.
I expect to override any engine's function if I want to do it
I'm afraid that's not how things work, as it has probably been said in other issues, to achieve that the engine would have to use slow call
on absolutely every function that ever faces the script API, which likely won't happen as it's too much work and downsides to it (unless maybe the engine code and scripts were both in Java?)
I remember that in Godot 3 so far, if you write a function with the same name as an existing function, Godot lets you "shadow" it, although it is not true overriding. For example if you write "get_position" in Node2D
, GDScript might behave as if you overrode the function (because GDScript uses call
everywhere), but the engine will not use your function when getting the position of the node. Because for this to work, the C++ code has to expect and handle the fact the object could have a script attached to it with a function on it.
the editor should tell me that I cannot override such functions and explain why.
So there should be a decision wether or not Godot should consider it an error. I personally think it should, but like many edge cases like this one, other people might have decided to exploit it and want it to remain maybe 🤷
Another solution would be, having an empty object which hasn't had any functions but can be extended as a Singleton.
I don't understand why you need an empty object or a singleton.
I don't understand why you need an empty object or a singleton.
It would be great to have an empty class, that you can use as a singleton so you can have any functions you want. That's the bare minimum to me. For example:
extends EmptyObject
var __data = {}
func _ready():
set('random_key', 'random value')
func set(key, value):
print('Setting a new value')
__data[key] = value
It would be great to have an empty class, that you can use as a singleton so you can have any functions you want.
I still don't understand the relation with overriding functions, but you basically describe an utility class with static functions:
util.gd
class_name Util
static func foo(a, b) -> int:
return a + b
bar.gd
func _ready():
print(Util.foo(3, 4))
And I dont understand the example you give with it... the example sounds like just a dictionary.
var data = {}
data.set("hello", 42)
print(data.hello)
And wanting both in the same object can be done with a dictionary inside an autoload.
It has to have some custom code on set
and get
, with warnings, exceptions and specific behavior. I simply put the simplest code there.
Here a more complete code
extends Node
var __global = {}
signal key_updated(name)
func _ready():
reset()
func reset():
# Load the default locale by default
set('locale', OS.get_locale().left(2))
set('soundfx_volume', 0.5)
set('music_volume', 0.5)
func set(key, value):
if __global.has(key):
logger.debug('Overtwriting key %s', [key])
else:
logger.debug('Writing key %s', [key])
__global[key] = value
emit_signal('key_updated', key)
func get(key, default_value = null):
return __global[key] if __global.has(key) else default_value
And that example code doesn't work? It's basically still a fancy wrapped dictionary. You can put that in an autoload and it should work. Maybe the only difference is, with your code you'd have to call get
and set
(or call them something else if Godot prevents you), but if you use _get
and _set
you can do it, or even write with .varname
syntax.
It doesn't because of the set
get
functions that belongs to the Object
class.
I understand, but prefixing any function with _
has a meaning to me. Since there is no private/public
keywords, a function called _test
will be private, where a function called test
will be public.
If you use _set
and _get
for Godot it means adding extra code to get
and set
, this is one way you can achieve what you want to do here. This is how Godot expects you to "extend" get
and set
, basically (not override it, but that's not really what you need it seems?). You can write all the code you wanted in _get
and _set
, and Godot will still allow you to call get
and set
AND take the _get
and _set
into account.
basically:
func _get(key):
if key == "foo":
return 42
data.get("foo") # gets 42 because Godot will call `_get` internally
data.foo # gets 42 too
I understand why you'd expect to do that with get
and set
directly so badly in principle (as many other overridable functions existing under a different _
name), but it isn't how Godot's scripting API works. As of right now, a native function cannot both represent a regular function and a virtual one. This would need some work to have both, but in the meantime your use case can be solved, except for the internal naming, which is relatively minor (i.e code using your class can still be written as if the names were like you wanted). Not saying the issue shouldn't be investigated, but that there is an easy workaround.
Still doesn't solve my naming issue.
This was changed in #54676. It's being reconsidered / further discussed in #54949.
@akien-mga #54949 seems to concern member variables only, not functions, which are the problematic point here.
You are supposed to override only virtual functions and they are all prefixed with _
(e.g. _ready()
, _get_drag_data()
etc.). We should give a clear error when trying to shadow a built-in function, but non-virtual overriding likely won't happen.
You are supposed to override only virtual functions and they are all prefixed with
_
(e.g._ready()
,_get_drag_data()
etc.). We should give a clear error when trying to shadow a built-in function, but non-virtual overriding likely won't happen.
Thank you for those explanations. However it would be better that behavior is describe somewhere (since it didn't behave that way in Godot 2/3), and also Godot itself should raise an error to warn the user. atm, this is not user friendly.
I code like this:
class Test
enum Type{Input,Output}
It will report error: Enum member Input has the same name as a global class
I think this was valid code, because I can use it like this: Test.Type.Input
Another use case:
set_owner
and get_owner
, those functions are defined by godot so not usable in GDScript.
Wasted time because:
@override
such functionsTo repeat what @Zylann was explaining, not all functions in Godot can be overridden. That's the same in any language, some functions are virtual, others are not. In Godot's scripting API there's another level of indirection as some functions might be virtual in C++ but that doesn't transfer to the scripting API. "Virtual" methods are implemented by calling methods via StringName, which is much less efficient than what a C++ compiler can do for you with the virtual function table.
So there's no intention to support overriding any method such as Object.set
or Node.set_owner
. These methods are used by the engine API internally and making them overridable would lead to terrible performance, and a very simple footgun for users to break the API by changing the way things are expected to behave internally (i.e. if you were to replace set_owner
and expect it to be called internally, you'd have to make sure that you reimplement exactly what the C++ method does - which might not even be possible).
Now, we should indeed add explicit warnings whenever you shadow such a method unknowingly, as the way it behaves is not intuitive at all (and can still lead to bugs for internal engine calls using call
or call_deferred
):
extends Control
var test = 0
func set(property, value):
print("Setting '%s' to '%s'." % [property, str(value)])
set(property, value)
func _ready():
test = 1
set("test", 2)
self.set("test", 3)
$".".set("test", 4)
call("set", "test", 5)
$".".call("set", "test", 6)
call_deferred("set", "test", 7)
$".".call_deferred("set", "test", 8)
get_script().set("test", 9)
This prints:
Setting 'test' to '3'.
Setting 'test' to '5'.
Setting 'test' to '6'.
Setting 'test' to '7'.
Setting 'test' to '8'.
IMO it's far from obvious, and not something we should aim to support.
So I would ensure that shadowing an internal function raises a warning, as done in #54949 for variables. IMO this could even be an error, but given the reactions to #54676, I guess we can start with a warning and let people who still want to play with fire mark the warning as ignored and accept they're using an unsupported feature.
I know it is outstandingly difficult to reimplement, but it is worth reconsidering this stance, at least by adding way more overridable methods, since these "issues" are being reported by the userbase fairly frequently, as it often is limiting, compared to 3.x.
I need too to overwrite all functions to buld a spy to verify function interactions. This was still possible on Godot3 if this will be not allowed anymore than please give me a way to check if a function called or not. If this still possible than please tell me how to spy a function call without overwriting a function?
I need it! My plugin GdUnit3 provides spy and mock to verify interactions on a instance.
Any update here?
Hello I don't understand why you want to restrict developers so much, there is no real reason for that. Whether shadowing or override this is blurring in my eyes.
Overriding methods/functions is still a valid option in many languages, and was possible in Godot 3.x. Instead of responding to the users, it is worsened, I would like to refer only to the sorry topic private/protected/public accessors there is stubbornly blocked. What speaks against it? It is only important that if you overwrite a function a warning, error (configurable) gets as feedback in the editor to avoid accidental overwrites.
So please don't bring in such regressions! This makes me frustrating.
Here is my suggestion
a) allow function overrides
b) show a warning for overwritten functions
c) introduce an annotation @override
to mark as allowed override and disables the warning
d) an editor setting to configure accidental overrides as a warning or error
e) finally respond to the users and introduce the long requested private/protected accessors
@private, @protected
This would also help to disallow overwrites if explicitly requested by the dev
I feel @override
would not be the right name for this though, the problem is to allow shadowing core class methods. @override
would make it look like it makes it work like a virtual function override, when it doesn't works in all cases, and has limitations. Maybe something like @overwrite
or @shadow
or @scriptonlyoverride
, or whatever retains the fact it's not a typical override like every other language.
Again my question, why is it forbidden to override core class functions? I want to explicitly overwrite it and not shadow it!
I have written a unit test plugin 'GdUnit3', which provides mock and spy functionality and I'm explicitly instructed to be able to overwrite core functions to carry if and how often they were called.
And again this is a strong limitation compared to Godot3
Can the speed up really be justified to remove what was existing functionality in 3.0, if so I am curious as to just how much, I am not demanding numbers just curious if any one has numbers.
Would it be possible to allow the over-ride of core function calls at least for debug builds but release builds could still retain the speed to allow the testing in debug builds?
In order to work around this new feature, developers will need to write additional functions to make the call to a core class in order to successfully Spy on what functions were called. This is a waste of developer time by writing additional lines of code, if it could be avoided I would prefer such a solution.
Can the speed up really be justified to remove what was existing functionality in 3.0, if so I am curious as to just how much, I am not demanding numbers just curious if any one has numbers.
The GDScript implementations are entirely different between 3.x and 4.0, so it's not possible to compare performance with this change alone.
Would it be possible to allow the over-ride of core function calls at least for debug builds but release builds could still retain the speed to allow the testing in debug builds?
This would result in bugs that only occur in projects exported in release mode, which are difficult to track down. (Remember the whole dangling Variant thing?)
We should aim to have fewer behavior differences between debug and release, not more 🙂
Here are some numbers.
Script 1 (overwrites a function)
extends RandomNumberGenerator
func randi_range(a, b):
pass
Script 2 (calls script 1)
extends EditorScript
@tool
func _run() -> void:
var rng: RandomNumberGenerator = preload("res://FakeRNG.gd").new()
var time = Time.get_ticks_usec()
for i in 1000000:
var test = rng.randi_range(0, 100)
print(Time.get_ticks_usec() - time)
Execution time in 3.x release_debug
build:
210ms
Execution time in 4.0 debug
build:
140ms
In 4.0 this executes 33% faster, despite I used a debug build which comes with severe speed handicap. If we compared official builds, the difference would be even greater. This kind of optimization would be impossible if functions were overwritable.
This kind of optimization would be impossible if functions were overwritable.
As far i understand Godot4 used function ptr to jump to the right method.
Why should this be a problem for overriding?
Its just an another ptr to the overriden method or not?
Overriding could be used if ptr calls can't be determined (that is, calling a method on untyped object), but then you'd get inconsistent behavior. It would be confusing.
Worth keeping an eventual solution in the back of the head. super()
on virtual calls (_process
, _ready
...) became mandatory in Godot 4.0 instead of implicit, because it was unexpected behavior to a lot of users (even if it was intentional and documented). One could argue that the same could be said for being able to override built-in functions to an extent. It is no longer "allowed", and it was not really accounted for, yet some users believe this to be the expected behavior.
Sorry, but any good programming language allows us to override methods to extend existing functionallities. Now all of a sudden not allowing it anymore will push many devs over the head. The argumentation to write an equivalent function with underscore that calls the core function is total nonsense. This just creates useless code noise and forces us to define pseudo names for existing core functions. A simple example, you want to log if a child node was added.
Godot 3:
func add_child(node: Node, force_readable_name: bool = false) -> void:
LOG.debug("node added %s" % [node])
.add_child(node, force_readable_name)
for Godot4 it would look like this
func _add_child(node: Node, force_readable_name: bool = false, internal := INTERNAL_MODE_DISABLED) -> void:
LOG.debug("node added %s" % [node])
super.add_child(node, force_readable_name, internal)
now we have two functions with almost the same name _add_child
and add_child
.
Furthermore it leads to confusion when providing such an API, should i call _add_child
or add_child
.
Sorry, but any good programming language allows us to override methods to extend existing functionallities.
No they don't. Unless you say that C++, C#, Rust or any compiled language are not good. They require you to explicitly mark overrideable functions as virutal and most of their functions aren't virtual.
ok good programming language
was probably said a little too harshly ;)
to get ahead here and solve my own problem. How can I determine if and how often and with which arguments a function was called without overwriting it?
Due to the new limitation I can not realize a spy or mock on core classes anymore
Best regards
Just chiming in to say in my issue #69765 overriding would be really nice for plugin development. I wanted to have a RandomTimer
class (see here for Godot 3 implementation) with the same methods as the build in Timer
so that it can be used in the same way. I don't really want to tell users "This is a timer, but make sure to call start_random instead of start, otherwise it won't work as expected". Lots of room for error there.
3.x:
4.0-dev:
In both 3.x and 4.0-dev you can't really override non-virtual methods (to have C++ call them), this only works for GDScript.
I'm not sure if the behavior in 4.0 should be the same as in 3.x, or if we should completely disallow overriding non-virtual native functions. But if we choose the first option, then we should add a warning and/or make another indicator (blue arrow).
Given that call("native_func_name")
and native_func_name.call()
call the GDScript method, can we classify this as a bug?
I'm a bit confused by this debate. There seems to be many comments about to how difficult it is to allow overriding (or shadowing) methods of built-in classes. Since this was a capability in 3.5.1 it suggests it's difficult only because of engine changes made for "GDScript 2.0". That fact alone means it's a regression. Call it a bug or a feature, but it's a regression.
So here are a few serious questions.
EDIT: If this was an intentional change why is it implemented as a silent failure? At the very minimum there should be an error raised during editing that states built-in class methods are FINAL and cannot be overridden/shadowed.
Opinion: We are talking about a language feature that previously existed, is expected to exist, and is widely used. If it is truly difficult to support method override/shadow in 4.0, maybe the solution needs to be revisited?
In the old days issues like this were referred to as "trying to fix the solution rather than solving the problem".
@tx350z It didn't exist in 3.x. Unless the method is registered in a way that allows extending it with scripting, which few are, the engine cannot call into it, it has no idea that you've overridden the method. What might have worked is scripts calling methods of other scripts directly. But it wasn't really overriding the method, it was indeed just shadowing it for the scripting side of things, while the engine was not aware that you might've overridden it.
So it was a bad idea to do it, even if it could be perceived as working in some practical scenarios.
If method overriding/shadowing is allowed for custom methods in custom classes, why not built-in classes? What's so special about built-in classes?
All methods in custom classes are virtual by default.
If built-in classes are so special why not make them all FINAL so they can't be extended?
They are not special. You can extend them and override their virtual methods. Every single script extends some class, so making them non-extendable would make the engine literally unusable.
Is this just a case of "trying to protect the programmer"? If so, from what?
No, it was done for performance. Calling methods in GDScript 2.0 is much faster thanks to that.
Is GDScript intended to be an object-oriented-like language? Method overrides are a core feature of OOP languages.
And GDScript does support overriding virtual methods like every OOP language. The behavior in 3.x was a convenient bug that went against the OOP, because it allowed shadowing methods that aren't virtual. The methods that you can override are marked as such in the documentation:
If this was an intentional change why is it implemented as a silent failure?
Because it's bugged. It's the only reason why this issue is still open.
So the ability to shadow built-in class methods in 3.x was a bug that is now fixed in 4.0? If my vote counts, I'd like the bug back please.
@tx350z It didn't actually work in 3.x. Bringing it back, if it even was sensible, would only create an illusion that you can override built-in methods when in fact you cannot. And shadowing is not a feature of any language, it's a bug or a code smell at the very least.
@tx350z It didn't actually work in 3.x. [snipped]
This seems to work in 3.5.1
main.gd
extends Node2D
var cfg:ExtConfigFile = ExtConfigFile.new();
func _ready():
cfg.set_value("my_section","my_value",1);
print("Unsaved? %s"%[cfg.is_unsaved()]);
cfg.save("res://junk.ini");
print("Unsaved? %s"%[cfg.is_unsaved()]);
cfg.load("res://junk.ini");
print("Unsaved? %s"%[cfg.is_unsaved()]);
cfg.set_value("my_section","my_value2",2);
print("Unsaved? %s"%[cfg.is_unsaved()]);
cfg.erase_section_key("my_section","my_value");
cfg.save("res://junk.ini")
return;
ext_config_file.gd
class_name ExtConfigFile
extends ConfigFile
var _is_unsaved:bool = false;
## Returns true if there are unsaved changes.
func is_unsaved()->bool:
return _is_unsaved;
####
## The methods below shadow ConfigFile methods.
####
## Shadows ConfigFile.load()
## Adds logic to set saved state and and emit changed signal.
func load(path:String)->int:
print("ExtConfigFile.load()");
var err = .load(path);
_is_unsaved = true;
return err;
## Shadows ConfigFile.load_encrypted()
## Adds logic to set saved state and and emit changed signal.
func load_encrypted(path:String, key:PoolByteArray)->int:
print("ExtConfigFile.load_encrypted()");
var err = .load_encrypted(path,key);
_is_unsaved = true;
return err;
## Shadows ConfigFile.load_encrypted_pass()
## Adds logic to set saved state and and emit changed signal.
func load_encrypted_pass(path:String, password:String)->int:
print("ExtConfigFile.load_encrypted_pass()");
var err = .load_encrypted_pass(path,password);
_is_unsaved = true;
return err;
## Shadows ConfigFile.save()
## Adds logic to set saved state and and emit changed signal.
func save(path:String)->int:
print("ExtConfigFile.save()");
var err = .save(path);
if err == OK:
_is_unsaved = false;
return err;
## Shadows ConfigFile.save_encrypted()
## Adds logic to set saved state and and emit changed signal.
func save_encrypted(path:String,key:PoolByteArray)->int:
print("ExtConfigFile.save_encrypted()");
var err = .save_encrypted(path,key);
if err == OK:
_is_unsaved = false;
return err;
## Shadows ConfigFile.save_encrypted_pass()
## Adds logic to set saved state and and emit changed signal.
func save_encrypted_pass(path:String,password:String)->int:
print("ExtConfigFile.save_encrypted_pass()");
var err = .save_encrypted_pass(path, password);
if err == OK:
_is_unsaved = false;
return err;
## Shadows ConfigFile.set_value()
## Adds logic to set saved state and and emit changed signal.
func set_value(section:String, key:String, new_value)->void:
print("ExtConfigFile.set_value()");
# check for a value change
var old_value;
if .has_section(section) and has_section_key(section,key):
old_value = .get_value(section,key);
if new_value != old_value:
.set_value(section,key,new_value);
_is_unsaved = true;
return;
## Shadows ConfigFile.erase_section()
## Adds logic to set saved state and and emit changed signal.
func erase_section(section:String)->void:
print("ExtConfigFile.erase_section()");
var old_value:Dictionary;
if .has_section(section):
# Loop through keys in the section
for key in .get_section_keys(section):
old_value[key] = .get_value(section,key);
else:
return;
.erase_section(section);
_is_unsaved = true;
return;
## Shadows ConfigFile.erase_section_key()
## Adds logic to set saved state and and emit changed signal.
func erase_section_key(section:String,key:String)->void:
print("ExtConfigFile.erase_section_key()");
var old_value = null;
if .has_section(section) && .has_section_key(section,key):
old_value = .get_value(section,key);
else:
return;
.erase_section_key(section,key);
_is_unsaved = true;
return;
## Shadows ConfigFile.parse()
## Adds logic to set saved state and and emit changed signal.
func parse(data:String,auto_save:bool=false)->int:
print("ExtConfigFile.parse()");
var err:int;
err = .parse(data);
_is_unsaved = true; # Assume values were changed even if the parse failed
return err;
junk.ini after run.
[my_section]
my_value2=2
@tx350z It didn't actually work in 3.x. Bringing it back, if it even was sensible, would only create an illusion that you can override built-in methods when in fact you cannot. And shadowing is not a feature of any language, it's a bug or a code smell at the very least.
Thats not true! I still using override built-in methods on Godot 3.5.1 as build a mock/spy by fuction doubling and calling than the original implementation.
In 3.x you can "override" non-virtual methods, but only for GDScript, not C++. That is, if some method is called directly, the C++ original will be called, not your GDScript override (this does not apply to calling via Object.call
). This is especially noticeable when trying to override a property setter or getter.
@tx350z Yes, this will work for scripting, but it won't work if the engine needs to call the same method that you are trying to override. That's why we make a distinction between overriding and shadowing. Shadowing is a potential bug and code smell, we don't recommend getting into this situation, so there is little reason to support it. And apparently, avoiding it improves the execution speed of GDScript VM. Overriding of non-virtual methods is not available at all.
I disagree with the claim "there is little reason to support it". I can't think of a single use case where I want the engine to call my custom functionality. So I'll depart this discussion.
For my needs I'm now building an editor plugin that automates code generation of wrapper classes around built-in classes so I can extend functionality as needed. The result will no doubt be slower than having built-in support but it is what it is.
P.S. My hope was 4.0 would support polymorphism but that's not likely to happen if simple method overrides will never be supported.
I think we have two options:
I can't think of a single use case where I want the engine to call my custom functionality.
Whether you generally want to or not, if it was true overriding then the expectation from a programmer would be that the engine would call the override and not the original. Of course, not every public/exposed method is called by the engine, but some do, and consistency is expected. Allowing just shadowing wouldn't be very OOP of us, would it?
And overrides in general are allowed, but we have strict rules that the method should be declared as virtual. It's just not all built-in methods are, for one reason or another (already explained in the discussion above). This is all completely within the OOP principles.
Whether you generally want to or not, if it was true overriding then the expectation from a programmer would be that the engine would call the override and not the original. Of course, not every public/exposed method is called by the engine, but some do, and consistency is expected. Allowing just shadowing wouldn't be very OOP of us, would it?
And overrides in general are allowed, but we have strict rules that the method should be declared as virtual. It's just not all built-in methods are, for one reason or another (already explained in the discussion above). This is all completely within the OOP principles.
Four Pillars of OOP: 1 Inheritance: child classes inherit data and behaviors from the parent class with the ability to extend or replace parent behaviors 2 Encapsulation: containing information in an object, exposing only selected information 3 Abstraction: only exposing high-level public methods for accessing an object 4 Polymorphism: functions bearing the same name, but each may have different behaviors
Reopening for now as #72487 was reverted.
Consequences:
● String get_class() const
Returns the object's built-in class name, as a String. See also is_class().
Note: This method ignores class_name declarations. If this object's script has defined a class_name, the base, built-in class name is returned instead.
SCRIPT ERROR: Parse Error: The method "get_class" overrides a method from native class "Object". This won't be called b)
Yes, and this was never going to work correctly.
Godot version
4.0.dev 7791599d5bcfdbae0d2a96e5fc13d9021ec820be
System information
Linux binogure 5.14.0-2-amd64 #1 SMP Debian 5.14.9-2 (2021-10-03) x86_64 GNU/Linux
Issue description
I don't know if it is normal, but I cannot override any godot's native functions using GDScript. It never calls the gdscript function but the godot's core function instead.
Steps to reproduce
var __data = {}
func _ready(): set('random_key', 'random value')
func set(key, value): print('Setting a new value') __data[key] = value