Improving code around "Change Type" tool to better respect scripts. #37489
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mason-bially
force-pushed
the
mjb/master/script-change-refactor
branch
from
f138108 to
5375244
Compare
|
I'll also note that I suspect that the |
Apparently `SceneTreeDock::replace_node` duplicates a significant portion of `Node::replace_by`'s `p_keep_data` flag, but then set that flag anyway. This is some sort of code duplication. I set this to false because otherwise the other code should probably be deleted. This is probably to allow undo/redo to work correctly (though that could probably be done simply by setting the flag properly). This then allows two things to function correctly: undo/redo with change type and lets change type function correctly with "script types" (GDScript's with `class_name`s). This certainly breaks something(s).
A portion of the code in Node::replace_by is vestigial and does not actually do anything, both it and the other portion is identical to what was in SceneTreeDock::replace_node. I have refactored the working code of replace_node in to replace_by. I suspect this duplication/copy code should actually be moved even further down the tree into Object (because it is responsible for properties, and the skipping scripts and metadata would make sense in the class responsible for those things. I then elevated the concern of what script the resulting node should have to the "Change Type" code of SceneTreeDock, this seems like the more relevant location to make that decision. The decision is straight forward, if the new type has a script it uses that, if the old node has a script it uses that, then it gives up and uses nothing. This seems to give the best behavior for common use cases. An improved fix for godotengine#37479
mason-bially
force-pushed
the
mjb/master/script-change-refactor
branch
from
5375244 to
e90e991
Compare
|
Needs rebase 🙃 |
|
Closing in favor of #91341, which fixes the same issue. |
|
@KoBeWi As an aside, I stopped bothering trying to contribute to godot when no one replied to this for months, I assumed godot was not interested in outside contributions. And I stopped using godot about a year later. By the time I did get a reply for a rebase two and a half years later I was at a point in my life when I did not have the time to respond. So at four and a half years later, glad to know my attempt to contribute was somewhat appreciated. Good luck! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An improved fix for #37479 (Change Type does nothing for script "classes")
A portion of the code in
Node::replace_byis vestigial and does not actually do anything (besides looping over all properties and callingget), both it and the other portion is identical to what was inSceneTreeDock::replace_node. I have refactored the working code ofreplace_nodein toreplace_by. I suspect this duplication/copy code should actually be moved even further down the tree into Object (because it is responsible for properties, and the skipping scripts and metadata would make sense in the class responsible for those things).I then elevated the concern of what script the resulting node should have to the "Change Type" code of SceneTreeDock, this seems like the more relevant location to make that decision. The decision is straight forward, if the new type has a script it uses that, if the old node has a script it uses that, then it gives up and uses nothing. This seems to give the best behavior for common use cases.