-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
You mean UIDs I assume. 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. |
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 comments
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.
Suggestion: # uid://s0m3rand0mu1d # Auto-generated, do not modify or remove.
^^ |
Hmm, compatibility is indeed a concern 🤔 |
What about a middle ground? Would it be bad to have like a single file per-folder (e.g. a |
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. {
"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. |
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: |
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. Also I have unproved assumption that for git storing new file will be heavier (in size) then adding new line into existing text file. |
Also, this probably requires changing the binary tokenizer, since comments are stripped when exporting the project ( |
^ EDIT:
|
0475cc5
to
54699c4
Compare
If this is implemented (whether via comment or annotation) I believe people would expect an equivalent solution for |
That can be added, but script files are more common.
C# scripts are normal Resources, so they also get |
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=""""> | |||
</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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
I much prefer this over individual 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 |
It doesn't, this can be added later. |
+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. 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. |
I agree, although shaders may not be as common as scripts, they are still numerous in large projects. |
I can work on Shader solution once this one is merged. |
I agree that this is a better solution than As I see it there are two options for modifying the Script Editor:
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:
|
I still think it's a terrible practice to give significance to regular comments. #@@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 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" |
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: Option 2: make more clear:
|
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 I think you misunderstand what I said. |
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 |
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. |
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... |
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 calledfilesystem/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 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.