Serialization: Reuse SerializedBlock resources #137
Conversation
|
We can do something similar to SerializedBlockTreeNode and SerializedBlockTreeNodeArray. But this totally breaks the undo/redo. The benefit of having the tree of resources being rebuilt each time is that it makes undo/redo so trivial. So marking this as draft for now. |
| if resource == null: | ||
| resource = SerializedBlock.new() | ||
| resource.block_class = get_block_class() | ||
| resource.serialized_props = get_serialized_props() |
There was a problem hiding this comment.
It might be nice to have this check if there are changes and return a boolean indicating that. That might help with the undo/redo functionality as there's no reason to add a change point if nothing has changed. Otherwise I think you'd have to pass the EditorUndoRedoManager into here.
|
I certainly like that. Can you pass the |
manuq
force-pushed
the
improve-serialization
branch
2 times, most recently
from
496572d to
5b8c349
Compare
Good point, yes I did so. Now the undo/redo works with block properties, like changing a parameter or moving the block to another position. Then I tried to do the same for the children blocks (path_child_pairs), but is not behaving correctly. I'm running out of ideas, it seems to be the tree being built recursively. Any help welcome! |
addons/block_code/ui/main_panel.gd
Outdated
| if generated_script != block_script.generated_script: | ||
| undo_redo.add_undo_property(_current_block_code_node.block_script, "generated_script", _current_block_code_node.block_script.generated_script) | ||
| block_script.generated_script = generated_script | ||
| undo_redo.add_do_property(_current_block_code_node.block_script, "generated_script", _current_block_code_node.block_script.generated_script) |
There was a problem hiding this comment.
We shouldn't be changing the generated_script property on our own here, and we should consistently use the var block_script: BlockScriptData we defined earlier:
| if generated_script != block_script.generated_script: | |
| undo_redo.add_undo_property(_current_block_code_node.block_script, "generated_script", _current_block_code_node.block_script.generated_script) | |
| block_script.generated_script = generated_script | |
| undo_redo.add_do_property(_current_block_code_node.block_script, "generated_script", _current_block_code_node.block_script.generated_script) | |
| if generated_script != block_script.generated_script: | |
| undo_redo.add_undo_property(block_script, "generated_script", block_script.generated_script) | |
| undo_redo.add_do_property(block_script, "generated_script", generated_script) |
There was a problem hiding this comment.
Turns out we're doing this in a lot of places. I included this and a bunch of similar fixes in #140.
| @@ -38,6 +38,8 @@ var zoom: float: | |||
| get: | |||
| return _window.scale.x | |||
|
|
|||
| var _undo_redo: EditorUndoRedoManager | |||
There was a problem hiding this comment.
Personally I would just pass this down in to build_tree rather than stashing it in the object. It's only called internally to this object, so there's no danger of it breaking another user.
There was a problem hiding this comment.
Yes I though about that, I did it like this because is also used in the build_tree() method that's called recursively. But yes it can be passed around to build_tree() as parameter.
manuq
force-pushed
the
improve-serialization
branch
from
48bf072 to
f079f18
Compare
|
This is now ready for review thanks to @dylanmccall fixes. The undo-redo is now piling actions to the history instead of going back as before. I couldn't find how to fix it. But is usable (can undo, can redo). |
manuq
force-pushed
the
improve-serialization
branch
from
f079f18 to
eb1f780
Compare
The undo-redo piling actions was an issue in the input parameter, fixed in: #142 |
| func update_resources(undo_redo: EditorUndoRedoManager): | ||
| if resource == null: | ||
| resource = SerializedBlockTreeNode.new() | ||
| resource.serialized_block = SerializedBlock.new(get_block_class(), get_serialized_props()) |
There was a problem hiding this comment.
I think you can return early here. There's no need to compare the serialized properties again if the serialized block was just constructed with them.
There was a problem hiding this comment.
I added a fixup commit for this. Hopefully it won't conflict with Dylan's changes in the same function when automerging.
Add a resource property to blocks. The resource will be updated before building the tree. This mitigates having a huge diff in scenes with blocks for minimum changes like moving a block in the canvas.
In block_canvas.gd, instead of modifying _current_bsd.block_trees.array outside of the UndoRedo action, use add_undo_property and add_do_property.
Pass it around.
manuq
force-pushed
the
improve-serialization
branch
from
261f08d to
22ef121
Compare
Add a resource property to blocks of type SerializedBlock. The resource will be updated before building the tree.
This mitigates having a huge diff in scenes with blocks for minimum changes like moving a block in the canvas.
https://phabricator.endlessm.com/T35565