godot-proposals icon indicating copy to clipboard operation
godot-proposals copied to clipboard

Add property path support to NodePath

Open rcorre opened this issue 6 years ago • 12 comments

Describe the project you are working on:

A multiplayer versus game. Two of them actually, the one I'm currently working on, as well as a prior gamejam project.

Describe the problem or limitation you are having in your project:

In both games, I want to color objects based on their team. For any given object (player, projectile, item, ect.), there are a specific set of properties that need to be changed. For example, theming a lich means setting the color of its eyes and its light. Theming a projectile means setting the color on a MeshInstance and a particle effect.

I end up writing specific logic for every thing.

Describe how this feature / enhancement will help you overcome this problem or limitation:

I love Godot's AnimationPlayer. The ability to wire it up to an arbitrary property of any node in the scene tree makes it an incredibly flexible tool.

Wouldn't it be awesome if your scripts could tap into this same flexibility? You could write generic, flexible scripts that can be customized to specific scenes by pointing them at particular properties.

In my case, I could have a generic "themeable" script, and provide the paths to various properties I want to tweak on a particular object..

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:

  1. Attach the following script to an object:
extends Spatial

export(NodePath, PROPERTY_PATH) var path

func _ready():
	get_node(path).set_indexed(path.get_concatenated_subnames(), 2)
  1. In the editor, you see an editable property: editor_prop

  2. Click on that property to see a node selector: node_select

  3. After clicking OK, you now see a property selector for that node: prop_select

  4. Click on, translation. path is now set to NodePath("MeshInstance:translation").

Describe implementation detail for your proposal (in code), if possible:

  1. Add a PROPERTY_PATH hint to export(NodePath), which tells the editor it should point at a property. There's already a precedent for hints like this:
export(float, EASE) var transition_speed
export(Color, RGB) var col
export(Color, RGBA) var col
  1. Show the property selector (which is already implemented for AnimationPlayer) for NodePath properties exported with the PROPERTY_PATH flag.

The rest of the functionality already exists. Property/resources paths are already built in to NodePath Arbitrary properties are already accessible in code via get_indexed and set_indexed.

If this enhancement will not be used often, can it be worked around with a few lines of script?:

Not a few lines of script, but many lines repeated over and over. See above.

Is there a reason why this should be core and not an add-on in the asset library?:

Most of the functionality already exists in godot core, it just needs to be tied together.

rcorre avatar Nov 14 '19 14:11 rcorre

I'm worried about hiding a lot of the specifics of property assignment as it could cause a lot of user mistakes.

I'd vote for this only if the property assignment enforced explicit typing in the declaration, so for example it would look like this:

export(PropertyPath, Vector2) var vector_property_path

At least this way we can have some form of code completion and editor errors if a user tries to do something wrong like like assigning a SpatialMaterial to a Vector2.

In the editor, you would only be able to select properties that fulfill this criteria as well, making it less likely that users can mess up.

Also due to how properties work I do not think users would ever be able to do vector_property_path = Vector2(1,1) and instead it would have to look like this:

set_property_from_path(vector_property_path, Vector2(1,1))
player_location = get_property_from_path(other_vector_property_path)

As for internal code changes, it should not be very demanding, since the implementation would only have to call the set() method for the respective property.

realkotob avatar Nov 20 '19 10:11 realkotob

Hinting property typing might be done similar to arrays:

export(NodePath, PROPERTY, Vector2) var path_to_arbitrary_vector2: NodePath
export(NodePath, PROPERTY, NodePath) var path_to_arbitrary_nodepath: NodePath
export(NodePath, PROPERTY, NodePath, PROPERTY, Vector2) var path_to_arbitrary_path_to_arbitrary_vector2: NodePath

export(Array, int, "Elf", "Gnome", "Wizard") var class: Array
export(Array, NodePath, PROPERTY, Color) var colors_to_set: Array

Note that it should stay NodePath, PROPERTY for now, since there is no seperate type named PropertyPath yet.

And yeah, having methods to set the property on the target node directly would be useful -- maybe something like get_property/set_property, or as mentioned above, get/set_property_from_path.

bojidar-bg avatar Nov 20 '19 10:11 bojidar-bg

I'm worried about hiding a lot of the specifics of property assignment as it could cause a lot of user mistakes.

Meaning assigning the wrong type? I think that's an underlying limitation of a dynamically typed language, not a problem specific to this proposal. Still, I like the idea of adding a type hint, if nothing else for the filtering you'd get in the editor.

I was hoping set_indexed("global_transform", "foo") would error, but it passes silently, and the transform is unchanged. Hopefully it is ignoring the call, though maybe it is undefined behavior? Maybe having set_indexed error on a type mismatch is a bug/another proposal.

I do not think users would ever be able to do vector_property_path = Vector2(1,1)

I'm not sure what you mean by this, properties can be get/set as I mentioned in the OP:

get_node(path).set_indexed(path.get_concatenated_subnames(), 2)

Its a little non-obvious but it works. Are you suggesting that we add a helper method to Object that combines the node and property lookup into one? I was trying to keep the proposal minimal, but I guess a helper would be fine?

Actually, instead of adding a new method (Object already has a pretty big API), what if set_indexed would perform Node lookup?

# Old usage, still works as the NodePath points to the current object
set_indexed(NodePath("position:y"), Vector2(42, 0))

# New usage, looks up a node and doesn't conflict with any old usage
set_indexed(NodePath("SomeChild/position:y"), Vector2(42, 0))

I only mention this because to me the Godot API seems to have a bit of bloat -- there's a lot of helper methods where I think "Really? Do I actually need a helper to save me one line of code?" and it makes it harder to find the method you actually want. Imagine that now users have to look at the API and figure out the difference between get_indexed/set_indexed and get_property/set_property.

That's not a hill I'll die on though :) I can make my peace with a new method.

rcorre avatar Nov 20 '19 12:11 rcorre

I'm fine with get_indexed/set_indexed honestly as long as it throws an error instead of passing silently. I misread your original post and did not know the set/get_indexed already existed 😅

realkotob avatar Nov 20 '19 12:11 realkotob

I misread your original post and did not know the set/get_indexed already existed

I'm glad you mentioned it though, otherwise I wouldn't have noticed the silent failure

For enforcing types, my concern was that it would limit user's ability to create their own node that is as flexible as AnimationPlayer. Would something like this be possible with enforced types?

export(NodePath, PROPERTY, Variant)

Then users have the option of an exported field that can be any NodePath, while the common (encouraged) case would be something more specific than Variant?

rcorre avatar Nov 20 '19 12:11 rcorre

export(NodePath, PROPERTY, Variant) will not work, as GDScript does not have a reserved word for Variant. Instead, we could have it as just export(NodePath, PROPERTY). That way, it would follow the existing cases of omitted types being treated as Variant.

The trouble with supporting this with set/get_indexed directly is that they are defined in Object, and not in Node, and Object shouldn't really know about Node. Maybe the method could be overridden in Node, but this could have performance implications.

bojidar-bg avatar Nov 20 '19 12:11 bojidar-bg

@bojidar-bg good point, I don't think giving {get,set_indexed} knowledge of the node tree is a good idea.

In that case, my preference is just to use get_node(path).set_indexed(path.get_concatenated_subnames(), value), though I'm fine with adding new methods to Node as well.

rcorre avatar Nov 20 '19 13:11 rcorre

We discussed this in a proposal review meeting today, and overall we agree that this seems useful.

It would be good to get another round of discussion on this proposal to get a clearer idea of what should be implemented exactly:

  • Suggestions for a syntax using the new annotations in GDScript 2 / Godot 4.0
  • Maybe more details on use cases that this would enable (there's a few described by @rcorre but it would be useful if other interested users could describe how they might use a feature like this, to make sure we're not implementing something that would only be known and useful to few power users)
  • Some discussion on what should be exposed from NodePath exactly, as beyond properties it also supports subresources and various levels of nesting, etc.

akien-mga avatar Jul 07 '22 15:07 akien-mga

Here's my syntax suggestion:

@export_property_path(node_type: TypeUnion<? extends Node> | Array[TypeUnion<? extends Node>] = Node, ...property_types: TypeUnion<? extends ExportableType> | Array[TypeUnion<? extends ExportableType>])

Exports a NodePath property with a property path and a filter for allowed node and property types.

See also PROPERTY_HINT_NODE_PATH_PROPERTY.

Examples
# Simple case. Any property on any node.
# Equivalent to @export_property_path(Node).
@export_property_path()
var any_property
# An error. The first argument must be either not given or a subclass of Node.
@export_property_path(Object)
var this_doesnt_work
# Any property on a Node2D.
# Hints PROPERTY_HINT_NODE_PATH_PROPERTY "Node2D:"
@export_property_path(Node2D)
var node2d_property
# A boolean property on any node.
# Hints PROPERTY_HINT_NODE_PATH_PROPERTY "Node:%s" % [TYPE_BOOL]
@export_property_path(Node, bool)
var bool_property
# A transform, either 2D or 3D.
# Hints PROPERTY_HINT_NODE_PATH_PROPERTY "Node:Transform2D,Transform3D"
@export_property_path(Node, Transform2D | Transform3D)
var transform_property
# A float property on a PhysicsMaterial on a RigidBody2D.
# Hints PROPERTY_HINT_NODE_PATH_PROPERTY "Node:Transform2D,Transform3D:%s" % [TYPE_FLOAT]
@export_property_path(RigidBody2D, PhysicsMaterial, float)
var friction_or_bounce
# This one is probably easy.
# Hints PROPERTY_HINT_NODE_PATH_PROPERTY "MultiMesh:PackedVector2Array"
@export_property_path(MultiMesh, PackedVector2Array)
var instance_transforms_2d
PROPERTY_HINT_NODE_PATH_PROPERTY = 37

Hints that a NodePath property points to a property. The hint string is a colon-separated list of comma-separated types. An empty part is assumed to be Variant.


And my NodePath suggestions:

Variant get_property()

Returns the value of the property. Throws an error if the property doesn't exist, or if the NodePath does not have a property. The behavior is left to further debate if a property is null along the way.

void set_property(Variant value)

Sets the value of the property. Throws an error if the property doesn't exist, or if the NodePath does not have a property. The behavior is left to further debate if a property is null along the way.

Object get_container()

Returns the object containing the property: the second-to-last property for multi-property paths, the node for single-property paths, and throws an error for propertyless paths.


I'm fine with implementing this if it looks good to everyone.

PoolloverNathan avatar Mar 07 '23 13:03 PoolloverNathan

Maybe more details on use cases that this would enable (there's a few described by @rcorre but it would be useful if other interested users could describe how they might use a feature like this, to make sure we're not implementing something that would only be known and useful to few power users)

Something like this would be nice for my plugin that turns the output of a voice analyzer into a lip syncing animation. This would allow me to have my plugin users select specific properties they want to have animated. For example a shader paramater hat offsets the UV coordinates over a texture with different mouth shapes.

What is interesting to point out is that UI wise, there already exists a property selector implementation in godot/editor/property_selector.cpp. So i believe that this class can be used to again for this, cutting down on the work required to set this up.

I am not sure if it warrants a new annotation, it would be nice. But i think that exposing the property selector through a property hint would be already a lot of quality of life.

Pheubel avatar Apr 22 '24 22:04 Pheubel

Another use case is having a simpler way to connect puzzle like elements like a door and a button without using signals. Button script could have a property "is_pressed" which door script could read "../Button:is_pressed" and based on that open or close. If we want more complicated system and implement for example logic gate we could have it read properties of two objects (two buttons) in scene and have door be opened if the value outputted by the gate is true. This way we can have a system which is easier for use by level designers.

DrRevert avatar Jun 28 '24 13:06 DrRevert

Here's a quick implementation I tossed up by reusing PropertySelector and making a few tweaks.

https://github.com/user-attachments/assets/cc82c852-cef7-4b11-9f61-cec0ad265c60

Some thoughts I've had while tossing this up:

  • We might have to change PropertySelector to be able to pick a property of a property.
  • Creating a HINT in Godot 4 was harder than I thought. Especially because @export_custom_path has a hardcoded hint from what I've seen. Need to dig more into that.
  • Since I didn't make a hint for this, I resorted to making the user choose whether he wanted to select a property or not via a Confirmation Dialog that appears after the first Node Picker (SceneTreeDialog). An extra dialog popup is not ideal as it can disrupt user flow. However, all alternatives I could think of would involve changing SceneTreeDialog (maybe having an extra button "Select" and "Open Property Select")

Dowsley avatar Mar 21 '25 22:03 Dowsley