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

[Editor] Fix threading problems with TileMap preview #87470

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jan 22, 2024

As investigated in #87458 this is due to threading problems in the editor, specifically the preview generation, this solves this by handling deferred calls on a separate queue, rather than adding thread safety mechanisms to TileMapLayer which I'd say aren't necessary otherwise due to thread safety guarantees, I also suspect there are other possible race conditions here that won't be solved by adding those thread safety features, instead I suggest removing the thread safety from the equation entirely by controlling the timing

The general topic of thread safety of nodes not on the tree is a problem and other nodes suffer from problems due to deferred calls AFAIK, so a broader investigation of that and how TileMap and TileMapLayer relates to this should be done as well, but I think the pressing concern here is the editor related crash

Alternative to:

@AThousandShips
Copy link
Member Author

We could also handle this by not doing the internal update off the tree at all, but that is a solution that has various possible impacts so a more complicated thing

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 the idea of further investigation, and with the disclaimer that I haven't checked the details, this looks like the right thing to do.

@groud
Copy link
Member

groud commented Jan 22, 2024

This might fix #86097 too.

@AThousandShips
Copy link
Member Author

Sounds like a duplicate of #86226, but unable to verify right now

@YuriSizov YuriSizov merged commit b5f0334 into godotengine:master Jan 22, 2024
16 checks passed
@AThousandShips AThousandShips deleted the tile_thread_fix branch January 22, 2024 19:46
@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 22, 2024

Thanks!

Edit: Almost forgot to meme! Well, if @RandomShaper is happy, then we're happy.

image

@AThousandShips
Copy link
Member Author

Thank you!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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.

Crash when editing/opening a huge tileset atlas
4 participants