Godot-proposals: Add property path support to NodePath

[rcorre]

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:
   

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

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

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.

[asheraryam]

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.

[bojidar-bg]

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.

[rcorre]

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.

[asheraryam]

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 😅

[rcorre]

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?

[bojidar-bg]

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.

[rcorre]

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