Open rcorre opened 4 years ago
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.
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
.
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.
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 😅
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
?
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 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.
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:
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>])
And my NodePath suggestions:
Variant get_property()
void set_property(Variant value)
Object get_container()
I'm fine with implementing this if it looks good to everyone.
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.
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.
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:
In the editor, you see an editable property:![editor_prop](https://user-images.githubusercontent.com/2496231/68863814-0c694580-06be-11ea-9d7a-9a509616f33e.png)
Click on that property to see a node selector:![node_select](https://user-images.githubusercontent.com/2496231/68863820-0ecb9f80-06be-11ea-83dd-d8482fb96a24.png)
After clicking OK, you now see a property selector for that node:![prop_select](https://user-images.githubusercontent.com/2496231/68863828-11c69000-06be-11ea-87c9-c72a4f0c2cd7.png)
Click on,
translation
.path
is now set toNodePath("MeshInstance:translation")
.Describe implementation detail for your proposal (in code), if possible:
PROPERTY_PATH
hint toexport(NodePath)
, which tells the editor it should point at a property. There's already a precedent for hints like this:AnimationPlayer
) for NodePath properties exported with thePROPERTY_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.