godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.17k stars 21.19k forks source link

Better custom EditorPlugin nodes #6067

Closed zatherz closed 4 years ago

zatherz commented 8 years ago

Nodes created by the add_custom_type function in EditorPlugin have the selected script assigned to it when adding. This makes them almost useless, as you can only use the functions defined in that script in other nodes.

This is completely different from other nodes and makes node addons pretty much useless/much more annoying to use.

reduz commented 8 years ago

I don't understand what you mean

vnen commented 8 years ago

@reduz when you add to the scene a node which a type created by a plugin, it already has the plugin script attached. So it's impossible to add another script with custom behavior.

reduz commented 8 years ago

Of course not, this is by core design and will not change.

On Aug 7, 2016 18:11, "George Marques" notifications@github.com wrote:

@reduz https://github.com/reduz when you add to the scene a node which a type created by a plugin, it already has the plugin script attached. So it's impossible to add another script with custom behavior.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-238108767, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z29F5q8PaoBv4OrzAUayzrfNjfHyZks5qdkoUgaJpZM4JejbZ .

vnen commented 8 years ago

This is sad. But you can always add the script to the parent or to a child.

Closing as wontfix.

reduz commented 8 years ago

I may be misunderstanding something, but if your custom type is a script, how will it not be included in the created node? It doesn't make sense for it to be different

On Aug 7, 2016 9:52 PM, "George Marques" notifications@github.com wrote:

This is sad. But you can always add the script to the parent or to a child.

Closing as wontfix.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-238120392, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z27Zo4Ixvo4APSC4Fxf5ZqCJRsAxXks5qdn3igaJpZM4JejbZ .

vnen commented 8 years ago

I guess it would need the ability of having two (or more) scripts per node, though this really does not make much sense.

reduz commented 8 years ago

Godot orginally supported this, but it was more trouble than advantage

On Aug 7, 2016 10:36 PM, "George Marques" notifications@github.com wrote:

I guess it would need the ability of having two (or more) scripts per node, though this really does not make much sense.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-238123729, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z27jidVZl-hHWW3G_ESr8Cqj3eX7Eks5qdogRgaJpZM4JejbZ .

zatherz commented 8 years ago

The problem is that custom nodes are next to useless as they aren't really "custom nodes", they're just base nodes with a pre-set script and different icon. What I expected from "custom nodes" is that I could extend the node to use whatever is defined in the script. Example scenario: I have a custom node called Test that is a child of Sprite and adds a test() function that returns true. Then, I would like to create a new Test node, assign a script to it and use the test() function. This is impossible.

I guess back to the scene to inherit + script to extend combo then.

reduz commented 8 years ago

Well, being a preset node with a pre-set script and a different icon is IMO enough of custom, but if you really want to put your own custom code in there, you can always inherit the ones that comes with the node. We could make this a bit easier from the UI if really desired.

On Mon, Aug 8, 2016 at 10:40 AM, Dominik Banaszak notifications@github.com wrote:

The problem is that custom nodes are next to useless as they aren't really "custom nodes", they're just base nodes with a pre-set script and different icon. I guess back to the scene to inherit + script to extend combo then.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-238240130, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z27eHoXwgr6buF6CCAHfxwx1dKkCiks5qdzHhgaJpZM4JejbZ .

zatherz commented 8 years ago

I'd love if it could be automated/made easier through the UI.

akien-mga commented 8 years ago

As I understand it the expectation is to have the possibility to create custom nodes that will behave like built-in nodes, i.e. they should have the following features:

That would be the "natural" expectation when declaring a custom node, and would be a very powerful feature; I gather from the above that it does not work this way so far, partly due to design decisions. If there's a way to have a functionality such as what I described above, I'm pretty sure it would have a lot of applications. The assetlib would be full of custom nodes that do a lot of work out of the box and can be used as if they were built-in.

Reopening until there's a consensus on what can/should be done or not.

ghost commented 8 years ago

+1 This was one the first major roadblocks for me when I tried to port an existing project from "OtherEngine(tm)" to Godot. Custom types, like @akien-mga explained above, should just behave like any other built in type after registration.

reduz commented 8 years ago

Please explain in which way you believe they do not

On Aug 8, 2016 11:50 AM, "Ralf Hölzemer" notifications@github.com wrote:

+1 This was one the first major roadblocks for me when I tried to port an existing project from "OtherEngine(tm)" to Godot. Custom types, like @akien-mga https://github.com/akien-mga explained above, should just behave like any other built in type after registration.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-238261603, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z25PJzv9w7iXO350tRF3FcEuYQYUKks5qd0IrgaJpZM4JejbZ .

ghost commented 8 years ago

As already said previously, the biggest drawback at the moment is that custom types are little more than a quick way of instancing the base type of that script and assign the appropriate script to that instance. This makes extending that node in the editor with another script impossible - like you can do with built in types.

But it is also impossible to build inheritance trees with custom nodes in the "Create new Node/Resource" dialogs because they only show up in these trees when you register them with a built in type as a parent via add_custom_type.

For example, lets assume I want to inherit all my custom types in my project from a single base type.

base.gd

extends Node
export (Color) var color

type_a.gd

extends base.gd

type_b.gd

extends base.gd

As it is right now, I have to register those types like this. In this case, the second argument of add_custom_type has to be "Node", otherwise they won't show up in the dialogs.

func _enter_tree():
    add_custom_type("Base", "Node", preload("base.gd"), preload("base.png"))
    add_custom_type("TypeA", "Node", preload("type_a.gd"), preload("type_a.png"))
    add_custom_type("TypeB", "Node", preload("type_b.gd"), preload("type_b.png"))

... and this is the result.

add_node_flat

While it is nice to be able to register custom types like this, the dialogs don't reflect the nature of the inheritance of those types like other built in types. For any built in type, I can look at that tree and see at a glance what I am dealing with. I can, for example, be sure that Sprite is a Node2D and therefor inherits all of the functions provided by Node2D. The same is not true for custom types.

Now, if custom types could be fully registered into the global scope, like @akien-mga mentioned above, things would be much simpler to understand and use.

First, you could inherit from a custom type referenced by type name instead of the file path/name.

base.gd

extends Node
export (Color) var color

type_a.gd

extends Base

type_b.gd

extends Base

... then, the registration of the custom types could be simplified like this. Note the missing second parameter of add_custom_type.

func _enter_tree():
    add_custom_type("Base", preload("base.gd"), preload("base.png"))
    add_custom_type("TypeA", preload("type_a.gd"), preload("type_a.png"))
    add_custom_type("TypeB", preload("type_b.gd"), preload("type_b.png"))

... and you would get a much nicer overview in the "Create new Node/Resource" dialogs:

add_node_tree

... and of course the ability to extend those types in the editor with a custom script:

custom_type_no_script

... which would also be referenced by type name instead of the script name/path

extends Base

Here's the example plugin from above to play around. addons.zip

reduz commented 8 years ago

Oh I see.. these two thigs should definitely be fixable, together with adding inheritance support to script create dialog

On Aug 8, 2016 1:54 PM, "Ralf Hölzemer" notifications@github.com wrote:

As already said previously, the biggest drawback at the moment is that custom types are little more than a quick way of instancing the base type of that script and assign the appropriate script to that instance. This makes extending that node in the editor with another script impossible - like you can do with built in types.

But it is also impossible to build inheritance trees with custom nodes in the "Create new Node/Resource" dialogs because they only show up in these trees when you register them with a built in type as a parent via add_custom_type.

For example, lets assume I want to inherit all my custom types in my project from a single base type.

base.gd http://base.gd

extends Node export (Color) var color

_type_a.gd http://type_a.gd_

extends base.gd

_type_b.gd http://type_b.gd_

extends base.gd

As it is right now, I have to register those types like this. In this case, the second argument of add_custom_type has to be "Node", otherwise they won't show up in the dialogs.

func _enter_tree(): add_custom_type("Base", "Node", preload("base.gd"), preload("base.png")) add_custom_type("TypeA", "Node", preload("type_a.gd"), preload("type_a.png")) add_custom_type("TypeB", "Node", preload("type_b.gd"), preload("type_b.png"))

... and this is the result.

[image: add_node_flat] https://cloud.githubusercontent.com/assets/8785013/17486294/9bcc029c-5d90-11e6-81e6-6fce6b0e7cf0.png

While it is nice to be able to register custom types like this, the dialogs don't reflect the nature of the inheritance of those types like other built in types. For any built in type, I can look at that tree and see at a glance what I am dealing with. I can, for example, be sure that Sprite is a Node2D and therefor inherits all of the functions provided by Node2D. The same is not true for custom types.

Now, if custom types could be fully registered into the global scope, like @akien-mga https://github.com/akien-mga mentioned above, things would be much simpler to understand and use.

First, you could inherit from a custom type referenced by type name instead of the file path/name.

base.gd http://base.gd

extends Node export (Color) var color

_type_a.gd http://type_a.gd_

extends Base

_type_b.gd http://type_b.gd_

extends Base

... then, the registration of the custom types could be simplified like this. Note the missing second parameter of add_custom_type.

func _enter_tree(): add_custom_type("Base", preload("base.gd"), preload("base.png")) add_custom_type("TypeA", preload("type_a.gd"), preload("type_a.png")) add_custom_type("TypeB", preload("type_b.gd"), preload("type_b.png"))

... and you would get a much nicer overview in the "Create new Node/Resource" dialogs:

[image: add_node_tree] https://cloud.githubusercontent.com/assets/8785013/17487264/619f4da0-5d94-11e6-880f-a00791e30125.png

... and of course the ability to extend those types in the editor with a custom script:

[image: custom_type_no_script] https://cloud.githubusercontent.com/assets/8785013/17487807/b54aced2-5d96-11e6-90e5-ee838b6a1335.png

... which would also be referenced by type name instead of the script name/path

extends Base

Here's the example plugin from above to play around. addons.zip https://github.com/godotengine/godot/files/407291/addons.zip

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-238299152, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z20FQioFVkVcvhhbvDQ7jQKv5De7Hks5qd19QgaJpZM4JejbZ .

reduz commented 8 years ago

What will never happen is scripts dissapearing from the instanced nodes, or being hidden, It needs to be made clear that the node is scripted, but adding a script to it will still allow to replace the script for one that inherits from it

On Aug 8, 2016 2:02 PM, "Juan Linietsky" reduzio@gmail.com wrote:

Oh I see.. these two thigs should definitely be fixable, together with adding inheritance support to script create dialog

On Aug 8, 2016 1:54 PM, "Ralf Hölzemer" notifications@github.com wrote:

As already said previously, the biggest drawback at the moment is that custom types are little more than a quick way of instancing the base type of that script and assign the appropriate script to that instance. This makes extending that node in the editor with another script impossible - like you can do with built in types.

But it is also impossible to build inheritance trees with custom nodes in the "Create new Node/Resource" dialogs because they only show up in these trees when you register them with a built in type as a parent via add_custom_type.

For example, lets assume I want to inherit all my custom types in my project from a single base type.

base.gd http://base.gd

extends Node export (Color) var color

_type_a.gd http://type_a.gd_

extends base.gd

_type_b.gd http://type_b.gd_

extends base.gd

As it is right now, I have to register those types like this. In this case, the second argument of add_custom_type has to be "Node", otherwise they won't show up in the dialogs.

func _enter_tree(): add_custom_type("Base", "Node", preload("base.gd"), preload("base.png")) add_custom_type("TypeA", "Node", preload("type_a.gd"), preload("type_a.png")) add_custom_type("TypeB", "Node", preload("type_b.gd"), preload("type_b.png"))

... and this is the result.

[image: add_node_flat] https://cloud.githubusercontent.com/assets/8785013/17486294/9bcc029c-5d90-11e6-81e6-6fce6b0e7cf0.png

While it is nice to be able to register custom types like this, the dialogs don't reflect the nature of the inheritance of those types like other built in types. For any built in type, I can look at that tree and see at a glance what I am dealing with. I can, for example, be sure that Sprite is a Node2D and therefor inherits all of the functions provided by Node2D. The same is not true for custom types.

Now, if custom types could be fully registered into the global scope, like @akien-mga https://github.com/akien-mga mentioned above, things would be much simpler to understand and use.

First, you could inherit from a custom type referenced by type name instead of the file path/name.

base.gd http://base.gd

extends Node export (Color) var color

_type_a.gd http://type_a.gd_

extends Base

_type_b.gd http://type_b.gd_

extends Base

... then, the registration of the custom types could be simplified like this. Note the missing second parameter of add_custom_type.

func _enter_tree(): add_custom_type("Base", preload("base.gd"), preload("base.png")) add_custom_type("TypeA", preload("type_a.gd"), preload("type_a.png")) add_custom_type("TypeB", preload("type_b.gd"), preload("type_b.png"))

... and you would get a much nicer overview in the "Create new Node/Resource" dialogs:

[image: add_node_tree] https://cloud.githubusercontent.com/assets/8785013/17487264/619f4da0-5d94-11e6-880f-a00791e30125.png

... and of course the ability to extend those types in the editor with a custom script:

[image: custom_type_no_script] https://cloud.githubusercontent.com/assets/8785013/17487807/b54aced2-5d96-11e6-90e5-ee838b6a1335.png

... which would also be referenced by type name instead of the script name/path

extends Base

Here's the example plugin from above to play around. addons.zip https://github.com/godotengine/godot/files/407291/addons.zip

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-238299152, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z20FQioFVkVcvhhbvDQ7jQKv5De7Hks5qd19QgaJpZM4JejbZ .

ghost commented 8 years ago

@reduz Is there some technical reason to make it clear that the node is scripted and if so, does it have to be an occupied script slot?

It seems to me a rather clumsy way of indicating a custom type and I think nobody would come up with the idea that by replacing the current script on a node, you get a script that extends the script that was already there. That's just not the way extending via script works for the base types.

And what would happen if the user clears that script field? Does the custom type revert back to the previous script or does it completely revert back to the base type? Again, I don't think this would be a good idea.

Instead, it could just be indicated as a custom type in the node tree or the properties editor via different color, some icon or something else, without sacrificing the empty script slot.

akien-mga commented 8 years ago

What will never happen is scripts dissapearing from the instanced nodes, or being hidden, It needs to be made clear that the node is scripted, but adding a script to it will still allow to replace the script for one that inherits from it

The point here is that the script that defines the custom node should be hidden, because it's not a property of the instanced node but of its type. So this script should confer properties to the custom node, but it should be as invisible to the user of the instanced node as the C++ classes of built-in nodes are. It would provide an API, but would not be modifiable, only extend-able. Just like when you instance a Sprite, you don't get scenes/2d/sprite.cpp attached as the script of the instanced node, you shouldn't get my_custom_node.gd attached as the modifiable script of the instance custom node.

Now, I don't know if it's technically possible right now, but it would be the natural use case AFAIU. If you modify the script of the custom type, it would modify the type itself, and thus would impact all instances of this type. But the instanced nodes using the custom type should have their own script that extends CustomNode.

Zylann commented 8 years ago

I think that feature would need Object to have an additional pointer to the "base custom script type", because you need that information when you want to remove a user script from it (which should actually replace it by the base script). Once you have that, the rest is a matter of including it to all cases, as it may introduce numerous side-effects. In the end, there will be only one script attached, just a different way of dealing with it. I'm not a big fan of inheritance in general, but this is how I would do it.

Zylann commented 8 years ago

Actually, that pointer could not even be needed. Marking the script would be enough, for example if you add_custom_type() with a script, the engine can set a flag on the class so the information is available, as "hey, this script class is an engine type extension". Removing a user script would then replace it with the first inherited script class marked as "custom type", or remove it if there are none.

reduz commented 8 years ago

Sorry, I am against hidding a script if the node has a script. What's the point of simulating something that isnt?

The fact it has a script does not mean the slot is busy or that it needs a second script, because you can simply create a new script inheriting the existing one.

What we can do, if you agree, is hide the script icon in the scene tree if the script assigned is the one from the custom type, and make the script create dialog automatically offer you to inherit upon script creation. Would this be enough?

On Aug 10, 2016 11:01 PM, "Marc" notifications@github.com wrote:

Actually, that pointer could not even be needed. Marking the script would be enough, for example if you add_custom_type() with a script, the engine can set a flag on the class so the information is available, as "hey, this script class is an engine type extension". Removing a user script would then replace it with the first inherited script class marked as "custom type", or remove it if there are none.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-239055986, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z2xLGOhgMk__ZoRW1neRu1aRb5Qr_ks5qeoJogaJpZM4JejbZ .

bojidar-bg commented 8 years ago

@reduz I think that would be good enough :smile:

Zylann commented 8 years ago

@reduz I agree, and I didn't said we need a second script. I was just wondering what would happen if you add a script inheriting the first one (the custom defined by plugin), but then decide to remove it. It would then revert the node to a basic engine type without any script then?

reduz commented 8 years ago

I suppose we could somehow make it more user friendly in that regard

On Aug 11, 2016 06:10, "Marc" notifications@github.com wrote:

@reduz https://github.com/reduz I agree, and I didn't said we need a second script. I was just wondering what would happen if you add a script inheriting the first one (the custom defined by plugin), but then decide to remove it. It would then revert the node to a basic engine type without any script then?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6067#issuecomment-239109334, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z26JUJ0gjaCFwlsIDsINWp3_nqliwks5qeubygaJpZM4JejbZ .

HeartoLazor commented 7 years ago

I dig the part of extends by the node defined Type instead of the path described on this comment https://github.com/godotengine/godot/issues/6067#issuecomment-238299152.

Some more suggestions to add:

Example:

add_custom_type("MyCustomNode", "Node2D", preload("my_custom_node.gd"), preload("icon.png"))

node.get_type() should return "MyCustomNode" instead of "Node2D"

Example: User A makes a plugin for a more precise visibility notifier based on Node2D add_custom_type("PreciseNotifier", "Node2D", preload("precise_notifier.gd"), preload("icon.png"))

Then User B develop a Trigger addon based on the precise notifier on ANOTHER addon folder with another config. add_custom_type("Trigger", "PreciseNotifier", preload("trigger.gd"), preload("icon.png")) and on the trigger.gd script he should extend it with the type name too extends PreciseAddon Of course the user should add both addons for use the trigger one

willnationsdev commented 7 years ago

It seems like one group wants to define a custom node type without a script as one which is still deriving from the custom node type it's based on while the other group wants to define that same scenario as a node that has reverted to its base in-engine type. While I belong to the former group, I can also see the latter group's point in regards to the custom type still being "scripted" and wanting to make that clear in the UI.

A compromise may then be to have an additional UI button on the node's row in the scene tree dock next to its script icon with a sort-of script++ icon. Clicking it would go through the typical "Create a Script" popup window with a script that is already inheriting from the custom type, e.g. extends Base or extends "Base". The defined script would then be created and would immediately replace whatever the preset script was. So you'd still clearly show that there is an already-existing script on the node, but you would also have a familiar interface to easily replace that preset script.

This proposal would probably be less intuitive since it still treats custom nodes somewhat differently from their in-engine counterparts. Thoughts?

henriquelalves commented 7 years ago

Giving my 2 cents way later on this discussion, I think the problem lies is the fact that multiple nodes inheriting an addon share the same original script by default; I don't mind the code visibility, as Juan argued on the very beginning of this thread, is a design choice and also something important to make node behavior transparent to the developer using the addon. But usually you'd want to change the behavior of different nodes in code, and right now the only way of doing so is by removing the original script reference, creating a new script and copy-pasting the base script code. You can't even save as the script of the new addon node to a new .gd file, as this will change the reference to all other nodes using the original script, so there is this three-step copy-paste procedure quirk.

Than again, is not THAT complicated, is just that in this particular case, the save as option on the GDScript editor doesn't behave as I would expect, and I think it would be more UI friendly to have a 'copy and save' button on the GDScript editor to allow quick customization of addons (and architecture-wise, making this button visible makes sense as this is a good approach on creating games on Godot without the need of using inherited scripts).

Zylann commented 7 years ago

@henriquelalves I thought customising custom nodes in code is basically inheritance? Like, extends "addons/thing/thing.gd"? The inherited script will still do the same things as the addon version does, given you know what you're overriding. No need for copy/paste.

henriquelalves commented 7 years ago

@Zylann You are right, but usually I don't like this particular approach because of code visibility and auto-completion quirks (at least on 2.1, I haven't tested it by now). And most of the time I don't want to override methods, only change particular things that are not extended variables on the original addon script. That's what bothers me on the current save as behavior, I can't quickly create the copy of a script without changing every node reference to such script; and solving this in a UI friendly way solves the original problem of having multiple nodes to customize, plus having code visibility and such (at least in my workflow, I can be the only one thinking like this though haha).

Zylann commented 7 years ago

@henriquelalves well I can't stand copy/paste tbh xD And you could also fork the plugin with versionning control and use that instead in the first place.

henriquelalves commented 7 years ago

@Zylann Forking a plugin is adding even more steps to something that should be dead simple, hahaha. I guess I'll stick with manual copy-pasting if this is not a priority, even though I still think the save as behaviour is weird.

Zylann commented 7 years ago

@henriquelalves Copy/pasting IS forking^^ but if done without version control, it will bite you in the back in the future if the plugin gets updated.

ghost commented 7 years ago

The current behavior of having a script attached to the custom node provides almost no benefit to having that custom node as an addon versus simply attaching the script to node. The only benefit is that it shows up in the node dialog, which is not really a benefit at all.

I'm in the camp that says a custom node should behave just like a first-rate built-in type. Otherwise, why bother making/using an addon for it? Just so you can see it in the dialog and not have to click the add script button?

willnationsdev commented 6 years ago

@RodeoMcCabe The problem is that under the hood, custom types are still scripts layered on top of an in-engine node in practice, and that can't be changed since custom type nodes are not compiled into the engine source directly. What we need to do is formulate a concrete set of steps / features that would allow these scripted nodes to simulate, in the user's eyes, the behaviors of an in-engine node. For example...

  1. Adding the node to the "create a node" window (done)

  2. Allowing the user to provide a textual "brief description" of the node for the "create a node" window (doesn't appear to be done? - at least, it's not part of add_custom_type).

  3. Allowing the user to display node hierarchies and define abstract custom types in the "create a node" window. This would likely involve adding a bool to the add_custom_type function for whether it is an abstract type or not. The "Create a node" wizard would need to be updated to block the creation of abstract custom types accordingly.

  4. Have the node SEEM as if it DOES NOT have a script.

    a. The node should not have a "script is on this node" icon in the Scene dock. In order to make things more transparent, perhaps there should be a "this is a custom type node" icon instead. Clicking that could then take the user directly to the custom-type-added script. It breaks the "immersion", but it would be a necessary sacrifice for usability reasons (obviously, you're gonna wanna be able to look and see how a custom type node works, if you want to).

    b. The script property that is displayed in the Inspector should, by default, show itself to be empty, unless the user loads a script onto it, in which case the script must derive from the script type used as the custom type. The user should not have to know the location of the custom type's script file however (the concept that it IS a script should be hidden from them). This means that either the string name for the custom class should be recognized by the GDScript parser like the other in-engine classes (not sure how easy/hard that would be) or there should be some sort of global function to fetch the record via the class name, e.g. for the script in add_custom_type("MyClass", "Node", load(res://addons/git-repo/api/my_class.gd), load(res://addons/git-repo/icons/icon_myclass.svg), the user could make a script with extends MyClass or extends custom("MyClass").

    c. If a user does load a custom-type-derived script onto the node, only then should the "this node has a script" icon show up in the Scene dock.

  5. Whatever editor icon is used for the script should be added to the category block instead of the white-box-like icon currently used for scripts (in property_editor.cpp). This should be part of the __meta__ Dictionary property for the custom type node object.

  6. When you click "add a script" on a custom type, it should prefill the "Inherits" field with whatever method is used in 4b.

  7. If you remove a script, the custom type script should still exist under the hood and NOT be removed. The custom type nodes would effectively use the custom type script as a backup script in cases where the loaded script is set to null. As such, you should still see the base types' editor icon and script properties in the Editor's Scene dock and Inspector dock. I'm already in the process of merging in a feature to replace "Script Variables" with the actual scripts' names, though it would likely need to be updated if all of these changes are added.

  8. Object::get_script() should return null for nodes with a custom type script and no loaded script.

  9. Object::get_property_list, and the analogous functions for methods and signals should include the contents of the custom type script even if there is no loaded script.

  10. There would likely need a second C++ Object function like Object::get_custom_script() or something so that the engine can see if there is a script, even if the scripting side has no awareness of this second script.

  11. Attempts to load a non-custom-type-derived script onto an Object should fail cleanly and report an error (probably an &is_valid reference bool in the associated function) to confirm whether the Object is allowed to do it. The associated Godot Editor scenarios that must provide feedback on this information would also need to be updated to account for this.

This is just scratching the surface, but I think the kind of behavior that the users are looking for is about this extensive (so it's pretty intense). We want to make the existence of custom types accessible if necessary (so have the custom type icon in the Scene dock on the node's row to view it's script), but we also want to hide their existence as much as possible so that we can perceive them as in-engine types. It'll take a lot of work to really do it right since it'll break things in a LOT of places probably.

Does any of this sound good? I'm sure there are more items I'm missing since I'm just pondering it a bit. If this sounds crazy, then just let me know. ;-)

Edit: ah, but reduz's suggestion of just hiding the script icon in the Scene dock if the script matches the custom type script could also be valuable. Only, the get_script() method should still return nothing. Maybe there is a way to do it without having to create a separate script property on the Object itself? Idk, cause there are a lot of assumptions already in the codebase for 1 script per object, which I think we wanna keep, but which are hard to maintain with the custom_type script concept.

ghost commented 6 years ago

Good suggestions, well thought out. I think if all this was implemented it would give the behavior we're looking for, however I think that breaking the 1 script per node rule could be bad news that might resonate through many unexpected parts of the code. Take that with a grain of salt since I don't know the codebase that well and my C++ is mediocre. Reduz stated above that they originally tried having more that one script per node and "it was more trouble that it was worth", which sounds reasonable to me.

Points 1 through 4 are great, and I think achievable without breaking the 1-script rule. Abstract custom types obviously are not a problem since you can't instantiate them anyway and therefore the user can't add a script to it in the editor. However, it's conceivable they might try to do so through code, so necessary checks and errors would have to be put in place.

Point 6 is good too, and here's where I think we can get away with this. Creating a new script as per point 6 will change the script currently attached to the custom node to the new (derived) script. The old (base) script is removed from the node. Since the new attached script is derived from the original one, all functionality remains. I often do this, where I have a base class (abstract or not) and attach derived scripts to nodes. The difference here is that the new script might have to say extends "res://addons/path/to/base-script.gd" rather than simply the custom node's name, because the custom type no longer has that script attached .... Although on second thought, remove_custom_type() was not called, and the script attached still derives from the old one, so maybe this is not necessary? Please clarify for me on this point.

Points 7, 8, and 9 are good, and probably not too hard while keeping the 1-script rule. 10 is not necessary if we keep the 1-script rule. 11 is good, since this is how built-in nodes behave if you try to attach a script to them that does not extend the node type.

In any case, it does sound like lots of work, and we're in beta already, so I guess this will be for 3.1 or even 3.2 (haven't looked at the roadmap recently).

willnationsdev commented 6 years ago

@RodeoMcCabe Yeah, this definitely wouldn't be for 3.0.

breaking the 1 script per node rule could be bad news that might resonate through many unexpected parts of the code

My thoughts exactly. I'm thinking of a public interface in which the Object is aware of its custom backup script, but other objects are unaware of it, and simply perceive the Object as having a single script. The trick would be in editing the setter and getter for the script property. The getter should check for null. If it's null, it should return the backup script instead. The setter should likewise double-check that any new script extends the backup script, otherwise it should report a failure somehow.

Abstract custom types obviously are not a problem since you can't instantiate them anyway and therefore the user can't add a script to it in the editor.

There are no abstract types on the scripting side (afaik). Are you saying you know how to make a script abstract? The can_instance method is exposed to the scripting side, but all it does is check whether the script itself is valid and, if it's a tool, whether the ScriptServer has scripting enabled at the moment. It has nothing to do with the abstractness of the script type, I don't think.

You need to be able to check whether the class is abstract in order for the CreateDialog::_update_search method to know when to make the text greyed-out/unselectable, etc.

willnationsdev commented 6 years ago

FYI, I created an issue about the abstract part of the problem (#13401).

I think overall it would be doable if we just have a backup_script private member added to the Object type, and then use that to do the script property checks.

eon-s commented 6 years ago

What I do on my custom nodes is to inherit a base script, that makes the custom node script empty by default...


Auto-inherit will affect the plugin itself, and won't be possible to make more than one without duplicating the plugin, right?.

Multiscript was added for a second time this year and was easy to kill it again (raises multiple issues).


Allowing a scene as a base for the custom node instead of a script may let to have the root free of scripts.

willnationsdev commented 6 years ago

What I do on my custom nodes is to inherit a base script, that makes the custom node script empty by default...

Sorry, are you saying this somehow affects abstractness or some other suggestion I made earlier? I'm not seeing where this connects...

Auto-inherit will affect the plugin itself, and won't be possible to make more than one without duplicating the plugin, right?

Are you trying to say that attempting to include the plugin into the /addons/ folder twice and enabling them both in the Plugin section of Project Settings would cause issues somehow (I mean, that sounds pretty normal if you plugin is adding custom types. Can't have multiple custom types defined with the same name).

Not sure quite what you mean by "won't be possible to make more than one without duplicating the plugin." You can create multiple instances of a custom type node just fine(?) since it'll just create the node and auto-attach the defined custom type script. My suggestion would involve changing the CreateDialog's script creation process to do...

  1. Let the node's built-in script decide whether the node can be instanced or not (abstract).
  2. Create built-in node type.
  3. Set custom script as the node's backup_script property (not exposed to scripting API).
  4. Let the node's own Object code handle the task of tricking everyone else into viewing the backup_script as the official script property on the object.
  5. Update the metadata for the editor icon
  6. Update the metadata for the brief description(?)

...instead of...

  1. Create the built-in node.
  2. Attach the custom type script as the script property.
  3. update the meta data for the custom editor icon.

Multiscript was added for a second time this year and was easy to kill it again (raises multiple issues).

I agree. I think multiscripting objects would be a bad idea at this point (and isn't even really needed anyway). That is not what I'm suggesting. In public view, Object should still have only 1 script, but I'm recommending having a backup script available that takes over the role of being the script (so assigns itself to the script property) whenever the main script property is set to null / unloaded, etc. This will allow us to support "custom types" more effectively without altering the codebase's interface towards the Object class. We can then just have a dedicated setter/getter for this backup script property that allows code (such as the code assigning custom type scripts in CreateDialog) add or remove its existence. This way, it's an opt-in modification to how the script property works and will result in comparatively fewer necessary changes to the engine.

MarianoGnu commented 6 years ago

I think a new option to contextual menu on nodes would resolve this issue "Replace script for inherited" this could even have a submenu with all detected scripts wich inherit from current and a "New script" at the end, when clicking it the new script dialog should displays the path of the script on the "extends" field.

henriquelalves commented 6 years ago

Isn't it just easier and more transparent to just add a "Replace and Inherit script" editor tool? I mean, this is only a problem because when we create an Addon node, we expect it to be scriptless - but addon nodes are really just customized nodes you are adding to the Create Node tree, and the user should know that. From an UX perspective (I'm no expert though), I don't think we should sacrifice transparency for convenience.

willnationsdev commented 6 years ago

@MarianoGnu @henriquelalves Neither of those are really the same. The implication of a "custom type" is that you are simulating an in-engine node type. That would imply that the script itself can't be removed. The functionality it imbues into the node is built-into it, the same way as you wouldn't be able to remove the Node2D-ness of a Node2D in order to make it act like a pure Node.

Akien's popular upvoted post above covers similar details:

The point here is that the script that defines the custom node should be hidden, because it's not a property of the instanced node but of its type. So this script should confer properties to the custom node, but it should be as invisible to the user of the instanced node as the C++ classes of built-in nodes are. It would provide an API, but would not be modifiable, only extend-able.

If we do create any editor tools to facilitate "replace script for inherited" stuff, that'd be cool, but any of the functionality shouldn't be able to see the layers of script hierarchy at or below the custom type script since it's supposed to simulate "built-in" behavior.

willnationsdev commented 6 years ago

In fact, in order for the content to be accessible from the gdscript_parser and other scripting content, it may be best to make sure custom type information is included in the ClassDB singleton rather than just EditorNode::get_editor_data().get_custom_types() since modules won't have access to that information, but really should have access to it.

MarianoGnu commented 6 years ago

i don't have any personal issue with having the script exposed, this is really a matter of terminology and philosophy, acording what you mentioned, what could be done is adding a new Class to ClassDB and add a script property to that class instead of the node and the class should check in that script of methods exists and call them before calling it's class parent. I belive it's posible, but would break backward compatibility with previous versions if not done before RC1

henriquelalves commented 6 years ago

@willnationsdev I see, I just not wholeheartedly agree with custom-types being treated as in-engine nodes. For me the 'powerfulness' of Editor Addons lies on how easy is to create a "pack" of custom nodes and editor tools and share it via github; all those tools and nodes are, in fact, just scenes and scripts you can copy-paste, but Godot provides the Addon API to facilitate this. When I first used it I too expected the Addons to be scriptless, but only because the engine itself looked like it was treating them as in-engine nodes, and that's an UX problem, not architectural. What I think would correct this is:

  1. Custom-types in Create Node tree shows with a different color (clearly indicating they are custom-types).
  2. When you add them to your scene, the script icon is there, but slightly transparent (indicating they have a default script with them, but can be substituted). Clicking the script icon would show the custom-type script on the editor.
  3. "Adding Script" button on custom-types would auto-fill the "Inherits" option with the custom-type script.

Than again, I'm definitely no expert, so my original assumption of Custom-Types != In-Engine Nodes might be wrong; also, your solution is the most clear one that I saw on this thread, so I totally understand if Godot development goes that way.

EDIT: I read it again, and my "solution" is basically your first 6 solution steps, hahaha.

willnationsdev commented 6 years ago

@henriquelalves @MarianoGnu I think a mix of those would certainly be necessary. Having the custom type class added to the ClassDB is a must imo. Really loving all three suggestions you had Henrique. Especially idea 2 (much better than my separate custom-type icon suggestion). And I agree with you that we need to make sure that "WHAT custom types are" remains transparent to some degree. I feel that a balance needs to be maintained: people should be able to understand whether something is a custom type AND what that means, but we should also do as much as we can to make custom types feel like engine types.

eon-s commented 6 years ago

@willnationsdev I took this from the assetlib, moved the code of the custom node to a "base" script: collision_path_2d-1-modified.zip

The immediate issue I see on that is that the plugin loads the script, and that script is unique, if I add another custom node, it will have the same script.

Then, what a script replacement means to the plugin in that case? (maybe nothing, I'm not sure).

If there is no issues replacing a script (tool mode may not be happy in some cases), the menu for custom nodes can add an entry of "extend custom script" to do that replacement process.

ghost commented 6 years ago

@henriquelalves @eon-s I think we are thinking the same things. I'm on board with this being a UX issue more than anything. So far I'm partial to this approach because I think it's keeping things as simple as possible, which is always good IMO.

If there is no issues replacing a script (tool mode may not be happy in some cases), the menu for custom nodes can add an entry of "extend custom script" to do that replacement process.

I think since the extending script has to inherit from the plugin script, replacing it wouldn't be a problem. But, again, I'm not very familiar with the codebase. Anyways, I've given my 2 cents, I'll leave it to the real devs from here ;)

@willnationsdev Sorry, I got mixed up about the abstract stuff. My "abstract" base classes simply have a print statement in _init() to tell me if I instantiated it by accident ....

Web-eWorks commented 6 years ago

Just to chime in here, custom node types should absolutely behave like engine types, including setting a custom script, ability to refer and inherit by name in GDScript, and the inability to remove the type's base script. This mimics the behavior of nodes implemented by the engine, promoting internal consistency and ease-of-use.

Regarding the issue of showing that it's a "custom" node, I propose that the node has an entry in the Inspector's dropdown above "Show In Help" that shows the object's base script file. From a user and developer standpoint, you should know it's a custom node just by using it (who else added it to the project?), and the goal of "custom" nodes (IMO) is to allow defining types in GDScript that are indistinguishable from built-in engine types.

willnationsdev commented 6 years ago

@Web-eWorks I am actually working on this (at this very moment) and I've gotten a decent chunk of it done (still have a little ways to go though). I've setup a backup script in the Object class and created storage for custom types in the ClassDB along with updating the EditorPlugin methods to use the new API. Still have to make updates to the related ClassDB functions (stuff like can_instance and get_parent_class, etc.) and update all the Editor / CreateDialog stuff.