-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix wrong undo-redo action when dropping files containing circular dependencies #89204
Conversation
CC @KoBeWi; as you mentioned the possibility to modify It feels more "natural" to do so in |
4d4b16c
to
8db1480
Compare
Rebased against 9b94c80 and applied the suggestions. @AThousandShips let me know if the way I handled showing the error file is fine #89204 (comment). |
8db1480
to
6476a28
Compare
6476a28
to
71426d0
Compare
Rebased against aef11a1. Applied #89204 (comment), #89204 (comment) and #89204 (comment) |
Thanks! |
Found in #89106 (comment) originally.
When trying to drop files containing a scene that contains cyclic dependencies (such as dropping the scene into itself), a new undo-redo action "Create Node" will be created and a generic error will appear. This happens in both
2D
and3D
as far as I'm concerned.Behavior BEFORE change (tested @ 1b2e0b3):
godot.windows.editor.x86_64_2pBYJv8ecv.mp4
Fixed behavior in this PR:
godot.windows.editor.x86_64_AjeYE6YGVt.mp4
I have my doubts on how to communicate the "cyclical dependency found" (as well as the message itself), currently I'm using the same kind of messaging/notification as that of "Instantiating : ..." in the bottom left of the screen for both
2D
and3D
. I experimented with showing a popup (like the one with the bug) with a custom message to let the user about the cyclical dep but requires a lot more changes and some hacks.Also, not sure if the way I handle checking for cyclical dependencies are optimal, specially for the
3D
version, basically all files should be checked until at least one cyclical dep is found, this causes more iterations in general.Finally I decided to use
_remove_preview
/_node
to remove the "cyclical dependency found" message once the dragging stops as it is being called in the notification, and decided to match the behavior of setting the message to""
in2D
always as how its done in3D
.