godot: GDScript cannot override Godot's native functions

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

  • Create a new project
  • Attach this script to the main node:
extends Node

var __data = {}

func _ready():
    set('random_key', 'random value')

func set(key, value):
    print('Setting a new value')
    __data[key] = value

The set function is never called, and there is no warning/error in godot explaining that is prohibited.

Minimal reproduction project

No response

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Comments: 58 (39 by maintainers)

Most upvoted comments

@MikeSchulze Again, this didn’t work in Godot 3.x either. It is not related to GDScript, it’s just how Godot is, and always was. If a method is not made to be overridable, you can’t override it, neither from scripting, nor from extensions. The only change in GDScript that is relevant here is that we now tell the user that what they are trying to do is wrong and is not going to work.

There are no plans to allow overriding any core function, because it’s impractical from the performance perspective. And once again, this is not a decision of the GDScript team, it’s a core decision.

Furthermore, cases like get_class are a different kind of issue entirely. Users don’t want to override this method, they were trying to override it because it doesn’t work as expected with custom classes out of the box. We need to address the underlying issue here instead of pleading to make the entire core extensible.

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: image

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.

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.

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

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.

Shouldn’t it be _set? I don’t recall the engine expecting set to be overridable

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.

To 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.

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.

Even though it did work as needed in 3.x.

In 3.x, this is also not overriding, but shadowing. You’re just creating another method with the same name, which is definitely confusing. In 4.0, an additional problem is that even GDScript can’t always distinguish between these methods. We could fix this, but it would hurt performance for no good reason, since the behavior in 3.x is still not correct and expected by users (“I’ve overridden queue_free(), why isn’t it called, when the node is removed by the engine?”).

Discussion in Rocket Chat

Duplicate another my comment:


Why it’s done:

  • The engine doesn’t call your “overrides” in most cases (unless it calls them via call()). This behavior is also present in 3.x, it’s not a bug, it can’t be fixed without a huge performance hit, it would introduce a lot of new bugs due to the violation of the engine’s expectations when the user overrides the method incorrectly.
  • Unlike 3.x, the “override” call may not happen even in GDScript, due to type collisions (in some cases, GDScript calls a native method for optimization, unless you work around it with a cast or call()). For this reason, non-virtual native methods should never be “overridden”, even if you know about the previous point and don’t worry about it.

A few tips:

  1. You can disable or ignore this warning, but correct behavior is not guaranteed.
  2. This warning does not apply to native virtual methods and any custom methods. You can override them.
  3. If you need to customize some engine method, you should create a wrapper with a different name, not “override” it.

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.

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.

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.

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.

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.

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.

@tx350z It was a bug/inconsistency that you exploited

Yes, and this was never going to work correctly.

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.

@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.

@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 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.

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?

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.

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.

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 🙂

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 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

This was changed in #54676. It’s being reconsidered / further discussed in #54949.

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.