Allow type checking of child classes using the `is` keyword

[Error7Studios]

Describe the project you are working on: RPG

Describe the problem or limitation you are having in your project: Using the is keyword to check if an object is an instance of a child class doesn't work.

Using a variable instead of self will make the error "go away", but you'll get a cyclic dependency when you try to run the game.

Describe the feature / enhancement and how it helps to overcome the problem or limitation: In the script editor, when using the is keyword to check if object A is an instance of class B, only show an error if class B does not extend class A. When running the code, return true if object A is an instance of class B as usual. This would allow you to put all the code in the base class script instead of in each child class script. (This would require solving the cyclic dependency problem, though)

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: (pseudo code - when Godot sees the is keyword)

var object_A = < object before is keyword >
var class_A = < class of object_A >
var class_B = < class after is keyword >

if game_is_running:
    return (object_A is class_B)
else:
    # (in editor)
    if class_B extends class_A:
        pass
    else:
        < Error >

If this enhancement will not be used often, can it be worked around with a few lines of script?: I've been working around this issue by overriding get_class() in each of the child classes, and then using:

match __character_object.get_class():
        "Player":
            < Player code >
        "NPC"
            < NPC code >
        _:
            < Error, forgot to override get_class() >

Is there a reason why this should be core and not an add-on in the asset library?: This is a core script parser feature.

This seems like it should work, since you can say "This animal is a dog", and it makes perfect sense. Even though dog is a "subclass" of animal, it's still a logically correct statement.

It would just be simpler if the is keyword worked this way, so that the user doesn't have to remember to override get_class() and check that every time.

[KoBeWi]

This totally kills the purpose of polymorphism. Why don't you just override _ready in the subclass?

[Error7Studios]

It's not just the ready function. Anywhere there's a complex function that would require different behavior for each child class, I can either put a single match statement in the base class or override it 500 times in all the child classes, which is easy to forget to do.

[dalexeev]

Yes, this is not a very good style of code, but it also has a right to life. Also, the error message is clearly wrong, something like "A rectangle can never be a square."

[jonbonazza]

This can't be added even if we wanted to. It would create a cyclic reference

[KoBeWi]

override it 500 times in all the child classes, which is easy to forget to do.

Well, if you forget to override something, then you probably didn't need to override it, because it already works 🙃

Are you saying that you have 500 sub-classes of something? Why would you need that many? Maybe we could add an annotation that warns you if a method is not overriden in a sub-class.

This can't be added even if we wanted to. It would create a cyclic reference

Might not be a problem in 4.0

[jonbonazza]

As a workaround, could you do something like

if self as Player:
    # result is not null so it is actually an instance

[Error7Studios]

Well, if you forget to override something, then you probably didn't need to override it, because it already works

Not if the base function handles just the boilerplate code, and the additional custom behavior is determined by the class of the object calling the function. In that case it would only half work. The function in the subclass would be:

func some_func():
    .some_func()
    < custom behavior here >
    ...

Are you saying that you have 500 sub-classes of something?

Well, 500 was hyperbole, but it would be a lot of overrides for different functions. I'm actually using this for a menu/inventory system, but the Character/Player/NPC example was just easier to understand. I have a Menu class which populates a VBoxContainer with several custom BTN class objects. I'm composing multi-column menus by putting multiple 'Menu' instances stuck together. For example, a shopkeeper's inventory menu might look like this:

Potion  10  $5
Herb    8   $3
Sword   1   $150

Each column is a different Menu instance, where all the BTN objects have a tag array variable set depending on the class of the Menu instance they belong to.

So far, that section looks like this:

        match get_class():
            G.CUSTOM_CLASS_NAME.INVENTORY_MENU:
                var __tag: String = G.get_tag_from_inventory_object_name(__text)
                match __tag:
                    G.TAG.WEAPON, G.TAG.ARMOR, G.TAG.ITEM:
                        __BTN.add_tag(__tag)
                    _:
                        assert(false)

            G.CUSTOM_CLASS_NAME.DIALOGUE_CHOICE_MENU:
                __BTN.add_tag(G.TAG.DIALOGUE_CHOICE_MENU_BTN)

            G.CUSTOM_CLASS_NAME.PLAYER_MENU:
                __BTN.add_tag(G.TAG.PLAYER_MENU_BTN)

            G.CUSTOM_CLASS_NAME.INVENTORY_COUNTS_MENU:
                __BTN.add_tag(G.TAG.INVENTORY_COUNT_BTN)

            G.CUSTOM_CLASS_NAME.SHOP_INVENTORY_MENU:
                var __tag: String = G.get_tag_from_inventory_object_name(__text)
                match __tag:
                    G.TAG.WEAPON, G.TAG.ARMOR, G.TAG.ITEM:
                        var __shop_tag := "Shop " + __tag
                        assert(G.TAG.values().has(__shop_tag))
                        __BTN.add_tag(__shop_tag)
                    _:
                        assert(false)

            G.CUSTOM_CLASS_NAME.SHOP_INVENTORY_PRICES_MENU:
                __BTN.add_tag(G.TAG.SHOP_PRICE)

            _:
                print("get_class(): ", get_class())
                assert(false)

Based on the BTN's tags, different things will happen when you hover over it or click it. But if this practice is really that frowned upon, I can rework it using overrides.

As a workaround, could you do something like

if self as Player:

result is not null so it is actually an instance

I just tried it, and no. self as Player is an invalid cast.

var __self = self
__self as Player

Is a script error or cyclic reference.

This can't be added even if we wanted to. It would create a cyclic reference

I'm not suggesting you do this, because I have no idea how much work it would take, but why does it need to create a cyclic reference to just check if a class extends another? I don't know C++ and haven't looked Godot's source code, but couldn't there just be a global text file that stores each script name and their inheritance hierarchy?

It would just look like the top of each class documentation file, just modified a bit. Class: VBoxContainer --- Inherits: BoxContainer < Container < Control < CanvasItem < Node < Object

That text file would have lines for all the scripts that you use the class_name keyword on (plus built-in classes). Then when you check the class type using the is keyword, it would just open that text file to check the class inheritance chain. You're not trying to call a function on the class, you're just checking the inheritance, so I don't know why it has to load/open the class file and create the cyclic dependency in the first place. Again, I have no clue how type checking and cyclic dependencies actually work, so I know it's not that simple.

Maybe we could add an annotation that warns you if a method is not overridden in a sub-class.

That would be pretty useful, but I'm handling that by just putting an assert(false, "Must be overridden") at the bottom of the function I need to override.

Admittedly, I'm not well-versed in programming best practices, since GDScript is my first programming language. I know I'm ignorant on the topic, but sometimes it seems like the "best practice" actually violates the DRY principle. Why override a function in multiple classes when I can just put a match statement in a base class? It's one action vs multiple actions (i.e., repeating yourself).

Also, if the code changes a lot, I would have to then go and change all of the code in each of the overridden functions in each subclass, instead of just in one place (the base class). This doesn't sound a whole lot better than copy-pasting a magic number all over the place instead of saving it into a variable and reusing it. The end result is a similar hassle and bug-risk when something changes.

[dalexeev]

Option 1, normal:

tool
extends EditorScript

class Character:
    var msg = "I'm an abstract something"

    func hello():
        print(msg)

class Player extends Character:
    func _init():
        msg = "I'm the Player"

class NPC extends Character:
    func _init():
        msg = "I'm an NPC"

func _run():
    var player = Player.new()
    player.hello()

Option 2, almost-as-you-want:

tool
extends EditorScript

class Character:
    var _class = "Character"

    func hello():
        match _class:
            "Character":
                print("I'm an abstract something")
            "Player":
                print("I'm the Player")
            "NPC":
                print("I'm an NPC")

class Player extends Character:
    func _init():
        _class = "Player"

class NPC extends Character:
    func _init():
        _class = "NPC"

func _run():
    var player = Player.new()
    player.hello()

Option 3, other:

If you have many classes that are slightly different from each other, then perhaps the number of classes should be reduced, and the difference in behavior should be implemented using variables. Perhaps, it is generally more convenient to store some data in a file rather than in a code. Especially when it comes to character stats.

[Error7Studios]

Perhaps, it is generally more convenient to store some data in a file rather than in a code.

Thanks, good idea. You're right. I think that's the best solution. I'll just make a base class, and convert a spreadsheet with the unique data for all the subclasses to a .txt file, and read from that file in the ready function to set the variable(s). Is that what you mean?

[dalexeev]

Is that what you mean?

Yes, this is one of the solutions to the problem. You know better what is best for your case.

[YuriSizov]

I know I'm ignorant on the topic, but sometimes it seems like the "best practice" actually violates the DRY principle. Why override a function in multiple classes when I can just put a match statement in a base class?

Because DRY is not as fundamental as encapsulation. The implementation of the child's logic is the concern of that child alone and nobody else. More often than not this implementation also relies on some inner properties, and it's pretty awful if you start defining child specific properties on the parent just to keep your huge match/switch pattern working.

[MikeFP]

Even if the cyclic dependency is inevitable when checking the current type for subclasses, the error "A value of type A will never be an instance of B" persists even when making the check on another unrelated class. Is that intended? IMO that's totally unexpected.

[MajorMcDoom]

I think some people are assuming that the check has to happen within the base class, which derails this discussion into one about polymorphism. This check could totally happen somewhere else. Additionally, traditional polymorphism often doesn't apply in Godot because scripts, by design, can extend one class, but be attached to a node of a more specific class.

For example, I can have a script Thing that extends from PhysicsBody3D, but I can attach it to a RigidBody3D or a StaticBody3D. 99% of this script's functionality applies to any kind of PhysicsBody3D, with a single line that wants to distinguish between RigidBody3D and StaticBody3D. This error makes it impossible to check (from inside the Thing script or otherwise) which case it is.

[dalexeev]

For example, I can have a script Thing that extends from PhysicsBody3D, but I can attach it to a RigidBody3D or a StaticBody3D.

I think this should not be allowed (since Script.get_instance_base_type() will return the wrong actual type), this is probably a bug. Or at least not the main use case. Checking for a subclass in a base class is still bad style, as we discussed above.

To check for native classes in your case, you can use the Object.is_class() method instead of the is operator.

[MajorMcDoom]

I think this should not be allowed (since Script.get_instance_base_type() will return the wrong actual type), this is probably a bug. Or at least not the main use case.

It is allowed and is not a bug. If you write a script that extends Node3D, that means it can be attached to any Node3D or subclass. This is super common.

Checking for a subclass in a base class is still bad style, as we discussed above.

Except you don't have to do the check in the base class. That was the first point I made.

To check for native classes in your case, you can use the Object.is_class() method instead of the is operator.

This requires passing a string, so it's probably less performant, but I'm not 100% sure on that one.

[Arecher]

I think this is something that the average Godot user runs into a lot. I personally know I do. It's absolutely possible to work around it with get_class() but the is operator is the way user friendlier and more readable version. It's also messy to be inconsistent in how you check classes, and switch between is and get_class() constantly. It would be nice to just be able to choose your preferred format, and be consistent with it throughout your project.

Personally I run into this a lot when creating custom classes that inherit high level behaviour.

In this scenario, my BaseComponent extends from Node3D because it's a pretty high level class, and there is no need to specify it to only work on a more specific inherited node class. I made a few different components, nearly all of which inherit from PhysicsBody3D. There is just the one that inherits from a simple Node3D. Which causes errors, because a Node3D cannot set itself as a collision exception.

I can add a new script in that component and overwrite this one function to do nothing (and also do that for all future components that do not inherit from PhysicsBody3D), or I can simply add validation to the high level class, and make sure the problem is solved forever with a single check.

And the silly thing is that this just works. Which makes the entire error feel like a complete false alarm every time I see it.

In general the error also feels like it's saying that Nodes cannot be more than one class, but all the node types in Godot use inheritance, and therefore are a whole range of classes. So to have it tell you that "there is no way this object will EVER be this class" is silly.