SimpleSpawner: Add spawn_frequency migration

Renaming the spawn_frequency properties and blocks will break any
existing scene since those are serialized. Migrate the spawn_frequency
property by providing it as an additional unstored property that simply
wraps spawn_period. Migrate the block names and arguments using the
serialization renaming support from the previous commit. A test using a
scene created before the renaming is provided to verify the migration.

While this does not handle the worst compatibility breakage, it can be
used as a model for other migrations. Note that the migration stub in
MainPanel is likely too late for any legitimate fixes. Any migration
likely needs to happen when the scene is loaded and instantiated, which
is before MainPanel gets a hold of the objects.

https://phabricator.endlessm.com/T35547

addons/block_code/serialization/block_serialization.gd

@@ -17,7 +17,9 @@ func _init(p_name: StringName = &"", p_children: Array[BlockSerialization] = [],
 
 
 # Block name and arguments backwards compatibility handling.
-const _renamed_blocks: Dictionary = {}
+const _renamed_blocks: Dictionary = {
+	&"simplespawner_set_spawn_frequency": &"simplespawner_set_spawn_period",
+}
 
 
 func _set_name(value):
@@ -31,7 +33,12 @@ func _set_name(value):
 		name = value
 
 
-const _renamed_arguments: Dictionary = {}
+const _renamed_arguments: Dictionary = {
+	&"simplespawner_set_spawn_period":
+	{
+		"new_frequency": "new_period",
+	},
+}
 
 
 func _set_arguments(value):

addons/block_code/serialization/value_block_serialization.gd

@@ -13,7 +13,9 @@ func _init(p_name: StringName = &"", p_arguments: Dictionary = {}):
 
 
 # Block name and arguments backwards compatibility handling.
-const _renamed_blocks: Dictionary = {}
+const _renamed_blocks: Dictionary = {
+	&"simplespawner_get_spawn_frequency": &"simplespawner_get_spawn_period",
+}
 
 
 func _set_name(value):

addons/block_code/simple_spawner/simple_spawner.gd

@@ -168,3 +168,40 @@ static func setup_custom_blocks():
 	block_list.append(block_definition)
 
 	BlocksCatalog.add_custom_blocks(_class_name, block_list, [], {})
+
+
+# Backwards compatibility handling
+func _get_property_list() -> Array[Dictionary]:
+	return [
+		{
+			# spawn_frequency was renamed to spawn_period
+			"name": "spawn_frequency",
+			"class_name": &"",
+			"type": TYPE_FLOAT,
+			"hint": PROPERTY_HINT_NONE,
+			"hint_string": "",
+			"usage": PROPERTY_USAGE_NONE,
+		},
+	]
+
+
+func _get(property: StringName) -> Variant:
+	match property:
+		"spawn_frequency":
+			return spawn_period
+		_:
+			return null
+
+
+func _set(property: StringName, value: Variant) -> bool:
+	match property:
+		"spawn_frequency":
+			print("Migrating SimpleSpawner spawn_frequency property to new name spawn_period")
+			spawn_period = value
+		_:
+			return false
+
+	# Any migrated properties need to be resaved.
+	if Engine.is_editor_hint():
+		EditorInterface.mark_scene_as_unsaved()
+	return true

tests/data/simple_spawner_compat.tscn

@@ -0,0 +1,66 @@
+[gd_scene load_steps=14 format=3 uid="uid://br3g0jiuidqmb"]
+
+[ext_resource type="Script" path="res://addons/block_code/simple_spawner/simple_spawner.gd" id="1_liuhg"]
+[ext_resource type="Script" path="res://addons/block_code/block_code_node/block_code.gd" id="2_xxmhe"]
+[ext_resource type="Script" path="res://addons/block_code/serialization/block_serialization_tree.gd" id="3_4xc0e"]
+[ext_resource type="Script" path="res://addons/block_code/serialization/block_serialization.gd" id="4_ao2cd"]
+[ext_resource type="Script" path="res://addons/block_code/serialization/value_block_serialization.gd" id="5_387nk"]
+[ext_resource type="Script" path="res://addons/block_code/serialization/block_script_serialization.gd" id="5_a3p2k"]
+[ext_resource type="Script" path="res://addons/block_code/code_generation/variable_definition.gd" id="6_1vau2"]
+
+[sub_resource type="Resource" id="Resource_kt8ln"]
+script = ExtResource("4_ao2cd")
+name = &"simplespawner_set_spawn_frequency"
+children = Array[ExtResource("4_ao2cd")]([])
+arguments = {
+"new_frequency": 10.0
+}
+
+[sub_resource type="Resource" id="Resource_4x50y"]
+script = ExtResource("5_387nk")
+name = &"simplespawner_get_spawn_frequency"
+arguments = {}
+
+[sub_resource type="Resource" id="Resource_ar5am"]
+script = ExtResource("4_ao2cd")
+name = &"print"
+children = Array[ExtResource("4_ao2cd")]([])
+arguments = {
+"text": SubResource("Resource_4x50y")
+}
+
+[sub_resource type="Resource" id="Resource_yeqkk"]
+script = ExtResource("4_ao2cd")
+name = &"ready"
+children = Array[ExtResource("4_ao2cd")]([SubResource("Resource_kt8ln"), SubResource("Resource_ar5am")])
+arguments = {}
+
+[sub_resource type="Resource" id="Resource_o6xkk"]
+script = ExtResource("3_4xc0e")
+root = SubResource("Resource_yeqkk")
+canvas_position = Vector2(54, 47)
+
+[sub_resource type="Resource" id="Resource_u6cb0"]
+script = ExtResource("5_a3p2k")
+script_inherits = "SimpleSpawner"
+block_serialization_trees = Array[ExtResource("3_4xc0e")]([SubResource("Resource_o6xkk")])
+variables = Array[ExtResource("6_1vau2")]([])
+generated_script = "extends SimpleSpawner
+
+
+func _ready():
+	do_set_spawn_frequency(10)
+	print((spawn_frequency))
+
+"
+version = 0
+
+[node name="Root" type="Node2D"]
+
+[node name="SimpleSpawner" type="Node2D" parent="."]
+script = ExtResource("1_liuhg")
+spawn_frequency = 5.0
+
+[node name="BlockCode" type="Node" parent="SimpleSpawner"]
+script = ExtResource("2_xxmhe")
+block_script = SubResource("Resource_u6cb0")

tests/test_compatibility.gd

@@ -0,0 +1,150 @@
+extends GutTest
+## Tests for scene file backwards compatibility
+
+
+func get_scene_state_node_index(state: SceneState, name: String) -> int:
+	for idx in state.get_node_count():
+		if state.get_node_name(idx) == name:
+			return idx
+	return -1
+
+
+func get_scene_state_node_prop_index(state: SceneState, node_idx: int, name: String) -> int:
+	for prop_idx in state.get_node_property_count(node_idx):
+		if state.get_node_property_name(node_idx, prop_idx) == name:
+			return prop_idx
+	return -1
+
+
+func _get_block_names_recursive(block: Resource, names: Array[String]):
+	names.append(block.name)
+	if "children" in block:
+		for child in block.children:
+			_get_block_names_recursive(child, names)
+	for value in block.arguments.values():
+		if not value is Resource:
+			continue
+		if not value.script:
+			continue
+		if value.script.resource_path != "res://addons/block_code/serialization/value_block_serialization.gd":
+			continue
+		_get_block_names_recursive(value, names)
+
+
+func get_block_script_block_names(block_script: Resource) -> Array[String]:
+	var names: Array[String]
+	for tree in block_script.block_serialization_trees:
+		_get_block_names_recursive(tree.root, names)
+	return names
+
+
+func _get_block_argument_names_recursive(block: Resource, names: Array[String]):
+	names.append_array(block.arguments.keys())
+	if "children" in block:
+		for child in block.children:
+			_get_block_argument_names_recursive(child, names)
+	for value in block.arguments.values():
+		if not value is Resource:
+			continue
+		if not value.script:
+			continue
+		if value.script.resource_path != "res://addons/block_code/serialization/value_block_serialization.gd":
+			continue
+		_get_block_argument_names_recursive(value, names)
+
+
+func get_block_script_argument_names(block_script: Resource) -> Array[String]:
+	var names: Array[String]
+	for tree in block_script.block_serialization_trees:
+		_get_block_argument_names_recursive(tree.root, names)
+	return names
+
+
+func test_simple_spawner():
+	const old_block_names: Array[String] = [
+		"simplespawner_get_spawn_frequency",
+		"simplespawner_set_spawn_frequency",
+	]
+
+	const new_block_names: Array[String] = [
+		"simplespawner_get_spawn_period",
+		"simplespawner_set_spawn_period",
+	]
+
+	const old_argument_names: Array[String] = [
+		"new_frequency",
+	]
+
+	const new_argument_names: Array[String] = [
+		"new_period",
+	]
+
+	var scene: PackedScene = load("res://tests/data/simple_spawner_compat.tscn")
+	assert_not_null(scene)
+	assert_true(scene.can_instantiate(), "Scene should be instantiable")
+
+	var scene_state := scene.get_state()
+	var spawner_idx := get_scene_state_node_index(scene_state, "SimpleSpawner")
+	assert_gt(spawner_idx, -1, "SimpleSpawner node could not be found")
+
+	# The packed SimpleSpawner node should have a simple_frequency
+	# property but no simple_period property.
+	var frequency_idx := get_scene_state_node_prop_index(scene_state, spawner_idx, "spawn_frequency")
+	var period_idx := get_scene_state_node_prop_index(scene_state, spawner_idx, "spawn_period")
+	assert_gt(frequency_idx, -1, "Old SimpleSpawner node should have spawn_frequency property")
+	assert_lt(period_idx, 0, "Old SimpleSpawner node should not have spawn_period property")
+
+	var packed_frequency = scene_state.get_node_property_value(spawner_idx, frequency_idx)
+	assert_typeof(packed_frequency, TYPE_FLOAT)
+	assert_eq(packed_frequency, 5.0)
+
+	var block_code_idx := get_scene_state_node_index(scene_state, "BlockCode")
+	assert_gt(block_code_idx, -1, "BlockCode node could not be found")
+	var block_script_idx := get_scene_state_node_prop_index(scene_state, block_code_idx, "block_script")
+	assert_gt(block_script_idx, -1, "BlockCode node block_script could not be found")
+	var packed_block_script = scene_state.get_node_property_value(block_code_idx, block_script_idx)
+	assert_typeof(packed_block_script, TYPE_OBJECT)
+	assert_eq("Resource", packed_block_script.get_class())
+	assert_eq(packed_block_script.script.resource_path, "res://addons/block_code/serialization/block_script_serialization.gd")
+
+	# Unlike Nodes, Resources are created immediately when loading the
+	# scene, so the block names and arguments should already be migrated.
+	var packed_block_names := get_block_script_block_names(packed_block_script)
+	for name in old_block_names:
+		assert_does_not_have(packed_block_names, name, "Block script should not have old name %s" % name)
+	for name in new_block_names:
+		assert_has(packed_block_names, name, "Block script should have new name %s" % name)
+
+	var packed_argument_names := get_block_script_argument_names(packed_block_script)
+	for name in old_argument_names:
+		assert_does_not_have(packed_argument_names, name, "Block script should not have old argument %s" % name)
+	for name in new_argument_names:
+		assert_has(packed_argument_names, name, "Block script should have new argument %s" % name)
+
+	# Instantiate the scene and check the Node properties.
+	var root := scene.instantiate()
+	assert_not_null(root)
+	autoqfree(root)
+
+	var spawner: SimpleSpawner = root.get_node("SimpleSpawner")
+	assert_eq(spawner.spawn_frequency, 5.0)
+	assert_eq(spawner.spawn_period, 5.0)
+
+	# Pack the scene and check that the old properties won't be saved.
+	var err: Error = scene.pack(root)
+	assert_eq(err, OK, "Packing scene should not cause an error")
+
+	scene_state = scene.get_state()
+	spawner_idx = get_scene_state_node_index(scene_state, "SimpleSpawner")
+	assert_gt(spawner_idx, -1, "SimpleSpawner node could not be found")
+
+	# The newly packed SimpleSpawner node should have a simple_period
+	# property but no simple_frequency property.
+	period_idx = get_scene_state_node_prop_index(scene_state, spawner_idx, "spawn_period")
+	frequency_idx = get_scene_state_node_prop_index(scene_state, spawner_idx, "spawn_frequency")
+	assert_gt(period_idx, -1, "New SimpleSpawner node should have spawn_period property")
+	assert_lt(frequency_idx, 0, "New SimpleSpawner node should not have spawn_frequency property")
+
+	var packed_period = scene_state.get_node_property_value(spawner_idx, period_idx)
+	assert_typeof(packed_period, TYPE_FLOAT)
+	assert_eq(packed_period, 5.0)