-
-
Notifications
You must be signed in to change notification settings - Fork 22.8k
Fix and improve editor state persistence for the VisualShader editor #98566
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
Conversation
bba8bc3
to
44884d1
Compare
44884d1
to
74f9ff2
Compare
4ece071
to
aa0bec8
Compare
aa0bec8
to
aa6f1fa
Compare
I'm not sure if project metadata is supposed to store that much information 🤔 It's mostly used for random dialog options. The editor does not properly handle built-in shaders, they are all stored under |
8ad0c77
to
656157b
Compare
@KoBeWi Rebased and updated to use a separate config file ( |
245fa3b
to
c528c95
Compare
Rebased. |
One remaining problem is that when you close the shader before debounce Timer finishes, the values will not be saved. You can check whether the Timer is running when the shader is being closed and manually call the callback EDIT: godot.windows.editor.dev.x86_64_2VTNgv1rsP.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the problems mentioned above, it looks good now.
Rebased and retested, but I can't reproduce your issue - maybe that was caused by something unrelated to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Needs rebase.
196e549
to
b55b9fe
Compare
Rebased. |
Thanks! |
This PR introduces a breaking change into GDExtension: I think a compatibility method should be registered for those 2 methods. |
GraphEdit itself is marked as experimental. |
While The problem is, setting the graph offset via |
These do need compatibility methods to prevent crashing, even if they're stubs |
We discussed this at the GDExtension meeting, and we think this is a case where it makes sense to add compatibility methods. Even if the original methods wouldn't have had any effect, if someone did call them, it would lead to a crash after updating Godot. Adding compatibility methods that do nothing or print an error/warning would be acceptable. |
I will add a note to the docs about that we should in general add these even if they do nothing as it would otherwise crash |
This PR implements editor state preservation for the VisualShader editor while addressing related design issues.
Previously, the
scroll_offset
(essentially the view position of theGraphEdit
node used in the VisualShader editor) was saved in theVisualShader
resource, which could create Git noise when multiple people work on the same project. If one person opens the shader and just pans around, the resource would change. Since the scroll offset is more accurately part of the editor state than theVisualShader
resource itself, it is now saved as project metadata. The same goes for the currently edited type/stage. Additionally, the zoom level is now also persistent.For saving the scroll offset and zoom level, a debounce timer was added to reduce disk access.
TODO:
GraphEdit refactoring: Scrolling/Panning
Currently,
GraphEdit
uses the scrollbars to manage and track the scroll/panning offset as well as to control how far the view can pan/scroll. Data like this should not be stored in or managed by child control nodes but rather directly withinGraphEdit
. Scrollbars should function purely as UI components, signaling user interactions and reflecting changes from the parent node. After all, they provide just another way to pan the view and GraphEdit should theoretically be functional without them.This part could be split into another PR if desired (since the changes shouldn't alter the behavior), however I have only tested it in conjunction with the above.
Additional refactoring
VisualShaderGraphPlugin::get_shader_type
method.updating
member.