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 loading icons from another thread and issue with call_deferred no… #96

Closed
wants to merge 2 commits into from

Conversation

plink-plonk-will
Copy link
Contributor

call_deferred does not work from _init calls. Instead it directly calls the function: this may be new behaviour in Godot 4.3b2, and is also observed in 4.3b3.

The solution presented is more robust, as it implements a mutex protected list that is processed when reloads of textures are requested.

@rsubtil
Copy link
Owner

rsubtil commented Jul 20, 2024

Hi, thanks for the PR! Do you have a reproduction project with this issue to test this behavior?

The solution looks quite complex, so I'd like to help in figuring out a simpler solution for the issue that's ocurring.

@plink-plonk-will
Copy link
Contributor Author

Hi! Yes of course. The example project here using 3.1.1 will crash immediately when pressing play in the editor. Using this PR it works as intended:

  1. Starts with a very simple "Loading..." screen, that uses a loading thread to load the "press_start_screen" scene.
  2. Once the ResourceLoader reports the scene is ready, instances the scene at the root, and kills the boot screen.
  3. The Press Start screen should be displayed with the "ui_accept" button press texture beneath it.

Included zip file is with the 3.1.1 add-on in the addons directory to more easily show the issue.
threaded_load_controller_icons.zip

@rsubtil
Copy link
Owner

rsubtil commented Jul 27, 2024

Thank you for the MRP! From my testing, while the game doesn't crash, I do get some very nasty errors when I quit the project (malloc(): unsorted double linked list corrupted / corrupted double-linked list).

This seems to be an engine issue; from my testing, 4.3-dev6 works, while 4.3-beta1 is the first version to have this problem. Could you test the dev6 in your setup as well to check if it has no crashes, please?

In the meantime I'll try to bisect this issue and report it as an engine bug. Since 4.3 isn't released yet, we could have an engine fix before an official release, which would be better than having to change the texture loading behavior.

@plink-plonk-will
Copy link
Contributor Author

I can confirm that with dev6 the minimal project above works fine for me. No shutdown errors either.

@rsubtil
Copy link
Owner

rsubtil commented Aug 4, 2024

@plink-plonk-will I've found some time today to report this on Godot. I'll now wait to see if it can be fixed on their side before the release, or if this PR will be needed to work around this 👍

@plink-plonk-will
Copy link
Contributor Author

Excellent: thank you. I’ve subscribed to that thread 👍

@plyoung plyoung mentioned this pull request Aug 6, 2024
@rsubtil
Copy link
Owner

rsubtil commented Aug 6, 2024

@plink-plonk-will a fix has been made and merged, and from my testing this it working correctly now. Can you confirm if this works for you as well? If everything's working, I'd vote to close this PR since it was an engine bug; while this is remains a valid solution, it makes this part of the code quite more complex without having much reason now to do so.

@plink-plonk-will
Copy link
Contributor Author

I can confirm the fix works. Feel free to close this, and thanks again 🙂

@rsubtil rsubtil closed this Aug 7, 2024
@plink-plonk-will
Copy link
Contributor Author

I'm still getting the crash with rc3 on my project which I don't get with this PR. But, the MRP works fine. So I'll try and create an MRP that reproduces the crash. The fix put into Godot was dubious, as it didn't really fix the core issue: just masked an issue caused by it. I'll keep an eye out on the other PRs that are meant to fix it properly and see if any of those work.

@rsubtil
Copy link
Owner

rsubtil commented Aug 9, 2024

In that case, have a look at this comment; it mentions that another PR might be the true fix for this issue (godotengine/godot#94169)

EDIT: Oops, I reread your comment now, I think you're already aware of this 😅

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

Successfully merging this pull request may close these issues.

2 participants