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 thread-safe access for render entities #1692

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

Conversation

heinezen
Copy link
Member

@heinezen heinezen commented Oct 3, 2024

The current render entity code was not thread-safe because of an oversight in the implementation. When syncing data between the render entity and the objects in the renderer, sometimes references were passed instead of copying the data over, e.g.

const curve::Continuous<coord::scene3> &WorldRenderEntity::get_position() {
std::shared_lock lock{this->mutex};
return this->position;
}

After returning the reference, the lock is released even though a read has not taken place yet. This results in the simulation thread writing data into the render entity while it is still being read, with the consequence that over- or underreads might happen. I fixed this behavior now by ensuring that locks are present for the full read duration.

Changes in this PR:

  • Document which methods of the render entities are thread-safe in the docstrings
  • Add a new method get_read_lock() which returns a std::shared_lock locking the render entity's mutex. This allows manual control of data reads from the render entity.
  • Add a renderer::RenderEntity base class for the shared functionality of the different render entity types in HUD, terrain, and world render stages.
  • Make all data transfers thread-safe

@heinezen heinezen added area: renderer Concerns our graphics renderer lang: c++ Done in C++ code bugfix Restores intended behavior labels Oct 3, 2024
@heinezen heinezen marked this pull request as ready for review October 3, 2024 17:43
@heinezen heinezen requested a review from TheJJ October 3, 2024 17:43
@heinezen heinezen force-pushed the fix/render_entity_threadsafe branch from d41cec3 to 61d3c34 Compare October 6, 2024 15:09
@heinezen heinezen added the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Oct 6, 2024
@SFTbot SFTbot removed the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Oct 6, 2024
@heinezen heinezen added the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Oct 6, 2024
@SFTbot SFTbot removed the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Oct 6, 2024
@heinezen heinezen added the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Oct 15, 2024
@SFTbot SFTbot removed the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: renderer Concerns our graphics renderer bugfix Restores intended behavior lang: c++ Done in C++ code
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants