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

Cannot Save Scene while Shader Editor is Open #84601

Closed
WickedInsignia opened this issue Nov 8, 2023 · 10 comments
Closed

Cannot Save Scene while Shader Editor is Open #84601

WickedInsignia opened this issue Nov 8, 2023 · 10 comments

Comments

@WickedInsignia
Copy link

Godot version

4.5 Beta 5

System information

Windows 11, Nvidia RTX4070ti, AMD Ryzen7700x

Issue description

Cannot save any scene after changes while the Shader Editor is open.

Steps to reproduce

  • Open MRP
  • Open Shader Editor by double-clicking on the Visual Shader
  • Move the cube
  • Try to save scene with Ctrl+S

Minimal reproduction project

ShaderEditorSaveBug.zip

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Nov 8, 2023

I guess related to #84064.

Confirmed, also it seems only the scene tab is not saved, but with visual shader editor and script editor open at the same time, whichever get the focus will be saved.

@reduz
Copy link
Member

reduz commented Nov 8, 2023

Folks, we are paper cutting with this all the time. I don´t know where this should be documented so we don´t keep running into this issue again every some years, probably the source code.

To clarify, the intended behavior is:

No matter where you are, if script editor, shader editor, scene editor, etc. Ctrl-S (or the shortcut assigned to save-scene) should always take precedence over everything else. When you save scene, all edited subresources or external resources are also saved, so its always a "save all" shortcut. If the script or shader editors have the same shortcuts as the scene save, then it should check this and trigger a scene save instead (and opened scripts/shaders will be saved anyway).

@WickedInsignia
Copy link
Author

Maybe a dedicated shortcut to save file separate from Ctrl+S is needed, such as Ctrl+Alt+S? There may be some cases someone wants to save a shader without saving their changes to a scene (maybe if they were experimenting with objects in the scene to test the shader and did not want to save those scene changes).
Using File > Save File seems fine to me though and I agree with you. I don't believe a good use-case existed to justify that commit.

@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 8, 2023

Maybe a dedicated shortcut to save file separate from Ctrl+S is needed, such as Ctrl+Alt+S?

That's what we had in the script editor, and it was a usability nightmare for many years, until we resolved it in Godot 4, allowing contexts for shortcuts. Then this was extended to the shader editor as well, because it's no different from the script editor at this point.

You can edit any number of shaders, unrelated to the current scene included. You can detach it and work in a different window, within a separate context from the main editor. You don't even have to have the 2D/3D view open, so saving the scene instead of the currently edited shader would be completely unexpected.

We want to have a WYSIWYG experience, so you save with intent. Alternatively, we need a true save everything shortcut, but that never existed and is not currently possible. (What Juan refers to as "Save all" is "Save scene with related resources", so not the same thing.)

@WickedInsignia
Copy link
Author

Not being able to save scene while the shader editor is open is a usability nightmare all its own unfortunately. Saving the scene with related resources is the expected behavior currently across the editor, and is only broken in a couple corner cases that aren't expected by the user. "Save" isn't usually contextual in apps like other shortcuts and almost always refers to the overall scene or project file. This is consistent with other engines and 3D DCC tools.
The key to "saving with intent" is the intent part. There should be consistency the user can expect. Hitting Ctrl+S while editing a StandardMaterial shouldn't suddenly change behavior entirely when editing a VisualShader.
StandardMaterials have a "Save Resource" button that is easily accessible, and there's ample space to include the same thing in the shader/visualshader editor. Alternatively Blender uses Alt+S to save files external from the project file, such as images, so there'll be some overlap with users already familiar with that.

Not exactly sure what the best solution is, but Ctrl+S being hijacked while the shader editor is open ain't it. That's inconsistent UX design of arguably the single most important operation in the editor.

@YuriSizov
Copy link
Contributor

Not being able to save scene while the shader editor is open is a usability nightmare all its own unfortunately.

That's just a bug, and it's fixable without drastic changes 🙃

@akien-mga
Copy link
Member

To clarify, the fact that it's not possible to save a scene with Ctrl+S while the shader editor is visible is a bug, which #84614 will fix. After that fix, you can save the scene by giving focus to any other editor (so e.g. the steps to reproduce here won't reproduce the bug, since the 3D viewport will have focus at the time of pressing Ctrl+S).

The other part of this discussion is that it's actually intentional that Ctrl+S saves the shader instead of the scene when the shader editor has focus. Giving focus to any other editor that doesn't use Ctrl+S will let the event bubble up to the main File menu where Ctrl+S is "Save Scene". The script editor has the same behavior.

This behavior is a change as it wasn't possible in the past, and might warrant more discussion / options, as not everyone seems to be happy with it. Especially since we don't make it very obvious to the user which context is currently active.

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Nov 8, 2023

Just pop here to mention that before the shortcut commit, when no scene is opened and I'm editing my shader, if I press Ctrl +S to save my shader, godot's will have an Alert window as mentioned in #84039.

If we change ctrl + s 's behaviour back to global save, this alert window might need some consideration.

@WickedInsignia
Copy link
Author

@akien-mga That makes sense but is a pretty serious UX inconsistency, even with adequate contextual highlighting. Although the Shader Editor can be broken out to float, it will still see plenty of use when docked and where Ctrl+S is expected to save the scene + resources. In every other docked window with the 3D editor open that's exactly what it does, including the ShaderMaterial editor.
In most other engines and DDC tools Ctrl+S is expected to save the project and not change contextually, such as how Blender uses Alt+S to save external files and avoid throwing a curveball.
Users will expect Ctrl+S to save the scene + shader file, just as it does with the StandardMaterial editor.
If the user wants to save the shader file but not the scene, they know to look for it since they know Ctrl+S always saves the scene and resources. For Ctrl+S not to do that is not expected behavior.

I may be pushing the issue here but Save is the single operation where users should always know exactly what to expect. If there's one thing to respect the industry standard on, it's this.

@akien-mga
Copy link
Member

I think we can consider this as fixed by #84614, and the discussion on the UX downgrade can be continued in #84623 which aims at addressing it.

reduz added a commit to reduz/godot that referenced this issue Nov 14, 2023
Fixes godotengine#84601.

The scene will also silently be saved (just like before), since many users
are not necessarily aware that the script or shader editor editors are focused
at the time of saving. Now, however, if something causes the save to fail it
will not be notified to the user, in case the user to avoid the annoyance
caused by godotengine#84039.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants