godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.07k stars 69 forks source link

Add the ability to export sub classes #9343

Open sydist opened 3 months ago

sydist commented 3 months ago

Describe the project you are working on

I'm working on an rpg where I want different characters to have different "indicators" for when they're talking with the text box. Maybe one character would float, another would switch to a animated sprite where the mouth moves, etc....

Describe the problem or limitation you are having in your project

Like any programmer I pull out the Strategy pattern, I would like to export the variable of type "TalkStrategy" to the property editor. However, I can't do this because resources only appear in the property editor if they're in their own script file! So I would have to create seperate script files when I need them to be consolidated in one place (the npc class, since only npcs will ever talk and only they will ever have the talking strategy)

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Instead I would rather be able to inline classes inside the npc class that define the behaviour for different types of talking strategies, and be still be able to select those from the editor. That way everything is encapsulated neatly.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

@export var talk_strategy: TalkStrategy

func animate_talk():
    talk_strategy.talk(sprite)

class TalkStrategy extends Resource:
    func talk(_sprite: Sprite2D):
        pass

class TalkStrategyAnimation extends TalkStrategy:
    @export var animated_texture: AnimatedTexture

    func talk(sprite: Sprite2D):
        if sprite.texture != animated_texture:
            animated_texture.current_frame = 0
            sprite.texture = animated_texture

class TalkStrategyFloat extends TalkStrategy:
    @export var speed: float = 1.0

    func talk(sprite: Sprite2D):
        sprite.offset.y += sin(Time.get_ticks_msec())

If this enhancement will not be used often, can it be worked around with a few lines of script?

I wasn't able to find a way to work around this.

Is there a reason why this should be core and not an add-on in the asset library?

It's intuitive for the property editor to be able to detect sub classes of the node.

AThousandShips commented 3 months ago

This would be very complicated and have very limited use I'd say, implementing this would be difficult and error prone, and would not be used by the vast majority of people

sydist commented 3 months ago

I believe it's very important for the clarity of code. Having to make seperate files for logic that belongs to another class causes confusion in the code base. Unless there's a way to avoid this and I'm missing it.

AThousandShips commented 3 months ago

If they're internal to the class they are internal, no? That would logically mean they're not to be exported IMO

I'd say that if this is a serious issue for you you might need to rethink how you structure things, I'm not sure your data is well structured, rather than this being an issue with the engine or missing feature

sydist commented 3 months ago

I understand your point about internal class elements not needing to be exported. However, my concern is more about the organization and readability of the code. When a class grows large, having related logic in separate files can make it harder to navigate and maintain the codebase. Now include the amount of times you need a strategy pattern "exported" and it just becomes a mess.

Essentially it's a less annoying version of exporting an enum and doing a switch statement that links the enum with the strategies

AThousandShips commented 3 months ago

I'd argue the opposite is true, having exported details not be in dedicated files makes it impossible to find them

I'd suggest exporting the relevant data directly instead

dalexeev commented 3 months ago

I think there was a miscommunication here. Probably @sydist did not mean that the exported variables of the inner class should somehow be displayed in the inspector of the outer class (without instantiation the inner class as a property of the outer class). I think that @sydist meant a bug that the inspector cannot work with non-global classes (GlobalClass is ok, but not GlobalClass.InnerClass, "res://unnamed_class.gd" or "res://unnamed_class.gd".InnerClass). This is a problem, we don't have a unified type system or at least a universal FQTN (fully qualified type name) that the inspector and other parts of the engine can work with.

hilfazer commented 3 months ago

The concept OP means isn't a sub class but rather an inner (a.k.a nested) class.

sydist commented 3 months ago

i did not even notice the inner classes had @exports in them. LOL. completely my fault, my bad. and yes I do mean inner classes (although im not sure what sub classes are in this case then, i thought they meant the same thing.)