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

Add methods and signal in GridMap for multithreading #63201

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LiBooks
Copy link

@LiBooks LiBooks commented Jul 19, 2022

Fixes #42917. Fixes #63106.
This PR adds two methods and a signal in GridMap for for multithreading:

  • [1 ] The first method called "set_cell_item_thread_safe" has the same parameters as "set_cell_item" . However, it does not let the main thread execute rendering and physical processing, and is only responsible for writing data and creating basic rendering and physical objects .

  • [ 2] The second method called "update" can let the main thread refresh the 3D rendering, physics and navgation of cells later according to the data in the GridMap.

  • [ 3] The siganl called "update_finished" will be emitted after the main thread finishes its work .And It is very useful for thread synchronization .

When the "set_cell_map" method is executed, the update operation of gridmap will be automatically executed by the main thread, and the user cannot control it. When the main thread executes the update operation, the "set_cell_map" method executed by other threads will still write data to the gridmap, which destroys the thread synchronization and causes an error message, making the program skip the update of octant.

In order to achieve thread synchronization, I think it is important for users to control the timing of updates. This is necessary when making sandbox games.

@LiBooks LiBooks requested a review from a team as a code owner July 19, 2022 14:30
@Calinou Calinou added this to the 4.0 milestone Jul 19, 2022
@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 12, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.x Jun 16, 2023
@akien-mga
Copy link
Member

Sorry for the delay reviewing.

This approach may not be compatible with the major changes done for SceneTree multithreading in #75901 and follow-up PRs. This would be good to reassess, and maybe see how other baking operations handle MT (see @smix8's recent #77412).

CC @RandomShaper

@RandomShaper
Copy link
Member

The changes here aren't thread-safe unless the user script does some extra sync work, which is not very different from the original situation.

Now, nodes in Godot are not designed to be thread-safe in general, for performance reasons. They may be enhanced to support threaded group processing, which enables parallel processing under very specific constraints that don't impact performance.

So, you are not expected to be able to have the GridMap added to the tree and then from a thread call its setters. The only generally accepted way to deal with a node in a thread is to set it up out of the scene tree and add it later. Now, GridMap has an issue in that workflow (that can be fixed), but that won't close any of the referenced issue reports, which are trying unsupported operations.

There should be a strong use case that makes dealing with GridMaps from multiple threads a real need in order to consider it a exceptional case.

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