fix: fix multi-threading problems #11
Conversation
Make access to ActorManager::m_skeletons and ActorManager::Skeleton::m_armors thread-safe. CTDs might have happened when using 'smp' commands and Dynamic HDT.
| return _deprefix(a->cstr()) == _deprefix(b->cstr()); | ||
| } | ||
|
|
||
| void hdt::util::transferCurrentPosesBetweenSystems(hdt::SkyrimSystem* src, hdt::SkyrimSystem* dst) |
There was a problem hiding this comment.
There is a function declaration in DynamicHDT.h, please also delete it
hdtSMP64/ActorManager.h
Outdated
|
|
||
| static bool _match_name(hdt::IDStr& a, hdt::IDStr& b); | ||
|
|
||
| static std::string _deprefix(std::string str_with_prefix); |
There was a problem hiding this comment.
You might want to move these two static methods to utilities. Just a suggestion.
There was a problem hiding this comment.
Good idea, thanks, will do.
| { | ||
| std::lock_guard<decltype(m_lock)> l(m_lock); | ||
|
|
||
| auto p = findArmor(on_actor_formID, on_item_formID, new_physics_file_path); |
There was a problem hiding this comment.
Shouldn't new_physics_file_path be NULL or ""?
There was a problem hiding this comment.
You can push another commit with these changes to threadsafingArmors branch. So I can give you an approving review. :)
There was a problem hiding this comment.
Ok, checked, no I pass the physics_file_path, as you did in ReloadPysicsFileImpl.
Seems correct to me.
| @@ -64,8 +64,7 @@ namespace hdt | |||
|
|
|||
| if (timeStep <= RESET_PHYSICS) | |||
| { | |||
| if (!this->block_resetting) | |||
| updateTransformUpDown(m_skeleton, true); | |||
| updateTransformUpDown(m_skeleton, true); | |||
There was a problem hiding this comment.
If an armor tries to read its initial position or runs into any situation that triggers the above function, flickers may occur. block_resetting was used to keep this function from being used when replacing the old system with the new one.
Please confirm that this will not lead to any flickers during the transition of physics files.
There was a problem hiding this comment.
My understanding at the time was that the lock in ActorManager, once set, was protecting against all these kinds of things.
I think a test is required. I personally witnessed no flicker at all, but could you please test yourself when I will have integrated all your suggestions, as you know how to reproduce flickers and what they look like?
| @@ -53,9 +53,6 @@ namespace hdt | |||
| } | |||
| } | |||
|
|
|||
| void suspendSimulationUntilFinished(std::function<void(void)> process); | |||
| std::atomic_bool m_isStasis = false; | |||
There was a problem hiding this comment.
Is there a method for us to suspend the simulation while game is not paused besides this?
There was a problem hiding this comment.
I want to deepen my analysis on this.
Make access to ActorManager::m_skeletons and ActorManager::Skeleton::m_armors thread-safe.
CTDs might have happened when using 'smp' commands and Dynamic HDT.