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

Script UID improvements #87418

Open
3 tasks
Tracked by #92907
KoBeWi opened this issue Jan 20, 2024 · 12 comments
Open
3 tasks
Tracked by #92907

Script UID improvements #87418

KoBeWi opened this issue Jan 20, 2024 · 12 comments

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Jan 20, 2024

Tested versions

4.3

System information

Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1060 (NVIDIA; 30.0.15.1403) - Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz (8 Threads)

Issue description

GDScript has a basic UID support now, but it has some missing features:

  • Allow extending script by its UID (extends "uid://acdc")
  • Allow using script preloaded with UID as a type (const Type = preload("uid://acdc"))
  • Add "Copy Script UID" option to Script Editor, in addition to Copy Script Path
    image

The first two result in an error currently, the last one does not exist (though you can copy UID using FileSystem context menu).

Steps to reproduce

Try using any of the above.

Minimal reproduction project (MRP)

N/A

@dalexeev
Copy link
Member

dalexeev commented Jan 24, 2024

Allow using script preloaded with UID as a type (const Type = preload("uid://acdc"))

Preloading with only a UID is less readable and inconsistent with other resources (it is assumed that UIDs can be safely removed/changed). I would expect an optional uid argument for load/preload:

const Type = preload("res://script.gd", "uid://acdc")

The same for extends, but the syntax probably looks worse due to the lack of parentheses:

extends "res://script.gd", "uid://acdc"

Also note that you can use relative paths with preload and extends:

extends "./script.gd"

const Type = preload("./other_script.gd")

For load, we can also add support for relative paths (see #66550 (comment)) or add "magic" constants like __FILE__, __DIR__ (but we should distinguish between absolute and res:// paths).

@AThousandShips
Copy link
Member

This would also allow us to have a warning if the plain path is out of date, possibly with an option to update it in the editor

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 24, 2024

Preloading with only a UID is less readable and inconsistent with other resources (it is assumed that UIDs can be safely removed/changed).

No, the whole purpose of UID is that it's not supposed to change, unlike paths. The engine ensures that. Other resources already support load and preload, it only doesn't work for scripts.

The readability can be always improved by adding a tooltip when hovering UID or even displaying it in a special way.

This would also allow us to have a warning if the plain path is out of date, possibly with an option to update it in the editor

That's not any different from what we have now. When you change path, any script referencing it will break. Issuing a warning would require scanning every script on every path change and updating is useless unless it happens automatically. We could implement that even without UIDs.

@AThousandShips
Copy link
Member

I meant with both uid and path, that specific suggestion

@dalexeev
Copy link
Member

The readability can be always improved by adding a tooltip when hovering UID or even displaying it in a special way.

This is only possible for the built-in editor. For external editors, GitHub, etc. this is less convenient (requires plugins or is not possible). When you see extends "uid://lalala" or preload("uid://lalal") you don't know where to look for that file and it's much harder to guess what it does, especially in the case of extends.

No, the whole purpose of UID is that it's not supposed to change, unlike paths. The engine ensures that.

Sometimes Godot changes UIDs for some reason, even if I didn't recreate the resource, but only edited/renamed/moved that resource or its dependencies. I think that in the current implementation this is also possible for GDScript, since the UID line is not only inserted when creating the .gd file, but can also be changed later via the set_uid() method.

So I wouldn't be sure that UID persistence is guaranteed. Also, as far as I understand, UID was not intended to be a replacement for file paths, only a secondary addressing system. All other resources store both file path and UID. The UID may be missing, but the path must be there. This is what I meant by inconsistency.

That's not any different from what we have now. When you change path, any script referencing it will break.

The difference is that if you encounter an incorrect file path (for example, if you got someone else's project), you can try to look for the components of the path in the file system, most often the file can be quickly found. If you encounter an incorrect UID that has no references left along with the file path, then it could be almost any script/resource (if you are not familiar with the project).

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 24, 2024

This is only possible for the built-in editor. For external editors, GitHub, etc. this is less convenient

External editors have extensions. VS Code extension can e.g. show paths of external resources referenced in the scene:
image
UID display is likely possible too and might be even easier than in the Godot editor, because we don't have script editor extensions.

Sometimes Godot changes UIDs for some reason, even if I didn't recreate the resource

That's a bug.

since the UID line is not only inserted when creating the .gd file, but can also be changed later via the set_uid() method.

The editor should not change UIDs without a reason. IIRC set_uid() specifically was added to fix UID conflicts when duplicating files. User using set_uid() is irrelevant.

If you encounter an incorrect UID that has no references left along with the file path, then it could be almost any script/resource (if you are not familiar with the project).

That's fair I guess. But as I said above, incorrect UIDs are not something expected and AFAIK we have most cases already fixed.

Using UIDs in preloads is currently not supported well, because you need to manually copy the UID. If we add a special syntax for both path/UID, we could automatically insert it when drag and dropping files. But I'd make both path and UID optional, so user could use either of them or both.

@KesKim
Copy link

KesKim commented Nov 13, 2024

I've been reading through the UID discussions/propositions/issues/PRs here this morning, and if I've understood correctly #97352 this has been moving in the direction where each script file gets its own .uid file where the UID is stored, and the editor handles renaming that .uid to match the name of the script file if the script file gets renamed.

That's fair I guess. But as I said above, incorrect UIDs are not something expected and AFAIK we have most cases already fixed.

So, just for this part I wanted to chime in that as I understand Unity has a similar system with its .meta files accompanying actual asset/script files and the UID within used for referencing, there have at least in my career repeatedly been cases where issues come up with "incorrect UIDs". Mainly goes something like this:

  • user A creates a script foo.cs
  • Unity creates foo.cs.meta ("UID file")
  • user A commits and pushes foo.cs, forgetting to commit the UID file
  • user B pulls, receiving foo.cs
  • Unity creates foo.cs.meta with different UID, any references to foo.cs on user B's end are invalid/missing
  • user B commits and pushes their UID file
  • user A fails to pull due to conflict of having the original UID file in their workspace as git is attempting to pull a tracked file version of it
  • worst case: user A proceeds to delete their own UID file and pulls user B's not-referenced-anywhere UID. In this case we end up with any possible references to foo.cs script file just being written as cryptic UIDs that can only be manually fixed by a user who knows the file they should referenced and knows how to edit the UID files and what UID should go there.

Depending on team sizes and skillsets there's multiple problems:

  • even skilled users can be guilty of this, as it's simply something that happens by accident every now and then when you work long enough
  • less skilled users get into situations where they seemingly "can't pull", don't understand why, and generally make the mistake of not committing the UID file more often, or might even delete the UID file after having had the script referenced by its UID somewhere

Pretty much all of this is manageable with experience and training, but I just wanted to note that having a broken project state with invalid UIDs as the only reference can be a bit of a frustration and may require some tinkering and sometimes just remaking entire assets (Node Trees) with broken references being the last remaining solution. This is not to say it's the wrong way to implement this, just that it's good to recognize any potential situations that may come with it.

Thus I think the following point has merit, even if the additional human-readable filename(context) part is just the filename without path, only to help solve issues when they arise by providing context and the UID is used for actual references universally.

The difference is that if you encounter an incorrect file path (for example, if you got someone else's project), you can try to look for the components of the path in the file system, most often the file can be quickly found. If you encounter an incorrect UID that has no references left along with the file path, then it could be almost any script/resource (if you are not familiar with the project).

Also, do correct me if this is an unhelpful way to comment on development as I'm not really experienced in big open source projects at this point, so I'll know the etiquette a bit better in the future.

@Delsin-Yu
Copy link
Contributor

So, just for this part I wanted to chime in that as I understand Unity has a similar system with its .meta files accompanying actual asset/script files and the UID within used for referencing, there have at least in my career repeatedly been cases where issues come up with "incorrect UIDs". Mainly goes something like this

As a former Unity Engine user, I want to add an additional, worse case when working with VCS:

  1. User A moves a bunch of assets to another path
  2. The user uses the VSC to undo that
  3. Unity thinks those assets are new and reassigns a new UID to those assets
  4. The User was unaware and pushed the changes to the remote
  5. User B pulls the changes, and the references become broken on its end
  6. User B fixes the references and pushes the changes to the remote
  7. User A pulls the changes, and the references become broken on its end

@molingyu
Copy link

Is it possible to use a .ignore file to determine which files and paths will not generate uid?

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 13, 2024

I think this might've gone offtopic.

The issue is about script UIDs. They were implemented, then reverted, now we have them again. This means scripts now have UIDs and are expected to work with UIDs everywhere where paths are supported (#99116 (comment)). I listed a few cases where they are not supported right now, and as per linked comment, they are expected to be resolved eventually.

About UID design in general I think we should open a separate discussion.


If you encounter an incorrect UID that has no references left along with the file path, then it could be almost any script/resource (if you are not familiar with the project).

Back on this, I think we can introduce some sort of local UID archive, where you can lookup what paths every UID pointed to. This could help in resolving conflicts/problems should they arrive.

Is it possible to use a .ignore file to determine which files and paths will not generate uid?

Currently no, you can open a proposal.

@dalexeev
Copy link
Member

Back on this, I think we can introduce some sort of local UID archive, where you can lookup what paths every UID pointed to. This could help in resolving conflicts/problems should they arrive.

A local UID archive won't help if the bug was introduced by another developer. Especially if you are continuing development on an existing project.

Also, I'd like to reiterate that UID is not human-readable and is not accessible (external IDEs without special plugins, on GitHub, viewing diffs, etc.). File paths and class names are primary identifiers, and UIDs are secondary. They should not be explicitly used by a human, in my opinion.

I think we should either make UID an additional optional parameter to load()/preload()/extends or completely avoid UIDs in human-generated files. Instead, we could use relative paths. extends and preload() already support them and there is a PR for load().

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 13, 2024

A local UID archive won't help if the bug was introduced by another developer.

The other developer will also have an archive, which they can send to you if they can't fix the issue themselves.
I can open a proposal about that, but first we'll see how much of a problem it will be.

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

No branches or pull requests

6 participants