From b13afd758c9da47414df8d3cd8c60c2625ce3be3 Mon Sep 17 00:00:00 2001 From: fredzio Date: Thu, 20 May 2021 05:53:26 +0200 Subject: [PATCH] Remove both racy and useless code. Actor's position can be determined in 3 ways: 1/ as a result of physics simulation 2/ after a script require a relative position change (SetPos, Move) 3/ absolutely set from games mechanics event (teleport) or script (PositionCell) In case 1/, RefData::mPosition is updated with the physics simulation result In case 2/, when RefData::mPosition is updated, physics simulation is informed of the change and update accordingly In case 3/, when RefData::mPosition is updated, the physics simulation state is reset In all 3 cases, we don't need to check the RefData::mPosition to get a correct behaviour. TSAN reported the following data race: Read of size 4 at 0x7b50005b75b0 by thread T12 (mutexes: write M656173, write M84859534346343880): #0 ESM::Position::asVec3() const /build/openmw/openmw/master2/.build/freebsd/TSAN/../../.././components/esm/defs.hpp:55:27 (openmw+0xb809d5) #1 MWPhysics::Actor::updateWorldPosition() /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwphysics/actor.cpp:131:59 (openmw+0xb809d5) #2 MWPhysics::Actor::setPosition(osg::Vec3f const&) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwphysics/actor.cpp:177:5 (openmw+0xb809d5) #3 MWPhysics::PhysicsTaskScheduler::updateActorsPositions() /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwphysics/mtphysics.cpp:524:28 (openmw+0xb91ac0) #4 MWPhysics::PhysicsTaskScheduler::afterPostStep() /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwphysics/mtphysics.cpp:614:13 (openmw+0xb915e7) #5 MWPhysics::PhysicsTaskScheduler::worker()::$_5::operator()() const /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwphysics/mtphysics.cpp:498:45 (openmw+0xb915e7) #6 void Misc::Barrier::wait(MWPhysics::PhysicsTaskScheduler::worker()::$_5&&) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../.././components/misc/barrier.hpp:30:21 (openmw+0xb915e7) #7 MWPhysics::PhysicsTaskScheduler::worker() /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwphysics/mtphysics.cpp:498:31 (openmw+0xb915e7) #8 MWPhysics::PhysicsTaskScheduler::PhysicsTaskScheduler(float, btCollisionWorld*, MWRender::DebugDrawer*)::$_0::operator()() const /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwphysics/mtphysics.cpp:162:45 (openmw+0xb92630) #9 decltype(std::__1::forward(fp)()) std::__1::__invoke(MWPhysics::PhysicsTaskScheduler::PhysicsTaskScheduler(float, btCollisionWorld*, MWRender::DebugDrawer*)::$_0&&) /usr/include/c++/v1/type_traits:3899:1 (openmw+0xb92630) #10 void std::__1::__thread_execute >, MWPhysics::PhysicsTaskScheduler::PhysicsTaskScheduler(float, btCollisionWorld*, MWRender::DebugDrawer*)::$_0>(std::__1::tuple >, MWPhysics::PhysicsTaskScheduler::PhysicsTaskScheduler(float, btCollisionWorld*, MWRender::DebugDrawer*)::$_0>&, std::__1::__tuple_indices<>) /usr/include/c++/v1/thread:280:5 (openmw+0xb92630) #11 void* std::__1::__thread_proxy >, MWPhysics::PhysicsTaskScheduler::PhysicsTaskScheduler(float, btCollisionWorld*, MWRender::DebugDrawer*)::$_0> >(void*) /usr/include/c++/v1/thread:291:5 (openmw+0xb92630) Previous write of size 8 at 0x7b50005b75b0 by main thread: #0 memcpy /wrkdirs/usr/ports/devel/llvm-devel/work-default/llvm-project-3f6753efe1990a928ed120bd907940a9fb3e2fc3/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:827:5 (openmw+0x55a057) #1 MWWorld::RefData::setPosition(ESM::Position const&) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwworld/refdata.cpp:216:19 (openmw+0xa3de1c) #2 MWWorld::World::moveObject(MWWorld::Ptr const&, MWWorld::CellStore*, float, float, float, bool) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwworld/worldimp.cpp:1130:26 (openmw+0xa57300) #3 MWWorld::World::moveObject(MWWorld::Ptr const&, float, float, float, bool, bool) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwworld/worldimp.cpp:1253:16 (openmw+0xa580c8) #4 MWWorld::World::doPhysics(float, unsigned long long, unsigned int, osg::Stats&) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwworld/worldimp.cpp:1530:17 (openmw+0xa5af8f) #5 MWWorld::World::updatePhysics(float, bool, unsigned long long, unsigned int, osg::Stats&) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/mwworld/worldimp.cpp:1862:13 (openmw+0xa61a7c) #6 OMW::Engine::frame(float) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/engine.cpp:333:42 (openmw+0xcce9e7) #7 OMW::Engine::go() /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/engine.cpp:935:14 (openmw+0xcd86ed) #8 runApplication(int, char**) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/main.cpp:296:17 (openmw+0xcbffac) #9 wrapApplication(int (*)(int, char**), int, char**, std::__1::basic_string, std::__1::allocator > const&) /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../components/debug/debugging.cpp:205:15 (openmw+0x1335442) #10 main /build/openmw/openmw/master2/.build/freebsd/TSAN/../../../apps/openmw/main.cpp:308:12 (openmw+0xcc008a) :wqa --- apps/openmw/mwphysics/actor.cpp | 21 ++++----------------- apps/openmw/mwphysics/actor.hpp | 8 -------- apps/openmw/mwphysics/physicssystem.cpp | 1 - 3 files changed, 4 insertions(+), 26 deletions(-) diff --git a/apps/openmw/mwphysics/actor.cpp b/apps/openmw/mwphysics/actor.cpp index 9793906098..f0bc234138 100644 --- a/apps/openmw/mwphysics/actor.cpp +++ b/apps/openmw/mwphysics/actor.cpp @@ -117,27 +117,15 @@ int Actor::getCollisionMask() const void Actor::updatePosition() { std::scoped_lock lock(mPositionMutex); - updateWorldPosition(); - mPreviousPosition = mWorldPosition; - mPosition = mWorldPosition; - mSimulationPosition = mWorldPosition; + const auto worldPosition = mPtr.getRefData().getPosition().asVec3(); + mPreviousPosition = worldPosition; + mPosition = worldPosition; + mSimulationPosition = worldPosition; mPositionOffset = osg::Vec3f(); mStandingOnPtr = nullptr; mSkipCollisions = true; } -void Actor::updateWorldPosition() -{ - if (mWorldPosition != mPtr.getRefData().getPosition().asVec3()) - mWorldPositionChanged = true; - mWorldPosition = mPtr.getRefData().getPosition().asVec3(); -} - -osg::Vec3f Actor::getWorldPosition() const -{ - return mWorldPosition; -} - void Actor::setSimulationPosition(const osg::Vec3f& position) { mSimulationPosition = position; @@ -174,7 +162,6 @@ osg::Vec3f Actor::getCollisionObjectPosition() const bool Actor::setPosition(const osg::Vec3f& position) { std::scoped_lock lock(mPositionMutex); - updateWorldPosition(); applyOffsetChange(); bool hasChanged = mPosition != position || mWorldPositionChanged; mPreviousPosition = mPosition; diff --git a/apps/openmw/mwphysics/actor.hpp b/apps/openmw/mwphysics/actor.hpp index acbf33ca75..7b53e8812a 100644 --- a/apps/openmw/mwphysics/actor.hpp +++ b/apps/openmw/mwphysics/actor.hpp @@ -55,13 +55,6 @@ namespace MWPhysics */ bool isRotationallyInvariant() const; - /** - * Set mWorldPosition to the position in the Ptr's RefData. This is used by the physics simulation to account for - * when an object is "instantly" moved/teleported as opposed to being moved by the physics simulation. - */ - void updateWorldPosition(); - osg::Vec3f getWorldPosition() const; - /** * Used by the physics simulation to store the simulation result. Used in conjunction with mWorldPosition * to account for e.g. scripted movements @@ -204,7 +197,6 @@ namespace MWPhysics osg::Vec3f mScale; osg::Vec3f mRenderingScale; - osg::Vec3f mWorldPosition; osg::Vec3f mSimulationPosition; osg::Vec3f mPosition; osg::Vec3f mPreviousPosition; diff --git a/apps/openmw/mwphysics/physicssystem.cpp b/apps/openmw/mwphysics/physicssystem.cpp index a5d6018f46..705e15187d 100644 --- a/apps/openmw/mwphysics/physicssystem.cpp +++ b/apps/openmw/mwphysics/physicssystem.cpp @@ -938,7 +938,6 @@ namespace MWPhysics void ActorFrameData::updatePosition(btCollisionWorld* world) { - mActorRaw->updateWorldPosition(); mActorRaw->applyOffsetChange(); mPosition = mActorRaw->getPosition(); if (mWaterCollision && mPosition.z() < mWaterlevel && canMoveToWaterSurface(mActorRaw, mWaterlevel, world))