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

ArticulationLinkComponent not thread-safe - Missing PhysX 5 scene locking for various API mehtods #18506

Open
lgleim opened this issue Nov 25, 2024 · 0 comments
Labels
feature/physics This item is related to the physics subsystem. kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/major Major priority. Work that should be handled after all blocking and critical work is done. sig/simulation Categorizes an issue or PR as relevant to SIG Simulation

Comments

@lgleim
Copy link
Contributor

lgleim commented Nov 25, 2024

Describe the bug
The ArticulationLinkComponent (implemented for the PhysX 5 gem and used extensively by the ROS2 gem) does not correctly employ PHYSX_SCENE_READ_LOCK / PHYSX_SCENE_WRITE_LOCK to lock the scene before reading/writing the physx scene, resulting in unsafe access, runtime assertions, and potentially lost physics state updates.

Since PhysX continually raises assertions in O3DE debug builds for this issue, it further makes debugging any other issues in levels with ArticulationLinks practically impossible (at least without hacks for automatically terminating the raised debug-breaks).

The fix should be straightforward, as in wrapping all methods reading / writing the physx scene with adequate scoped locks.
The ArticulationLinkComponent already wraps reads/writes correctly in four methods. The same logic just needs to be applied to all getters/setters.

Assets required
A project that reads/writes articulation link states at runtime, e.g. the ROS2 manipulation template levels.

Steps to reproduce
Since the PhysX API raises assertions, the best way to investigate them is

  1. Build O3DE in debug configuration
  2. Run with debugger attached
  3. Use a project that reads/writes ArticulationLink properties at runtime, e.g. a ROS2 project with JointPosition / Manipulation Control Component
  4. Observe stack trace / assertion error message in debug breaks raised by physx

Expected behavior
API methods exposed by the ArticulationLinkComponent should work correctly and be safe to use

Actual behavior
API methods exposed by the ArticulationLinkComponent raise assertions, may cause stale reads, lost updates, and pollute debugging workflows with numerous assertions per simulation tick

Screenshots/Video

Found in Branch
both main & development

Commit ID from o3de/o3de Repository
3aba99a

Additional context
Example of proper implementation in https://github.com/o3de/o3de/blob/3aba99af74ad59cd50d22b9eab9d58686b345cc8/Gems/PhysX/Core/Code/Source/RigidBody.cpp

@lgleim lgleim added feature/physics This item is related to the physics subsystem. kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/major Major priority. Work that should be handled after all blocking and critical work is done. sig/simulation Categorizes an issue or PR as relevant to SIG Simulation labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/physics This item is related to the physics subsystem. kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/major Major priority. Work that should be handled after all blocking and critical work is done. sig/simulation Categorizes an issue or PR as relevant to SIG Simulation
Projects
None yet
Development

No branches or pull requests

1 participant