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 adding a translation CSV results in errors on initial import for many types of resources #93919

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Jul 4, 2024

Fixes #93704

@mihe was right, #92320 did not fix the problem. In fact, there were two problems with CSV importation, and #92320 solved only one of them.

The remaining problem was caused by the creation of the translation file in res://, which triggered update_files and then _update_pending_scene_groups. This caused _update_pending_scene_groups to load all the scenes during the first scan before all the resources were imported. On the first scan, without the .godot folder, all the scenes are processed. _update_pending_scene_groups must be called after all resources are loaded.

At first, I tested only adding !first_scan condition before _process_update_pending. It worked when starting the MRP project without the .godot folder. But if I started an empty project and pasted the CSV file and a scene with resources at the same time, the same problem occurred. So the fix is to never call _process_update_pending while scanning. Anway, it should be called at the end of the scanning, once all the resources are loaded.

Note: I think #93723 could also fix this problem because with these modifications, scene are not loaded anymore to find the groups.

Tested with the MRP from #93704 and #83200.

@mihe
Copy link
Contributor

mihe commented Jul 4, 2024

I can confirm that this does fix #93704, and as far as I can tell _process_update_pending and the filesystem_changed signal are both called once the importing is finished, so the change looks reasonable, assuming _update_file_icon_path isn't dependent on either of them in some weird way.

@akien-mga akien-mga requested a review from KoBeWi July 4, 2024 10:32
@akien-mga akien-mga changed the title Fix adding a translation CSV results in errors on initial import for … Fix adding a translation CSV results in errors on initial import for many types of resources Jul 4, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, and doesn't seem to introduce regressions in some quick testing on projects with CSV translations.

call_deferred(SNAME("emit_signal"), "filesystem_changed"); //update later
if (!is_scanning()) {
_process_update_pending();
call_deferred(SNAME("emit_signal"), "filesystem_changed"); //update later
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
call_deferred(SNAME("emit_signal"), "filesystem_changed"); //update later
call_deferred(SNAME("emit_signal"), "filesystem_changed"); // Update later.

Copy link
Member

Choose a reason for hiding this comment

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

Forgot about this before merging. That will be for the next PR in that code :D

@akien-mga akien-mga merged commit 0452cbe into godotengine:master Jul 4, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Adding a translation CSV results in errors on initial import for many types of resources
6 participants