godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix and improve editor state persistence for the VisualShader editor

Open Geometror opened this issue 1 year ago • 1 comments

This PR implements editor state preservation for the VisualShader editor while addressing related design issues.

Previously, the scroll_offset (essentially the view position of the GraphEdit node used in the VisualShader editor) was saved in the VisualShader 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 the VisualShader 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:

  • [x] Use the UID instead of the resource path as the key.
  • [x] Save the state per type! (this was overlooked by me since the graph_offset was also not saved per type/stage before - but since it has never worked correctly no one ever noticed)

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 within GraphEdit. 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

  • Renamed several methods to improve descriptiveness and clarity, and correct misleading terminology.
  • Removed redundant VisualShaderGraphPlugin::get_shader_type method.
  • Removed redundant updating member.

Geometror avatar Oct 26 '24 21:10 Geometror

I'm not sure if project metadata is supposed to store that much information 🤔 It's mostly used for random dialog options. Check script_editor_cache.cfg, a similar solution is more fit for this task. That said, we don't really have guideline on what to use and when. There is also editor layout file.

The editor does not properly handle built-in shaders, they are all stored under uid://<invalid> key. A simple fix is to fallback to path if resource's UID is invalid.

KoBeWi avatar Dec 15 '24 22:12 KoBeWi

@KoBeWi Rebased and updated to use a separate config file (vs_editor_cache.cfg), also implemented a path fallback in case the UID is invalid.

Geometror avatar Apr 18 '25 13:04 Geometror

Rebased.

Geometror avatar May 13 '25 22:05 Geometror

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: Actually there is one more problem 🙃 When you change shader type (or whatever this is called), the scroll will shift for some reason, instead of restoring previous value.

https://github.com/user-attachments/assets/86ab0de5-1dd2-4ae4-b50f-b5dcc84995a3

KoBeWi avatar May 14 '25 13:05 KoBeWi

Rebased and retested, but I can't reproduce your issue - maybe that was caused by something unrelated to this PR. The editor state is now also saved when closing a shader.

Geometror avatar Jun 08 '25 13:06 Geometror

Rebased.

Geometror avatar Jun 14 '25 11:06 Geometror

Thanks!

Repiteo avatar Jun 18 '25 23:06 Repiteo

This PR introduces a breaking change into GDExtension: VisualShader::set_graph_offset() + VisualShader::get_graph_offset() have been removed. To my knowledge, they're not experimental.

I think a compatibility method should be registered for those 2 methods.

Bromeon avatar Jun 20 '25 06:06 Bromeon

GraphEdit itself is marked as experimental.

KoBeWi avatar Jun 20 '25 09:06 KoBeWi

While GraphEdit is marked as experimental, VisualShader ist not.

The problem is, setting the graph offset via VisualShader was never functional (the Editor basically ignored it), so they shouldn't be used at all. That said, theoretically someone could have used this property for their own stuff (e.g. for a custom VisualShader editor), but I highly doubt that. Adding compatibility mehods would be possible, but the only reasonable implementation would be to make them empty stubs.

Geometror avatar Jun 20 '25 09:06 Geometror

These do need compatibility methods to prevent crashing, even if they're stubs

AThousandShips avatar Jun 20 '25 10:06 AThousandShips

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.

dsnopek avatar Jun 24 '25 22:06 dsnopek

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

AThousandShips avatar Jun 25 '25 07:06 AThousandShips