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

Detect external modification of scenes #31747

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 28, 2019

With this change, editor will look for external modifications in opened scenes when focused and asks for reload. This is analogical to scripts in ScriptEditorPlugin.

Closes #11803
Also closes #20250, which is a duplicate
Also closes #22450, which is a duplicate duplicate

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 29, 2019

I finally got around to fixing this and the crash is gone (not sure if related to what I did xd). Also the reloading is more seamless, there's no undo history noise at all.

Probably needs some testing, but otherwise, the PR complete. I'd really like it for 3.2 >_>

@akien-mga
Copy link
Member

We're a bit too close to 3.2-stable for such a change IMO, so I'd prefer to merge after the release. We can cherry-pick for 3.2 though if it proves to work well in the master branch.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Oct 30, 2019
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 30, 2019
@alexzheng
Copy link

Why such a nice feature still not available in the latest release?
Scene can not be reload automatically will overwrite the external modification.

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the scene_stalking branch 2 times, most recently from c56ebd5 to 61a053c Compare June 9, 2020 15:04
@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 9, 2020

Ok, rebased and fixed. However due to #37531, the dialog will pop up infinitely when you try to cancel/close it (because editor gets refocused), so you are either forced to save scene or reload.

@Dragoncraft89
Copy link
Contributor

As far as I can tell from the diff, this only takes into account open scenes, right?

But these are not the only files, that need to be reloaded. The project.godot file faces similar problems, although fixing that could be a lot more difficult

@Zylann
Copy link
Contributor

Zylann commented Oct 3, 2020

Note that the user can have more than just scenes open. What's on disk are resources, so it's actually about resources. Resources can be scripts, scenes, or standalone resources open in the inspector. Is the latter handled as well?

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 3, 2020

Resources in Godot are usually built-in, so this PR handles most of the problems. But I didn't test file Resources, they might indeed be a problem. project.godot is not handled yet either.

@Zylann
Copy link
Contributor

Zylann commented Oct 3, 2020

Resources in Godot are usually built-in

No? I dunno what that comes from (we dont have stats?) but there are a bunch of cases where resources authored inside Godot are files (themes, custom resources, some materials, default environment, scripts, audio bus layout...). I also use lots of file resources myself, and actually rarely make them builtin.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 3, 2020

Dunno, 99% of non-imported resources in my project are built-in (they are mostly collision shapes). Script changes are already handled by script editor.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 17, 2021

I rebased the PR. Also I added detecting changes to project.godot, but right now it's useless due to #45243 (I tested with the mentioned lines commented out and it works)

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 9, 2021

Ah, I forgot to write here, but I tested this PR during the GGJ and it works fine.

There are two problems though:

However they are separate issues not really related to this PR. I'd say it's ready to be merged.

editor/editor_data.h Outdated Show resolved Hide resolved
Calinou
Calinou previously requested changes Feb 9, 2021
editor/editor_node.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit b1c60c7 into godotengine:master Feb 10, 2021
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the scene_stalking branch February 10, 2021 13:59
Comment on lines +6695 to +6696
disk_changed->connect("confirmed", callable_mp(this, &EditorNode::_reload_modified_scenes));
disk_changed->connect("confirmed", callable_mp(this, &EditorNode::_reload_project_settings));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're connecting two callables to the same signal, bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is intended. I made reloading scenes and project settings into separate methods.

@akien-mga
Copy link
Member

If you're up for it, I'd suggest making a dedicated backport PR for 3.2, as the cherry-picking needs some care and proper testing to make sure that going back to _bind_methods for callbacks, etc. works as intended.

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