Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better custom EditorPlugin nodes #6067

Closed
zatherz opened this issue Aug 7, 2016 · 121 comments
Closed

Better custom EditorPlugin nodes #6067

zatherz opened this issue Aug 7, 2016 · 121 comments

Comments

@zatherz
Copy link
Contributor

zatherz commented Aug 7, 2016

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
Copy link
Member

reduz commented Aug 7, 2016

I don't understand what you mean

@vnen
Copy link
Member

vnen commented Aug 7, 2016

@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
Copy link
Member

reduz commented Aug 7, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z29F5q8PaoBv4OrzAUayzrfNjfHyZks5qdkoUgaJpZM4JejbZ
.

@vnen
Copy link
Member

vnen commented Aug 8, 2016

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

Closing as wontfix.

@vnen vnen closed this as completed Aug 8, 2016
@vnen vnen added the archived label Aug 8, 2016
@reduz
Copy link
Member

reduz commented Aug 8, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z27Zo4Ixvo4APSC4Fxf5ZqCJRsAxXks5qdn3igaJpZM4JejbZ
.

@vnen
Copy link
Member

vnen commented Aug 8, 2016

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

@reduz
Copy link
Member

reduz commented Aug 8, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z27jidVZl-hHWW3G_ESr8Cqj3eX7Eks5qdogRgaJpZM4JejbZ
.

@zatherz
Copy link
Contributor Author

zatherz commented Aug 8, 2016

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
Copy link
Member

reduz commented Aug 8, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z27eHoXwgr6buF6CCAHfxwx1dKkCiks5qdzHhgaJpZM4JejbZ
.

@zatherz
Copy link
Contributor Author

zatherz commented Aug 8, 2016

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

@akien-mga
Copy link
Member

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:

  • Custom icon, custom identifier
  • Instantiable via the Add node widget (and I guess via script, so exposed to Global Scope?)
  • Have a custom API coded via a script (e.g. RedNode2D would extend Node2D, and have a red modulation defined via a custom script)
  • Then this custom node should behave like a built-in node, i.e. the user should be able to instance it with no script at all (the custom API would not be directly exposed to the user, just like for built-in script where it's hardcoded in C++), and attach a script to it that would extend the custom node (e.g. extends RedNode2D).

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.

@akien-mga akien-mga reopened this Aug 8, 2016
@akien-mga akien-mga removed the archived label Aug 8, 2016
@ghost
Copy link

ghost commented Aug 8, 2016

+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
Copy link
Member

reduz commented Aug 8, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z25PJzv9w7iXO350tRF3FcEuYQYUKks5qd0IrgaJpZM4JejbZ
.

@ghost
Copy link

ghost commented Aug 8, 2016

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
Copy link
Member

reduz commented Aug 8, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z20FQioFVkVcvhhbvDQ7jQKv5De7Hks5qd19QgaJpZM4JejbZ
.

@reduz
Copy link
Member

reduz commented Aug 8, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z20FQioFVkVcvhhbvDQ7jQKv5De7Hks5qd19QgaJpZM4JejbZ
.

@ghost
Copy link

ghost commented Aug 8, 2016

@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
Copy link
Member

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
Copy link
Contributor

Zylann commented Aug 11, 2016

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
Copy link
Contributor

Zylann commented Aug 11, 2016

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
Copy link
Member

reduz commented Aug 11, 2016

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
#6067 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2xLGOhgMk__ZoRW1neRu1aRb5Qr_ks5qeoJogaJpZM4JejbZ
.

@bojidar-bg
Copy link
Contributor

@reduz I think that would be good enough 😄

@Zylann
Copy link
Contributor

Zylann commented Aug 11, 2016

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

@mattrobin
Copy link

Just wanted to mention, as a new user of Godot, I made a plugin under the impression my "Custom Node" from the plugin would act as a builtin node. That is, when I selected it from the node list, I expected I would get a node without a script, but with the functionality I had scripted in the plugin being inherited already builtin (like it is for the builtin nodes). Just thought I should give another opinion from someone just coming in (though it looks like the discussion here has already been extensive).

@MarcusElg
Copy link
Contributor

As a "custom node" is just a script what is even the difference from just assigning the script to a built in node? What is the custom node feature supposed to do?

@swarnimarun
Copy link
Contributor

@MCrafterzz It was discussed that there should be the ability to custom nodes to have the ability to add a script over a custom node created from a plugin.

Currently custom node is just a node created with a script already attached and icon and name already changed.

@swarnimarun
Copy link
Contributor

swarnimarun commented Jun 2, 2018

@willnationsdev How far have you come by implementing this feature.
I would really like this feature to be here before 3.1 it will be a really amazing addition.
For the Godot plugin and addon system.

I don't want to pressure anyone. So please don't take it the work way.

@willnationsdev
Copy link
Contributor

I've been implementing a new backend for the custom type system entirely. The new one allows users to do all of the original functionality, plus making it so that the Add-a-Script button adds a new script which is extending the custom script by default (rather than opening the custom script) and makes it so that you cannot remove the custom script (you would have to use the Change Type editor command, just like any other engine type).

@willnationsdev
Copy link
Contributor

@swarnimarun

In addition, it will allow users to register types to a global which provides access to typename-to-filepath mappings. GDScript will be able to perceive these mappings as essentially simple global typenames. Furthermore, types do not have to be custom types in order to be registered and the types can be either scripts or scenes.

So far, I've gotten the new backend implemented and I've re-setup the original custom type script functionality. Currently will be working on new features. Starting a week from now probably. I'm otherwise busy this week.

@swarnimarun
Copy link
Contributor

@willnationsdev No worries, if it makes into 3.1 I will be content.
And I realized that it might be quite a bit of work.
Keep up the good work.

@aaronfranke
Copy link
Member

aaronfranke commented Mar 5, 2019

Currently, the editor does have the ability to easily extend any node's script:

extend

The node itself currently looks like this:

2

And @reduz 's main concern was with hiding the fact that there is already a script attached. On one hand, it makes sense to not hide that from the user. But on the other hand, custom nodes are pretty much always going to have scripts attached. Without scripts, custom nodes are just existing regular nodes with custom icons and no extra functionality added.

And even though you can extend the custom script, it's very annoying that plain custom nodes look the same as extended custom nodes.

So, here's my simple solution: When a custom node's script is the same one as the script in the custom node's definition, the script icon should be faded out, like this:

3

Meaning:

4

@burstina
Copy link

burstina commented Apr 18, 2019

Guys, real life example:If i have Many custom note of the same kind, each script customized for its own instance, what does happen if I want to change the base custom node script? I tell you: THE WORST, because I need to modify it, AND all of other instances accordingly, delivering high chance of human and typo errors.

You dont want to hide script? Fine, then show the custom_node script AND the project node script.

@Zizico2
Copy link

Zizico2 commented Jul 13, 2019

@aaronfranke In my opinion this is the best solution. Even instanced scenes could benefit from this.

bojidar-bg added a commit to bojidar-bg/godot that referenced this issue Jul 19, 2019
@aaronfranke
Copy link
Member

@zatherz et al, now that #30697 has been implemented, what else can be improved? Is there anything else in #6067 (comment) that is important to have?

@aaronfranke
Copy link
Member

aaronfranke commented Apr 19, 2020

Closing as fixed by #30697. While the original proposal in the OP elaborated in #6067 (comment) has not been implemented, this won't happen due to @reduz's concerns in #6067 (comment).

@akien-mga akien-mga added this to the 3.2 milestone Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests