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

Ability to export Node types instead of just NodePaths #1048

Closed
nathanfranke opened this issue Jun 12, 2020 · 40 comments
Closed

Ability to export Node types instead of just NodePaths #1048

nathanfranke opened this issue Jun 12, 2020 · 40 comments
Milestone

Comments

@nathanfranke
Copy link

nathanfranke commented Jun 12, 2020

Header/Notice: This is a revival of the old issue here. Mentioning @Zylann as they are the creator of the old issue.

Describe the project you are working on:
This is useful for all project types, but it is particularly useful with things like UI with lots of containers and hierarchy.

Describe the problem or limitation you are having in your project:
If someone would like to export a reference to a node that can be edited from the editor, there is a lot of boilerplate script code for each variable and the editor-side is not necessarily intuitive. (Clicking the NodePath variable and selecting a node rather than just dragging the node into the space).

Previous code example:

@export var my_label_path: NodePath
@onready var my_label: Label = get_node(my_label_path)

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
The feature enhancement I (and others) propose is to have a shorter syntax that handles the NodePaths. Basically, a simpler code format that behaves the same as the previous code.

New code proposal:

@export var my_label: Label

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
See the code proposal above.
dragging a label node into the export property

If this enhancement will not be used often, can it be worked around with a few lines of script?:
This can be worked around in only two lines, but those lines are very redundant, especially in cases where several nodes are exported. Also, this is an extremely common use-case that has come up several times in most (if not all) of my projects.

Is there a reason why this should be core and not an add-on in the asset library?:
This is core in the script and not sure if it is possible to work around with an add-on.

@creikey
Copy link

creikey commented Jul 3, 2020

I think I am against having the exported property function the same as an exported resource, where there is a dropdown for new etc. , would be better for it to functionally act the same as a NodePath export

@creikey
Copy link

creikey commented Jul 3, 2020

I will look into drafting a PR for this soon

@creikey
Copy link

creikey commented Jul 5, 2020

I have completed the basic functionality, however I think it's a little bit wonky, and have not implemented the setgets originally mentioned by @Zylann . Right now I'm having it function like a hidden onready, so
export (Sprite) var sprite will function as an exported NodePath and onready will set sprite to get_node(sprite). I think that in order to fully flesh out the issue, there may need to be an additional amount of data stored in the Node base class, a dictionary of NodePaths and expected types, and then the respective reference to the node. This dictionary will be updated when setting any of these exported properties

@creikey
Copy link

creikey commented Jul 5, 2020

I think the biggest issue with being able to update the exported nodepath in real time is that there would need to be some sort of generic templated setgets for every single export of a type, where the setter would:

  • only accept nodepaths
  • if I'm not in the tree return
  • if I am in the tree, fetch the node, ensure it's not null, and check its type, and if all is good cache it in a temporary variable
    and the getter:
  • if the temporary variable is null, return the nodepath
  • if it's not null, return the temporary variable

All of this could possibly be encapsulated in either a new variant type or object maybe

@creikey
Copy link

creikey commented Jul 5, 2020

The really hairy part about the setter only taking a NodePath is that then the variable cannot be reassigned with a new instance of its Node. Maybe the setter needs to be able to take a NodePath, then it will run get_node, or another instance of its native type or script, then it will simply assign.

@TheDuriel
Copy link

After some discussion with creikey:

Goal: Allow exporting of NodePath properties which automatically resolve to a reference to the Node. Condensing the acquisition of the path, and storage of its return value to one line.

How, part 1:

export(NodeRef, <Type>) var foo: <Type>

This creates a NodePath dialogue in the Inspector.

When the User selects a Path, get_node() is called, and the returned value is set to foo.

How, part 2:

When a Scene is saved, since Object References can not be serialized. The relative path to the node is gotten and stored instead. Using Node.get_path_to()

When a Scene is loaded, and a properties type or export hint is not of type NodePath, but a NodePath is found in the file. This path is resolved upon onready.

How, part 3:

To preserve RemoteTree functionality, like when loading a Scene: When a Node Reference would be remotely set. Instead set the NodePath, then resolve it in the running Game.


The Second half of Part 2, and Part 3, can also be described thus:

When setting a value to a property. If that value is a NodePath, and the Type hint is not. Resolve the path, then set the result instead.

@Shadowblitz16
Copy link

Shadowblitz16 commented Jul 31, 2020

one of the issues with nodepaths is that they are relative to the node the script is on.
this makes it hard to actually implement swapping logic in which a node's parent changes since the node path could no longer be pointing to the right node of even one that exists.

passing a reference to a node would be much better as it would no longer be tied to the node the script is on and you would directly be able to access its properties and methods as well

however I disagree with this..
export(NodeRef, <Type>) var foo: <Type>
I think it would be like so..
export(<Type that inherits from node>) var foo

there is no reason someone needs to dynamically type a node.
it just creates more problems when using it in the inspector and script

@TheDuriel
Copy link

TheDuriel commented Aug 1, 2020

You can not export a direct reference. It must be serialized as a NodePath for saving and loading. This has nothing to do with runtime behavior. Setting a direct reference to a node not within the current scene isnt something you can do in the editor either.

export(<type>) will export a field for Script Resource extending that type. This is wrong.

var foo: <type> this is required as otherwise the value is not typed at all from the IDEs perspective.

Ultimately

export(NodePath) var foo: <type> would be preferred.

@nathanfranke
Copy link
Author

Yes, we know that you can't export a reference. That's why this issue exists...

@Zylann
Copy link

Zylann commented Aug 1, 2020

This original issue existed because the use case of connecting up an existing node directly to a node script property isn't as straightforward as it could be. It works in a certain way because it's only about setting a reference to a node that already exists and can be referred to in tscn.

I see "reference" mentionned but we need to be careful about what we mean by that, so to clarify just in case:
Types inheriting Reference but not Resource are a bit different: wanting to export Reference types assumes we can create an instance of it on-site (cuz where else would we grab it from if it's neither a node nor a Resource file?), and that they would be serialized together with the scene or resource owning the property. Which is, basically what an embedded Resource does.

however I disagree with this..
export(NodeRef, <Type>) var foo: <Type>
I think it would be like so..
export(<Type that inherits from node>) var foo

I tend to agree to that. Why would the user be required to type NodeRef explicitely? Can't the engine figure this out from the specified type? Cases are exclusive. If it's Node, do the node behavior. If it's Resource, do the resource behavior. Else, error.
If the reason is because serialization is different, I'd say this is implementation detail. The same happens in Unity, yet there is no difference in scripts. So the question here would rather be if we want the user to be explicit or not.

Besides with export type inference it could be shortened as:

export var player : Sprite

And it would also work in C#:

[Export] public Sprite _player;

Or any language that can tell Godot "this property has this type and I export it".

Then it could be assigned by the scene loader (since the scene loader knows the paths, it's a specific syntax in .tscn, it can remember which properties need a node reference, and assign them in a second pass), so there is no need to wait for ready, no need to store a NodePath anywhere in the node.

@name-here
Copy link

At present, as far as I know, there's also no way to make the editor only let you set the exported NodePath to a certain Node type. Built-in Node types seem able to do this (see Node A and B properties on Joint2D that only accept RigidBodies, for example), but, as far as I know, there's no way to do it through script.

@Calinou Calinou changed the title Ability to export Node types Ability to export Node types instead of just NodePaths Nov 20, 2020
@jordo
Copy link

jordo commented Feb 25, 2021

It seems a bunch of users doing these one-liners:

export(NodePath) onready var _dependent_node = get_node(_dependent_node) as Node2D

It's nice to represent a dependency in one line like this, but also this is way too much code to express this. It also works (_dependent_node is either a valid Node2D or not). It would maybe be nice to perhaps syntactic sugar this in 4.0, if this is the easiest solution to expressing an external reference dependency, but only being able to serialize a NodePath.

But ya, I agree, something better would be a NodePath + Type. Maybe type information could somehow be added to path syntax (prefix or suffix or something). example: 'res://::Node2D::./../nodes/someNode2D'.

@Zylann
Copy link

Zylann commented Feb 25, 2021

Unfortunately that code seems to exploit a particular scenaro in GDScript. If you do this in a tool script it will break because _dependent_node will no longer contain a nodepath after being assigned. And similarly, when static typing is used it will cause an error because NodePath is not a Node2D.

I think such a feature does not even need the user to deal with NodePath or even onready (because that's what the feature abstracts away in the first place, it's more than syntactic sugar), as I wrote at the bottom of #1048 (comment)

@jordo
Copy link

jordo commented Feb 25, 2021

This works fine for static typing. And any script or tool script will break if _dependent_node isn't valid. Exploit or not, it seems a lot of users are using that code to setup a dependent reference in a single declaration.

And ya I agree, such a feature doesn't need. I'm just more or less commenting on whats being done in the wild right now.

@Zylann
Copy link

Zylann commented Feb 25, 2021

any script or tool script will break if _dependent_node isn't valid

It will break if the tool script requires the node to be set. That's precisely why I mention tool scripts. Sure just adding tool does not cause an error, but it will only have set _dependent_node to a NodePath, while the internal code expects it to be a Node2D and will fail as soon as it tries to access it, that's what I meant (because the inline get_node(path) only runs in ready).

This works fine for static typing

In your scenario yes, but theoretically, not in editor. If you add a static type _dependent_node : Node2D while exporting as NodePath, the editor will try to set _dependent_node (which is Node2D) with a value of type NodePath. That's obviously a type mismatch. In practice, if that doesn't fail then it's just another exploited bug where static typing has been forgotten to be checked.

When left untyped, it's a nice workaround for in-game properties though.

@jordo
Copy link

jordo commented Feb 25, 2021

In your scenario yes, but theoretically, not in editor. If you add a static type _dependent_node : Node2D while exporting as NodePath, the editor will try to set _dependent_node (which is Node2D) with a value of type NodePath. That's obviously a type mismatch. If that doesn't fail then it's just another exploited bug where static typing has been forgotten to be checked.

Ya, again, like I mentioned this is more commenting on what users are doing. Whether that's in theory or not, in practice it's obvious that people want to express this dependency as a single declaration. And this is what's currently being used as a workaround as opposed to declaring & maintaining multiple variables to express a single dependency.

I'm not sure if you'll be able to get rid of NodePath entirely in this proposal. Important to be able to serialize the dependency in a way that will fail at runtime if the serialized path can't resolve to an actual node. Also consider copy and paste working now with nodes, (I think copy & paste in the editor is purely in-memory, but I could be wrong).

@jordo
Copy link

jordo commented Feb 25, 2021

Mostly users just want to export+assign and use the dependency immediately as a node reference in code. They don't really care that under the hood it's serialized as a NodePath or whatever.

But consider this: If I export a NodeRef or whatever type is proposed here and assign it to some sibling node, and then copy pasta this node into an entirely different scene what happens? (Tricky to do with a reference type. Lots of edge cases). Which is why I'd argue NodePath or some underlying resource type is still what we want to use to implement this functionality. Which then brings this full-circle to still doing something that is more syntactic sugar to accomplish what users actually want (less boilerplate to express this type of dependency).

@Zylann
Copy link

Zylann commented Feb 25, 2021

Important to be able to serialize the dependency in a way that will fail at runtime if the serialized path can't resolve to an actual node

There still has to be a nodepath in tscn and PackedScene because this is what identifies the node when it is not built in memory. Just like resources are represented as file paths, it doesn't mean they have to also be file paths in properties referencing them. A system like this usually won't have bad node paths because the editor simply won't let you do that. But if it it's not valid for any reason, same as resources: can't load it. Either fail to load the scene, print an error, or leave the property null.

f I export a NodeRef or whatever type is proposed here and assign it to some sibling node, and then copy pasta this node into an entirely different scene what happens?

You can serialize the copied scene (so it becomes a nodepath, no longer an object in memory) and then deserialize into the destination scene. The same logic runs: if what you copied had a path to a sibling that's not part of your selection, then deserialization won't assign the property because it won't be within the data you copied, and it will come up as null.
You could try looking up nodes in the scene as well and assign them if they happen to have the same path but I'm not sure it would be a good idea.

Which is why I'd argue NodePath or some underlying resource type is still what we want to use to implement this functionality. Which then brings this full-circle to still doing something that is more syntactic sugar to accomplish what users actually want (less boilerplate to express this type of dependency).

Then that's a different feature: typing nodepaths. If you want this then the property will be a nodepath.

@TheDuriel
Copy link

My proposed approach already solves all this.

@jordo
Copy link

jordo commented Feb 25, 2021

You can serialize the copied scene (so it becomes a nodepath, no longer an object in memory) and then deserialize into the destination scene. If what you copied had a path to a sibling that's not part of your selection, then deserialization won't assign the property because it won't be within the data you copied, and it will come up as null.

You COULD do this, but last I checked this is not how copy/paste, undo/redo is done in the engine right now, and would be a very large undertaking to change this. imo probably very unlikely to be implemented like this.

@TheDuriel, I'm in agreement with your approach syntax wise, but I disagree that the path should be resolved/written at scene save/load time. I think it should be resolved at assignment time.

@Zylann
Copy link

Zylann commented Feb 25, 2021

You COULD do this, but last I checked this is not how copy/paste, undo/redo is done in the engine right now

Yeah... I don't like this tbh, that's one of the reasons. I had to deal with that stuff already in the past and that's how I solved it (it even worked across two editors)

@fire-forge
Copy link

fire-forge commented Jan 18, 2022

So basically it's the same as a NodePath export but it automatically sets it to get_node() of it? That would be useful.

Right now I do this, which is messy and adds an extra variable and line:

export var node_: NodePath
onready var node: = get_node(node_) as Node

With this feature that code could be replaced with this and it would look identical in the Inspector:

export var node: Node

@Ansraer
Copy link

Ansraer commented Mar 17, 2022

For the record: on the 22.2.2022 there was some discussion regarding this on rocketchat in #gdscript. The consensus was that this is a desired feature, but the implementation details are still a bit unclear.

https://chat.godotengine.org/channel/gdscript?msg=dMaetkhmJxXGGaR73

@TheDuriel
Copy link

User Facing:

export(Sprite) var my_sprite: Sprite

Or if we wish to be explicit:

export(NodePath, Sprite) var my_sprite: Sprite

This would expose a NodePath picker in the inspector, limited to the specified type.

export(NodePath, <Any>) may be neccecary for flexibility?

Implementation Concept:

There are two places in which behavior must be added.

When Serializing Scenes (in the Editor).

When Instancing a PackedScene.

Serialization:

In packed_scene.cpp

If SceneState::_parse_node

In the loop below 

List<PropertyInfo> plist;
p_node->get_property_list(&plist);

Check if an Exported property is a NodePath. Save the NodePath to the Node that the property currently contains.

This may actually be done in Node.cpp instead?

Instancing:

In node.cpp

In Node::_notification

At NOTIFICATION_READY

Check if an Exported property is a NodePath, get_node() that path and set the new value.

@Zylann
Copy link

Zylann commented Mar 17, 2022

I wonder again why node resolution has to wait all the way to ready at all... since the scene loader instance() already knows everything it needs to know to assign the instance to the property. It can create each node, then assign references in a second pass, and return. No need to cache paths in the node, no need to re-resolve it in the scene tree for each instance.

@TheDuriel
Copy link

For this example I put it in ready, simply because it seems like a big hassle to do so in PackedScene and I couldn't find the place to put it.

@sairam4123
Copy link

sairam4123 commented May 6, 2022

I think this is all about exposing already present feature in Godot. You can always filter nodes by Type in the Node Selector panel.

Edit: After reading a bit, I understand the problem. Maybe the node could be instanced just like onready?

@creikey
Copy link

creikey commented Jun 26, 2022

Is this happening in 4.0 or not yet? I'd like to add it to 4.0 if it's not currently in the codebase and the gdscript source is settling down somewhat

@TheDuriel
Copy link

@creikey See godotengine/godot#62185

@akien-mga
Copy link
Member

Implemented by godotengine/godot#62185 (core) and godotengine/godot#62470 (GDScript). Might still need some similar changes for C# support, and any other language bindings that implement property hints as a language feature.

@MartinHaeusler
Copy link

@akien-mga Yes please, we need this on the C# side too. The inability to type node paths properly is a big deal, and I'm very happy to see it's being addressed in Godot 4.

@Shadowblitz16
Copy link

Shadowblitz16 commented Jul 15, 2022

Wait what about exporting custom node types defined with class_name?
I tested it and it doesn't work.

@nathanfranke
Copy link
Author

Wait what about exporting custom node types defined with class_name? I tested it and it doesn't work.

Proposed here: #4785

@tjp1205
Copy link

tjp1205 commented Mar 21, 2023

I am currently on Godot 4.0 stable, where nodes can be exported and used in a running. However, when paired with the [Tool] attribute, the exported nodes would return null while the script is running in the editor. Is this intended, or is this feature not supported yet?

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