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.
I don't understand what you mean
@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.
Of course not, this is by core design and will not change.
On Aug 7, 2016 18:11, "George Marques" [email protected] 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
.
This is sad. But you can always add the script to the parent or to a child.
Closing as wontfix.
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" [email protected] 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
.
I guess it would need the ability of having two (or more) scripts per node, though this really does not make much sense.
Godot orginally supported this, but it was more trouble than advantage
On Aug 7, 2016 10:36 PM, "George Marques" [email protected] 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
.
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.
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 [email protected]
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
.
I'd love if it could be automated/made easier through the UI.
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:
RedNode2D
would extend Node2D
, and have a red modulation defined via a custom script)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.
+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.
Please explain in which way you believe they do not
On Aug 8, 2016 11:50 AM, "Ralf Hölzemer" [email protected] 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
.
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.
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:
... and of course the ability to extend those types in the editor with a custom 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
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" [email protected] 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.pngWhile 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/pathextends 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
.
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" [email protected] wrote:
Oh I see.. these two thigs should definitely be fixable, together with
adding inheritance support to script create dialogOn Aug 8, 2016 1:54 PM, "Ralf Hölzemer" [email protected] 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.pngWhile 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/pathextends 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
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.
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
.
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.
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.
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" [email protected] 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
.
@reduz I think that would be good enough :smile:
@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?
I suppose we could somehow make it more user friendly in that regard
On Aug 11, 2016 06:10, "Marc" [email protected] 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
.
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
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?
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).
@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.
@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).
@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.
@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.
@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.
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?
@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...
add_custom_type
).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.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.
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.
Object::get_script()
should return null
for nodes with a custom type script and no loaded script.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.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.&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.
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).
@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.
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.
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.
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...
backup_script
property (not exposed to scripting API).Object
code handle the task of tricking everyone else into viewing the backup_script
as the official script
property on the object....instead of...
script
property.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.
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.
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.
@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.
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.
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
@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:
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.
@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.
@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.
@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 ....
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.
@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.
@willnationsdev it should be noted that registering in ClassDB also means people making plugins should all prefix their classes to prevent collisions^^"
@Zylann Yeah, that would be necessary, unfortunately.
At this point I think I've sorted out most of the bindings and core changes. It's just a matter of updating the Editor, CreateDialog, and EditorHelp / DocData classes now (and the GDScript compiler).
As I mentioned before.. I still don't think the proposed solution is good.
Plese check with me before attempting something that will most likely be
rejected.
On Feb 7, 2018 20:05, "Will Nations" notifications@github.com wrote:
@Zylann https://github.com/zylann Yeah, that would be necessary,
unfortunately.At this point I think I've sorted out most of the bindings and core
changes. It's just a matter of updating the Editor, CreateDialog, and
EditorHelp / DocData classes now.—
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-363893711,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2xIYRcs0BFDqTyiFwqlhQWTqjEZtks5tSgIVgaJpZM4JejbZ
.
@reduz Well, I've been trying to take into account the concerns you previously laid out.
I am not implementing any sort of multi-script system. I am also not making scripts masquerade as ClassInfo objects in the ClassDB. All I've done thus far is attach a script (via RefPtr) to the inheritance hierarchies in the ClassDB. The Object class will only ever use a "backup_script" whenever the regular script assignment attempts to infringe on the backup script's inheritance hierarchy.
Oh I see.. what is the use case for this then?
On Feb 7, 2018 20:32, "Will Nations" notifications@github.com wrote:
@reduz https://github.com/reduz Well, I've been trying to take into
account the concerns you previously laid out.I am not implementing any sort of multi-script system. I am also not
making scripts masquerade as ClassInfo objects in the ClassDB. All I've
done thus far is attach a script (via RefPtr) to the inheritance
hierarchies in the ClassDB. The Object class will only ever use a
"backup_script" whenever the regular script assignment attempts to infringe
on the backup script's inheritance hierarchy.—
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-363900920,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2yn68Iy6AgAKJHZ4qImH4UBAm5skks5tSghSgaJpZM4JejbZ
.
The use case is for when someone wishes to introduce a new type of Node to the engine using an EditorPlugin, but they do not wish the scripted type of that node to be editable (they want the interaction with it to mimic an in-engine type). So, for example, if I attempt to get_script()
on a node with only its backup script, then it will return null. If I attempt to set_script(null)
, then the script gets set to the backup script. If I attempt to set_script(a_script)
, then it will only successfully assign the script if the new script is in the inheritance hierarchy of the backup script.
For UX, I plan on making the script icon appear in the editor as a transparent icon if only the backup script is present. In that case, the button to add a script will still be an "add a script" button rather than a "remove script" button, and clicking it will prefill the text with the name of the type (although, I may need to make it prefill with the script path if the user changes the type of script away from GDScript / VisualScript, etc.). Once a different script has been assigned, the transparent icon would become opaque again. Clicking the script icon in either case would take you to the active script source code (the backup script source code if no other script is present).
I have to admit I don't really see the benefit of this, so again what's the
specific use cases you are thinking of?
On Feb 7, 2018 20:46, "Will Nations" notifications@github.com wrote:
The use case is for when someone wishes to introduce a new type of Node to
the engine using an EditorPlugin, but they do not wish the scripted type of
that node to be editable (they want the interaction with it to mimic an
in-engine type). So, for example, if I attempt to get_script() on a node
with only its backup script, then it will return null. If I attempt to
set_script(null), then the script gets set to the backup script. If I
attempt to set_script(a_script), then it will only successfully assign
the script if the new script is in the inheritance hierarchy of the backup
script.For UX, I plan on making the script icon appear in the editor as a
transparent icon if only the backup script is present. In that case, the
button to add a script will still be an "add a script" button rather than a
"remove script" button, and clicking it will prefill the text with the name
of the type (although, I may need to make it prefill with the script path
if the user changes the type of script away from GDScript / VisualScript,
etc.). Once a different script has been assigned, the transparent icon
would become opaque again. Clicking the script icon in either case would
take you to the active script source code (the backup script source code if
no other script is present).—
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-363904934,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z26oeOMT4HpjzDsX8eygMSeUhbTgVks5tSgurgaJpZM4JejbZ
.
The high-level use case is to allow plugins (EditorPlugins, the PluginScript/SubModule system) to define 'fool-proof' custom node types. Currently, the custom type system is a thin factory for "script on a node", which has numerous workflow and usability issues if you are attempting to build new node types, the least of which is the complete inability to easily, intuitively, and effectively extend the custom node with gameplay code (contrary to built-in nodes). A more pressing issue is the inability to create custom nodes that inherit from other custom nodes.
Some specific use-cases are to create low-level nodes: perhaps a marching-cubes based voxel GridMap implementation for destroyable terrain, ala Minecraft or any other terrain system using marching-cubes/dual contouring; custom Control types (circular menu container, as implemented in many modern games, any type of low-level Control that's just high enough to be outside the engine spec); and any place you want to build a plugin that registers new nodes. In each of these cases, you usually want to add a custom script to handle gameplay code, and have it be separate from the "implementation" code (written in GDScript/GDNative).
I think what's failing to be communicated here is that the goal is to make custom nodes behave like engine nodes without having to write an engine module and compile against it.
This improvement is not necessarily driven by a specific feature (though there are many), but rather by the rationale of "the current way is backward, flimsy, and tedious". If implemented fully, this system would allow an extremely powerful method of extending the engine's built-in types from GDScript / GDNative without needing to recompile the engine. From a design standpoint, this is literally a straight improvement over the current system.
If you want a particular use case after all of that: the entire plugin / AssetLibrary ecosystem.
That's the scope of what benefits from this change.
Ok, so here is an explicit example:
I have a plugin I am working on, godot-skills
, which introduces a variety of new types of nodes:
Effect: nodification of a Functor
Targeter: similar, but just finds and collects other nodes
Skill: combines hierarchies of Targeters and Effects to find nodes and apply effects to them all
SkillUser: "owns" several Skills
I would want each of these nodes to be things that users can create and add to a scene directly. However, each and every one of these nodes are designed to serve as a base type. They don't have a lot of functionality out of the box, and must have a deriving script placed onto any node they would otherwise be involved with. For use cases such as these, it can be useful to create a node from which you don't want the user to be able to remove a particular set of scripted functionality. For example, I want to prevent users from placing a non-Targeter-deriving script onto any Targeter node they add to their scene. This is because there is instant functionality that Skills get from having Targeters as children (similar to Area2D and CollisionShape2D nodes).
The current workflow requires that users...
This system would allow you to replace all of this with just:
Users would be able to dynamically add their own custom types that derive the in-engine types (for simpler usability). It would also reinforce the security of the plugin's functionality by enforcing the presence of the scripted functionality on the node in question. Currently, users can easily just remove the script and break things (not desirable). Furthermore, if someone wants to "clear" the script on the node, then that node shouldn't lose it's custom type functionality (this is a big one). It should revert to having the custom type script.
Ok, so an update. I currently have the following.
ClassDB
can now add and remove custom types directly.EditorPlugin
has similar custom type methods that add and remove not just the custom type, but other editor-related data such as the icon. These methods are connected to a new signal which is emitted whenever the EditorPlugin
is toggled active/inactive.set_custom_script(RefPtr());
.project.godot
file is populated with all custom types so that executed games can set them up in main.cpp
prior to the rest of the game starting.Remaining tasks to be completed...
script_create_dialog.cpp
.* I actually have a few questions about this.
First, I only see add_global_constant
/ _add_global
in the gdscript_compiler.cpp
file without any means of removing globals from the GDScriptLanguage class. Does it just get completely re-initialized whenever singletons are added or removed? How are global identifiers edited dynamically? This would be so that users can do MyNode.static_func()
without having to load the script into a variable first.
While I'm at it, I feel like Godot's GDScript could benefit greatly from the introduction of namespacing, especially as part of this change. We'd have to find the right delimiter to flag them, but you could do away with concerns about name collisions from engine and plugin types when making your own scripts. For example, assuming \
is the delimiter of choice...
extends Node # an in-engine type.
extends \ # triggers an error if left alone, but would prompt a dropdown of the plugin API names for auto-completion as well as the names of any scripts in the current project that are not part of a plugin.
extends \MyPlugin\ # triggers an error if left alone, but would prompt a dropdown of the scripted types that exist for the plugin
extends \MyNode # extends my_node.gd somewhere in res://
extends \MyPlugin\MyNode # extends my_node.gd somewhere in res://addons/[my_plugin_something?]/
func _ready():
print(\MyNode.CONSTANT) # would print CONSTANT in the current project's my_node.gd script
Anyway all of this ^ is just ideas being thrown around at the moment. If namespacing were introduced, you could even have scripts automatically be associated with a name based on their file name using the resource_saved
signal in EditorNode
. They WOULDN'T necessarily be custom types (in the sense that you can't remove the script from the object), but they WOULD be accessible by name alone which would be extremely useful and intuitive. Scripts could even override the default auto-naming (filename.capitalize().replace(" ", "")
?) by providing a namespaced title at the top, e.g. \MyPlugin\mynode
. Now suddenly "\MyPlugin\mynode" is a global identifier for that script.
Here's a snippet of my progress thus far.
@willnationsdev please don't choose \
, it's already used for escaping and multiline. .
, :
or ::
look better.
@Zylann I planned on using the delimiter as a prefix for any nodes that get added to the project as well. So then, for example, a script at res://container.gd
wouldn't want to have collisions with the actual Container
class. If we used a period (.), then it might look like this.
var engine_type = Container.new()
var script_type = .Container.new()
var plugin_script_type = .MyPlugin.Container.new()
Anyone have objections to that kind of format?
Edit:
That might run into issues within derived class functions though that call the parent's method within them:
func _virtual_method():
._virtual_method() # parent stuff
# my stuff
Hmm...does that pose a problem for constants and/or variables? If not, then we can just assume that parentheses means it's a super function and otherwise it's a possible namespaced identifier.
Honestly, I'd hold off on the namespacing for right now - it could be introduced later, and I'd rather get the current system in as soon as possible. We would also need a _really_ well thought-out system for automatic name registration, possibly extending to a reworking of the entire addon organizational structure, which is a fair amount of work and beyond the scope of this issue.
Personally, I'd rather the name for an 'auto-registered' script be controlled by a declaration in the file, i.e.
register <NAME> extends <PARENT>
or a similar syntax based on the current extend <NAME>
prolog that's already present for GDScript.
EDIT: I also wanted to ask how assigning scripts deriving from a parent (say Spatial) to a custom child (say Sprite3D->CustomType) is going to be handled? This already works for engine types, and restricting assigned scripts to be derived from the custom type itself is not going to work out very well.
@Web-eWorks Fair enough. I'll work on perfecting the current API and then I'll open up a new Issue for namespacing once the PR is submitted.
If I understand well what @Web-eWorks means, I still have this question myself: how will you get around the fact an Object
can only have a single script? How will they be... "scriptable"? Because apart from that, registering to ClassDB is nothing more than a global identifier in terms of use for programmers and designers.
@Zylann What I've done in my fork is create a custom_script
property for Object
. It only ever becomes significant if a value is assigned to it, but if a script is assigned to it, then it serves as a backup script / constraint on what the main script
property can be. Clearing script
will set it to be custom_script
, and if you assign a new Script
to script
, then it will only be assigned successfully if the Script
instance derives from the custom_script
. And all of this is managed by Object
behind the scenes, so as far as the engine is concerned, Object
s still only have 1 actual script.
The ClassDB content just has a secondary map of custom types that get registered, mapping the script itself and some other info to the name you've assigned it. It then provides that name and its inheritance hierarchy with related inheritance methods (get_class_list
, is_parent_class
, get_parent
, etc.). And this is done as an opt-in flag for some of the methods. get_class_list
will now take a bool flag where it only includes custom types if you pass true
as a second parameter. This is how it prevents breaking compatibility with the rest of the engine.
@willnationsdev so, with a system that makes scripts behave like builtin classes, what's preventing users from wanting all their scripts and addons to be considered first-class citizens like the builtin classes, and drop the "old-school" way?
Ah. The way scripts currently work is that you can put a script that extends Node
on a VBoxContainer
, and it still works just fine. If the current implementation breaks that behavior for custom types, there will be some issues - is there any way to keep the custom script's behavior while having a 'normal' script deriving directly from an engine type?
@Zylann That's actually precisely why I want to introduce namespacing so that scripts and plugins' scripts can be safely integrated via names without colliding with in-engine types. There would be nothing preventing people from using only the new way and ignoring the old way. And I actually think that would be better overall. It's laborious to have to use an explicit path every single time.
@Web-eWorks Not sure if I'm following right here, but...
Custom types specifically work like in-engine types for inheritance. So if you extend from a custom type, it'll be as if you are extending from a completely different branch of the in-engine hierarchy. Ex. if I create a custom type MyNode
that extends Node
, I won't be able to put that script on a VBoxContainer
just like I can't put a Node2D
"on" a VBoxContainer
. This would actually be the intended behavior.
Now, with the namespacing concept, you would ideally have named scripts that are NOT custom types. So, in that case, I could create a script that extends Node
and put it on Node
, VBoxContainer
AND MyNode
. Does that make sense?
So what I'm saying is if you create a custom type MyNode
inheriting say MeshInstance
, instance a node of it in the editor, and attempt to add a script inheriting Spatial
to the MyNode
typed node, will that work properly? It works properly with 'engine' nodes, and that's the behavior we need to emulate.
It should be namespace independent, as this is more an issue of making sure the custom type's script is getting called regardless of the point in the inheritance hierarchy of the 'user-level' script.
@Web-eWorks Yes, that would work. Every "first level" entry in the ClassDB
's custom_types
map will have to link back to a type in the classes
map, otherwise an error occurs and the custom type isn't created. This ensures that the chain of inheritance is preserved when moving from custom types back into the engine types all the way up to Object
.
@willnationsdev Just to be sure - if you add a _ready
method in MyNode
, and a ready node in your user script that derives from Spatial, it will be called like Spatial._ready(), MeshInstance._ready(), MyNode._ready(), script._ready()
? (Or whatever order _ready
is dispatched in.)
I still stand by my view that this is unnecesary and confusing. This problem needs to be solved from the editor UI, not core engine.
@reduz This isn't something that can be solved from the Editor UI alone though. Not if you want to control users' manipulation of the script on the object. It seems to me like that is a feature that many people are wanting to have setup.
Are you suggesting that the gdscript_compiler should reference content in EditorData in order to setup new global identifiers for scripted types, rather than tracking them all in the ClassDB and allowing the ClassDB to manage all of the inheritance information in the engine?
Certainly there's a part of it that needs to be solved from the perspective of the editor UI, but there's also a part that needs solving in the core. Adding custom types to the engine, _actual_ types from the perspective of the ClassDB, is a very powerful and ultimately necessary feature. As far as confusing, I still maintain that it is no more confusing than the current system, and very probably less.
@Web-eWorks I agree. To make the inheritance data be tracked in the editor would actually be much more confusing in my opinion. And it would result in odd workarounds as that type information wouldn't exist anymore once you run the game without the editor (which is why I actually had to start loading the information in project.godot
in order to have it loaded into ClassDB
on startup. No EditorPlugins
to add the custom types was causing the GDScripts to fail compilation when running the scene).
@Web-eWorks And to answer your earlier question, yes, that works properly.
In my test scenario, I have Node.gd
which is extending GDSkillsEffect
> GDSkillsAPI
> Node
, and the _ready methods implemented in each script get triggered in the proper order: API, Effect, Node.gd.
I am currently experiencing an infinite loop error of some kind though whenever two scripts both try to inherit from custom type scripts by name. Ex. if Node.gd has extends GDSkillsEffect
and gdskills_effect.gd
has extends GDSkillsAPI
, then trying to modify and save gdskills_effect.gd
or gdskills_api.gd
will cause the editor to infinitely "load" and then crash. That's probably my biggest bug at the moment.
@willnationsdev Ahh, no, I'm talking about Node.gd
extending Node
, _not_ GDSkillsEffect
. If _that_ works, then everything is good.
For example, here's a representation of the scene tree:
-> User Script:
extends Node
func _ready(): pass
@Web-eWorks Ah, I see what you mean. Well, it SHOULD work, but I just tested it and it seems like my logic in Object::set_script()
is blocking it for some reason, so that's a bug I'll need to fix. XD It's failing because it has detected that Node
doesn't derive the custom type script (which is true, yay), but it shouldn't be failing in that case, lol.
@Web-eWorks As I mentioned, this is NOT something that needs to be changed in ClassDB.
I don't really think this needs to be solved in the core, as each scripting language handles it different. You can easily create a new class in Mono and it's going to work. For GDScript it may be a simple case of exposing a script file as a preloaded global and that's it. A one-size-fits-all solution, as proposed in the PR, is definitely not the way to go.
@willnationsdev I appreciate your effort, but this was discussed to no end already and the consensus is that:
@willnationsdev For reference, engine behavior is that any script extending _something_ equal to or previous in the inheritance hierarchy can be added to a node. Thus, a script that extends Spatial
can be added to a MeshInstance
but not a Control
or Node
, as neither of the latter are or inherit from Spatial
. This is behavior that must be replicated. (No pressure...)
@reduz But does creating a new class in Mono add it to the engine's inheritance hierarchy? Or the editor's Create Node dialog? Perhaps a new issue needs to be opened that concisely states what the purpose of this is, because I think we're not talking about the same thing here.
The point of the ClassDB modifications are to allow registering new types of Nodes (or Resources) without needing to recompile the engine. Not to add new custom globals in GDScript, though that's a tangential issue that @willnationsdev is working on in the same branch.
The point of this is not to add hacks, but to create a well designed _system_ to extend the engine's types.
@Web-eWorks The engine inheritance hierarchy is for C++, not scripts. Trying to mix both of them is conceptually wrong to begin with.
Maybe the misunderstanding is that the create node dialog makes it look like a new class was added, so it may be a good idea to mark such custom types as clearly containing a script.
@Web-eWorks If you are using Mono, it will look transparently as if a new class was added, because it IS a new class. For GDScript, as classes are files by default, it can't be done this way so the way to do this may be to add a new global with a dedicated UI to set it up (as we have for singleton autoloads)... but this is still a script class, not an engine class.
@willnationsdev My whole point in this is that i think it's wrong to make it look as if you are adding actual new classes to the engine, because they are not. It's open to confusion. It's just a node with a script, so instead of hiding the script, I think the right thing to do is make it more visible in the create dialog.
@reduz Well, that's the thing. Node+Script is a Scene. The add_custom_type mechanism should behave like Mono, registering a 'new class', even if it's not technically a 'class' by whatever standard. If there's a way to do that without needing to touch the ClassDB, I'm all ears.
Also, @akien-mga suggested that in the scene tree view, we could hide the scripts of custom nodes, to avoid confusing users about them having created that script and edit it by mistake (but still show it in the inspector at the bottom).
In such nodes, adding a script will open the create dialog in an inheritance mode, so it will still be pretty transparent to the user workflow wise, while it's clear that it's a custom node and not part of the engine.
@willnationsdev Any objections to me opening a new, more concise issue specifically detailing the scope of implementation?
@Web-eWorks Node + Script is not a scene, nodes in a tree layout are a scene. You can also add custom resource types, not just nodes.
And no, it should not behave like registering a new class. It needs to be pretty clear that it's a C++ class with a script pre-added.
@Web-eWorks Yeah, that's no problem with me. Please mention me so I get a ping if you do. Not sure if we need a separate issue though?
Look at it from another point of view. Your argument is to make it look to the user as if you were extending the engine types with new engine types.
My point of view is that you are extending the engine types with scripts, not with new engine types. And that the user is not stupid and needs to know this.
@reduz What I meant about Node+Script was that that mechanism was already covered via instantiating Scenes.
Maybe I'm mis-understanding the Mono implementation, but aren't Mono classes still Scripts?
@reduz I mostly agree with you, also when you said that adding a script to a custom node would simply propose to inherit it. But how do you expect this to work with GDNative custom types?
@reduz One of our goals though is to allow custom type scripts, these pre-assigned scripts placed on the C++ classes, to not be removable. That is something that can't be done in the editor context since you could always remove a script at runtime (unless you want to preserve that functionality and only secure a custom type script at design-time in the editor)
I would certainly be open to an alternative method. Either way, the easiest implementation is to have the Object conditionally assign scripts to itself (as I've done in my implementation) by checking some third-party registration class (whether that's the ClassDB, ProjectSettings, EditorData, or who knows what else). It then becomes very easy for the editor itself to check with that third party and display things differently (also as I've done in my implementation). It wouldn't be too much work I don't think to move the registration to another context than the ClassDB though.
^ Would that compromise be acceptable? Otherwise, you have to micromanage every possible location in which Object gets a script assigned to check with the third party. That would be much more complex.
The implementation I have done has been very clear thus-far as to which "types" are scripts and which aren't. After all, you can still clearly see and access the script associated with custom types in the scene dock in the video I shared.
@willnationsdev I understand what you are trying to do, but I don't think this can ever be made cleanly. It's hiding a complexity that has no reason to be hidden, in a way that will never fully work as expected.
@Zylann I think it's the same, but either a) the script is gdnative internal b) it uses a method internal to gdnative. I am not familiar enough with it honestly to tell.
@reduz Are you saying that you don't want to allow any form of creating script-based constraints on Objects? Cause I could move the check logic into the Editor itself and that would give you design-time checking of the constraints just as easily (UI changes, as you say), but they wouldn't work at runtime, so people would still potentially be able to change things once the game is running. Would that work better for you? I'd still prefer to be able to check stuff at runtime though...
@willnationsdev what kind of script based constraints do you want to do?
The same constraints that the custom script property has been about since the beginning: enforcing that the assigned script derives some other script.
I'd recomend to hide the script icon for the extended script in the scene hierarchy and in the case someone clicks the "Add Script" button instead of inheriting the base class of the node it inherits the script attacked. It's transparent and doesn't need to mess around with ClassDB at all
So, just reviewing some of the information in script_language.h
, I feel like I could really just move the new content I added in the ClassDB
over to the ScriptLanguage
class and create a sort of ScriptDB
singleton that only ever gets created if the scripting language intends to use it. And because object.cpp
already includes script_language.h
, it would be able to do the following in set_script(const RefPtr &p_script)
...
p_script
is in fact a scriptp_script
belongs top_script
and the Object
instance's custom_script
to the ScriptDB
for the associated ScriptLanguage
,ScriptDB
as to whether the p_script
can be assigned to the Object
given the constraints implied by the custom_script
on the Object
instance.This would allow us to keep both ClassDB
clean of any modifications and keep object.h
clean (aside from a setter/getter for custom_script
). It also prevents unnecessary complication of the scripting languages that have no need for relying on Godot to define identifiers and namespaces (like GDScript and VisualScript would need, unlike C#, C++, Python, etc.).
Then all you'd need to do is define ScriptServer
methods that can perform identifier / namespace checks across all applicable scripting languages in order to replace the experimental ClassDB
functionality of confirming custom types / custom type inheritance hierarchies. Easy enough.
Does any of this sound like an acceptable alternative @reduz?
Just adding my voice here. I just started working with Godot and, when I read about custom type addons, I did indeed think that I would be creating a new reusable base type. Like how when you create a KinematicBody
you can attach a script to it, but it would still do all of it's KinematicBody
things.
This is reinforced two ways:
@JPTeasdale Unfortunately, at this point the bigger concern is the implementation, not so much the "how do you define this" question. And Godot custom types ARE extendable (they are just scripts). The editor just doesn't yet have the code support to display inheritance relationships between custom types in the node creation dialogue, but the data is available. I also don't really see a reason to rename them from "custom type" to something else. "Prefab" and "template" aren't really appropriate terms, nor are they scenes in the first place (they are just scripts, plain and simple).
i think it's much simplerthan the intended solution:
@MarianoGnu Well, in terms of creating the editor changes, it is simpler to do by just auto-filling the script
property and preventing it from ever becoming null in the first place (because the editor only ever looks at the script
property, and we don't have to find every time it references the script
property and modify that code if all we do is change what the value of script
is).
With that said, part of the goal of redoing this process is to allow scripting languages in general to opt-in to use identifiers rather than file paths in order to locate script classes. That's the only reason the ScriptDB
concept or ClassDB
modifications were ever suggested in the first place. But now I see why Juan has been objecting to the ClassDB
changes so much, hence the suggestion of storing the information in the scripting API alone. It self-contains all the information in the individual scripting language.
A combination could be done, instead of storing the invisible script in the object store it in the ScriptDB, add a pre-pass to the script reader to replace " extend CustomClassName" to "extend "res:/path/stored/inScriptDB.gd" in the precompiler of GdScript, because having the script out of the script inherit hierarchy is very troublesome (hierarchy must be a stright line bottom to root, having a branch leads to confusion)
because having the script out of the script inherit hierarchy is very troublesome (hierarchy must be a stright line bottom to root, having a branch leads to confusion)
@MarianoGnu Maybe some misunderstanding here. At what point do you think there isn't a straight line of inheritance? As far as I've gone implementing things, there is always a very clear inheritance hierarchy between scripts. I haven't changed anything about how the inheritance hierarchies themselves work. I've just made it possible to map a given script to a StringName and look it up in a DB of some kind.
And if we are already going through the work of doing that, there's no reason to replace the text of the script back to a path in the GDScript compiler. All the compiler does is assign a Script
instance to a script
variable. Whether I've loaded the script through the ResourceLoader::load(path)
method or via fetching it with Class/ScriptDB::get_script(namespace_and/or_identifier)
doesn't really matter.
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).
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?
@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.
@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.
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).
@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.
@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.
Currently, the editor does have the ability to easily extend any node's script:
The node itself currently looks like this:
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:
Meaning:
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.
@aaronfranke In my opinion this is the best solution. Even instanced scenes could benefit from this.
@zatherz et al, now that #30697 has been implemented, what else can be improved? Is there anything else in https://github.com/godotengine/godot/issues/6067#issuecomment-238250383 that is important to have?
Closing as fixed by #30697. While the original proposal in the OP elaborated in https://github.com/godotengine/godot/issues/6067#issuecomment-238250383 has not been implemented, this won't happen due to @reduz's concerns in https://github.com/godotengine/godot/issues/6067#issuecomment-239060186.
Most helpful comment
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:
RedNode2D
would extendNode2D
, and have a red modulation defined via a custom script)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.