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

Add project setting to disable UIDs and UID caching #7195

Open
dhoverml opened this issue Jul 3, 2023 · 8 comments · May be fixed by godotengine/godot#81122
Open

Add project setting to disable UIDs and UID caching #7195

dhoverml opened this issue Jul 3, 2023 · 8 comments · May be fixed by godotengine/godot#81122

Comments

@dhoverml
Copy link

dhoverml commented Jul 3, 2023

Describe the project you are working on

Any Godot project.

Describe the problem or limitation you are having in your project

UID caching was introduced with Godot 4.0, and along with it came a number of bugs (these are what are opened as of today, this doesn't include the fixed bugs)

godotengine/godot#62258
godotengine/godot#64330 (related to 68661)
godotengine/godot#67851 (related to 68661)
godotengine/godot#68661
godotengine/godot#68672 (related to 62258)
#7181

I've also ran into issues with UIDs, and considering I don't require its features I've consider disabling them locally. I'm worried about future Godot features that absolutely require UIDs, as well as other unintended side effects.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Add a project setting called application/config/enable_uid_cache (or application/config/enable_uids) to enable (default) or disable UIDs and UID caching (uid_cache.bin), plus a description for the pros and cons of doing so.

Pros:

  1. Reduce noise in git since UIDs won't change in scene or resource files, since there are none.
  2. Can be used as a workaround for bugs that a user cannot otherwise workaround, like for the bugs listed earlier.
  3. Reduces one point of confusion when incorrect scenes or resources are loaded, especially for new users.
  4. Saves some space in the final exported project, granted it's not much (usually <100KB; highly dependent on the project).

Cons:

  1. Moving a file can break other scenes/resources outside of the editor.
  2. Existing code that loads UIDs explicitly, like with load("uid://someuid") will break.
  3. 3rd party addons that load UIDs explicitly will also break.

This appears as though Godot would now have to support "UIDs" and "no UID"s, however the code already does this for the most part by checking for ResourceID::INVALID_ID and falling back to paths (or doing nothing, depending on the context).

I've been hesitant on this proposal because I'm pretty sure it's going to be rejected only because UIDs were added to solve the problems (the Cons) above, and could only cause even more confusion (like if a user has UIDs disabled, then adds an addon that uses UIDs and sees it not working).
Though I want to open some discussion on it considering how low-impact the underlying change is, and how easy it is for a user to just switch back to UIDs (by re-enabling this option).

Possible alternative:
A hybrid approach could mitigate some of the Cons (2 and 3) by using existing UIDs, but not generate new UIDs for new scenes/etc. This can be thought of as "locking in" existing UIDs. Empty projects that set this setting will not have UIDs assigned to anything. However if they include an addon that uses UIDs then the addon will still function normally since UID lookup will still work correctly.
This could be considered even more confusing from a user perspective though.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Below describes the original idea, not the hybrid approach.

  1. At initialization in main.cpp, choose either ResourceUID or a new class that derives from ResourceUID (ResourceUIDDummy or ResourceUIDStub) based on whether application/config/enable_uid_cache is true or false.
  2. ResourceUIDDummy deletes uid_cache.bin, and always returns ResourceID::INVALID_ID, empty strings, Error::OK, etc.
  3. So when ResourceUIDDummy is in use, scenes and resources are saved and loaded similar to pre 4.0, as in, based on path rather than UID. There is no longer a UID stored in the scene (tscn) or resource (.import).
  4. UIDs in existing scenes or resources are ignored when loaded, however saving those scenes or resources will erase the UID.
  5. Keep UIDs and UID caching enabled by default since many projects are currently (and usually unknowingly) using them, as well as not break projects that explicitly use them (like load("uid://something")).

I have a potential (and very lightly tested, unsubmitted) PR that implements this.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, this is part of the core.

Is there a reason why this should be core and not an add-on in the asset library?

ResourceUID must be modified, which cannot be done with an addon.

@jtnicholl
Copy link

I'd like to add that godotengine/godot#68672 (comment) is another reason I want to see this.

I also fail to see what is even the point of UIDs. They're advertised as allowing you to easily move files around without breaking references. However you could already do that without them, as long as you do it through the editor and not your system file manager. The editor scans through the whole project and updates the file path references whenever you move stuff.

@Zireael07
Copy link

The editor scans through the whole project and updates the file path references whenever you move stuff.

This is not always true. People have gotten references to break by moving stuff, especially where inheritance is involved, or even corrupt scenes. UUIDs provide some more protection from this

@dhoverml
Copy link
Author

Hardcoded load("res://some/path") (or preload()) in scripts can easily be broken by moving files, whether you move them in the editor or externally. This kind of case wouldn't break with UIDs (i.e load("uid://some_uid"), though now reading the code makes it hard to know which file is being loaded)

However 99% of the time I'm not moving files, and, when I do, grep and sed (and git to prevent permanently breaking my project) help a lot in ensuring the move addressed all paths. UIDs, in how I work with Godot (which likely differs from others), only produce noise in git.

I think adding the option to disable them is fair, especially since most (?) users may not know what UIDs are, or not care, and will leave it set to default (enabled).

@jtnicholl
Copy link

This is not always true. People have gotten references to break by moving stuff, especially where inheritance is involved, or even corrupt scenes. UUIDs provide some more protection from this

I've never run into such an issue myself, but if it is a problem it's just an editor bug that should be fixed. Adding another, even buggier feature with its own maintenance cost as an alternative is a terrible solution.

@Calinou
Copy link
Member

Calinou commented Aug 30, 2023

The issue with a project setting is that it'll break add-ons in your project that make use of UIDs (such as uid:// paths in their scripts). Adding settings that can affect script behavior is often a bad idea, as it also affects any add-ons you install in your project.

I'd prefer we find ways to fix the bugs rather than adding an alternative codepath which requires its own testing.

@dhoverml
Copy link
Author

I agree, though some things to consider:

  • How many plugins use the UID syntax? This needs to be asked somewhere that'd reach more people, like discord or rocketchat, since I don't know the answer or how to find the answer
  • How many people know about UIDs? It almost seems like people learn about them when they encounter a bug with them.
  • The PR mentions that the UID syntax will break. Though maybe it should stand out by making it bold and, if possible, surrounded with warning signs.

When disabled, [code].godot/uid_cache.bin[/code] is deleted and UIDs for every scene and resource are cleared only when the scene or resource is resaved. Code that relies on opening files with the [code]uid://[/code] syntax will not work.

  • Almost all of the code in master already handles invalid UIDs. The PR addressed the few places that didn't.
  • Perhaps there could be a sanity check when the property is disabled, as in, if it finds the uid syntax, then report errors about how it will not work. Alternatively (or in addition) put up a popup saying that it is risky and that some scripts may not work.

@Calinou
Copy link
Member

Calinou commented Aug 30, 2023

  • How many plugins use the UID syntax? This needs to be asked somewhere that'd reach more people, like discord or rocketchat, since I don't know the answer or how to find the answer

Probably not many right now, but we should avoid breaking things now that 4.0 is released. We should avoid introducing features that are support burdens for add-on developers, since maintaining an add-on is enough work as it is.

  • How many people know about UIDs? It almost seems like people learn about them when they encounter a bug with them.

Probably not many right now either, but this is because the feature lacks documentation and bug fixes. We should try to improve that before attempting to remove UIDs 🙂

  • Almost all of the code in master already handles invalid UIDs. The PR addressed the few places that didn't.

That part could be split to its own PR, since it's a bug fix.

@dhoverml
Copy link
Author

Sounds good, thanks @Calinou

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

Successfully merging a pull request may close this issue.

4 participants