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

Hide @uid annotation in script editor #8916

Closed
timothyqiu opened this issue Jan 20, 2024 · 23 comments
Closed

Hide @uid annotation in script editor #8916

timothyqiu opened this issue Jan 20, 2024 · 23 comments

Comments

@timothyqiu
Copy link
Member

timothyqiu commented Jan 20, 2024

Describe the project you are working on

Trying out latest Godot state.

Describe the problem or limitation you are having in your project

godotengine/godot#67132 introduced the @uid annotation.

Every GDScript file now has an auto-generated @uid line, and it should not be modified manually.

It makes GDScript so ugly and distracting.

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

Hide the @uid line in the script editor.

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

  • When opening a GDScript file:
    • Remember the UID if @uid exists, then remove that line in the editor.
  • When using the GDScript file (saving, linting, etc):
    • If @uid exists in the editor version, use that as the UID
    • Otherwise, if there is remembered UID, use that as the UID
    • Otherwise, generate the UID as usual
    • Prepend the @uid line to the actual file.

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

  • Probably, if only loading & saving is involved. I'm not sure how linting works.
  • It's a UX problem.

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

Not in core, but an editor UX feature.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 20, 2024

If you delete the line in the editor, the script you edit and the script in running project will have different line numbers, which will break debugging.

@arkology
Copy link

arkology commented Jan 20, 2024

Personal opinion:
I never move scripts outside editor (something wrong with me?) so this uids do nothing for me in terms of functionality. Moreover every single script file will be polluted by uids which is really annoying. I just look at uid string and feel something not good. It really makes GDScript ugly and distracting. Godot philosophy "add only stuff that most users will benefit" is broken here.
I think uids generation inside scripts should be optional and disabled by default.

@AThousandShips
Copy link
Member

First of all please don't add an opinion you know is unconstructive 🙃 what's the point really?

Second a single line doesn't "It really makes GDScript ugly and distracting." please be reasonable

And it does help people if you want resources to be tracked when moved, and it's a single line, can it be distracting? Sure, but it's worth the improvement it brings

Just because you don't use it doesn't mean it's useless...

@KoBeWi
Copy link
Member

KoBeWi commented Jan 20, 2024

Godot philosophy "add only stuff that most users will benefit" is broken here.

Is it?
GUID support has been requested since forever: godotengine/godot#15673
The implementation PR had overwhelmingly positive reception: godotengine/godot#50786
Also the script UID proposal and its GDScript implementation PR have much more upvotes than downvotes. So your statement does not really hold truth, unless you consider yourself to be "most users".

Also script UIDs aren't only for moving stuff around outside the editor. You can extend or preload a script using its UID and it won't break when you move the file around. It's especially relevant for addons, where registering a class for every script is not really viable.

That said, adding a project setting to disable script UIDs would be trivial, but they should be enabled by default. Seems like there is a related proposal, but more general: #7195

@ninstar
Copy link

ninstar commented Jan 20, 2024

I think it having its own syntax highlight could help making it less distracting, something similar to comments.

@dalexeev
Copy link
Member

Hide the @uid line in the script editor.

Personally, I don't like when text editors implicitly hide content or add elements that do not exist in the text. It's probably ok in this case, but users will ask why line numbers start with 2. :)

I conceptually don't like having machine-readable data in human-editable files. This is differs from other programming languages and will disappoint many users when first encountering the change. But I think the problem is exaggerated, one line won't be that distracting and annoying once you get used to it. Of course, for existing projects it will take time for UIDs to be assigned to all files or need to apply an editor script to do it at once.

I never move scripts outside editor (something wrong with me?) so this uids do nothing for me in terms of functionality.

Me too, usually. However, when doing massive refactoring, I often close the Godot editor and use search and replace and sometimes scripts, since Godot does not auto-replace some things (like nodes in inherited scenes), plus the editor has bugs and crashes when moving/renaming a lot of files.

Personally, I have little need for UIDs, but many users don't have advanced regexp skills and sometimes don't even realize why moving resources in external file manager can break project. So the problem exists for many users, and they would probably be happy to get a solution at the cost of an ugly line. I'd like to see a setting proposed in #7195, but UID should apparently be enabled by default.

@timothyqiu
Copy link
Member Author

I agree UID is useful, but the proposal is about the @uid annotation.

The first time I see it, it reminds me of the typical C++ hello world where the code looks simple but it uses advanced elements like operator overloading which is actually difficult to explain to beginners.

Let's create your first GDScript. The first line is an annotation. It's auto-generated, and helps keep things from breaking apart if you need to move the script file outside the editor. You can't modify or remove this line, as it will be regenerated automatically. So we can simply ignore this line for now.

  • It makes beginners think GDScript is so complex. Even the first line is something they don't understand.
  • For video tutorials, users will also wonder why their UID isn't the same as the one in the video. Should they correct it according to the UID shown in the tutorial? Why is the editor keep reseting my modification? Is the tutorials wrong, again? Is it a bug of Godot editor?
  • For text tutorials, authors tend to show the code snippet for easy copy-pasting. Then should they include the @uid annotation? Readers will also face the same confusion as those above.

Sure, it will be nothing "once you get used to it". But my first point is about beginners and teaching.

My second point is that this annotation is not intended for the user at all. The "argument" to @uid is always decided by the system. It'll get reset even only a letter is changed. It's metadata generated by machine, for the machine. Then why showing it to the user in the first place?

At least, metadata like this is usually a comment like shebang and vim modeline. It's natual to ignore generated comments.

Off-topic thinking I remember there are also issues about GDScript can't save data put in the Inspector's metadata fields. It's because there are no `.import` file for it. I think a `.import` file is a better solution than `@uid`.

@dalexeev
Copy link
Member

There is a potential problem with hiding UID lines in that if there is an error (due to change via external editor, wrong merge, etc.) we may end up with a situation where GDScript reports an error in @uid (duplicated annotation or invalid argument), but user cannot correct it via built-in editor.

At least, metadata like this is usually a comment like shebang and vim modeline. It's natual to ignore generated comments.

Personally, I also preferred it to be a comment line rather than an annotation (plus it's more portable). However, wouldn't you still have to explain the line to newbies, albeit with less emphasis?

I think a .import file is a better solution than @uid.

In theory, the solution is more correct, since we use .import files for external formats like PNG. But this would cause file system clutter, which users would probably dislike even more. Since assets are usually stored in a separate folder and edited much less frequently than scripts and scenes.

@Mickeon
Copy link

Mickeon commented Jan 21, 2024

Would you look at that: the exact issue I believed was going to happen in RocketChat, summarized nicely by Timothyqiu, did happen. I think the most important getaway from all of this is, this should first be a project setting. Now, what should be the default is still up to debate.

Personally, if I was new to the engine and all my scripts started with a line I should explicitly not touch I would be reminded of other, more complex languages. For GDScript users that just wanted to upgrade version, this feels alienating as well.

@Calinou
Copy link
Member

Calinou commented Jan 22, 2024

I think in the long term, we'll need a system that automatically collapses the comment at the top of the file if it's longer than 4 lines (for license headers). This could include the UID annotation located just below, even if it's not a comment.

@Mickeon
Copy link

Mickeon commented Jan 22, 2024

I'd rather have UIDs be opt-in. But some way to not keep them intrusive, while making it obvious that a line is being folded there would be a decent compromise. Perhaps with its own icon.

@JoNax97
Copy link

JoNax97 commented Jan 23, 2024

I believe it's a mistake to mix auto generated metadata in a user-editable file. Even if they are native to the engine, gdscript files should be treated as imported files for the purpose of metadata tracking.

It's a bit ugly and a bit confusing now, but what will happen in the future if the need arises to add more metadata? Will you just keep piling annotations and hope the users don't break things?

There is still time to move this into a .import file and avoid falling down this slope. Unity got this right with .meta files, they can do whatever they want with them, they are always available no matter the file type, and users got used to the simple rule of "always move the meta along the source file".

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 23, 2024

While we do have .import files in user projects, they are not suitable for this purpose. Their job is to store import configuration, and that's the expectation that comes with them. Adding a .import file for a resource which doesn't need to be imported, containing information that is not related to any import configuration, is a bad and ugly hack that will ripple across our core codebase and into user projects as well.

If a secondary file is the only viable solution, it should be a new kind of file. We can't repurpose .import files for this.

@JoNax97
Copy link

JoNax97 commented Jan 23, 2024

Call the file whatever you want, that's unimportant. I suggested repurposing the .import files as they are the closest to an "accessory file" Godot has.

If a new file type is the way to go, then by all means do it. But I will argue that this is a decision that should be made before 4.3 ships and it gets set in stone.

That said, I will argue that is a good idea (maybe not now, but for 5.0) to have a general purpose file that supersedes the .import file, to store metadata related to any file in the project, be it import configuration, uids or folder colors.

@poohcom1
Copy link

poohcom1 commented Jan 29, 2024

Something else we could also do alongside (or instead of) a global project setting is adding a button somewhere to manually generate the uid annotation/attribute for the selected file(s). I think it can work well as a file context menu option or a new button in the import tab for script files.

Or better yet, allow users to just add a blank @uid or [Uid()] annotation, and the engine will automatically detect and fill it in. That way, users who wants to use it can opt-in from the script itself without having to touch the editor.

This makes it more flexible as it's opt-in on a per file basis and works for scripts created outside of the editor (like most C# scripts) or existing scripts without uids.

@slyisdreaming
Copy link

A note from a newcomer. I just started to learn Godot and I'm working on a pet project. Accidentally ran a custom build and I was shocked to see the uid line. I thought that maybe I broke something. I had a corrupted scene a few minutes ago so I thought maybe Godot tries to recover it. I have never seen "do not remove" warnings in other languages so that line looks completely alien to me. I guess I can deal with it but now I feel like my code doesn't belong to me anymore. There is something in the code that I should care that I don't want or need to care about. What if I remove that line will it break my project? What if I copy code from other file will it break something? I would prefer not to keep such things in my head if I don't need them.

@ninstar
Copy link

ninstar commented Feb 3, 2024

To be honest a separate file for handling this is the most logical solution in the long term, as others already suggested. I still think a different highlight color can help until a new file (or other solution) is implemented.

@arkology
Copy link

arkology commented Feb 3, 2024

After some time, I think that having separate file with only uid would be overkill and as bad as having uid in script directly (or maybe even much worse because of files amount will increase x2 times and users will have to keep in mind that they have to move not only script but also one more file with uid).
Right now if I have to select between separate file or uid annotation I will definitely choose uid annotation.
But it should be more user-friendly: should be possible to disable, have less limitations (like previous "only on first line of the script"), more option to generate and delete uid from user interface, more permissible: handle without error if it was deleted accidentally or intentionally, documentation should be very descriptive to make users understand why it exists and how to properly work with it.
I think merging script uid pr was a bit early and users and actual implementation (in terms of mentioned above reasons) was not quite ready that's why it was reverted. But after making script uid more "user-friendly" I think many of those how against it right now will be ok with it.
Conclusion: Currently uid annotation is my candidate, but there is some significant amount of work to make it suitable for everyone.

@Mickeon
Copy link

Mickeon commented Feb 3, 2024

For your interest, as of writing this, Script UIDs have been reverted but are still planned for 4.3.
The current agreement is that they should be opt-in within the "New Script" panel. Now, the way these UIDs should be included in the script is another question entirely.

@theraot
Copy link

theraot commented Jun 24, 2024

If uids are to be optional... Meaning that if a script does not have an uid Godot will continue using a path...


Addendum: I made this into a proposal: #10029

I submit to your consideration: Let the resource format use script class names.

Using class_name is already optional, and refers uniquely to the script, the means to store and keep track of it are already in place.

There are a few possible concerns I can think of:

  • What if the developer removes or changes the name of the class?

    Not that in the case of removal, the same issue goes for generated uids stored on the script (yes, I know that the reverted implementation generated on save, but currently you are saying they should be optional, so removing it is on the table).

    In both cases Godot already has to update the global class list, so this is not new.

    Thus Godot would need code hooked to the change so it can propagate it to where it is used.

    Consider also that - in the long run - a class rename refactor could piggyback on this. For example, when the developer changes the name of the class Godot could offer to also change the name in other scripts.

  • What about the scripts that don't have a class name?

    Note that given that uids would be optional, uids would also have this issue.

    For that, let this happen instead: Add an option to save scene dependencies paths as relative to the scene #2327 (which still have a very good reception, being the only drawback "uids solved this problem"... But here we are). Relative path would allow folders to be moved together with no issue, even between projects, it is useful for addons that do not want to pollute the global scope, will not generate merge conflicts, nor id collisions.

  • What about backwards compatibility?

    If you use the class name nothing new is added to scripts so they have no issue.

    When a scene saved with a newer version is moved to a prior version of Godot: The relative path would work, as Godot can already read these (it does not save like that, but it can read them).

    Thus the issue is the type. For that do not replace the path with the type. Instead the type should be in a separate field.

    Of course resources should continue to support to full paths.

  • What if I move these scene and scripts to another project (which does not have the same global class list)?

    First of all, Godot would add the scripts to its global class list, so that part is not an issue. But even if it were, Godot should fallback to use the path.


I am, of course, only submitting this to your consideration. If you do not agree, I hope at least I gave you another perspective into this issue.

@Mickeon
Copy link

Mickeon commented Jun 24, 2024

There's a huge problem with this suggestion currently. That is, class_name is a misleading keyword and what it actually does is make the script global, globally accessible, available for selection, and always loaded at all times from the moment the application starts. On large projects, excessive class_name usage can all become unwieldy and generally unpleasant.

There have been several proposals attempting to address this but none have been implemented and class_name on the outside has been the same as it was in 3.x.

@theraot
Copy link

theraot commented Jun 25, 2024

@Mickeon I undertand that class_name does more than it needs to.

With that said, I'll reiterate that uids would be opt-in, and class_name is optional already.

So I'm NOT suggesting that everything should have a class_name.

In fact, I'm pointing to #2327 for most other cases. And the roadblock for that, as far as I can tell, is that some of you wanted uids.

Thus, I believe that if you don't think about uids for scripts, this makes sense.


Now, I'll submit something else to your consideration: model how uids work with scripts on the parts of class_name that would be useful for it.

Let us think about it, class_name already works, so you can base the uid implementation on it... Based on that, if we are going to have an uid added to the script file, we can let the developer set a meaningful name... But there is already a way to give it a meaningful name (class_name), so perhaps it can be an annotation that if left empty uses the class_name.

If we are considering a new annotation that might be used to give a name to a script when it does not have class_name, we might also have found a way to slice the functionality of class_name without changing its functionality (i.e. if you want all the features you use class_name, and a subset might be available for scripts without class_name but with the new annotation), this way there is no broken compatibility.

How about that?

Addendum: I recognize this new idea is less developed, I might revisit it later and put it more concrete terms.

@timothyqiu
Copy link
Member Author

Closing as @uid is reverted and not part of 4.3.

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