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

Prevent threading problems in TileMap #87478

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jan 22, 2024

Still needing to rework the places this is used and do some deeper testing but just to do some testing with this base and for some input

Will also update documentation etc. to clarify that it doesn't update automatically when not in the tree but that manual updates are required

Follow-up/related to (which it reverts on master as its no longer needed):

See also:

Note: This does not make TileMap thread safe, like all nodes they aren't safe to manipulate from multiple threads, or from a thread when in the scene tree

What it does is make it possible to use it in a thread safe fashion, by preventing it from being manipulated on the main thread due to deferred updates


@AThousandShips
Copy link
Member Author

Due to the various effects of this I'm not marking it for cherry-picking and keeping #87470 as the main fix for the editor crashes

I'd say the possible drawbacks of not being able to have the map update when not in the tree are worth the thread safety improvements (without having to make the whole class thread safe), the main problem is that even when not in the tree it isn't thread safe as the update is deferred and can collide with threaded processing, especially when adding a lot of tiles for example adding a pattern

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 22, 2024

Reverted the changes from #87470 for this as they are no longer needed if this works, and it just takes up some performance and memory, but as that fix is cherry-pickable and I don't feel confident cherry-picking this one at least not without extensive testing, and it does change behavior

It also makes it easier to test this as it can be tested in the editor with #86226, though general thread safety should be ensured here

To clarify though:
This isn't about general thread safety, TileMap is still not thread safe when in the scene tree, the bug here is that it's thread unsafe outside the scene tree

@AThousandShips AThousandShips marked this pull request as ready for review January 22, 2024 20:24
@AThousandShips AThousandShips requested review from a team as code owners January 22, 2024 20:24
@AThousandShips
Copy link
Member Author

Not fully ready by a bit yet but marking ready for review to get some eyes on it, still needs some testing and cleaning up and decisions

This changes behavior in cases where it is already thread safe, i.e. when modified from the main thread, when not in the scene tree

In these cases, where, for example, you update a tile map incrementally but on the main thread, outside of the scene tree, the updated internals won't be available even if you do it, for example, in _process, this was the case normally, and does work fully safely

I'm not feeling confident that this change won't cause confusion in these specific cases, or in "partially" thread safe use, i.e. when you're lucky and it doesn't crash (which is largely the case when you don't do large batch updates), as you now have to explicitly call update_internals

But I'm not familiar enough with all the details to know what if any of that data is relevant outside of the tree, if it isn't I'd say to also cherry pick this one

@AThousandShips

This comment was marked as outdated.

@AThousandShips AThousandShips removed request for a team February 6, 2024 16:13
@AThousandShips AThousandShips changed the title [WIP] Prevent threading problems in TileMap Prevent threading problems in TileMap Feb 7, 2024
@AThousandShips AThousandShips force-pushed the tile_thread_fix_2 branch 2 times, most recently from 2d77408 to c66faea Compare February 12, 2024 13:31
@akien-mga
Copy link
Member

CC @groud

doc/classes/TileMap.xml Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member Author

There, removed the additional note in the documentation, cleaned up the comments, and added a comment to the blocked update

@AThousandShips

This comment was marked as outdated.

@AThousandShips AThousandShips added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 27, 2024
@akien-mga akien-mga merged commit 33bc762 into godotengine:master Feb 27, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the tile_thread_fix_2 branch February 27, 2024 15:40
@AThousandShips
Copy link
Member Author

Thank you!

@grusad
Copy link

grusad commented Mar 5, 2024

Hey. Was just testing this one out from Godot 4.3 Dev 4 build and it didn't seem to solve the issue i have regarding instantiating big Tilemaps on separate thread. #87402

Loading the resource works as it always has on a separate Thread, but not instantiating it.

I still get the logs spammed with this message:
USER ERROR: Condition "p_elem->_root != this" is true.
at: remove (./core/templates/self_list.h:80)

From what i saw, this PR is in the Dev 4 build.

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 5, 2024

Left a comment on that issue, I'm not sure it's entirely a bug as resources are not thread safe, but worth investigating, unsure what's causing it in this case, but the stack trace isn't for the tile map

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.

Instantiate big Tilemap on thread dont work.
4 participants