From 4793b69114083bf2ef2f2245364c9c0084c1ae90 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 24 Oct 2024 12:24:38 -0600 Subject: [PATCH 1/2] serialization: Handle renamed blocks and arguments If a block or its arguments has been renamed, loading a scene with the old block name will fail and the user will be required to replace the block. Handle renamed blocks and arguments by migrating the names to the new values in the serialization resources. In order for the setters to run in the editor, the classes need to be tool scripts. https://phabricator.endlessm.com/T35547 --- .../serialization/block_serialization.gd | 55 ++++++++++++++++++- .../value_block_serialization.gd | 55 ++++++++++++++++++- 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/addons/block_code/serialization/block_serialization.gd b/addons/block_code/serialization/block_serialization.gd index 5a62b008..0fd12c44 100644 --- a/addons/block_code/serialization/block_serialization.gd +++ b/addons/block_code/serialization/block_serialization.gd @@ -1,13 +1,64 @@ +@tool extends Resource const BlockSerialization = preload("res://addons/block_code/serialization/block_serialization.gd") -@export var name: StringName +@export var name: StringName: + set = _set_name @export var children: Array[BlockSerialization] -@export var arguments: Dictionary # String, ValueBlockSerialization +@export var arguments: Dictionary: # String, ValueBlockSerialization + set = _set_arguments func _init(p_name: StringName = &"", p_children: Array[BlockSerialization] = [], p_arguments: Dictionary = {}): name = p_name children = p_children arguments = p_arguments + + +# Block name and arguments backwards compatibility handling. +const _renamed_blocks: Dictionary = {} + + +func _set_name(value): + var new_name = _renamed_blocks.get(value) + if new_name: + print("Migrating block %s to new name %s" % [value, new_name]) + name = new_name + if Engine.is_editor_hint(): + EditorInterface.mark_scene_as_unsaved() + else: + name = value + + +const _renamed_arguments: Dictionary = {} + + +func _set_arguments(value): + if not value is Dictionary: + return + + var renamed_args = _renamed_arguments.get(name) + if not renamed_args: + # Try with the new block name if it hasn't been migrated yet. + var new_block_name = _renamed_blocks.get(name) + if new_block_name: + renamed_args = _renamed_arguments.get(new_block_name) + + if renamed_args: + var changed: bool = false + value = value.duplicate() + for old_arg in renamed_args.keys(): + if not old_arg in value: + continue + + var new_arg = renamed_args[old_arg] + print("Migrating block %s argument %s to new name %s" % [name, old_arg, new_arg]) + value[new_arg] = value[old_arg] + value.erase(old_arg) + changed = true + + if changed and Engine.is_editor_hint(): + EditorInterface.mark_scene_as_unsaved() + + arguments = value diff --git a/addons/block_code/serialization/value_block_serialization.gd b/addons/block_code/serialization/value_block_serialization.gd index 6aea5a77..e200cee0 100644 --- a/addons/block_code/serialization/value_block_serialization.gd +++ b/addons/block_code/serialization/value_block_serialization.gd @@ -1,9 +1,60 @@ +@tool extends Resource -@export var name: StringName -@export var arguments: Dictionary # String, ValueBlockSerialization +@export var name: StringName: + set = _set_name +@export var arguments: Dictionary: # String, ValueBlockSerialization + set = _set_arguments func _init(p_name: StringName = &"", p_arguments: Dictionary = {}): name = p_name arguments = p_arguments + + +# Block name and arguments backwards compatibility handling. +const _renamed_blocks: Dictionary = {} + + +func _set_name(value): + var new_name = _renamed_blocks.get(value) + if new_name: + print("Migrating block %s to new name %s" % [value, new_name]) + name = new_name + if Engine.is_editor_hint(): + EditorInterface.mark_scene_as_unsaved() + else: + name = value + + +const _renamed_arguments: Dictionary = {} + + +func _set_arguments(value): + if not value is Dictionary: + return + + var renamed_args = _renamed_arguments.get(name) + if not renamed_args: + # Try with the new block name if it hasn't been migrated yet. + var new_block_name = _renamed_blocks.get(name) + if new_block_name: + renamed_args = _renamed_arguments.get(new_block_name) + + if renamed_args: + var changed: bool = false + value = value.duplicate() + for old_arg in renamed_args.keys(): + if not old_arg in value: + continue + + var new_arg = renamed_args[old_arg] + print("Migrating block %s argument %s to new name %s" % [name, old_arg, new_arg]) + value[new_arg] = value[old_arg] + value.erase(old_arg) + changed = true + + if changed and Engine.is_editor_hint(): + EditorInterface.mark_scene_as_unsaved() + + arguments = value From 6191c88704436b61456ee634d24440c257ab4b1c Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 24 Oct 2024 09:15:18 -0600 Subject: [PATCH 2/2] 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 --- .../serialization/block_serialization.gd | 11 +- .../value_block_serialization.gd | 4 +- .../simple_spawner/simple_spawner.gd | 37 +++++ tests/data/simple_spawner_compat.tscn | 66 ++++++++ tests/test_compatibility.gd | 150 ++++++++++++++++++ 5 files changed, 265 insertions(+), 3 deletions(-) create mode 100644 tests/data/simple_spawner_compat.tscn create mode 100644 tests/test_compatibility.gd diff --git a/addons/block_code/serialization/block_serialization.gd b/addons/block_code/serialization/block_serialization.gd index 0fd12c44..d25e0dd6 100644 --- a/addons/block_code/serialization/block_serialization.gd +++ b/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): diff --git a/addons/block_code/serialization/value_block_serialization.gd b/addons/block_code/serialization/value_block_serialization.gd index e200cee0..c9762a11 100644 --- a/addons/block_code/serialization/value_block_serialization.gd +++ b/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): diff --git a/addons/block_code/simple_spawner/simple_spawner.gd b/addons/block_code/simple_spawner/simple_spawner.gd index 8e63a512..1bb196e3 100644 --- a/addons/block_code/simple_spawner/simple_spawner.gd +++ b/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 diff --git a/tests/data/simple_spawner_compat.tscn b/tests/data/simple_spawner_compat.tscn new file mode 100644 index 00000000..1ee3552f --- /dev/null +++ b/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") diff --git a/tests/test_compatibility.gd b/tests/test_compatibility.gd new file mode 100644 index 00000000..0c840c17 --- /dev/null +++ b/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)