Allow change import type without restarting editor

[KoBeWi]

When trying to change import type in the Import dock, there is this dreadful popup that you need to restart the editor. The popup exists, because the resource might be used somewhere and changing the type of an existing object is just not possible.
But it doesn't make any sense if the Resource is not used anywhere. So this PR fixes that. When a resource is not loaded anywhere, you can freely change its type. If something uses this resource, you will get a warning that you can ignore. If a resource is currently loaded, the dialog will tell you to close all scenes or restart the editor.

godot.windows.editor.dev.x86_64_7bEfZVxzQx.mp4

EDIT:
Restart is not needed anymore ever. See comments below.

[fire]

Horray! Can't wait to review this.

[reduz]

I would go a bit further with this and try to do something with the resource if its in use in the scene.

Probably do a logic like this if the resource is actually loaded (ResourceCache can be checked):

1. Clear the path in the old resource and reimport the new one.
2. In the current scene, go through all nodes (and built-in resources) and replace the resource if found in a property.
3. Go through all external resources and replace the resource if found in a property
4. Go through the packed scene states of the other tabs and replace the resource. In most cases (depending what you are changing) replacing will probably not work as it will cast to null and clear it, but this is good enough and there should be no error reported.
5. Clear the undo/redo history and the resource clipboard.
6. Now, check again if the resource is actually in use (refcount should be 1 in the reference you have). If it is (far most likely it should be), all good stop here. Otherwise go on:
7. Add some bool metadata to the replaced resource (old one) to indicate that this is no longer used. Kind of like "skip_save = true"
8. In the saver code (Check the savers, text,binary) and if it has a subresource (only need to check subresource, not external resource) that has this metadata, do not save it. Otherwise you risk that this can bundle a binary resource like an image inside of a text resource, making it very inefficient for users who may have no idea what's going on, nor realize this is the old one. It should be really hard to reach this stage anyway, but better safe than sorry.

IMO this should be good enough for far most cases.

[KoBeWi]

I think what you described can be done independently, in another PR. It sounds like a lot of work.

Go through all external resources and replace the resource if found in a property

All resources in filesystem or all currently loaded? The latter is difficult to determine, because a resource an be held by any random editor plugin. Same with the resource we are replacing.

[reduz]

@KoBeWi I disagree that it's a lot more work, and I think if this is implemented it should be done properly. Here is some code that walks the whole thing for the edited scene:

https://github.com/godotengine/godot/blob/master/editor/editor_node.cpp#L1482

Here is code that walks the cached resources:

https://github.com/godotengine/godot/blob/master/editor/editor_node.cpp#L1697

All you have to do here is check if this is an external resource with res->get_path().is_resource_file() and then call the same function you use to walk the nodes to check the properties.

For PackedScene or SceneState (not active tabs) you may need to implement a function in that class that iterates all the internal properties and replace one if needed, but it should be simple to implement since they are in an array.

For editor plugins you can check if the resource is being edited and just clear the editors, or simply for now clear all editors and inspector if that is too much work.

Then just clear the undo/redo history and resource clipboard.

It is true that it may happen that this resource still remains in use somewhere, but the chance of this happening should be rather low at this point and setting the skip_save metadata should solve any significant consequences.

[fire]

I agree with reduz's reasoning that we don't allow changing import type without doing the proper change as designed earlier. The feature can be broken into several pull requests, but only the final pull request should enable changing types.

[reduz]

Also on some extra notes:

• Replacing references in other files using the resource I think is unnecessary. If the resource is compatible, it will work. If its not, it will not but the scenes should still load fine.
• SceneState (which stores the closed scene tabs) has a Vector<Variant> variants; inside, you will need to add some API to request them by index, get it and set it to check them and replace if this uses that resource.
• I forgot this, but EditorFileSystem also needs to be updated, there is an update function you can call after changing the type.

[KoBeWi]

godot.windows.editor.dev.x86_64_0fLg54ySAA.mp4

[KoBeWi]

Ok I completely removed the need to restart. If any used resource changes type, the editor will replace its instances in every opened scene and every cached external resource.

I did not implement skip_save though, as I didn't really understood what do you mean. Like

if it has a subresource (only need to check subresource, not external resource)

You mean I should check skip_save in sub-resources of a saved resource? What should happen if there is skip_save? If I just abort saving parent resource, the user will not know about it. Also checking every sub-resource on each save sounds inefficient 🤔

[reduz]

@KoBeWi Wow you are amazing!

For skip_save, my idea is that if for some reason this resource remains somewhere (because of most likely a bug or we did not search exhaustively enough), you set it to the old one (pre replace) with res->set_meta("skip_save",true);. Then in these pieces of the code:

https://github.com/godotengine/godot/blob/master/core/io/resource_format_binary.cpp#L1958

you can check for the metadata and skip it (dont add to resource_set).

Then here:

https://github.com/godotengine/godot/blob/master/core/io/resource_format_binary.cpp#L1782

If it also has the skip_save metadata, also write OBJECT_EMPTY

Likewise same locations for the text saver:

https://github.com/godotengine/godot/blob/master/scene/resources/resource_format_text.cpp#L1871

and

https://github.com/godotengine/godot/blob/master/scene/resources/resource_format_text.cpp#L1834

To skip saving it, something like:

if (res->get_meta("skip_save",false)) {
   return "null";
} else if (internal_resources.has(res)) {
....

[KoBeWi]

Done.

[fire]

Is this something we should cherry pick for 4.0?

[YuriSizov]

Is this something we should cherry pick for 4.0?

Probably not cherry-pickable, definitely not for 4.0. Can discuss for 4.1 when this gets approved and merged.

[fire]

Aside from the fixed build error, I did a standard test of switching a scene import to an animation library and no restart was needed.

[theraot]

About using metadata, be aware be aware that scripting might mess with it.

Also, would you have it documented?

I believe any third party doing some kind of serialization would benefit from learning about it.

[KoBeWi]

Metadata beginning with _ is for editor-only usage and its name and purpose is an implementation detail.

[YuriSizov]

Thanks!