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.

Sign up for GitHub

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

Jump to bottom

Fix life cycle of ResourceImporterTexture better #79981

Merged
merged 1 commit into from
Jul 29, 2023
Merged

Fix life cycle of ResourceImporterTexture better #79981

merged 1 commit into from
Jul 29, 2023

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Jul 28, 2023 •
edited by KoBeWi
Loading

Follow-up of #79954.

Since multiple instances of this class can exist at the same time, only one should own the singleton variable. Otherwise, without this PR, any of the instances being destroyed would nullify the singleton, when other instances still exist.

CC @aaronfranke

UPDATE: For the records, SceneTree uses the same singletoning pattern. It may be beneficial to make it a de facto standard in Godot? Food for thought.

Bugsquad edit: Fixes #79999

@RandomShaper RandomShaper added this to the 4.2 milestone Jul 28, 2023
@RandomShaper RandomShaper requested a review from YuriSizov July 28, 2023 09:54
@RandomShaper RandomShaper requested a review from a team as a code owner July 28, 2023 09:54
@YuriSizov YuriSizov requested a review from aaronfranke July 28, 2023 10:21
aaronfranke
aaronfranke approved these changes Jul 28, 2023
View reviewed changes
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Tested opening projects 5 times, got no crashes, so this seems to fix the problem. Thank you!

fire
fire approved these changes Jul 28, 2023
View reviewed changes
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Looks like a nullcheck which is relatively safe.

@smix8 smix8 mentioned this pull request Jul 28, 2023
Closed
@aaronfranke aaronfranke merged commit 75f9c97 into godotengine:master Jul 29, 2023
@aaronfranke
Copy link
Member

Thanks! I hit the merge button myself because this is very vital and the PR is very simple.

@RandomShaper RandomShaper deleted the fix_res_imp_life_2 branch July 31, 2023 09:51
This was referenced Aug 2, 2023
Merged
Closed
@YuriSizov YuriSizov mentioned this pull request Aug 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronfranke aaronfranke aaronfranke approved these changes

@fire fire fire approved these changes

@YuriSizov YuriSizov Awaiting requested review from YuriSizov

Assignees
No one assigned
Labels
bug crash topic:core
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Random crashes when opening a project
3 participants
@RandomShaper @aaronfranke @fire