Adding undo/redo support for replace node. #22921
Conversation
editor/scene_tree_dock.cpp
Outdated
| @@ -1705,7 +1708,10 @@ void SceneTreeDock::_create() { | |||
| ERR_FAIL_COND(!newnode); | |||
|
|
|||
| replace_node(n, newnode); | |||
|
|
|||
There was a problem hiding this comment.
Clang-format says this empty line before the } should be removed. https://travis-ci.org/godotengine/godot/jobs/439926660#L534
There was a problem hiding this comment.
I saw that yesterday before going to bed, fixed it, amended the commit and pushed, saw tarvis start up again so I turned my computer off and went to bed. So when I read this comment this morning I thought "yeah, I already fixed it", but I saw tarvis was still broken, and the line was still there... Apparently I forgot to add the changes to the file before amending the commit, kids, don't git while tired.
|
Your conclusions about the change reflects mine as I tried to fix the issue! Also, related to #21363. |
|
Moving to 3.2 milestone as we're about to enter the beta stage and release freeze for Godot 3.1. Only bug fixes against issues of the 3.1 milestone (or enhancements to core features of the 3.1 roadmap) will be considered until 3.1-stable is released. Note: If it's merged after 3.1-stable is released, this change might be considered for cherry-picking into a later 3.1.x release. |
|
In #28349 I made some changes to SceneTreeDock's replace_node, which resulted in merge conflicts with this PR. The change was to actually add support for replacing node without freeing it, which should make the original purpose of this PR more straightforward to achieve. |
|
Superseded by #31237. |
This is a proposed solution for #19326, I'm not sure about some of these changes:
memdelete(n);otherwise undo would fail because the node was already freed, can I trust thatadd_undo_referencewill free the pointer when the history gets deleted?editor_data->get_undo_redo().clear_history();was being called there.to_eraseshould be stored as an undo reference (since they weren't really children of n for what I understood), on the other hand I think that freeing those nodes via hardcode as it was here might cause problems when undoing multiple times (this was not a problem until now because of the clear_history, which might explain why that was there in the first place)editor->push_item(newnode);because it was spiting an error about the newnode being inside the tree, which I have no idea why it started to happen with these changes, but afaik it shouldn't.