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

Universalize UID support in all resource types #97352

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

reduz
Copy link
Member

@reduz reduz commented Sep 23, 2024

Ensures all resource types support UIDs in a project. This is required to fix:

  • Scripts and many other resource types can´t be referenced by UID, only by path. If used in text (in a script) when refactored the references are lost.
  • Path export properties can´t use UID for unsupported types and must resort to paths. When refactored this is always lost (the editor can´t feasibly get into this data to change it).
  • Refactoring problems when files are moved outside the editor (this PR effectively fixes it).
  • Editor properly refresh paths if they changed externally while opened (as example, git update). This needs
    to be addressed in a subsequent PR, but this one effectively sets the prerequisites.
  • Slow editor refactoring times (as file paths need to be modified all across the board every time something is renamed). This PR allows always using UIDs, so refactoring paths file by file is no longer needed.

Resource types that do not support UID will get a .uid file appended to them (this includes .gd , .gdshader, .gdextension, etc files).

SCREENSHOT:

2D Platformer demo with uids auto generated:

image
(Note RESOURCE.md, these are considered text resources by Godot and exported, so the .uid is actually correct.
Edit: This was fixed before merging.)

TODO:

  • Editor refactoring not yet supported.
  • De-duplication (if two UIDs are the same) not yet supported.
    FAQ:

Why not using metafiles instead to make it more generic?

  1. There has not been any need for metafiles. Most resource types contain metadata on their own.
  2. The .uid files are only used by the editor and are effectively gone on export, as this information is moved to the uid database. As such they are unsuitable as metafiles.

@Jordyfel
Copy link
Contributor

Jordyfel commented Sep 23, 2024

As someone who spent some time working on the refactoring problems and thinking about the UID system I fully support this approach.

This will need some code in the filesystem dock to move the .uid files along with the main file, much like with .import.

To make sure, refactoring paths file by file will no longer be needed, means that users should be encouraged to hardcode uids instead of paths, right?

What do you mean by "editor refactoring not yet supported"?

Significant issue that will not be fixed by this: #95637

This comes from the problem that paths for extraction from imported 3d files, stored as import options, currently do not support uids at all, nor do they work with the refactoring logic in the filesystem dock. I've though about this a bit but am unsure (maybe store uid as a hidden import option?), but it seems like paths in .import files will need some significant effort to be made to fully work with refactoring.

@lyuma
Copy link
Contributor

lyuma commented Sep 23, 2024

it seems like paths in .import files will need some significant effort to be made to fully work with refactoring.

Agree that this is important: hardcoded paths is one of the biggest issues with the advanced importer, especially given that it causes failed imports or even outright engine crashes in some cases if the dependent files are missing.

Allowing .import files to reference by UID is blocking some workflow improvements such as #86430 so it would be good to get this fixed in 4.4

@reduz
Copy link
Member Author

reduz commented Sep 23, 2024

@lyuma this seems to be a separate issue that will need a separate fix

@JoNax97
Copy link
Contributor

JoNax97 commented Sep 23, 2024

This is a great step forward, but I think your assessment about meta files is a bit disingenuous.

Import files already contain editor-relevant metadata, and if in the future there's a need for saving anything else about code files, then you'll be in a situation where you either have to add another secondary file, rename the uid extension, or have a disconnect between the file intended type and it's content.

I think it would be preferable to future proof a bit here, and allow for meta files that are general and flexible enough to avoid needing more as hoc solutions going forward.

@reduz
Copy link
Member Author

reduz commented Sep 23, 2024

@JoNax97 If you ask me and I could redesign the engine from scratch, I would just have used generic metadata files for everything. Because that's not possible and the other formats already contain their own stuff, and their own metadata, this is the simplest solution for that.

@JoNax97
Copy link
Contributor

JoNax97 commented Sep 23, 2024

I understand your point. Maybe this could be the first step in a progressive migration path.

@reduz
Copy link
Member Author

reduz commented Sep 23, 2024

@JoNax97 AFAIK that is probably Godot 5 material, so not likely to happen for a long time. We need to commit to a stable engine before major changes like this happening again.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Very surface-level documentation notes.

Why do _get_resource_uid and _has_custom_uid_support have to be two separate methods? Would it not be possible to verify if only _get_resource_uid is overridden, and/or check if it returns 0?

doc/classes/ResourceFormatLoader.xml Outdated Show resolved Hide resolved
doc/classes/ResourceFormatLoader.xml Outdated Show resolved Hide resolved
@arkology
Copy link
Contributor

Did I understand correctly that it will be illegal to add uid files to gitignore?

@reduz
Copy link
Member Author

reduz commented Sep 23, 2024

@arkology I would not do it 😆

@wstumpfz
Copy link

I too would like to vote to make this more like a generic metadata file. Currently I'm trying to add "pack file tags" to assets to facilitate the pack file exporting workflow in godot (godotengine/godot-proposals#10580). The initial roadblock I've encountered is there's no real way to add metadata to PackedScenes [.tscn files] and GDScripts. As a proof of concept I added the ability for PackedScenes and GDScripts to have .import files to make it possible to save custom metadata for those assets. I had to edit some logic around how godot handles .import files, but it works. Having dedicated metadata files could be a better alternative, so it would be great though if you could expand the scope of this effort to make it a little more generic. It would make it significantly more futureproof.

@lyuma
Copy link
Contributor

lyuma commented Sep 26, 2024

@wstumpfz

there's no real way to add metadata to PackedScenes [.tscn files] and GDScripts

The Godot philosophy is that the solution should follow the problem. I think a more palatable approach (less likely to break assumptions) would be to add a place in the .tscn format for metadata to be written. That would solve that specific sub-problem

scripts like .gd and .gdshader are a bit more complicated, because the code is human-readable, but you could imagine attaching something bulky like a texture into the metadata fields. Imagine for example, a plugin that assigns icons to scripts in metadata. If this image were dumped in base64 format into an annotation like @meta("foo", "Resource(bar...)") that could be unexpected.

If you limit script metadata to resource references and text strings, it could still be feasible to add annotations to scripts, but it wouldn't generalizable to 100% of cases, so this could be unexpected.

There's actually a funky example in Unity which is similar to what you describe. Unity shaders can be in text format like in Godot. However, there is a feature where a default texture can be assigned for a given texture slot. Unity actually puts that texture reference into the .shader.meta file. In Godot, it would not be feasible to put a texture into the body of a shader, and I can see the appeal to having a .meta file to store this type of info, as in these other engines.

My in-depth explanation of the problem with using .import for native formats (copied from RocketChat)

I think changing the definition of .import in this way is going to break a lot of assumptions, so you can do this in your own project, but I would consider it unlikely that it will be approved to merge into Godot
what you are asking for is something like generic meta files,

I would read the discussion on the PR I linked, because it basically explains the problem with moving towards generic meta files. In fact, Reduz would be in favor of this type of change, but only for a major version

Sorry I didn't do a good job of explaining the issue with .import - basically, the way Godot thinks of things, assets are natively in resource format (.res / .scn). The purpose of the importer is to take a file in another non-native format and convert it to a native godot resource format, "remapped" to a native resource in .godot/imported

In other words, through this process, there are now two files at the same logical path in the project: there is the original file (such as a .fbx), and there is the .res in .godot/imported that it has been remapped to. For this reason, the original .fbx is read-only (you wouldn't want to overwrite a source file with a godot native resource in a different format.)

Now, separately, there is something called ResourceFormatLoader, which is what allows Godot to read .tscn, .res, .gdshader and so on, without a .import file. In essence, having a ResourceFormatLoader is what makes a format "native".

I think it is a very valid question to ask why loaders and importers are different types of things, but the fact is that they are. I think one answer is that loaders must be fast, since you will go through them every time you open a file, while importers usually are slow and therefore must cache a second native resource in .godot/imported.

For example, in Unity, every asset has a .meta file, and the meta file is the same format for every file type: even native assets have an importer, but it is set to NativeFormatImporter. (Though I wouldn't be surprised if it works similarly to Godot in the sense of reading the file directly and bypassing the import system)

So to circle back to your proposal, the problem here is that we are taking something with a resource loader, and treating it as if it is imported, but somehow without a resource in .godot/imported.

@huwpascoe
Copy link
Contributor

Because that's not possible and the other formats already contain their own stuff, and their own metadata, this is the simplest solution for that.

A future PR will inevitably need to store more data, the simplest solution will add another string after the UID, and eventually the Godot metadata format will become ill-defined text files with .uid extension.

I'm calling it now. 🙂

@akien-mga
Copy link
Member

It's easy enough to rename the extension in the future if we need, and have some compatibility code to handle the transition gracefully (keeping support for reading the old format, and adding support for converting to the new format). So there's no need to future proof and bikeshed now.

@reduz
Copy link
Member Author

reduz commented Oct 6, 2024

@huwpascoe Please check the FAQ I wrote, metadata does not make sense because:

  • This is only for very few file types, not all.
  • These files do not get exported.

If at some point you want to have metadata, it will have to be done in a different way.

@huwpascoe
Copy link
Contributor

That's okay, Akien's response was convincing enough. I probably should've replied with more than an emote!

I do have a question though, sometimes assets are marked as don't import and show up as a little x icon, or maybe they'd be set for importing again later. Does the .uid handle this edge case?

@reduz
Copy link
Member Author

reduz commented Oct 7, 2024

@huwpascoe I think it should be fine, since these is in practice for assets that are not imported

@reduz reduz marked this pull request as ready for review October 7, 2024 07:33
@reduz reduz requested review from a team as code owners October 7, 2024 07:33
@reduz
Copy link
Member Author

reduz commented Oct 7, 2024

Changes made:

  • Proper uid:// syntax saved in uid file for more consistency.
  • Unexposed the _has_custom_uid_support and just checked if virtual has been overriden, as suggested above.
  • Moving, renaming, duplicating now supported in filesystem dock.

This PR is now ready for review!

@Saul2022
Copy link

Saul2022 commented Oct 7, 2024

  • Moving, renaming, duplicating now supported in filesystem dock.

Does that mean it no longer slow af to rename files when there’s a lot of files in the project?( 10k, 20k)

@RevoluPowered
Copy link
Contributor

@reduz Would it be worth adding editor functionality to resolve UID issues like this too?
editor/editor_file_system.cpp:1259 - UID duplicate detected between res://textures/special/clip.png and res://textures/shaders/tangent-test.png.

I have a project that I created in 4.3.dev 3 exhibiting this problem with UID's I think the function might be generating duplicates higher than it should be.

@reduz
Copy link
Member Author

reduz commented Oct 7, 2024

@RevoluPowered Do you have an open issue about this? I feel that is unrelated.

@thygrrr
Copy link
Contributor

thygrrr commented Nov 22, 2024

May I suggest a deterministic initial creation of the UID based on File Location and Name, plus Content Hash?

This will mitigate many, many cases of evil twin conflicts (that come when a file is checked in without its UID file)

When two git users end up committing their own locally generated .uid files, and work a few changesets off of that, one or both of them usually loses a portion of their work, because this conflict is difficult or at least needlessly disruptive to manually merge.

Proposal

Were there basically a heuristic (i.e. deterministic first UID for any given file) that ensures that the same initial "new" file gets the same UID, merges and corrections in evil twin conflict cases are much less disruptive, since the .uid files will be identical, and are thus interchangeable.

This is by far the most common case:

  1. Artist: "The new sprites are in folder res://sprites!"
  2. artist commits, but forgets to alt-tab into Godot before committing and after exporting PNGs from Krita
  3. coder1 + coder2 pull and begin to work, unaware that Godot has silently created .uid and .import meta files on both their systems
  4. Coder1 finishes and commits.
  5. Coder2 finishes and has merge conflict. They must now choose:
  • lose and re-do own work referencing the new files (merge & discard, --theirs)
  • destroy Coder1's work (merge & destroy, --ours)
  • reach out to Coder1 and pull two people out of their flow trying to reconcile divergent UIDs because of a (perfectly human!) mistake someone else on the team caused much earlier in the day.

@kristianpiacenti
Copy link

Scripts have UIDs now, so if you re-save the .res file, it should reference the script's UID and the "auto-updating" should be working. If there are cases where some dependencies still break, open an issue if it isn't tracked already.

dotresbug.mp4

Am I testing this wrong?

@KoBeWi
Copy link
Member

KoBeWi commented Nov 22, 2024

No, that looks like a bug. Does Interactable.cs.uid file exist?

@kristianpiacenti
Copy link

No, that looks like a bug. Does Interactable.cs.uid file exist?

Indeed
image

@KoBeWi
Copy link
Member

KoBeWi commented Nov 22, 2024

I can't reproduce this. Open a new issue with a minimal project.
Note that it's not really related to UIDs, you are moving the file inside the editor, so Godot should be able to automatically fix dependencies.

@kristianpiacenti
Copy link

I can't reproduce this. Open a new issue with a minimal project. Note that it's not really related to UIDs, you are moving the file inside the editor, so Godot should be able to automatically fix dependencies.

I figured it out!

It looks like the reference update is not retroactive to .res files.
To fix this you need to open the .res, save and then it will get converted to support UIDs

As a workaround I made a plugin that does that automatically to every .res in the project, if anyone is in need

https://drive.google.com/file/d/1wpAQRoI5TxulDXekcgkADf0YdYureV43/view?usp=sharing

Make sure to have all your scenes closed.
Add it to your "addons" folder, enable the plugin and then Project -> Tools -> Fix All .res

@meanwhile0
Copy link

Should the signals in EditorFileSystem (particularly resources_reimported, resources_reimporting, resources_reload) be changed to return UIDs? As of 4.4dev5, they still return file paths; I feel like it'd be more consistent if they returned UIDs, i.e. for cases where you're checking for modifications to an exported resource linked in the editor, skipping the currently necessary conversions through ResourceUID functions.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 24, 2024

Depending on what users are doing with those signals, changing them may break existing plugins, so it's worth thinking about it carefully.

@arkology
Copy link
Contributor

arkology commented Nov 25, 2024

Scripts have UIDs now, so if you re-save the .res file, it should reference the script's UID and the "auto-updating" should be working.
If there are cases where some dependencies still break, open an issue if it isn't tracked already.

If it actually updates script path to script's UID automatically, and as Godot 4.3 doesn't have script UIDs, this PR breaks compatibility.
And this PR doesn't have corresponding tag.
Could someone please add break compat tag?
Nevermind, I'm wrong

@KoBeWi
Copy link
Member

KoBeWi commented Nov 25, 2024

The references are stored both as path and UID. You can see it in tscn files. There is no compatibility breakage.

@scott-lin
Copy link

I don't see any documentation changes to go along with the code changes. How can I understand the tradeoffs (benefits/downsides) of this change, the ways I can use it and its limitations?

I don't intend to come across as bitter, but as someone testing the developer releases, I find myself dealing with an influx of new files that clutter my file views, such as OS Explorer and VSCode. This clutters my workspace and disrupts my workflow, making it more difficult to locate the files I need. I can be persuaded the changes are good for me, but I don't understand how yet due to lack of details.

@SpockBauru
Copy link

For me it seems like a medicine that is worse than the disease itself

@Anixias
Copy link

Anixias commented Dec 25, 2024

I'm all for UID files since the alternative is a messy find-and-replace approach, but I really dislike that it produces extra files all over my project. They really need to be placed in a dedicated directory.

If it's having the same filepath that matters, just recreate the relative path from the root project directory inside of a directory like .uid/ or similar. For example, if a resource has path res://a/b.c, it should look for the UID file at uid://a/b.c.uid (where uid:// maps to .uid/).

Edit: Someone else opened a proposal regarding these concerns.

@JoNax97
Copy link
Contributor

JoNax97 commented Dec 25, 2024

The whole point of having the uid files at the same path is that users move both files at the same time

@daveTheOldCoder
Copy link

daveTheOldCoder commented Dec 25, 2024

But that doesn't explain why the filenames can't start with a period, so that they can optionally be hidden and not clutter up the view.

Even if the uid file is always visible, that doesn't ensure that the user will remember to copy/move it along with its parent file.

@JoNax97
Copy link
Contributor

JoNax97 commented Dec 25, 2024

that doesn't ensure that the user will remember to copy/move it along with its parent file.

But it makes is trivial. Having it in a separate folder makes it cumbersome and difficult for new user to know they have to do it.

@daveTheOldCoder
Copy link

I suggested that the name start with a ".", not place it in a separate folder.

@JoNax97
Copy link
Contributor

JoNax97 commented Dec 25, 2024

Sorry I lost the thread of who said what. In any case, the "." makes all uids bunch together at the top.

The convenience of the current approach is that you can bulk select files and be sure the uids are selected as well

@scott-lin
Copy link

scott-lin commented Dec 26, 2024

As long as the onus is on the human user, there is absolutely no guarantee the user will move uid files where Godot intends them to be regardless of whether they're located in a separate folder or the same folder.

There are a few misguided assumptions in the comments so far regarding how users will interact with uid files.

  1. Will all users understand how uid files function and where Godot expects them to be in the first place? Placing the onus on users to manage uid files is bad design. What are the consequences of doing it wrong or forgetting?

  2. The "convenience" assumption. I don't find it convenient to remember at all. Furthermore, users may order files on their system differently; for example, if ordered by Type or Last Modified instead of by Name, then the uid files won't be side-by-side with the code file.

I do not want to be in the business of managing uid files manually when I move code files around period. The benefits of the feature do not outweigh the introduced cost in my opinion. The system needs to handle it automatically.

To reiterate my previous comment, I'd like to see documentation regarding how can I understand the tradeoffs (benefits/downsides) of this change, the ways I can use it and its limitations.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 26, 2024

Using a separate folder means you have to search for the corresponding .uid file if you want to move it.
Prepending names with dot means the files are hidden on some systems and users may not be aware of their existence.
Using a single UID index file per directory means you have to manually update the entries and is prone to conflicts.
UID comments might be the most convenient solution, but we had it and some people disliked it very strongly, so it was reverted.

Yes, people might not know how to deal with .uid files as they are currently implemented, but it's still better and more intuitive solution than the other suggestions, especially when we already have .import files.

Also, if you really don't want to deal with .uid files, just don't touch them and only move the base files. Obviously your dependencies will break, but it's the same thing we had before .uid files were introduced. The only thing that changes is that you'll get harmless warnings when doing so.

@arkology
Copy link
Contributor

As someone who disliked UID comments very strongly, I ended up with conclusion that if I have to choose between UID files zerg rush and UID comments/annotations inside scripts, comments/annotations are the lesser of two evils.

@JoNax97
Copy link
Contributor

JoNax97 commented Dec 26, 2024

I know comparisons are odious, but Unity's had this exact scheme for decades and it works. Most game devs have at least some degrees of familiarity with it and know the rules for dealing with metadata files.

It's not the most elegant but it gets the job done and version control is really not an issue. I think you might be overreacting a bit here.

@SpockBauru
Copy link

@JoNax97

I know comparisons are odious, but Unity's had this exact scheme for decades and it works. Most game devs have at least some degrees of familiarity with it and know the rules for dealing with metadata files.

As a former Unity dev, I must state that the .meta files system is terrible to manage and not having them was one of the many things that I like in Godot.

@KoBeWi

Also, if you really don't want to deal with .uid files, just don't touch them and only move the base files. Obviously your dependencies will break, but it's the same thing we had before .uid files were introduced. The only thing that changes is that you'll get harmless warnings when doing so.

So make a way to disable the .uid generation. As I state before, is a medicine worse than the disease is trying to heal:
image

@scott-lin
Copy link

I agree with @SpockBauru so far. Give me a choice in disabling this feature. I do not want to deal with the system's side effects, warnings, etc. I want to make games.

Has the feature owner / Godot considered letting us generate a uid file on-demand for a specific file? Do we really need a uid for all files? As a user, if I have specific files I need uids for, then I could ask the system to generate a uid for them; in this model, I choose to create a dependency I'm willing to manage and understand how to seemingly.

@Anixias
Copy link

Anixias commented Dec 27, 2024

The UID files wouldn't be quite as bad if it didn't make one for every file that could potentially be a resource. I have 168 C# files in my project at the moment, and less than 30 of them are attached to a Node in the editor. The rest are libraries and whatnot yet they all have UID files generated for them as well. It should generate them once they are initially assigned to something in the editor, as needed, and automatically deleted when the last reference in the editor is removed.

@Geometror
Copy link
Member

Just for reference, there's #99090.

As I stated in #99090 (comment), UIDs for everything are desirable, but having a choice on how they are stored/managed would be great.

@SpockBauru
Copy link

Just for reference, there's #99090.

Thanks! I believe this PR is a better approach, let's move the discussion to #99090

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

Successfully merging this pull request may close these issues.