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

Don't save unnecessarily with save_before_running #90034

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 30, 2024

When save_before_running is enabled, it will save all scenes, which can lead to longer running times if you have many opened scenes. That's rather absurd.

This PR changes that mechanism to only save when there are unsaved changes in the scene (which we can detect pretty reliably).

Fixes #78753
Fixes #94563

@Calinou
Copy link
Member

Calinou commented Apr 1, 2024

This PR changes that mechanism to only save when there are unsaved changes in the scene (which we can detect pretty reliably).

With editor plugins, this may not be the case: godotengine/godot-proposals#2153

I fully support this PR, but we need to have zero false negatives before it can be merged without risk. People are used to the current behavior, so not saving some scenes that should be saved due to a bug could end up wasting hours of people's time.

@ogapo
Copy link
Contributor

ogapo commented Apr 23, 2024

Excited to see this change come in! The current behavior is triggering a lot of unnecessary file watch behavior since unchanged scenes get new modify times.

I am wondering if rather than changing the behavior only for save_before_running it might make more sense to change the implementation of "Save All Scenes" instead (with the same caveats @Calinou pointed out)?

The current behavior of "Save All Scenes" is really "Re-Save All Open Scenes" and that's not super useful. I'd expect it to either 1) save all dirty scenes (strict subset of open ones so I think that's why the current implementation is what it is) or 2) Re-Save All Scenes regardless of open status. Doing a full re-save but only of open scenes feels like a weird in-between that isn't correct for either reasonable interpretation.

Personally I'd prefer to have it just do option #1 (hence this suggestion) but I could see a use case for full re-save too. Most applications that support multiple documents simply have a "Save All" option (commonly bound to Ctrl-Shift-S) which essentially just takes care of dirty docs so I think it's reasonable to expect users would interpret it as this.

Also I noticed it technically resaves everything that's open (including shaders and scripts) so "Save All" might be a more correct name as well.

@KoBeWi KoBeWi changed the title Don't save unnecessarily with save_before_running Don't save unnecessarily with save_before_running Apr 24, 2024
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 24, 2024

Ok I reworked Save all Scenes. It can be used only if any scene is unsaved.

godot.windows.editor.dev.x86_64_LaqdPMnqcw.mp4

editor/editor_node.cpp Outdated Show resolved Hide resolved
ogapo added a commit to dykwia-labs/godot that referenced this pull request Apr 24, 2024
godotengine#90034
Prevents saving scenes/files before debugging unless the scene/file has changed.
@ogapo
Copy link
Contributor

ogapo commented Apr 24, 2024

This change works like a charm for scenes and scripts. In testing, however, I noticed that any open shaders always re-save any time any scene is saved (whether or not they are modified). I tracked it down to needing an is_unsaved check in ShaderEditorPlugin::save_external_data() i.e.

void ShaderEditorPlugin::save_external_data() {
	for (EditedShader &edited_shader : edited_shaders) {
		if (edited_shader.shader_editor) {
			if (edited_shader.shader_editor->is_unsaved()) {
				edited_shader.shader_editor->save_external_data();
			}
		}
	}
	_update_shader_list();
}

It might be worth including that in this PR also for completeness sake.

@KoBeWi KoBeWi requested a review from a team as a code owner April 25, 2024 11:52
@akien-mga
Copy link
Member

How does this work with changes to resources, instead of scenes? Currently I think a lot of the Godot workflow hinges on scene saving actually saving all dependent resources, even if there's no change to the scene itself. Saving changes to resources specifically is pretty cumbersome currently with just the save button in the inspector IIRC.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 25, 2024

Apparently if you make a change in external resource, the editor will save the current scene which will also save that resource 🤔
And if you have no open scenes, the external changes are not saved when running. But this is the same behavior as before this PR, so it can be fixed separately.

EDIT:
Guess what, a fix already exists and waits: #85513

ogapo added a commit to dykwia-labs/godot that referenced this pull request Jun 23, 2024
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants