Use UIDs in addition to paths for extracted meshes, materials and animations #100786

lyuma · 2024-12-24T12:42:28Z

edit 2025-02-12: rewrote this PR from scratch.
edit 2025-02-16: migrate existing UIDs, use hash function to derive consistent UIDs for meshes and animations.

This doesn't necessarily fix a bug, but this is trying to migrate more things to support UIDs. In this case, the extracted file paths for meshes and animations did not support UID, so if the paths were renamed it was possible for the import to fail.

Similarly, renaming materials could cause them to get lost between imports. This change will keep the uid updated so that the material can be referenced even after it is renamed.

This PR adds use_external/fallback_path and save_to_file/fallback_path to the existing use_external/path and save_to_file/path properties. These properties already use the ResourceUID::ensure_path() function which means they support UIDs already. The main change is to handle the case where the UID lookup fails (is_empty()) and to regenerate the UID from the fallback path.
Additionally, refresh fallback_path properties on every import to ensure that the path is kept up to date.
Finally, this changes Material, Mesh and Animation extraction buttons in the SceneImportSettings dialog to generate UIDs and assign the fallback_path initially.

Also, fix a bug in CSV translations related to UID hashing (as this was the only other place in the code which used a hash function to construct a UID, but did so incorrectly (allowing negative numbers) and without checking for collision.

fire

Are you sure we are ok to merge this if the other parts are not implemented? There's also a call for all uid code contributors to meetup.

lyuma · 2025-01-07T19:42:30Z

@fire if you think the change is good and just needs the missing pieces implemented, such as animation slices, I will implement the missing parts next week. I just wanted get approval for the overall concept before feature freeze.

fire · 2025-01-07T19:56:27Z

It makes sense that uid works everywhere.

makinori · 2025-01-10T08:17:23Z

Thanks for the PR, I was having this exact issue. However, I noticed if I select an external material under mesh import in the UI, the res:// path is shown but the uid:// path is saved in the .import file. You mentioned that "the UID path isn't obvious and readable" and got confused when the engine deals with this issue

Jordyfel · 2025-01-14T00:38:43Z

Fixes #95637

The path import option can currently be set not only in _save_dir_confirm(), which is called only when the batch extraction dialog is used ("Actions..." button in the top left of the advanced import dialog brings that up), but also by the custom inspector thing which is the SceneImportSettingsData class in the same file.

What I'm thinking is that a user wouldn't really need to change the path after it was set, they would just move the extracted file in the filesystem instead.

This could justify something like, instead of introducing a new uid import option, store the uid in the currently existing path import option and use ResourceUID.ensure_path() for compatibility. If the user wants to reset the path and extract again, they would set the enabled option to false and use the other dialog (through _save_dir_confirm()) again. Following this logic it also makes sense to hide the import option so that it is only saved but not editable in the custom inspector thing.

KoBeWi · 2025-01-14T13:38:57Z

I tested this with simple cube and somehow ended up with this

which is related to this issue: #101256

I think it's caused by EditorPropertyPath automatically converting paths to UID. Not sure if there is a way to disable this behavior, but I think splitting between path and uid makes sense. It just has to work properly.

akien-mga · 2025-02-05T11:59:20Z

Would be good to rebase and test again as #101256 was fixed.

KoBeWi · 2025-02-05T13:06:13Z

The path and uid are now stored correctly.
However if you reimported files in 4.4 and they have uid in path field, it can't be fixed by simply reimporting; you need to extract to a new file.

editor/import/3d/scene_import_settings.cpp

editor/import/3d/resource_importer_scene.cpp

reduz · 2025-02-05T18:04:53Z

Please check how this is done for the translations:
https://github.com/godotengine/godot/blob/master/editor/import/resource_importer_csv_translation.cpp#L75

Since the path can change, the most reliable way to do this is using the UID of the parent resource, which is supplied as argument on the ::import call.

lyuma · 2025-02-05T18:39:59Z

So from looking into this, my understanding is @reduz PRs #97912 and #97363 address a lot of the issues that this PR was designed for. Here are some of the things I am still thinking about:

one of the things I am considering changing is to keep status quo with path = "uid://" (or res:// for compatibility) and put the file path into a property called "fallback_path" only to be used when the uid does not exist. That would make this PR smaller and less dangerous compared to beta, since it is only fixing the case where you have extracted files and the referenced file was deleted.

the other question I had is when is it appropriate to modify the .import settings, for example if a referenced file is moved. If we call it fallback_path... let's say the user extracts a material to "res://models/materials/red.material" and then moves it to "res://global/red.material". Should the fallback_path be updated to the new path next time it is imported. Then if the user deletes global/red.material and the gltf is reimported, what should happen?

Finally, we need to make sure all places that extract resources are updated to use uid:// at the time they perform the extraction (for example in the adv import dialog), and I should test if animation slices work)

I have changed the design and want to get consensus.

lyuma · 2025-02-12T11:53:56Z

Pushed a new version of this change. I updated the original comment to explain what the PR is now doing, but I wanted to have this ready for the core meeting. My goal is to have a minimal change. This does not change anything to users who use res:// paths in .import, and it does not change anything for users who use valid uid:// paths in .import.

During import, this PR should only impact cases when a uid:// path in .import is missing.

This PR also implements UID paths when the user selects a new directory to extract meshes, textures and materials, so in other words, it means that this code may also affect newly imported assets.

editor/import/3d/scene_import_settings.cpp

KoBeWi · 2025-02-12T21:16:43Z

I duplicated a GLB file and extracted mesh, but it saved as regular path for path and empty fallback_path. Seems like it's unreliable?

I think the general idea is fine.

lyuma · 2025-02-14T11:12:20Z

@KoBeWi I got this change working for meshes, materials and animations. I have been doing some tests on the fox gltf (which has animations, a mesh and a material) and so far I'm pretty satisfied with it.

It feels nice and transparent to the user, since the file pickers only show the res:// path. I tested renames, even in cases where Godot doesn't update the res:// path, and it seems to be able to locate the file fine using the uid://

Some known issues:

I do not currently upgrade res:// paths to uid:// - This is kind of intentional to reduce the scope of this PR and reduce risk, but I'm open to changing it, for example due to the second issue.
Due to how PROPERTY_HINT_SAVE_FILE seems to work, if a new file is created using the file picker in the inspector, it puts a res:// path into save_to_file/path which won't get upgraded. (The common flow of using extract / set save paths buttons works correctly)
The ResourceSaver::set_uid API is jank both in potentially causing glitches and performance, and from my research, there is no other way to save a resource to a file with a pre-determined uid:

ResourceSaver::save doesn't have a way to pass a UID, and I saw the code @reduz linked to first calls ResourceSaver::save, and then uses ResourceSaver::set_uid. However, set_uid is a really bad function for a few reasons:

a. it rewrites the file again, so it now means all meshes and animations are being written twice.

b. the temporary uid is saved to uid_cache.bin first, which causes other places that reference the extracted meshes to use the temporary uid, even though the .tres file itself has a different UID. It's very confusing and while I haven't run into a bug with this, it feels wrong and jank. I think the fix requires modifying ResourceSaver to allow passing a UID into save(), which will turn into a bigger change.

reduz · 2025-02-14T18:55:59Z

editor/import/3d/resource_importer_scene.cpp

+	ERR_FAIL_COND_V_MSG(err != OK, anim, "Saving of animation failed: " + res_path);
+	if (p_save_to_path.begins_with("uid://")) {
+		// slow
+		ResourceSaver::set_uid(res_path, ResourceUID::get_singleton()->text_to_id(p_save_to_path));


As I mentioned before, this is very undesired because if you move or rename the original scene, the uids for the external files will be changed.

Check the translation importer on how to generate uids for generated resources resulting from import.

I have made the change for Mesh and Animation to use hash for UID generation.
(Note: The hash generation in CSV Translation had a bug that was causing uid conflicts and uid://, which I opted to fix in this PR. Let me know if the translation fixes should be a separate PR)

For Material, it is common for people to use other Material objects, switch to ShaderMaterial, or share materials between models. I think randomly generated UID makes more sense for this workflow because they are not overwritten on each import.

editor/import/3d/resource_importer_scene.cpp

editor/import/3d/scene_import_settings.cpp

editor/import/3d/resource_importer_scene.cpp

KoBeWi · 2025-02-17T11:54:03Z

I tested that it works (as much as I'm familiar with model import at least).
@reduz will probably have more to say about implementation.

editor/import/3d/resource_importer_scene.h

editor/import/3d/resource_importer_scene.cpp

When extracting meshes, materials and animations, always store the uid:// path as well as a res:// fallback. When validating import settings, load the fallback path if the uid:// path fails to load. Update save_to_file/fallback_path every import to keep the file path in sync with the uid. Use UID hashing for meshes and animations.

akien-mga · 2025-03-19T11:38:30Z

Thanks!

OhiraKyou · 2025-03-20T22:25:13Z

Related? #74612

lyuma requested a review from a team as a code owner December 24, 2024 12:42

Chaosus added enhancement topic:import labels Dec 25, 2024

Chaosus added this to the 4.4 milestone Dec 25, 2024

fire mentioned this pull request Jan 7, 2025

Review Lyuma's 4 Godot Engine import pull requests V-Sekai/v-sekai-game#468

Closed

4 tasks

fire reviewed Jan 7, 2025

View reviewed changes

akien-mga requested a review from KoBeWi January 14, 2025 09:28

KoBeWi mentioned this pull request Jan 14, 2025

Resource importer scene: Don't validate save paths when they are UIDs #101509

Closed

KoBeWi reviewed Feb 5, 2025

View reviewed changes

editor/import/3d/scene_import_settings.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Feb 5, 2025

View reviewed changes

editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved

KoBeWi previously approved these changes Feb 5, 2025

View reviewed changes

lyuma force-pushed the save_file_uids branch from af4fba3 to 7264a77 Compare February 12, 2025 11:41

lyuma force-pushed the save_file_uids branch from 7264a77 to 8f95cec Compare February 12, 2025 11:55

KoBeWi reviewed Feb 12, 2025

View reviewed changes

editor/import/3d/scene_import_settings.cpp Outdated Show resolved Hide resolved

lyuma force-pushed the save_file_uids branch 3 times, most recently from 55b30de to bba2f1b Compare February 14, 2025 11:04

reduz requested changes Feb 14, 2025

View reviewed changes

lyuma force-pushed the save_file_uids branch 2 times, most recently from 561f3c8 to 4f171de Compare February 16, 2025 20:58

lyuma requested a review from a team as a code owner February 16, 2025 20:58

lyuma force-pushed the save_file_uids branch from 4f171de to 9947b36 Compare February 17, 2025 03:12

KoBeWi reviewed Feb 17, 2025

View reviewed changes

editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Feb 17, 2025

View reviewed changes

editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved

lyuma force-pushed the save_file_uids branch from 9947b36 to 1af46e9 Compare February 17, 2025 10:48

KoBeWi reviewed Feb 17, 2025

View reviewed changes

editor/import/3d/scene_import_settings.cpp Outdated Show resolved Hide resolved

lyuma force-pushed the save_file_uids branch 2 times, most recently from 2ab19fd to 4a0937e Compare February 17, 2025 11:22

KoBeWi reviewed Feb 17, 2025

View reviewed changes

editor/import/3d/resource_importer_scene.cpp Outdated Show resolved Hide resolved

lyuma force-pushed the save_file_uids branch from 4a0937e to c9f5084 Compare February 17, 2025 11:40

KoBeWi requested a review from reduz February 17, 2025 11:54

lyuma force-pushed the save_file_uids branch from c9f5084 to 423b693 Compare February 21, 2025 09:33

akien-mga modified the milestones: 4.4, 4.5 Feb 24, 2025

Repiteo modified the milestones: 4.4, 4.5 Feb 24, 2025

reduz approved these changes Mar 14, 2025

View reviewed changes

AThousandShips reviewed Mar 14, 2025

View reviewed changes

akien-mga force-pushed the save_file_uids branch from 423b693 to 8997c99 Compare March 19, 2025 10:50

akien-mga merged commit 16816f4 into godotengine:master Mar 19, 2025
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use UIDs in addition to paths for extracted meshes, materials and animations #100786

Use UIDs in addition to paths for extracted meshes, materials and animations #100786

lyuma commented Dec 24, 2024 •

edited

Loading

fire left a comment

lyuma commented Jan 7, 2025

fire commented Jan 7, 2025

makinori commented Jan 10, 2025

Jordyfel commented Jan 14, 2025 •

edited

Loading

KoBeWi commented Jan 14, 2025

akien-mga commented Feb 5, 2025

KoBeWi commented Feb 5, 2025 •

edited

Loading

reduz commented Feb 5, 2025 •

edited

Loading

lyuma commented Feb 5, 2025

lyuma commented Feb 12, 2025

KoBeWi commented Feb 12, 2025

lyuma commented Feb 14, 2025

reduz Feb 14, 2025

lyuma Feb 17, 2025

KoBeWi commented Feb 17, 2025

akien-mga commented Mar 19, 2025

OhiraKyou commented Mar 20, 2025

Use UIDs in addition to paths for extracted meshes, materials and animations #100786

Use UIDs in addition to paths for extracted meshes, materials and animations #100786

Conversation

lyuma commented Dec 24, 2024 • edited Loading

fire left a comment

Choose a reason for hiding this comment

lyuma commented Jan 7, 2025

fire commented Jan 7, 2025

makinori commented Jan 10, 2025

Jordyfel commented Jan 14, 2025 • edited Loading

KoBeWi commented Jan 14, 2025

akien-mga commented Feb 5, 2025

KoBeWi commented Feb 5, 2025 • edited Loading

reduz commented Feb 5, 2025 • edited Loading

lyuma commented Feb 5, 2025

lyuma commented Feb 12, 2025

KoBeWi commented Feb 12, 2025

lyuma commented Feb 14, 2025

reduz Feb 14, 2025

Choose a reason for hiding this comment

lyuma Feb 17, 2025

Choose a reason for hiding this comment

KoBeWi commented Feb 17, 2025

akien-mga commented Mar 19, 2025

OhiraKyou commented Mar 20, 2025

lyuma commented Dec 24, 2024 •

edited

Loading

Jordyfel commented Jan 14, 2025 •

edited

Loading

KoBeWi commented Feb 5, 2025 •

edited

Loading

reduz commented Feb 5, 2025 •

edited

Loading