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 optional GDScript UID comments #99090

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 11, 2024

Somewhat a revival of #67132 and a follow-up to #97352
Closes godotengine/godot-proposals#11579

This implements the original idea of UID comments in scripts (before they were changed to annotation and then reverted anyway), with a difference that it uses the new has_custom_uid_support() method to make them optional based on a new project setting called filesystem/gdscript/inline_script_uids. The setting is disabled by default. If you enable it, GDScript will create UID comments in scripts instead of using .uid files. The comment looks like this:

# uid://s0m3rand0mu1d # Auto-generated, do not modify or remove.

Note that

  • I don't expect it to be merged
  • I'm not going to change the implementation to use annotation/parser

I opened this, because I already had the code and some people were interested in the previous solution. The new setting allows to get rid of additional files, but it's for advanced users.

@KoBeWi KoBeWi added this to the 4.x milestone Nov 11, 2024
@KoBeWi KoBeWi requested review from a team as code owners November 11, 2024 20:46
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.

It returns.

I have a hunch several people are not going to like either of the options here. .uid files, which contribute to Filesystem bloat, or an extra comment, which can feel out of place without much user control. Either way, users are basically forced into this.

With that comes the question on whether these RIDs should be obligatory or not, but as of writing this, they are and it's for the best.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 11, 2024

With that comes the question on whether these RIDs should be obligatory or not, but as of writing this, they are and it's for the best.

You mean UIDs I assume.
They are not optional since #97352. Every Resource has them now. If they don't support them properly, they get .uid file.

A big advantage is that we can now safely assume that everything has UID and it allows to use UIDs instead of paths. E.g. the huge code in FileSystem that ensures every dependency is updated after you move a file can now be removed, because we have that for free.

@dalexeev
Copy link
Member

dalexeev commented Nov 11, 2024

I'm not sure which is the lesser evil: cluttering the filesystem or having a foreign line in every script. Both have their pros and cons, so we're bound to run into someone not liking it no matter which option we choose.

.uid files

  • ❌ Clutters the filesystem.
    • .import files are already cluttering up the file system.
    • .uid files are hidden in the editor's file system and can be hidden in external IDEs too.
  • ✅ Doesn't clutter scripts with foreign lines.
  • ❌ Can still be lost if the user moves only the .gd file via an external file manager.

uid comments

  • ✅ Doesn't clutter the filesystem.
  • ❌ Clutters scripts with foreign lines.
  • ✅ More reliable if the user moves the .gd file via an external file manager.

A project setting allows the user to choose the option they like best. This would seem to be a win-win situation, but it potentially hinders cross-project compatibility. For example, uid comments should not be automatically added to addons created without them and the same in the reverse situation.

# uid://s0m3rand0mu1d Auto-generated, do not modify or remove.

Suggestion:

# uid://s0m3rand0mu1d # Auto-generated, do not modify or remove.
                      ^^

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 11, 2024

Hmm, compatibility is indeed a concern 🤔
I could allow reading .uid file if it's provided and the setting is enabled, this way there is at least a compatibility with the default option. Addon creators would be expected to provide the .uid files.

@geekley
Copy link

geekley commented Nov 12, 2024

I'm not sure which is the lesser evil: cluttering the filesystem or having a foreign line in every script. Both have their pros and cons, so we're bound to run into someone not liking it no matter which option we choose.

What about a middle ground? Would it be bad to have like a single file per-folder (e.g. a folder/_.uid JSON) that maps all UIDs of files on that folder (that would otherwise be separate *.uid files)? Then you don't clutter, and you know what to do when moving/renaming outside the editor.
Though tbh, I don't know if it would be "best of both worlds" of "worst of both worlds". I think it may be ok if the editor could track (at least verbatim) file move/rename and auto-update this file.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 12, 2024

Then you don't clutter, and you know what to do when moving/renaming outside the editor.

And what to do if you want to move a single file out of the directory?

@geekley
Copy link

geekley commented Nov 12, 2024

Then you don't clutter, and you know what to do when moving/renaming outside the editor.

And what to do if you want to move a single file out of the directory?

Manually update files, moving the JSON entry from the file in one folder to the file in the other folder.
This file would be very self-explanatory, e.g.:

{
  "uid://abcdetc": "./MyClass.gd",
  "uid://wxyzetc": "./OtherClass.gd",
}

(I used uid->filename arbitrarily, could be the other way around if it makes more sense, e.g. for alphabetic ordering)

Again, for unchanged files moved outside the editor, the running editor could automatically still detect missing filename entries and match with the new location that's missing the corresponding UID, and auto-update it -- as long as it has the files contents "cached" in memory. If you move when it's closed, then it will later warn you that have to manually fix the UID files or use the existing locate... feature in the editor to match the old locations.

@geekley
Copy link

geekley commented Nov 12, 2024

Btw, I don't like at all adding significance to comments like this. The annotation would be better. Unless you were to use a documentation comment tag like: ## @uid: uid://abcdetc or similar. The "don't modify" also feels too redundant and unnecessary clutter IMHO. Nobody would be dumb enough to modify an uid on purpose and also not understand why they get issues afterwards.

@arkology
Copy link
Contributor

arkology commented Nov 12, 2024

As now we are forced to have uid for scripts, placing it inside script is my candidate👍 For me having separate uid files for scripts is the greater evil.
It would be amazing if this was merged before the next development build.

Also I have unproved assumption that for git storing new file will be heavier (in size) then adding new line into existing text file.
UPD: I searched a little about this and it looks like it's true. Adding a new line to Git is smaller than creating a new file. So I would definitely prefer to have new line in existing files.

@dalexeev
Copy link
Member

dalexeev commented Nov 12, 2024

Also, this probably requires changing the binary tokenizer, since comments are stripped when exporting the project (.gd -> .gdc). But I'm not sure, as I'm not familiar with UID handling after export.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 12, 2024

^
Exported project uses UID cache, so the comments are irrelevant after export (and the .uid files are not exported at all).

EDIT:
Pushed an update.

  • Improved docs
  • Tweaked the comment
  • Added compatibility with UID files

@KoBeWi KoBeWi force-pushed the comment_invasion_again branch from 0475cc5 to 54699c4 Compare November 12, 2024 11:42
@geekley
Copy link

geekley commented Nov 13, 2024

If this is implemented (whether via comment or annotation) I believe people would expect an equivalent solution for .gdshader as well. I don't know how this affects C#, does it need UIDs too?

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 13, 2024

I believe people would expect an equivalent solution for .gdshader as well.

That can be added, but script files are more common.

I don't know how this affects C#, does it need UIDs too?

C# scripts are normal Resources, so they also get .uid files now.

@geekley
Copy link

geekley commented Nov 13, 2024

C# scripts are normal Resources, so they also get .uid files now.

Does that also apply for scripts that aren't referenced anywhere outside the C# code? E.g. custom helper libraries, etc. Or is it only added when needed/referenced?

Also, if this in-file UID is ever implemented for C# scripts (though I imagine it's very unlikely), would the annotation (attribute) implementation make more sense for C#? Since breaking syntax is not an issue in this case?

@@ -1026,6 +1026,10 @@
</member>
<member name="editor/version_control/plugin_name" type="String" setter="" getter="" default="&quot;&quot;">
</member>
<member name="filesystem/gdscript/inline_script_uids" type="bool" setter="" getter="" default="false">
If [code]true[/code], the UIDs of [GDScript] resources will be stored inside the scripts themselves, inside a comment on their first line. UID comments will be automatically created when the scripts are saved. If [code]false[/code], the UIDs are stored in dedicated files. See also [ResourceUID].
Note that if some scripts already have associated UID files, you will need to manually delete them after enabling this setting. The files take priority in determining the script's UID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that if some scripts already have associated UID files, you will need to manually delete them after enabling this setting. The files take priority in determining the script's UID.
[b]Note:[/b] After enabling this setting, if some scripts already have associated UID files they may need to be manually deleted. The files take priority in determining the script's UID.

Also, I get why myself, but this behavior can be seen as perplexing especially given how alienating UIDs are to non-advanced users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the setting is for advanced users.

@Macksaur
Copy link
Contributor

Macksaur commented Nov 14, 2024

I much prefer this over individual .uid files!

Does the text editor hide this magic line? Perhaps an option could be made here (hide/show) for making this more palatable to average users? Then it could replace .uid files.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 14, 2024

Does the text editor hide this magic line?

It doesn't, this can be added later.

@Geometror
Copy link
Member

Geometror commented Dec 27, 2024

I much prefer this over individual .uid files!

+1

I think we all agree that UIDs are valuable and beneficial, but getting the implementation right is challenging, and we haven't quite reached the optimal solution yet.

We recently updated a small/medium-sized project to 4.4-dev7 after which we also did a bit of refactoring, and honestly, working with UID files wasn't/isn't fun - especially when using an external editor. As others already pointed out, it is much easier to mess things up with separate UID files.
If the first approach (storing UIDs directly in the file) was reverted because certain users had strong objections, I'd argue that some of those users might have accepted it had they known this would be the alternative.

This brings me to this PR: I appreciate that it gives users a choice while maintaining reasonable complexity. However, having this as a project setting that defaults to false creates an issue - porting a project to 4.4 initially generates numerous UID files, requiring manual cleanup if you later decide to switch to the in-script approach.

I propose we show a dialog when converting projects to 4.4 where users can choose between the two approaches upfront. I'd also suggest including this choice in the "Create New Project" dialog, allowing users to make an informed decision from the start. If possible, this approach should be implemented at least for GDScript, GDShader and C# files; falling back to UID files for other file types would be reasonable.

@SpockBauru
Copy link

I believe people would expect an equivalent solution for .gdshader as well.

I agree, although shaders may not be as common as scripts, they are still numerous in large projects.

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 29, 2024

I can work on Shader solution once this one is merged.

@larspet
Copy link
Contributor

larspet commented Dec 29, 2024

I agree that this is a better solution than .uid files, with almost no drawbacks if the Script Editor is modified slightly to deal with the UID comment. It doesn't feel out of place either since it is reminiscent of a shebang.

As I see it there are two options for modifying the Script Editor:

  • Fully hide the UID line.
    • Problem: Line numbers now start at 2 which is confusing. Compensating for this by displaying each line number as N-1 would also be confusing and cause cross-referencing issues with stack traces/git etc.
  • Display the line but make it uneditable or unselectable, while also rendering the line as "disabled" (e.g. lower opacity). The "do not modify" comment should also be hidden, as it's meant for external editors. The line could overall look quite different from a normal line, to more clearly represent its purpose as metadata/header.

Out of these two, displaying while also disabling the UID comment gets my vote. It would also be possible to make it so that clicking on the disabled UID line directly copies the UID to clipboard.

For this solution to be feasible however, it requires the UID comment to strictly be on the first line, but this PR seemingly allows it to be on any line. My suggestion is that when retrieving or adding the UID to:

  1. Quickly check if UID is on the first line.
  2. If UID was not found, try to salvage the file by doing a more rigorous search, in case it has been moved to a different line or been indented.
  3. If UID is found after the extra search, move it back to the first line. Also print a message/warning that this was done.

@geekley
Copy link

geekley commented Jan 2, 2025

I still think it's a terrible practice to give significance to regular comments.
If you're going with this comment solution, please at least make it more obviously distinct from regular comments.
If you use a non-comment syntax-coloring for this, there's no need for the bloated "auto-generated" text.

#@@uid: "uid://s0m3rand0mu1d"

Or... you could go with the documentation comment route, since those are already understood as being meaningful:

## @uid: uid://s0m3rand0mu1d

It seems there also the option of a dangling string literal I guess (?) but I don't know how that works and why such strings are even allowed (multiline comments?), so this would be also a bad practice IMO:

"@uid uid://s0m3rand0mu1d"

PS: This mess shows how the annotations system is pretty much useless in GDScript since they are not forward-compatible (unrecognized annotations are a syntax error, unlike other languages like C# and Java) and they don't do anything you couldn't do by adding a new keyword. It's the same problem as #pragma in GDShader, which doesn't follow GLSL/C/C++ semantics of ignoring unrecognized ones, so non-breaking backwards-compatible syntax cannot be introduced, making it pointless.

I think the best way to do this, and define a forward-compatible syntax for this kind of thing, would be through defining at least some "annotation-comment" syntax, with e.g. this pattern (by defining I mean coloring it as a non-comment, and mention in the docs that those kind of comments may have special significance):

#@@annotation_type: "string value"

@SpockBauru
Copy link

I believe that a simple explanation like in the first comment is enough. No need for complex solutions, just better wording.

Option 1: slight modification:
# uid://s0m3rand0mu1d # Used internally by Godot, do not modify or remove.

Option 2: make more clear:

# Used internally by Godot, do NOT change:
# uid://s0m3rand0mu1d

@geekley
Copy link

geekley commented Jan 2, 2025

As long as I can still manually remove the bloated extra text from my code, I guess...

But the best would be if this text could be only in new script file templates, so I don't HAVE to remove it every time I add a script externally (as templates can be edited). That's too much of a hassle, my own code is already too much stuff on screen to look at. I don't need more bloat. New file templates are enough to teach new users that the feature is there and they shouldn't mess with it.

@SpockBauru
Copy link

As long as I can still manually remove the bloated extra text from my code, I guess...

But the best would be if this text could be only in new script file templates, so I don't HAVE to remove it every time I add a script externally (as templates can be edited). That's too much of a hassle, my own code is already too much stuff on screen to look at. I don't need more bloat. New file templates are enough to teach new users that the feature is there and they shouldn't mess with it.

Strongly disagree.

I prefer two extra lines of code a way more than hundreds of new files to handle like is on the current 4.4 dev:
image

@geekley
Copy link

geekley commented Jan 2, 2025

@SpockBauru I think you misunderstand what I said.
I meant the option to add uid comments to scripts (instead of .uid files) shouldn't add the "this is autogenerated, don't remove" text on every uid comment. It should add just the # uid://asdfasdfasdf comment itself.
The # This uid is autogenerated, don't remove: text should be in new script templates only (to teach new users of the feature), so you don't have even more bloat within script files. Obviously I don't want a bunch of .uid files everywhere either.

@Daylily-Zeleen
Copy link
Contributor

Daylily-Zeleen commented Jan 3, 2025

Obviously, C# and other custom script languages are generated ".uid", too. I think we should make all script languages support this feature.

Maybe make it become some virtual methods of Script, ScriptLanguage, or ScriptServer (I'm not familiar with them).

@berarma
Copy link
Contributor

berarma commented Jan 15, 2025

I would have much preferred this approach, at least for Godot files, external scripts can rely on uid files still. Having one more line in a script is much better than having all files duplicated in a scripts directory. And also, you have to remember to manage the uid file when working externally with the scripts.

A variant of this approach would be putting the metadata at the end of the script and optionalyy have the editor parse it and show it as metadata in the dock. When editing files externally you could just ignore metadata at the end. The Vim editor allows putting configuration settings at the end of text files like this.

@Untailygean
Copy link

Are there plans to merge this? I tried 4.4rc1 and I would much rather have one extra line in my scripts instead of the army of tiny files that appeared in my project...

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.

Store script uids in script files instead of generating a seperate .uid file