viniciusgerevini / godot-aseprite-wizard

Godot Editor plugin to help import Aseprite animations to AnimationPlayers, AnimatedSprites and SpriteFrames.
MIT License
887 stars 42 forks source link

Godot crashes when total number of Aseprite files exceeds 200. #157

Open 0Volcanon opened 4 months ago

0Volcanon commented 4 months ago

Versions

v4.2.2 v4.3.beta.2

System Information

Windows 10 Pop OS! 22.04

Issue description

When importing a large number of files, the editor crashes. The behavior is different on Windows and Linux. The editor doesn't crash if you change import type one file at a time. On Windows, the project cannot be re-opened as it always crashes on import. On Linux, the editor crashes initially and not after. But when files exceed 1000, it sometimes freezes instead of crashing. I don't think this is a Godot issue as it only occurs when the addon is enabled. Also, the editor keeps re-importing all assets when the addon is enabled.

Steps to reproduce

Select a lot of Aseprite files and change the import type to Aseprite Texture.

Minimal reproduction project (MRP)

aseprite-wizard-test.zip

KeyboardDanni commented 3 months ago

Seems I ran into this or a similar issue trying to migrate my 4.2 project to 4.3 rc1. Because of the need to reimport the resources, I'm getting crashes whenever I try to open my project.

I occasionally see these errors in console (but not always):

ERROR: Condition "f.is_null()" is true. Continuing.
   at: get_multiple_md5 (core/io/file_access.cpp:812)
ERROR: Condition "p_enabled && addon_name_to_plugin.has(addon_path)" is true.
   at: set_addon_plugin_enabled (editor/editor_node.cpp:3621)
ERROR: Caller thread can't call this function in this node (/root/@EditorNode@16886/@EditorFileSystem@3). Use call_deferred() or call_thread_group() instead.
   at: (scene/main/node.cpp:942)

When it crashes, it leaves behind temporary files (.json and .import~*.TMP).

I have way less than 200 aseprite files (I only have about six that are set to import as Aseprite Texture).

KeyboardDanni commented 3 months ago

Seems I can work around this issue by deleting the .godot folder in my project, but it's definitely not ideal.

viniciusgerevini commented 3 months ago

Hello. Thanks for reporting this. I ran some tests last week and the process does break when importing multiple, but my machine doesn't get to the extremes of crashing Godot or putting it in a unrecoverable state (yet). But even without the fatal crash I think the issue I see is the equivalent to what you are reporting.

Not sure how to solve this yet. The issue is because of a scan required in the importer to get the newest files. As the PNG file is created by an external program, the importer is forced to trigger a scan to get the newest resource. The append_import_external_resource that was supposed to workaround this doesn't seem to work with existing files. Your test kinda supports that @KeyboardDanni , because the scan is only required when the file hasn't been imported before.

I've tried removing that scan multiple times in the past, but Godot is not happy with that. I'll check if there is any other workaround.

KeyboardDanni commented 3 months ago

Do you think this could be a threading issue? It does seem to be somewhat temperamental, and the imported/leftover files and console-printed messages are different every time. The ERROR: Caller thread can't call this function in this node message also seems to hint at this.

thyancey commented 3 months ago

I'm having similar issues, most often seeing the "f.is_null()" in errors. My project is using ~68 .aseprite files, most imported as SpriteFrames. Godot is importing assets, then often crashing on launch. Occasionally it will actually load in, but my console is full of errors. In addition, every time I click away from godot and back to the window, it attempts to (Re) Import all the assets again, with errors.

I've tried reimporting all my .aseprite files, deleting the .godot folder, deleting all of the .import files. I can eventually make some progress where my project will build with images, but with asset errors everytime the game closes. Upon relaunching, many of the assets are in an error state again. Here is an error log, where "seagull/shadow.aseprite" is indeed in the project, and even imported and showing properly in the editor.

For some extra context, my project was using Aseprite Wizard 7.4.0 on Godot 4.2 before I attempted to upgrade to 4.3. After upgrading godot, I had deleted the addons/AsepriteWizard directory and downloaded version 7.5.0

Windows 11, Godot 4.3, Aseprite Wizard 7.5.0.

Godot Engine v4.3.stable.official (c) 2007-present Juan Linietsky, Ariel Manzur & Godot Contributors. --- Debug adapter server started on port 6006 --- --- GDScript language server started on port 6005 --- core/io/resource_uid.cpp:132 - Condition "!unique_ids.has(p_id)" is true. Returning: String() core/io/resource_uid.cpp:132 - Condition "!unique_ids.has(p_id)" is true. Returning: String() Can't find file 'uid://c0v610eu5jru3'. Can't find file 'uid://8uob8ajo1fi2'. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. editor/editor_node.cpp:3627 - Condition "p_enabled && addon_name_to_plugin.has(addon_path)" is true. editor/editor_node.cpp:3627 - Condition "p_enabled && addon_name_to_plugin.has(addon_path)" is true. editor/editor_file_system.cpp:866 - Condition "thread.is_started()" is true. Can't find file 'res://src/entities/seagull/shadow.aseprite'.

thyancey commented 3 months ago

I'm having similar issues, most often seeing the "f.is_null()" in errors. My project is using ~68 .aseprite files, most imported as SpriteFrames. Godot is importing assets, then often crashing on launch. Occasionally it will actually load in, but my console is full of errors. In addition, every time I click away from godot and back to the window, it attempts to (Re) Import all the assets again, with errors.

I've tried reimporting all my .aseprite files, deleting the .godot folder, deleting all of the .import files. I can eventually make some progress where my project will build with images, but with asset errors everytime the game closes. Upon relaunching, many of the assets are in an error state again. Here is an error log, where "seagull/shadow.aseprite" is indeed in the project, and even imported and showing properly in the editor.

For some extra context, my project was using Aseprite Wizard 7.4.0 on Godot 4.2 before I attempted to upgrade to 4.3. After upgrading godot, I had deleted the addons/AsepriteWizard directory and downloaded version 7.5.0

Windows 11, Godot 4.3, Aseprite Wizard 7.5.0.

Godot Engine v4.3.stable.official (c) 2007-present Juan Linietsky, Ariel Manzur & Godot Contributors. --- Debug adapter server started on port 6006 --- --- GDScript language server started on port 6005 --- core/io/resource_uid.cpp:132 - Condition "!unique_ids.has(p_id)" is true. Returning: String() core/io/resource_uid.cpp:132 - Condition "!unique_ids.has(p_id)" is true. Returning: String() Can't find file 'uid://c0v610eu5jru3'. Can't find file 'uid://8uob8ajo1fi2'. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing. editor/editor_node.cpp:3627 - Condition "p_enabled && addon_name_to_plugin.has(addon_path)" is true. editor/editor_node.cpp:3627 - Condition "p_enabled && addon_name_to_plugin.has(addon_path)" is true. editor/editor_file_system.cpp:866 - Condition "thread.is_started()" is true. Can't find file 'res://src/entities/seagull/shadow.aseprite'.

I've been trying many things for several days, but after reading some comments here, I was able to get to a stable state with my project by:

  1. deleting the .godot folder
  2. relaunching godot over and over again - sometimes with it crashing, sometimes with it erroring / showing broken references on startup. occasionally building a dev version of the game when the editor starts up, but then closing godot and relaunching again.
  3. after about 20 minutes of doing this, it seemed to finally work though all the kinks. Sorry if that wasn't very procedural, but that's what worked for me.

When looking at my .git history, the files that were ultimately modified after all the issues worked out was the uid values in several .import files. If I pull a fresh copy of my project from .git in a new folder, the cycle starts all over again, as expected. Should I be committing my .import files to git? They seem to be tied to cached resources on my machine.

viniciusgerevini commented 3 months ago

Do you think this could be a threading issue? @KeyboardDanni not sure. If it is it's in Godot and not in the importer logic as we have no control over it. But also sometimes the error messages from Godot can be misleading

Thanks for the steps @thyancey . In my tests I've also seen this behaviour. On every attempt a few imports succeed until they all passed.

I still haven't been able to pin down what exactly is triggering the failures. Even removing all the scan references doesn't solve the issue. I'm starting to suspect it's just the import system that doesn't handle this amount of imports. That is far fetched because native imports (textures, etc) work, but maybe it's something with custom imports. I'll try to implement a custom bare bones import to see if I can get it to break.

viniciusgerevini commented 3 months ago

Alright! I think I had some progress removing some scans and using threaded loads. Still need to run some more tests and I only tested it for the texture importer, I still need to confirm it works for spritesheets :crossed_fingers:

viniciusgerevini commented 3 months ago

I have a fix in this branch, in case you want to test it out. https://github.com/viniciusgerevini/godot-aseprite-wizard/tree/fix_bulk_import

I've only tested it in Linux so far, but I hope it works the same in the other OS

thyancey commented 3 months ago

I have a fix in this branch, in case you want to test it out. https://github.com/viniciusgerevini/godot-aseprite-wizard/tree/fix_bulk_import

I've only tested it in Linux so far, but I hope it works the same in the other OS

I pulled the branch and that seemed to help out a lot! I wasn't having any more bulk import popups and errors on load, or when I switch to/from the window. Definitely seems to have fixed a few things

I am however still seeing another issue which might be related, which I didn't mention before - assuming it was all related.

When I Reimport an .aseprite file (using Aseprite SpriteFrames), another feedback loop happens, where a prompt saying "(Re)Importing Assets" appears over and over again for the same image.

Occasionally it will push the same error to the console: "Condition "f.is_null()" is true. Continuing. core/io/file_access.cpp:812 - Condition "f.is_null()" is true. Continuing."

Eventually it gives up after 20 or so times.

Windows 11, Godot 4.3, and this branch https://github.com/viniciusgerevini/godot-aseprite-wizard/tree/fix_bulk_import

Attached a screen recording if it helps

https://github.com/user-attachments/assets/f424a964-0e1a-4519-b12f-16b1928be734

0Volcanon commented 3 months ago

The fix branch does indeed fix the crashing but I'm now getting this error when running on a clean project. I don't think it's an addon problem since it doesn't appear on other projects, only new ones. I didn't test on Windows since I no longer use it, but it works for Pop OS. image

viniciusgerevini commented 3 months ago

When I Reimport an .aseprite file (using Aseprite SpriteFrames), another feedback loop happens, where a prompt saying "(Re)Importing Assets" appears over and over again for the same image.

@thyancey Thanks for sharing the screen capture. Does this happen on every import or randomly?

The plugin does trigger another scan after finishing the imports to update the new png files, but if the import was successful Godot would have skipped that file instead of going over and over.

The file access warning is also weird. I think the only place were the file access is used directly in this importer is when reading the json file from the disk. But that would have triggered a few other warnings as well. I suspect this warning might be a side effect coming from somewhere else.

If you have time to run a test, you can add this:

    if not FileAccess.file_exists(source_file):
        return result_code.error(result_code.ERR_INVALID_ASEPRITE_SPRITESHEET)

To the load_json_content in the file_exporter.gd. It should be the first line in the method. This won't necessarily solve the issue, but if that's the place causing the warning it would go away giving us a hint why this import is failing.

thyancey commented 3 months ago

When I Reimport an .aseprite file (using Aseprite SpriteFrames), another feedback loop happens, where a prompt saying "(Re)Importing Assets" appears over and over again for the same image.

@thyancey Thanks for sharing the screen capture. Does this happen on every import or randomly?

The plugin does trigger another scan after finishing the imports to update the new png files, but if the import was successful Godot would have skipped that file instead of going over and over.

The file access warning is also weird. I think the only place were the file access is used directly in this importer is when reading the json file from the disk. But that would have triggered a few other warnings as well. I suspect this warning might be a side effect coming from somewhere else.

If you have time to run a test, you can add this:

  if not FileAccess.file_exists(source_file):
      return result_code.error(result_code.ERR_INVALID_ASEPRITE_SPRITESHEET)

To the load_json_content in the file_exporter.gd. It should be the first line in the method. This won't necessarily solve the issue, but if that's the place causing the warning it would go away giving us a hint why this import is failing.

I've had a few different experiences with this today. At first I was still seeing the issue, even with your code, however after pulling the latest from the godot_4 branch, it sees to be all fixed up!

When trying to exercise the bug again, occasionally after importing my sprite is totally blank, even though sprite frames are populated. It's like this in the editor and in my compiled game. However when trying to reproduce this issue reliably, I was having inconsistent results, so I'll try to rule out some things on my end.

Thanks so much, this plugin is super helpful

viniciusgerevini commented 3 months ago

Thansk @thyancey . I still haven't been able to reproduce it on my end, but godot's importers can be flaky indeed.

occasionally after importing my sprite is totally blank, even though sprite frames are populated

Interesting. This issue happens when Godot fails to identity the texture file when creating the sprite frames. The calls to the file system should prevent that from happening, but as you were seeing the file access warning, it could be something related to that. This is more likely to happen when creating a new file, but I'll check if the deferred scans might be impacting that