Skip to content

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

Merged
merged 2 commits into from
Jun 18, 2025

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Oct 26, 2024

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:

  • Use the UID instead of the resource path as the key.
  • 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 Geometror added this to the 4.4 milestone Oct 26, 2024
@Geometror Geometror requested review from a team as code owners October 26, 2024 21:43
@Geometror Geometror changed the title Fix and improve editor state persistance for the VisualShader editor Fix and improve editor state persistence for the VisualShader editor Oct 27, 2024
@Geometror Geometror requested a review from a team as a code owner October 27, 2024 16:08
@Geometror Geometror requested review from a team as code owners October 27, 2024 17:02
@Geometror Geometror force-pushed the vs-refactor-p1 branch 2 times, most recently from 4ece071 to aa0bec8 Compare October 27, 2024 17:36
@Geometror Geometror marked this pull request as draft November 1, 2024 11:12
@Geometror Geometror marked this pull request as ready for review November 16, 2024 15:10
@Geometror Geometror requested review from KoBeWi and Chaosus November 20, 2024 18:40
@KoBeWi
Copy link
Member

KoBeWi commented Dec 15, 2024

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.

@Repiteo Repiteo modified the milestones: 4.4, 4.x Feb 24, 2025
@Geometror Geometror force-pushed the vs-refactor-p1 branch 3 times, most recently from 8ad0c77 to 656157b Compare April 18, 2025 13:39
@Geometror
Copy link
Member Author

Geometror commented Apr 18, 2025

@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 Geometror force-pushed the vs-refactor-p1 branch 2 times, most recently from 245fa3b to c528c95 Compare April 29, 2025 18:12
@Geometror Geometror requested a review from KoBeWi May 11, 2025 11:55
@Geometror
Copy link
Member Author

Rebased.

@KoBeWi
Copy link
Member

KoBeWi commented May 14, 2025

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.

godot.windows.editor.dev.x86_64_2VTNgv1rsP.mp4

Copy link
Member

@KoBeWi KoBeWi left a 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.

@KoBeWi KoBeWi modified the milestones: 4.x, 4.5 May 14, 2025
@Geometror
Copy link
Member Author

Geometror commented Jun 8, 2025

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.

Copy link
Member

@akien-mga akien-mga left a 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.

@Geometror
Copy link
Member Author

Rebased.

@akien-mga akien-mga added the bug label Jun 16, 2025
@Repiteo Repiteo merged commit 2fc899e into godotengine:master Jun 18, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jun 18, 2025

Thanks!

@Bromeon
Copy link
Contributor

Bromeon commented Jun 20, 2025

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.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 20, 2025

GraphEdit itself is marked as experimental.

@Geometror
Copy link
Member Author

Geometror commented Jun 20, 2025

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.

@AThousandShips
Copy link
Member

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

@dsnopek
Copy link
Contributor

dsnopek commented Jun 24, 2025

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.

@AThousandShips
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants