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

Fix global scripts not being added because of bad global_script_class_cache.cfg #102636

Merged

Conversation

amarsero
Copy link
Contributor

@amarsero amarsero commented Feb 9, 2025

Fixes #102557, probably also fixes #102568

The Issue:
A commit made "is_tool" and "is_abstract" mandatory to add new global classes to ScriptServer::global_classes. But global_script_class_cache.cfg from projects before 4.4 lack these properties, so these classes won't be added, causing errors when running the game.

Current Solution:
After the editor scans all the scripts for the first time, now forcibly updates global_script_class_cache.cfg, resolving the issue.
However, running a 4.3 game with 4.4 without first opening it with the editor at least once still fails.

I've tried to fix this in ScriptServer::init_languages, where the global classes are first loaded, but couldn't figure it out.

@AThousandShips AThousandShips added this to the 4.4 milestone Feb 10, 2025
@AThousandShips AThousandShips requested a review from a team February 10, 2025 08:47
@akien-mga akien-mga changed the title Fix global scripts not being added because of bad global_script_class_cache.cfg Fix global scripts not being added because of bad global_script_class_cache.cfg Feb 10, 2025
@akien-mga
Copy link
Member

Some quick thoughts, but @RandomShaper and @Hilderin should be able to assess this further:

  • This seems to remove an optimization (only update when it's need). Would be good to assess whether this will lead to a lot more updates than necessary and possibly a performance impact, or possible side effects of writing an unchanged file to disk too frequently (Windows can be touchy there). If yes, the alternative could be to figure out how to better check whether an update is needed, instead of always updating.
  • Another approach could be to make the new is_tool and is_abstract not mandatory, and treat those as false if they're missing. Or a third bool option ("unset") but that would require another variable or changing to an int internally, that's a bit hacky.

I'm not necessarily suggesting to implement any of the above, it's just food for thought. The proposed solution here might totally be the best one already.

@amarsero
Copy link
Contributor Author

Thanks for the thoughts!

About removing the optimization, i believe the method is called only once at the start of the editor. Only on the first_scan of the EditorFileSystem. So there should be no big performance impact.

And i thought about making them not mandatory, but tool scripts were added on 4.3, so any tool script migrating to 4.4 might break if we default to false.

The problem is not so much the editor, since it refreshes all the scripts at startup, but the games don't work when global_script_class_cache.cfg is bad.

@Hilderin
Copy link
Contributor

Hilderin commented Feb 11, 2025

After looking at the issue a bit, I found 2 problems.

  1. When _first_scan_process_scripts calls _register_global_class_script, the following lines are never called:
ScriptServer::save_global_classes();
EditorNode::get_editor_data().script_class_save_icon_paths();

That causes the global_script_class_cache.cfg to not be saved on first scan. I tested by modifying the _first_scan_process_scripts to return true when a new global script is registered and added the call to these 2 lines when it results true.

  1. The most problematic issue is that the UIDUpgradeTool::get_singleton()->finish_upgrade is done before the first scan is done. Since the global cache is invalid and because the first scan was skipped, no global classe are registered during the upgrade process, causing all the scripts processed in the update to be invalid. I fixed that by executing the upgrade after the first scan. Note: I'm kept the scan on upgrade_finished just in case, but I'm pretty sure it's useless since the first scan was already executed and that the upgrade only resaves resources which should trigger the EditorFileSystem to update directly.

Another minor modif: I added a break in EditorFileSystem::_get_global_script_class to prevent looping for nothing.

I created a branch with my modified test code to help you if you want: master...Hilderin:godot:fix-global-class-names-not-save-with-upgrade-tool

@amarsero amarsero force-pushed the bad-global-script-class-cache branch 2 times, most recently from 58f5661 to ee4867a Compare February 11, 2025 17:46
@amarsero amarsero requested a review from a team as a code owner February 11, 2025 17:46
@amarsero
Copy link
Contributor Author

Thanks for the help Hilderin!

Now if a global class is found or removed on the first scan, global_script_class_cache.cfg is updated.
This is to ensure migrations from earlier versions have all the needed properties.

Also i noticed that every time that EditorData::script_class_save_icon_paths was called, ScriptServer::save_global_classes was called immediately before. And both methods changed and saved global_script_class_cache.cfg. To avoid writing the file twice I took the liberty of unifying both methods into script_class_save_icon_paths (but without modifying save_global_classes).

@akien-mga akien-mga requested a review from KoBeWi February 11, 2025 22:18
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Notwithstanding other comments needing to be addressed, this makes sense to me.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 12, 2025

script_class_save_icon_paths() seems to be called as often as before. Now it updates all class properties, while previously it would load class cache, modify one property of each class and save it too, so not much changed on that regard. We could better optimize class cache saving, but this PR does not make it worse.

Co-authored-by: Hilderin <81109165+Hilderin@users.noreply.github.com>
@amarsero amarsero force-pushed the bad-global-script-class-cache branch from ee4867a to 2ba64a5 Compare February 12, 2025 17:42
@Repiteo Repiteo requested a review from Hilderin February 13, 2025 14:05
Copy link
Contributor

@Hilderin Hilderin left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected, fixes the issue and even prevents some potential out of sync of the global cache file on some limit cases or in the future if other props are added. Good job!

@akien-mga akien-mga merged commit 1d591bd into godotengine:master Feb 13, 2025
20 checks passed
@akien-mga
Copy link
Member

akien-mga commented Feb 13, 2025

Thanks a ton! This solves some of the remaining release blockers for 4.4, so that's a huge contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants