Handle SimpleSpawner compatibility #290

dbnicholson · 2024-10-24T18:51:56Z

This adds handling for the SimpleSpawner compatibility breakage in #272. While I don't expect it to significantly affect our users, I took it as a chance to investigate how these issues could be handled. It shows how to handle a renamed property and a renamed block. I added everyone as reviewers to see what I came up with.

The added test validates the migration, but you can also see it for yourself by loading the res://tests/data/simple_spawner_compat.tscn scene in the editor. It should show that the scene is unsaved. After saving you'll see the names in the scene file have been updated appropriately.

https://phabricator.endlessm.com/T35547

dylanmccall · 2024-10-24T22:31:59Z

addons/block_code/serialization/block_serialization.gd

+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


I'm happy with just this as a place to start. Two thoughts for the distant future:

There's a danger that the old name is now unusable in this case, so we might want to couple it with a version number or something.

This requires that we maintain a list of renamed blocks in the core plugin code. That's an issue for blocks added outside of the core plugin. Those aren't a thing at the moment, but a different (albeit fussier) approach could be a renamed_from field in block_definition.gd.

Indeed, this only handles pure renames. If you changed the arguments or something like that, this wouldn't work. We do have a version number in the higher level BlockScriptSerialization, but it's not available here right now.

@wjt poked at adding aliases for blocks in #284. I didn't look closely at his approach there, but it was my initial suggestion 😆

Unfortunately I actually did change the argument name in the "set ~~frequency~~ period" block...

dylanmccall · 2024-10-24T22:34:33Z

addons/block_code/simple_spawner/simple_spawner.gd

+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,
+		},
+	]


Ooh, interesting!

dylanmccall · 2024-10-24T22:36:05Z

addons/block_code/simple_spawner/simple_spawner.gd

+		"spawn_frequency":
+			return spawn_period
+		_:
+			return null


I'm somewhat disturbed that returning null means "handle the property normally" (ergo a property can't just be null), but, indeed. This is fun!

Indeed, null as sentinel and value is always fun.

dylanmccall · 2024-10-24T22:37:11Z

tests/test_compatibility.gd

+		"simplespawner_set_spawn_period",
+	]
+
+	var scene: PackedScene = load("res://tests/data/simple_spawner_compat.tscn")


Ooh, it's marvellous to have an example of a test with a scene file! Thank you for writing tests :)

wjt · 2024-10-25T08:00:51Z

addons/block_code/serialization/block_serialization.gd

@@ -16,7 +16,9 @@ func _init(p_name: StringName = &"", p_children: Array[BlockSerialization] = [],


 # Block name backwards compatibility handling.
-const _renamed_blocks: Dictionary = {}
+const _renamed_blocks: Dictionary = {
+	&"simplespawner_set_spawn_frequency": &"simplespawner_set_spawn_period",


I guess one could extend this so that the values are not just a string but a tuple of:

new block name

map from old parameter name to new parameter name

Yes, something like that. An issue here is that we don't control the order that Godot sets the property in. So, name might not be migrated in this case. Here's an untested thing I came up with:

diff --git a/addons/block_code/serialization/block_serialization.gd b/addons/block_code/serialization/block_serialization.gd index 4332b73..46ffd4b 100644 --- a/addons/block_code/serialization/block_serialization.gd +++ b/addons/block_code/serialization/block_serialization.gd @@ -6,7 +6,8 @@ const BlockSerialization = preload("res://addons/block_code/serialization/block_ @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 = {}): @@ -30,3 +31,35 @@ func _set_name(value): EditorInterface.mark_scene_as_unsaved() else: name = value + + +const _renamed_arguments: Dictionary = { + &"simplespawner_set_spawn_period": { + "new_frequency": "new_period", + }, +} + + +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: + 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) + + arguments = value

wjt

Very nice!

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

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

dbnicholson · 2024-10-25T16:53:50Z

I added block argument rename handling as described above. The required new_frequency to new_period migration wasn't happening before, but now it is. This also highlights that I hate that BlockSerialization and ValueBlockSerialization are entirely duplicated classes.

manuq

It is fantastic to see this migration happening, with unit test even!

starnight

I think the comment is the major concern. However, it is good to land this PR as try run.

dbnicholson requested review from manuq, wjt, dylanmccall and starnight October 24, 2024 18:51

dylanmccall reviewed Oct 24, 2024

View reviewed changes

wjt reviewed Oct 25, 2024

View reviewed changes

wjt approved these changes Oct 25, 2024

View reviewed changes

dbnicholson added 2 commits October 25, 2024 10:47

dbnicholson force-pushed the T35547-spawner-compat branch from 4a57cf8 to 6191c88 Compare October 25, 2024 16:51

manuq approved these changes Oct 25, 2024

View reviewed changes

starnight approved these changes Oct 28, 2024

View reviewed changes

starnight merged commit 229f63f into main Oct 28, 2024
3 checks passed

starnight deleted the T35547-spawner-compat branch October 28, 2024 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle SimpleSpawner compatibility #290

Handle SimpleSpawner compatibility #290

dbnicholson commented Oct 24, 2024

dylanmccall Oct 24, 2024

dbnicholson Oct 24, 2024

wjt Oct 25, 2024

dylanmccall Oct 24, 2024

dylanmccall Oct 24, 2024

dbnicholson Oct 25, 2024

dylanmccall Oct 24, 2024

wjt Oct 25, 2024

dbnicholson Oct 25, 2024

wjt left a comment

dbnicholson commented Oct 25, 2024

manuq left a comment

starnight left a comment

Handle SimpleSpawner compatibility #290

Handle SimpleSpawner compatibility #290

Conversation

dbnicholson commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjt left a comment

Choose a reason for hiding this comment

dbnicholson commented Oct 25, 2024

manuq left a comment

Choose a reason for hiding this comment

starnight left a comment

Choose a reason for hiding this comment