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

Class Name is lost when creating new one by duplicating existing script #83395

Open
NaleVex opened this issue Oct 15, 2023 · 12 comments
Open

Class Name is lost when creating new one by duplicating existing script #83395

NaleVex opened this issue Oct 15, 2023 · 12 comments

Comments

@NaleVex
Copy link

NaleVex commented Oct 15, 2023

Godot version

4.1.2

System information

Win 10

Issue description

After creating new script by duplicating script with already made custom class_name, that class_name is lost inside godot.

Steps to reproduce

Exists a script with any custom created resource class_name
for example we have this script

extends Resource
class_name NewResource

After creating this script we now can use NewResrource as resource, it's registered inside Godot.
Now if we want to create another class_name by duplicating existing one, new script will have same header with

extends Resource
class_name NewResource

and even if we rename class_name to something else in new script, class_name from original script is lost inside godot.
To register back old class_name we should go back into that script, make some changes to class_name name and save the script again.

Minimal reproduction project

"N/A"

@AThousandShips
Copy link
Member

This is kind of unavoidable, only one script can have a specific class_name, you will need to copy it manually instead

@NaleVex
Copy link
Author

NaleVex commented Oct 15, 2023

This is kind of unavoidable, only one script can have a specific class_name, you will need to copy it manually instead

Indeed, but since new script has class_name changed, class_name for old script should be working back again.
I'd suggest, if something ever happens when in new script there are class_name that has the name that is already in some other older scripts, that name in new script errored and ignored by godot keeping old one working, not removing old one and accepting new instead.

@AThousandShips
Copy link
Member

AThousandShips commented Oct 15, 2023

That would be highly complex and hard to do, see #76380

This would require reloading every other script to test

@NaleVex
Copy link
Author

NaleVex commented Oct 15, 2023

Highly complex is what I just suggested? If whenever new class_name created and that name is already registered, do not unregister that one and error this one instead? It shouldn't require to reload every script to check their name, only that one which is already registered with that same name.

@lostminds
Copy link

I've also run into this a number of times, and it's annoying since duplicating an existing class script feels like a natural way to start making a new similar class. And it's initially surprising that this breaks (at least temporarily) the old class, even if I think I now understand why it happens.

Based on the basic explanation of the system here: #76380 (comment) perhaps a way to handle this would be to allow the system that tracks class_name-script mapping to store multiple scripts per class_name. When compiling this list will be checked, and for any class_names with multiple scripts assigned to it will check these scripts to see if the scripts are still actually referencing the class_name and if the referenced scripts even exist (for move cases). If not, the outdated mapping are removed. If there are still multiple scripts mapped to a single class_name, an error is thrown explaining that multiple scripts (and what scripts) are referencing this class_name and that that's not allowed.

I think this could solve the issue both when duplicating scripts as described above and when moving scripts like in #76380 and as a bonus we could also get a better error message explaining what we've done wrong and how to fix it when there are real duplicate class_name references.

@AThousandShips
Copy link
Member

AThousandShips commented Oct 16, 2023

But that would require you to track old classes with the same name, how would you know which one is valid?

For example, you duplicate a script twice, how should that be handled? How many steps should it track the original one?

@AThousandShips
Copy link
Member

AThousandShips commented Oct 16, 2023

IMO the solution is a warning when duplicating a script like this, including when copying it in the file system, allowing you to not import it and fix it, any automatic system for this has its own drawbacks and quirks that might confuse people if it does things they don't expect

@lostminds
Copy link

But that would require you to track old classes with the same name, how would you know which one is valid?
For example, you duplicate a script twice, how should that be handled? How many steps should it track the original one?

Yes, that is the idea. Instead of replacing the script mapping for class_name (or invalidating it if it's a duplicate), we just add another script mapped to the class_name:

ClassA is mapped to ScriptA
Duplicate ScriptA and rename to ScriptB
ClassA is now mapped to ScriptA and ScriptB
Duplicate ScriptA again and rename to ScriptC
ClassA is now mapped to ScriptA, ScriptB and ScriptC (any number of references allowed, it's just a list)

In this situation we do not know which one is valid or intended, and we do not need to. If we compile this, we get an error explaining this is not allowed. And in this situation it can provide the user good information about what scripts are causing the issue.

However, if we have changed ScriptB so it now references ClassB and ScriptC so it now references ClassC and compile, the system can update. It can see that ClassA looks invalid with multiple mapped scripts: ScriptA, ScriptB and ScriptC. But crucially it can then also check these mapped scripts and discover that ScriptB and ScriptC are no longer referencing ClassA, and therefore remove these mappings from ClassA. So now the system again only has one script for ClassA: ScriptA and this is a valid class again. In other words, it would allow the class script mapping system to recover automatically once the user has fixed the duplicate reference issue.

@AThousandShips
Copy link
Member

As I said I don't think that's a good idea because it makes for a lot of complexity where none is needed IMO

@RobRuana
Copy link

RobRuana commented Jan 25, 2024

The behavior in 3 was to keep the mapping to the original script file, and only add a mapping for the duplicated script when class_name is updated. Seems like the old behavior was both simple and what the user would expect: if a mapping already exists to a valid script file, then don't update the mapping to the duplicate script file.

btw, when this happens to me, I solve it by manually editing .godot/global_script_class_cache.cfg after I've updated class_name in the duplicated file.

@lostminds
Copy link

The behavior in 3 was to keep the mapping to the original script file, and only add a mapping for the duplicated script when class_name is updated.

If it was possible to restore this functionality and just not break the existing mapping if a duplicate one was added it would solve most of the issues I'm running into. Typically I'm duplicating a file with a class_name definition to make a new similar class (usually a new subclass of the same parent class), and this breaks the mapping of the original class since the file duplication meant that for a time there were two scripts with that class_name definition and changing just the one in the new script doesn't automatically fix the issue.

@daenvil
Copy link

daenvil commented Mar 8, 2024

I just had this bug crash my whole project again. Duplicating an existing class and renaming it seems like a totally normal workflow and it's weird that Godot gives problems with this. IMO ignoring the issue or just throwing a warning to the user are not appropriate solutions. The logical thing would be for the new duplicated class to throw a "redefinition of class" error, instead of unregistering the old one which was likely to be already in use.

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

5 participants