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

Add ability to export Node pointers as NodePaths #62185

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented Jun 18, 2022

This PR implements:

  • A new hint: PROPERTY_HINT_NODE_TYPE for variant type OBJECT, which can take specific node types as hint string.
  • The editor will show it as a node path, but will set it as a pointer to a node from the current scene if you select a path.
  • When scene is saved, the node path is saved, then restored as a pointer.

This is the low level machinery required for doing something like:

@export var myvar : Button

And be able to edit this as a node path from the inspector, so you can assign another node. This functionality has been very requested, but it was not done due to how difficult it actually is to implement.

This is implemented by editing the property path in a hidden metadata property, then converted to an actual path in PackedScene and assigned on instantiation of it after the scene has been created.

Bugsquad edit: Implements godotengine/godot-proposals#1048

@Calinou Calinou added this to the 4.0 milestone Jun 18, 2022
@reduz reduz force-pushed the export-node-pointer-path branch from edeaefa to 8f6e3bf Compare June 19, 2022 07:59
@reduz
Copy link
Member Author

reduz commented Jun 19, 2022

Another way we may be able to implement this in a more clean way is to make this a regular NodePath property that somehow points to another, hidden property. Example:

PropertyInfo(Variant::NODE_PATH,"which_node_path",PROPERTY_HINT_NODE_PATH_TO_NODE_PROPERTY,"which_node:Button");

This means that, during inspector editing, only the node path will be set via the which_node_path property (pointer could be set too, I don't know) but upon saving the scene, the extra special property will be saved (as the current code does) that will result in setting this value to which_node after the scene is loaded.

This script languages would need to expose this as an extra property and, if you want to use this in GDExtension, you have to also use it manually by exposing another property.

@rburing
Copy link
Member

rburing commented Jun 19, 2022

For completeness let me point out an existing workaround/hack in 3.x:

export(NodePath) onready var button = get_node(button)

One would "just" have to add a better syntax and static typing to this solution. Maybe it can serve as inspiration.

@reduz reduz force-pushed the export-node-pointer-path branch from 8f6e3bf to 24f5ff8 Compare June 19, 2022 11:46
@jasonwinterpixel
Copy link
Contributor

Another way we may be able to implement this in a more clean way is to make this a regular NodePath property that somehow points to another, hidden property. Example:

Yes I think this is the way.

I've always imageind this feature just as syntax sugar that would back the exported Nodes with a hidden NodePath variable.

eg) User writes:

@export(Button) var myvar

But Godot actually creates this:

@export(NodePath) var __nodepath_myvar
@onready var myvar:Button = get_node(__nodepath_myvar)

@reduz reduz force-pushed the export-node-pointer-path branch from 24f5ff8 to cf61741 Compare June 19, 2022 14:37
@reduz
Copy link
Member Author

reduz commented Jun 19, 2022

@jasonwinterpixel My most recent commit does this transparently now by using the newly implemented metadata system. It will probably need a good amount of testing once @vnen implements it in GDScript 2.0 but I am confident this is probably the simplest way for users (even if it required a bit of hack in the inspector and PackedScene).

@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 19, 2022

Can the syntax be made in such a way so that we preserve actual type hints, not introduce export hints back? We just unified the mess we have in 3.x with both export hinted types and proper types.

@reduz
Copy link
Member Author

reduz commented Jun 19, 2022

@YuriSizov I think that is the case, I am just not used to writing modern GDScript so my example is likely wrong.

@neikeq
Copy link
Contributor

neikeq commented Jun 20, 2022

It has the downside that it requires a pointer to be node to be kept during editor.

I didn't quite understand this. Can you explain it a bit more?

@neikeq
Copy link
Contributor

neikeq commented Jun 20, 2022

If I understand this correctly, this implementation assigns the node properties when instantiating a packed scene rather than on _ready, right?

@reduz reduz force-pushed the export-node-pointer-path branch 2 times, most recently from be638ea to ebabdab Compare June 20, 2022 10:45
@reduz reduz marked this pull request as ready for review June 20, 2022 10:45
@reduz reduz requested review from a team as code owners June 20, 2022 10:45
@reduz
Copy link
Member Author

reduz commented Jun 20, 2022

@neikeq It no longer needs a pointer. It now edits a hidden metadata and stores the nodepath there instead of modifying the actual property. It sets this on PackedScene instantiation, not ready.

@dpalais
Copy link
Contributor

dpalais commented Jun 21, 2022

Ever since we got unique nodes the need for this with C# has been reduced drastically. It's still a nice feature to have but I just wanted to let you know about the unique nodes perspective of this.

@neikeq
Copy link
Contributor

neikeq commented Jun 21, 2022

 It sets this on PackedScene instantiation, not ready.

Then I think this is much better than the "export NodePath & onready get_node" syntax sugar proposed above. It behaves just like any other exported property.

@reduz
Copy link
Member Author

reduz commented Jun 21, 2022

@neikeq Yes, it aims to be as transparent as possible to the user code, so it can be used from C# or C++ directly without language hacks. Just export a property to a node pointer and give the editor hint, then the editor will manage the nodepath internally and set the pointer on scene instantiation, all transparently.

Doing it from within GDScript would have been much easier, but this I something you want in the other languages too, which is why it took so long to figure out how to do it the best possible way.

editor/editor_properties.cpp Outdated Show resolved Hide resolved
editor/editor_properties.cpp Outdated Show resolved Hide resolved
@reduz reduz force-pushed the export-node-pointer-path branch from ebabdab to 4c2578a Compare June 23, 2022 13:44
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I've been testing this a bit and it works well. We can probably merge this and do further improvement later.

@reduz reduz force-pushed the export-node-pointer-path branch from 4c2578a to ac09f6e Compare June 23, 2022 14:03
editor/editor_properties.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.h Outdated Show resolved Hide resolved
editor/editor_properties.cpp Show resolved Hide resolved
@reduz reduz force-pushed the export-node-pointer-path branch from ac09f6e to 74741a7 Compare June 25, 2022 13:44
This PR implements:
* A new hint: PROPERTY_HINT_NODE_TYPE for variant type OBJECT, which can take specific node types as hint string.
* The editor will show it as a node path, but will set it as a pointer to a node from the current scene if you select a path.
* When scene is saved, the node path is saved, then restored as a pointer.

NOTE: This is a proof of concept and this approach will most likely not work. The reason if that, if the node referenced is deleted, then when trying to edit this the node will become invalid.

Potential workarounds: Since this uses the Variant API, it should obtain the pointer from the Variant object ID. Yet, this would either only really work in GDScript or it would need to be implemented with workarounds in every language.
Alternative ways to make this work: Nodes could export an additional property with a node path (like for which_node, it could be which_node_path).
Another alternative: Path editing could happen as a hidden metadata (ignoring the pointer).
@reduz reduz force-pushed the export-node-pointer-path branch from 74741a7 to b7c41f9 Compare June 25, 2022 13:50
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to merge once it passes CI.

@AaronRecord
Copy link
Contributor

AaronRecord commented Jun 25, 2022

I might be missing something, but I just tested this with GDScript in a new project and it's not working:
image

It looks like these lines in the GDScript parser need to be changed to accept Nodes as a valid export type:

case GDScriptParser::DataType::NATIVE:
if (ClassDB::is_parent_class(export_type.native_type, SNAME("Resource"))) {
variable->export_info.type = Variant::OBJECT;
variable->export_info.hint = PROPERTY_HINT_RESOURCE_TYPE;
variable->export_info.hint_string = export_type.native_type;
} else {
push_error(R"(Export type can only be built-in, a resource, or an enum.)", variable);
return false;
}
break;

Also the body of the commit message should maybe be updated

@KoBeWi
Copy link
Member

KoBeWi commented Jun 25, 2022

@AaronRecord The GDScript part is going to be implemented in a follow-up PR.

@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.