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

Remove non-existing global class from cache #86067

Conversation

miv391
Copy link
Contributor

@miv391 miv391 commented Dec 12, 2023

Fixes problems with deleting script files containing global classes.

  • When a script file containg a global class is externally deleted, Godot editor doesn't handle this gracefully. Editor keeps spamming errors, but doesn't fix the situation.
  • When a directory is removed in Godot editor, directory's files are more or less ignored and global classes from those files are not properly removed. Master assumes wrongly that EditorFileSystem's scan will handle the situation, but it doesn't.

Fixes #81867

Test cases

Test project contains two new classes, my_good_sprite_2d.gd located in root and another in bad/my_bad_sprite_2d.gd.

g42_cache_test.zip

  1. Open Godot and test project

  2. Start adding new node by clicking + button in Scene tree.
    --> SUCCESS: MyBadSprite2D is possible node type, no errors

  3. Delete directory "bad"

  4. Start adding new node by clicking + button in Scene tree.
    --> FAIL: You get errors

kuva

  1. Close Godot and delete my_good_sprite_2d.gd from file system
  2. Open Godot and test project
  3. Start adding new node by clicking + button in Scene tree.
    --> FAIL: Now you errors also from MyGoodSprite2D

kuva

The only situation where removing classes from cache file works, is when only files, not directories, are deleted in Godot editor.

Fixes

The reason for the errors is that the global classes made by the user are not properly removed from .godot/global_script_class_cache.cfg.

  • Changes in create_dialog.cpp fix the situation when files are deleted externally. When a non-existing global class is detected, it is removed from the cache. This prevents further errors.

kuva

  • Changes in dependency_editor.cpp fix the situation when a folder containing a class file is deleted in Godot editor. When directories are deleted, master doesn't process each deleted file the same way as single deleted files are processed. Instead it is assumed that EditorFileSystem's scan will handle this, but it doesn't. Scan doesn't notice deleted directories at all. Now for every deleted file, EditorFileSystem's update_file is called and the global class (if file contains one) is deleted from cache file.

@akien-mga akien-mga added this to the 4.3 milestone Dec 12, 2023
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 12, 2023
@miv391 miv391 force-pushed the remove-non-existing-global-class-from-cache branch from d3248d8 to 33c11e6 Compare December 12, 2023 11:03
ScriptServer::save_global_classes();
print_verbose("Removed class " + p_type + " from global classes.");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if CreateDialog should be responsible for cleaning classes..

Comment on lines +605 to 607
String path = OS::get_singleton()->get_resource_dir() + file.replace_first("res://", "/");
print_verbose("Moving to trash: " + path);
Error err = OS::get_singleton()->move_to_trash(path);
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. It makes every file deleted separately when deleting directories.
You should still use files_to_delete for this action.

}

EditorFileSystem::get_singleton()->update_file(file);
Copy link
Member

@KoBeWi KoBeWi Dec 17, 2023

Choose a reason for hiding this comment

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

Is this always necessary? When you delete directory, this should be handled by rescan (like in the old code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then maybe the rescan should be fixed? Currently it doesn't handle delete directories. There is some code to handle deleted directories, but it is never called (at least not when directories are deleted from FileSystem).

Copy link
Member

Choose a reason for hiding this comment

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

Well I was more asking whether this line fixes anything. If there is a case not handled by the above lines then this one could stay.

@akien-mga
Copy link
Member

Superseded by #90186. Thanks for the contribution!

@akien-mga akien-mga closed this Apr 6, 2024
@akien-mga akien-mga added archived and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Apr 6, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Apr 6, 2024
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.

"FIle not found" errors after deleting folder containing GDScript file with custom class_name
4 participants