Add serialization of foreign external resources. #84168
Conversation
SaracenOne
force-pushed
the
foreign_external_resources
branch
3 times, most recently
from
d3c427e
to
e0e7478
Compare
SaracenOne
force-pushed
the
foreign_external_resources
branch
from
e0e7478
to
2803d08
Compare
|
Rebased to fix conflict. |
|
I can't comment in the depths of this issue, but I'll offer a usecase that perhaps can help with this. I have been developing my own tool for making VFX in Godot, and one of the things i needed to do was to create an animation where I could keyframe the emission of particles and add it to the scene. Because i often had to inherit those scene, I wanted each of the animations to be saved alongside of the scene with a name like I have struggled a bit with figuring out how to understand when the resource was foreign or built-in and make a reliable tool around it. I hope this helps. If it doesn't, carry on and ignore this comment ^^ |
SaracenOne
force-pushed
the
foreign_external_resources
branch
from
2803d08
to
f448550
Compare
|
I'm hoping this might be able to inadvertedly lay the groundwork for a more useful user-facing system for referencing resourced embedded in other resources, but for now, this does not expose anything user-facing. It's mostly just to fix the one specific bug we have animation libraries. @lyuma if you have a moment, since we discussed this a while back, do you think you could put down the concerns you had about the implications of allowing this kind of foreign resource reference system to be supported in Godot. I know we spoke about it before, but I've forgotten what the concerns were again. If those can be addressed, I think it would help either finalize this PR or warrant the concern required to rethink this approach and go a different direction. |
| @@ -1820,6 +1822,7 @@ void EditorNode::_save_scene(String p_file, int idx) { | |||
| flg |= ResourceSaver::FLAG_REPLACE_SUBRESOURCE_PATHS; | |||
|
|
|||
| err = ResourceSaver::save(sdata, p_file, flg); | |||
| sdata->set_path(p_file, true); | |||
There was a problem hiding this comment.
What's the rationale for this one being here?
| } | ||
| } else { | ||
| sdata.instantiate(); | ||
| sdata->get_state()->set_path(scene->get_scene_file_path()); |
There was a problem hiding this comment.
Same thing, what's the rationale for this one?
| @@ -2220,7 +2275,18 @@ Error ResourceFormatSaverBinaryInstance::save(const String &p_path, const Ref<Re | |||
| String res_path = save_order[i]->get_path(); | |||
| res_path = relative_paths ? local_path.path_to_file(res_path) : res_path; | |||
| save_unicode_string(f, res_path); | |||
| ResourceUID::ID ruid = ResourceSaver::get_resource_id_for_path(save_order[i]->get_path(), false); | |||
There was a problem hiding this comment.
I don't understand this logic, if this is a subresource it should not be able to be saved standalone.
| @@ -104,7 +104,7 @@ class Resource : public RefCounted { | |||
| virtual void set_path(const String &p_path, bool p_take_over = false); | |||
| String get_path() const; | |||
| void set_path_cache(const String &p_path); // Set raw path without involving resource cache. | |||
| _FORCE_INLINE_ bool is_built_in() const { return path_cache.is_empty() || path_cache.contains("::") || path_cache.begins_with("local://"); } | |||
| _FORCE_INLINE_ bool is_built_in(String p_local_path = "") const { return path_cache.is_empty() || (path_cache.contains("::") && (path_cache.begins_with(p_local_path) || p_local_path == "")) || path_cache.begins_with("local://"); } | |||
There was a problem hiding this comment.
Feels like it may be better to have a separate function for this, like is_built_in_for_local_path
Supercedes #82750
Closes #74766
So, this is a second attempt at fixing issues with AnimationMixer's libraries parameter after a discussion with @lyuma. The basic problem as I understand this now is that this bug is unfixable without adding the ability for external resources to point to resources inside of other resources.
The problem is as such: if you import a 3D model with animations included, the default settings, the imported scene will have all its animations stored in an AnimationLibrary resource which is a subresource of imported scene. As such, it can't be edited and it is referenced in the animation library as being 'foreign', which is an external resource which is a subresource of another resource, in this case, a PackedScene.
It can't be directly edited, but this is communicated correctly in the interface, but if you make ANY modifications the layout of the libraries property, renaming them or adding new ones, the library in the imported scene will have its reference broken, and instead be copied into the other scene, along with copies of all its embedded animations too, breaking the ability to update it and bloating the file size, and we also don't communicate that this is a limitation (VERY bad UX).
I believe strongly that the only solution to fixing this is to allow foreign resource references to be saved rather than having them automatically converted into local resources. Fortunately, the engine is mostly already capable of doing this, but none of it is exposed. What this PR does is make it foreign resources be saved with the following convention: it will use the UID for the resource it belongs to and the fact that it's a foreign resource is determined by the fact that the path uses a '::' notation, with the second half being the scene ID of the resource. The engine could already load file paths using this convention, but this PR allows the additional usage of the base resource's UID to provide consistent access even if the file is moved or renamed.
Since this PR is meant to fix the specific bug with references to the AnimationLibrary rather than a general purpose user-facing feature, we also give the AnimationLibrary generated upon scene import a unique consistent name which means that the path should remain consistent, even if its reimported, moved, or modified in any way. The name given is 'AnimationLibrary_GlobalImport', but I welcome alternative suggestions if this is bad for whatever reason.
It also includes some partial preliminary support for the dependency editor, though its not comprehensive. It will display the foreign resource scene IDs correctly, and the 'fix broken' button will actually attempt to fix the path with the scene id intact. There is however currently no way to intentionally select a foreign resource; I think to do this, we would have to build this up into a more comprehensive user-facing feature which would allow specifically named subresources to be accessible via the file browser (Unity has a comparable feature).
It might too difficult to get such a core feature fully battle-tested for 4.2, but I welcome anyone who wants to throughly test this under a variety of conditions. I've already tested it a bunch and it seems pretty robust, though I would recommend testing to make sure it works with localized paths (since I don't fully understand how they work in the context of UIDs). I will delegate this to discussion, though I will say that I feel the bug this intends to fix IS particularly nasty. If we absolutely can't do it, a compromise for 4.2 might be at least popping up a warning when attempting to modify the libraries parameter.
On the whole, I think this solution is relatively elegant and I'm happy introducing this doesn't require any changes to the resource format at all, everything is determined implicitly from data which was already accessible. The only thing I thought was a little hacky was using the SceneState object to store the original path of the scene before saving it in order to distinguish between a subresource and a foreign resource. It seems to get the job done though.