BlockScriptSerialization: Re-generate block definition for object property blocks

[starnight]

The drag & drop object property's blocks disappear after save & re-open the Godot project. And, shows error:

core/variant/variant_utility.cpp:1092 - Cannot construct block from null block definition. res://addons/block_code/ui/block_canvas/block_canvas.gd:348 - Invalid call. Nonexistent function 'set_parameter_values_on_ready' in base 'Nil'.

It is because the object property blocks are not the predefined blocks in the catalog. So, Block Coding plugin cannot find the block definition from the catalog when places the object property blocks into the cavas via _block_to_ast_node() after re-open the project.

Therefore, introduce _get_obj_property_block_definition() generating object property's getter/setter block definition for get_block_definition(). Besides, it also needs the object property's value type. So, pass the AST node's arguments which is a Dictionary and may contain the "value" feild into get_block_definition(), too.

Fixes: 89beea9 ("BlockCanvas: Implement drag & drop the node's property from Inspector") https://phabricator.endlessm.com/T35649

[wjt]

I really don't like how we construct a string and then pass it to a function which immediately splits it back up into the 3 pieces we had here.

[dylanmccall]

Have you tried building this feature around _get_parameter_block_definition? There's a bit of overlap here. In fact they're solving the same fundamental problem: it's a type of block where we generate a block definition when we need it, based on an identifier such as a property name.

As it exists, that function only generates the one kind of value block, which is an output parameter for another block. (It needs a base block definition so it knows what output parameters are defined). So it needs extra help, but it should be feasible to extend it similarly to what you have in get_obj_property_block_definition, so our parameter blocks would start looking like MyType_get:property.

I think there's a few ways to make that work. I think ideally we'd have _get_base_block_definition generate a plain MyType_get and MyType_set block for whatever type is at hand, and we'd have something in BlockDefinition that _get_parameter_block_definition calls which would be like "give me a parameterized version of this block definition with foo set to bar".

I realize I'm asking for a fair bit of extra machinery here so I understand if we just want to land a bug fix, but I did have this case in mind when I was adding parameter block definitions. Sorry I didn't really document it :)

[starnight]

Let me study this more.

[starnight]

I try some scenario as examples: the "speed" and "floor_stop_on_slope" properties

• "Object_get:speed" will be split for _get_parameter_block_definition()

  • block_name: "Object_get"
  • parameter_name: "speed". Its type is Vector2

• "Object_get:floor_stop_on_slope" will be split for _get_parameter_block_definition()

  • block_name: "Object_get"
  • parameter_name: "floor_stop_on_slope". Its type is boolean

Because of the different parameter_names, we have to generate different base block definitions having corresponding display_template for parent_out_parameters.has(parameter_name) dynamically, which means the object property block's logic is different from the parameter block.

[starnight]

Besides, it loses the property's variant type after close the project actually. Because, the property's variant type is not saved in the resource of .tscn. The setter block's variant type can be restored by the stored value. However, getter block does not have the value information.

[starnight]

Based on the limitation of the parameter_name and the getter's variant type, I think we cannot generate blocks via _get_parameter_block_definition() now. So, I go back to the _get_obj_property_block_definition() in the updated commits.

[dbnicholson]

Sorry for coming in late here, but the property doesn't need to be encoded in the scene file because it's part of the object it's being applied to. It's more work, but you can call get_property_list on the target object to determine the type. I think that would be more elegant than encoding the type in the block name. It wouldn't be that hard to get the property list once at the beginning of deserializing and then find the appropriate property as the definitions are loaded.

[starnight]

I encode the property's variant type into block name finally. So, after reopen the project, block coding can restore the variant type information of the property's getter block. Here is the demo video:
https://github.com/user-attachments/assets/82e9d524-0d5a-4a1d-947d-7af17a51314a

[manuq]

Trying to get the property name from the block name like foo from get_var_foo is not ideal. The VariableDefinition in addons/block_code/code_generation/variable_definition.gd already has name and type.

Now as discussed, reusing the variables in BlockScriptSerialization is a problem because properties shouldn't be declared as var in the script. And checking ClassDB.class_get_property_list() doesn't work for custom node's properties like the Simple ones. Two alternatives occur to me to separate variables from properties:

• Enhace VariableDefinition with an is_property boolean field
• Add @export var properties: Array[VariableDefinition] to the BlockScriptSerialization resource

[dbnicholson]

Now as discussed, reusing the variables in BlockScriptSerialization is a problem because properties shouldn't be declared as var in the script. And checking ClassDB.class_get_property_list() doesn't work for custom node's properties like the Simple ones. Two alternatives occur to me to separate variables from properties:

• Enhace VariableDefinition with an is_property boolean field
• Add @export var properties: Array[VariableDefinition] to the BlockScriptSerialization resource

I don't see how adding an is_property field helps here since you still need to figure out what type of property it is. I'm also not crazy about exporting the property list since it's bound to get out of sync with the target node's properties.

I came up with a 3rd option. What we really want is to look at the target node's instance, but that's not available in the serialization since it's a child of the node. However, we can find the node from the scene tree because we know that the parent BlockCode node's block_script property will be set to this instance. In editor-only code without safety:

var root := EditorInterface.get_edited_scene_root()
for node in root.find_children("*", "BlockCode"):
    if node.block_script == self:
        var target_properties := node.get_parent().get_property_list()

https://github.com/endlessm/godot-block-coding/tree/block-script-target adds some helpers to do it. When deserializing property block definitions, you'd do something like:

var property := _get_target_property("thename")
var variant_type: Variant.Type = property["type"]

[dbnicholson]

Another simpler possibility is that we have the BlockCode just tell BlockScriptSerialization about itself when it's instantiated:

diff --git a/addons/block_code/block_code_node/block_code.gd b/addons/block_code/block_code_node/block_code.gd
index 659719c..d08e89a 100644
--- a/addons/block_code/block_code_node/block_code.gd
+++ b/addons/block_code/block_code_node/block_code.gd
@@ -5,7 +5,15 @@ extends Node
 
 const TxUtils := preload("res://addons/block_code/translation/utils.gd")
 
-@export var block_script: BlockScriptSerialization = null
+@export var block_script: BlockScriptSerialization = null:
+       set(value):
+               # If the script is being cleared, remove ourself from the existing script.
+               if value == null:
+                       if block_script:
+                               block_script.block_code_node = null
+               else:
+                       value.block_code_node = self
+               block_script = value
 
 
 func _init():
diff --git a/addons/block_code/serialization/block_script_serialization.gd b/addons/block_code/serialization/block_script_serialization.gd
index 5f3051e..e0847f0 100644
--- a/addons/block_code/serialization/block_script_serialization.gd
+++ b/addons/block_code/serialization/block_script_serialization.gd
@@ -28,6 +28,8 @@ const SCENE_PER_TYPE = {
 @export var generated_script: String
 @export var version: int
 
+var block_code_node: BlockCode
+
 var _available_blocks: Array[BlockDefinition]

With the BlockCode node in hand, you can do all the same things like get the target object with get_parent() and then sift through its properties with target.get_property_list().

[starnight]

I came up with a 3rd option. What we really want is to look at the target node's instance, but that's not available in the serialization since it's a child of the node. However, we can find the node from the scene tree because we know that the parent BlockCode node's block_script property will be set to this instance. In editor-only code without safety:

var root := EditorInterface.get_edited_scene_root()
for node in root.find_children("*", "BlockCode"):
    if node.block_script == self:
        var target_properties := node.get_parent().get_property_list()

I was stuck at why I cannot get_parent(), which should be the BlockCode node's parent directly in the addons/block_code/serialization/block_script_serialization.gd until I learned block_script_serialization.gd is loaded as a property of the BlockCode node. Thank you @dbnicholson

[dbnicholson]

I was stuck at why I cannot get_parent(), which should be the BlockCode node's parent directly in the addons/block_code/serialization/block_script_serialization.gd until I learned block_script_serialization.gd is loaded as a property of the BlockCode node. Thank you @dbnicholson

Just to clarify a bit. Only Nodes like BlockCode are part of the scene tree and can navigate to their ancestors or descendents using things like get_parent(). BlockScriptSerialization is a Resource, which is not a Node. Godot doesn't provide any sort of methods like "what other objects hold a reference to this object".

[starnight]

As #316 (comment), I think having the bidirectional link between the BlockCode and BlockScriptSerialization is more readable and convenient in the future. So, fixed in the direction. Ready for review again!

[dbnicholson]

I think this makes sense to me. The only thing I'd want to see addressed is the change to pass in the block arguments when serializing. That doesn't seem necessary if you're already using the property information for the property getter block.

[dbnicholson]

It's a bit unfortunate that these property blocks will be in the Variables section, but I can't think of a better place at the moment.

[dbnicholson]

A potential future improvement here would be to cache the parent properties when set so that _get_parent_node_property_info didn't have to iterate the entire property list every time. I don't think that's worth it right now, though.

[dbnicholson]

That looks like it should work! 2 final nitpicks, but feel free to merge after that.

[starnight]

Thank you guys' help! I merge this!