Skip to content
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 and improve editor state persistence for the VisualShader editor #98566

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

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.

2 participants