Changeset View
Standalone View
source/simulation2/components/CCmpPathfinder.cpp
Show All 30 Lines | |||||
#include "simulation2/components/ICmpWaterManager.h" | #include "simulation2/components/ICmpWaterManager.h" | ||||
#include "simulation2/helpers/HierarchicalPathfinder.h" | #include "simulation2/helpers/HierarchicalPathfinder.h" | ||||
#include "simulation2/helpers/LongPathfinder.h" | #include "simulation2/helpers/LongPathfinder.h" | ||||
#include "simulation2/helpers/MapEdgeTiles.h" | #include "simulation2/helpers/MapEdgeTiles.h" | ||||
#include "simulation2/helpers/Rasterize.h" | #include "simulation2/helpers/Rasterize.h" | ||||
#include "simulation2/helpers/VertexPathfinder.h" | #include "simulation2/helpers/VertexPathfinder.h" | ||||
#include "simulation2/serialization/SerializedPathfinder.h" | #include "simulation2/serialization/SerializedPathfinder.h" | ||||
#include "simulation2/serialization/SerializedTypes.h" | #include "simulation2/serialization/SerializedTypes.h" | ||||
#include "simulation2/system/SimSynchronization.h" | |||||
#include "ps/CLogger.h" | #include "ps/CLogger.h" | ||||
#include "ps/CStr.h" | #include "ps/CStr.h" | ||||
#include "ps/Profile.h" | #include "ps/Profile.h" | ||||
#include "ps/ThreadUtil.h" | |||||
#include "ps/XML/Xeromyces.h" | #include "ps/XML/Xeromyces.h" | ||||
#include "renderer/Scene.h" | #include "renderer/Scene.h" | ||||
REGISTER_COMPONENT_TYPE(Pathfinder) | REGISTER_COMPONENT_TYPE(Pathfinder) | ||||
void CCmpPathfinder::Init(const CParamNode& UNUSED(paramNode)) | void CCmpPathfinder::Init(const CParamNode& UNUSED(paramNode)) | ||||
{ | { | ||||
m_MapSize = 0; | m_MapSize = 0; | ||||
Show All 14 Lines | void CCmpPathfinder::Init(const CParamNode& UNUSED(paramNode)) | ||||
CXeromyces::AddValidator(g_VFS, "pathfinder", "simulation/data/pathfinder.rng"); | CXeromyces::AddValidator(g_VFS, "pathfinder", "simulation/data/pathfinder.rng"); | ||||
// Since this is used as a system component (not loaded from an entity template), | // Since this is used as a system component (not loaded from an entity template), | ||||
// we can't use the real paramNode (it won't get handled properly when deserializing), | // we can't use the real paramNode (it won't get handled properly when deserializing), | ||||
// so load the data from a special XML file. | // so load the data from a special XML file. | ||||
CParamNode externalParamNode; | CParamNode externalParamNode; | ||||
CParamNode::LoadXML(externalParamNode, L"simulation/data/pathfinder.xml", "pathfinder"); | CParamNode::LoadXML(externalParamNode, L"simulation/data/pathfinder.xml", "pathfinder"); | ||||
// Previously all move commands during a turn were | |||||
// queued up and processed asynchronously at the start | |||||
// of the next turn. Now we are processing queued up | |||||
// events several times duing the turn. This improves | |||||
// responsiveness and units move more smoothly especially. | |||||
// when in formation. There is still a call at the | |||||
// beginning of a turn to process all outstanding moves - | |||||
// this will handle any moves above the MaxSameTurnMoves | |||||
// threshold. | |||||
// | |||||
// TODO - The moves processed at the beginning of the | |||||
// turn do not count against the maximum moves per turn | |||||
// currently. The thinking is that this will eventually | |||||
// happen in another thread. Either way this probably | |||||
// will require some adjustment and rethinking. | |||||
const CParamNode pathingSettings = externalParamNode.GetChild("Pathfinder"); | const CParamNode pathingSettings = externalParamNode.GetChild("Pathfinder"); | ||||
m_MaxSameTurnMoves = (u16)pathingSettings.GetChild("MaxSameTurnMoves").ToInt(); | m_MaxSameTurnMoves = (u16)pathingSettings.GetChild("MaxSameTurnMoves").ToInt(); | ||||
Stan: If you want to change this line replace the c style cast by a c++ cast :) | |||||
Done Inline ActionsDone in next diff. Kuba386: Done in next diff. | |||||
Not Done Inline ActionsDo we have a conversion to unsigned int ? Stan: Do we have a conversion to unsigned int ? | |||||
const CParamNode::ChildrenMap& passClasses = externalParamNode.GetChild("Pathfinder").GetChild("PassabilityClasses").GetChildren(); | const CParamNode::ChildrenMap& passClasses = externalParamNode.GetChild("Pathfinder").GetChild("PassabilityClasses").GetChildren(); | ||||
for (CParamNode::ChildrenMap::const_iterator it = passClasses.begin(); it != passClasses.end(); ++it) | for (CParamNode::ChildrenMap::const_iterator it = passClasses.begin(); it != passClasses.end(); ++it) | ||||
{ | { | ||||
std::string name = it->first; | std::string name = it->first; | ||||
ENSURE((int)m_PassClasses.size() <= PASS_CLASS_BITS); | ENSURE((int)m_PassClasses.size() <= PASS_CLASS_BITS); | ||||
Not Done Inline ActionsSame here If you want to change this line replace the c style cast by a c++ cast :) Stan: Same here If you want to change this line replace the c style cast by a c++ cast :) | |||||
Done Inline ActionsDone in next diff. Same for others. :) Kuba386: Done in next diff. Same for others. :) | |||||
pass_class_t mask = PASS_CLASS_MASK_FROM_INDEX(m_PassClasses.size()); | pass_class_t mask = PASS_CLASS_MASK_FROM_INDEX(m_PassClasses.size()); | ||||
m_PassClasses.push_back(PathfinderPassability(mask, it->second)); | m_PassClasses.push_back(PathfinderPassability(mask, it->second)); | ||||
m_PassClassMasks[name] = mask; | m_PassClassMasks[name] = mask; | ||||
} | } | ||||
m_Workers.emplace_back(PathfinderWorker{}); | u32 wantedThreads = Threading::GetNumberOfPathfindingThreads(); | ||||
// The worker thread will only call std::thread if we actually have > 1 threads, otherwise we're running in the main thread. | |||||
Not Done Inline ActionsIs it useful? Stan: Is it useful? | |||||
if (wantedThreads <= 1) // <= 1 as the above computations returns 0 for one core. | |||||
{ | |||||
m_UseThreading = false; | |||||
m_Workers.emplace_back(); | |||||
m_Workers.back().Start(*this, 0); | |||||
} | |||||
else | |||||
{ | |||||
m_PathfinderFrontier.Setup(wantedThreads); | |||||
m_UseThreading = true; | |||||
Done Inline ActionsIt's not good to access atlas stuff from simulation. You need to use a common source, like a thread manager. The same for the number of threads. vladislavbelov: It's not good to access atlas stuff from simulation. You need to use a common source, like a… | |||||
Done Inline ActionsSorry, I don't understand what you mean here. You're saying we should implement a "Thread Manager" class at a 'top-level' global that tells us how many threads we want for pathfinding? Also I somewhat intend to remove this as if we had proper mutex protection it wouldn't be needed. wraitii: Sorry, I don't understand what you mean here. You're saying we should implement a "Thread… | |||||
Done Inline Actionswell, in this part of code threads are going to be created, they do not exists at this point so no concurency here. wantedThreads is local (in scope of function) variable - > no problem even with threads as every would have its own unique variable with no option to see or change the one from another thread g_Atlas & & g_Atlas->running is only reading, as far as I know you can safely read from variable with as many threads as you wish and it is safe but again this function is running in main thread only Silier: well, in this part of code threads are going to be created, they do not exists at this point so… | |||||
Done Inline Actions
I mean that the component shouldn't decide about the number of threads and shouldn't communicate with the Atlas directly. We may use ThreadUtil to calculate right number of threads. Because we might have separate threads for other purposes, and we need to manage them somehow.
It's not the syntax error but the semantic for me. vladislavbelov: > Sorry, I don't understand what you mean here. You're saying we should implement a "Thread… | |||||
Done Inline ActionsOK, makes sense for ThreadUtil. wraitii: OK, makes sense for ThreadUtil. | |||||
// We cannot move workers or threads will run on deleted instances. | |||||
m_Workers.resize(wantedThreads); | |||||
for (size_t i = 0; i < wantedThreads; ++i) | |||||
m_Workers[i].Start(*this, i); | |||||
Done Inline ActionsLOGINFO wraitii: LOGINFO | |||||
} | } | ||||
}; | |||||
Not Done Inline Actionscpp cast here as well. Stan: cpp cast here as well. | |||||
Not Done Inline ActionsThough it might be better to use an u8 everywhere ? Stan: Though it might be better to use an u8 everywhere ? | |||||
Not Done Inline ActionsWhy 64 and not 255 ? Stan: Why 64 and not 255 ? | |||||
Done Inline Actions64 is max value in UI, and so should be there. Why not 255? Kuba386: 64 is max value in UI, and so should be there. Why not 255?
This case is intended to protect… | |||||
CCmpPathfinder::~CCmpPathfinder() {}; | CCmpPathfinder::~CCmpPathfinder() {}; | ||||
Done Inline ActionsUse std::unique_ptr instead, raw pointers are dangerous. vladislavbelov: Use `std::unique_ptr` instead, raw pointers are dangerous. | |||||
Done Inline ActionsIndeed. wraitii: Indeed. | |||||
void CCmpPathfinder::Deinit() | void CCmpPathfinder::Deinit() | ||||
Done Inline Actionselse wraitii: else | |||||
{ | { | ||||
for (PathfinderWorker& worker : m_Workers) | |||||
worker.PrepareToKill(); | |||||
GetSimContext().GetSynchronizationData().m_ComputingPaths = false; | |||||
m_PathfinderConditionVariable.notify_all(); | |||||
m_Workers.clear(); | m_Workers.clear(); | ||||
SetDebugOverlay(false); // cleans up memory | SetDebugOverlay(false); // cleans up memory | ||||
SAFE_DELETE(m_AtlasOverlay); | SAFE_DELETE(m_AtlasOverlay); | ||||
Not Done Inline ActionsWe don't use auto as a general rule of thumb. Stan: We don't use auto as a general rule of thumb. | |||||
SAFE_DELETE(m_Grid); | SAFE_DELETE(m_Grid); | ||||
SAFE_DELETE(m_TerrainOnlyGrid); | SAFE_DELETE(m_TerrainOnlyGrid); | ||||
Not Done Inline ActionsWe don't use auto as a general rule of thumb. Stan: We don't use auto as a general rule of thumb. | |||||
} | } | ||||
template<> | template<> | ||||
struct SerializeHelper<LongPathRequest> | struct SerializeHelper<LongPathRequest> | ||||
{ | { | ||||
template<typename S> | template<typename S> | ||||
void operator()(S& serialize, const char* UNUSED(name), Serialize::qualify<S, LongPathRequest> value) | void operator()(S& serialize, const char* UNUSED(name), Serialize::qualify<S, LongPathRequest> value) | ||||
{ | { | ||||
serialize.NumberU32_Unbounded("ticket", value.ticket); | serialize.NumberU32_Unbounded("ticket", value.ticket); | ||||
serialize.NumberFixed_Unbounded("x0", value.x0); | serialize.NumberFixed_Unbounded("x0", value.x0); | ||||
serialize.NumberFixed_Unbounded("z0", value.z0); | serialize.NumberFixed_Unbounded("z0", value.z0); | ||||
Serializer(serialize, "goal", value.goal); | Serializer(serialize, "goal", value.goal); | ||||
serialize.NumberU16_Unbounded("pass class", value.passClass); | serialize.NumberU16_Unbounded("pass class", value.passClass); | ||||
serialize.NumberU32_Unbounded("notify", value.notify); | serialize.NumberU32_Unbounded("notify", value.notify); | ||||
} | } | ||||
}; | }; | ||||
template<> | template<> | ||||
Done Inline ActionsIn general here: I don't like the NDEBUG stuff, either it's worth having as LOGINFO or it's not and then delete. wraitii: In general here: I don't like the NDEBUG stuff, either it's worth having as LOGINFO or it's not… | |||||
Not Done Inline ActionsI've just removed these since count of pathfinder threads is not likely to be different from desired one because of a bug. Kuba386: I've just removed these since count of pathfinder threads is not likely to be different from… | |||||
struct SerializeHelper<ShortPathRequest> | struct SerializeHelper<ShortPathRequest> | ||||
{ | { | ||||
template<typename S> | template<typename S> | ||||
Done Inline ActionsAlso missed these two. wraitii: Also missed these two. | |||||
void operator()(S& serialize, const char* UNUSED(name), Serialize::qualify<S, ShortPathRequest> value) | void operator()(S& serialize, const char* UNUSED(name), Serialize::qualify<S, ShortPathRequest> value) | ||||
{ | { | ||||
serialize.NumberU32_Unbounded("ticket", value.ticket); | serialize.NumberU32_Unbounded("ticket", value.ticket); | ||||
serialize.NumberFixed_Unbounded("x0", value.x0); | serialize.NumberFixed_Unbounded("x0", value.x0); | ||||
serialize.NumberFixed_Unbounded("z0", value.z0); | serialize.NumberFixed_Unbounded("z0", value.z0); | ||||
serialize.NumberFixed_Unbounded("clearance", value.clearance); | serialize.NumberFixed_Unbounded("clearance", value.clearance); | ||||
serialize.NumberFixed_Unbounded("range", value.range); | serialize.NumberFixed_Unbounded("range", value.range); | ||||
Serializer(serialize, "goal", value.goal); | Serializer(serialize, "goal", value.goal); | ||||
Not Done Inline ActionsComments start with Capitals. Stan: Comments start with Capitals. | |||||
serialize.NumberU16_Unbounded("pass class", value.passClass); | serialize.NumberU16_Unbounded("pass class", value.passClass); | ||||
serialize.Bool("avoid moving units", value.avoidMovingUnits); | serialize.Bool("avoid moving units", value.avoidMovingUnits); | ||||
serialize.NumberU32_Unbounded("group", value.group); | serialize.NumberU32_Unbounded("group", value.group); | ||||
serialize.NumberU32_Unbounded("notify", value.notify); | serialize.NumberU32_Unbounded("notify", value.notify); | ||||
} | } | ||||
}; | }; | ||||
template<typename S> | template<typename S> | ||||
Show All 14 Lines | |||||
{ | { | ||||
Init(paramNode); | Init(paramNode); | ||||
SerializeCommon(deserialize); | SerializeCommon(deserialize); | ||||
} | } | ||||
void CCmpPathfinder::HandleMessage(const CMessage& msg, bool UNUSED(global)) | void CCmpPathfinder::HandleMessage(const CMessage& msg, bool UNUSED(global)) | ||||
{ | { | ||||
switch (msg.GetType()) | switch (msg.GetType()) | ||||
Not Done Inline ActionsMaybe all the cases should have curly braces. No strong feeling. Stan: Maybe all the cases should have curly braces. No strong feeling. | |||||
{ | { | ||||
case MT_RenderSubmit: | case MT_RenderSubmit: | ||||
{ | { | ||||
const CMessageRenderSubmit& msgData = static_cast<const CMessageRenderSubmit&> (msg); | const CMessageRenderSubmit& msgData = static_cast<const CMessageRenderSubmit&> (msg); | ||||
RenderSubmit(msgData.collector); | RenderSubmit(msgData.collector); | ||||
break; | break; | ||||
} | } | ||||
case MT_TerrainChanged: | case MT_TerrainChanged: | ||||
Show All 11 Lines | void CCmpPathfinder::HandleMessage(const CMessage& msg, bool UNUSED(global)) | ||||
case MT_Deserialized: | case MT_Deserialized: | ||||
UpdateGrid(); | UpdateGrid(); | ||||
// In case we were serialised with requests pending, we need to process them. | // In case we were serialised with requests pending, we need to process them. | ||||
if (!m_ShortPathRequests.empty() || !m_LongPathRequests.empty()) | if (!m_ShortPathRequests.empty() || !m_LongPathRequests.empty()) | ||||
{ | { | ||||
ENSURE(CmpPtr<ICmpObstructionManager>(GetSystemEntity())); | ENSURE(CmpPtr<ICmpObstructionManager>(GetSystemEntity())); | ||||
StartProcessingMoves(false); | StartProcessingMoves(false); | ||||
} | } | ||||
break; | break; | ||||
Done Inline ActionsWill need to be untapped as that's how we do switch cases (don't ask me why). wraitii: Will need to be untapped as that's how we do switch cases (don't ask me why). | |||||
} | } | ||||
} | } | ||||
void CCmpPathfinder::RenderSubmit(SceneCollector& collector) | void CCmpPathfinder::RenderSubmit(SceneCollector& collector) | ||||
{ | { | ||||
m_VertexPathfinder->RenderSubmit(collector); | m_VertexPathfinder->RenderSubmit(collector); | ||||
m_PathfinderHier->RenderSubmit(collector); | m_PathfinderHier->RenderSubmit(collector); | ||||
} | } | ||||
void CCmpPathfinder::SetDebugPath(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass) | void CCmpPathfinder::SetDebugPath(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass) | ||||
{ | { | ||||
m_LongPathfinder->SetDebugPath(*m_PathfinderHier, x0, z0, goal, passClass); | m_LongPathfinder->SetDebugPath(*m_PathfinderHier, x0, z0, goal, passClass); | ||||
} | } | ||||
void CCmpPathfinder::SetDebugOverlay(bool enabled) | void CCmpPathfinder::SetDebugOverlay(bool enabled) | ||||
{ | { | ||||
if (enabled && m_UseThreading) | |||||
LOGWARNING("Warning: the vertex pathfinder only shows immediate requests when using threading. " | |||||
"Configure pathfinder threads to 0 to see vertex pathfinder requests"); | |||||
m_VertexPathfinder->SetDebugOverlay(enabled); | m_VertexPathfinder->SetDebugOverlay(enabled); | ||||
m_LongPathfinder->SetDebugOverlay(enabled); | m_LongPathfinder->SetDebugOverlay(enabled); | ||||
} | } | ||||
void CCmpPathfinder::SetHierDebugOverlay(bool enabled) | void CCmpPathfinder::SetHierDebugOverlay(bool enabled) | ||||
{ | { | ||||
m_PathfinderHier->SetDebugOverlay(enabled, &GetSimContext()); | m_PathfinderHier->SetDebugOverlay(enabled, &GetSimContext()); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 249 Lines • ▼ Show 20 Lines | Grid<u16> CCmpPathfinder::ComputeShoreGrid(bool expandOnWater) | ||||
return shoreGrid; | return shoreGrid; | ||||
} | } | ||||
void CCmpPathfinder::UpdateGrid() | void CCmpPathfinder::UpdateGrid() | ||||
{ | { | ||||
PROFILE3("UpdateGrid"); | PROFILE3("UpdateGrid"); | ||||
ENSURE(!GetSimContext().GetSynchronizationData().m_ComputingPaths); | |||||
CmpPtr<ICmpTerrain> cmpTerrain(GetSimContext(), SYSTEM_ENTITY); | CmpPtr<ICmpTerrain> cmpTerrain(GetSimContext(), SYSTEM_ENTITY); | ||||
if (!cmpTerrain) | if (!cmpTerrain) | ||||
return; // error | return; // error | ||||
u16 terrainSize = cmpTerrain->GetTilesPerSide(); | u16 terrainSize = cmpTerrain->GetTilesPerSide(); | ||||
if (terrainSize == 0) | if (terrainSize == 0) | ||||
return; | return; | ||||
▲ Show 20 Lines • Show All 238 Lines • ▼ Show 20 Lines | for (PathfinderPassability& passability : m_PassClasses) | ||||
ExpandImpassableCells(*m_TerrainOnlyGrid, clearance, passability.m_Mask); | ExpandImpassableCells(*m_TerrainOnlyGrid, clearance, passability.m_Mask); | ||||
} | } | ||||
} | } | ||||
////////////////////////////////////////////////////////// | ////////////////////////////////////////////////////////// | ||||
// Async pathfinder workers | // Async pathfinder workers | ||||
CCmpPathfinder::PathfinderWorker::PathfinderWorker() {} | CCmpPathfinder::PathfinderWorker::PathfinderWorker() : m_Computing(false), m_Kill(false) | ||||
{ | |||||
} | |||||
CCmpPathfinder::PathfinderWorker::~PathfinderWorker() | |||||
{ | |||||
Done Inline ActionsUse std::unique_ptr. vladislavbelov: Use `std::unique_ptr`. | |||||
if (m_Thread.joinable()) | |||||
m_Thread.join(); | |||||
} | |||||
void CCmpPathfinder::PathfinderWorker::Start(const CCmpPathfinder& pathfinder, size_t index) | |||||
{ | |||||
Not Done Inline ActionsMerge loops ? Stan: Merge loops ? | |||||
m_VertexPathfinder = std::make_unique<VertexPathfinder>(pathfinder.m_MapSize, pathfinder.m_TerrainOnlyGrid); | |||||
if (pathfinder.m_UseThreading) | |||||
m_Thread = std::thread(&CCmpPathfinder::PathfinderWorker::InitThread, this, std::ref(pathfinder), index); | |||||
} | |||||
void CCmpPathfinder::PathfinderWorker::InitThread(const CCmpPathfinder& pathfinder, size_t index) | |||||
Not Done Inline Actions+ debug_SetThreadName("Pathfinder thread " + std::to_string(index)); Stan: + debug_SetThreadName("Pathfinder thread " + std::to_string(index)); | |||||
Done Inline ActionsFine, didn't know about that. Kuba386: Fine, didn't know about that. | |||||
Done Inline ActionsIt didn't work on linux until very recently thanks to @linkmauve Stan: It didn't work on linux until very recently thanks to @linkmauve | |||||
{ | |||||
g_Profiler2.RegisterCurrentThread("Pathfinder thread " + std::to_string(index)); | |||||
debug_SetThreadName(("Pathfinder thread " + std::to_string(index)).c_str()); | |||||
WaitForWork(pathfinder); | |||||
} | |||||
template<typename T> | template<typename T> | ||||
void CCmpPathfinder::PathfinderWorker::PushRequests(std::vector<T>&, ssize_t) | void CCmpPathfinder::PathfinderWorker::PushRequests(std::vector<T>&, ssize_t) | ||||
Done Inline ActionsWhat does this function do ? Name seems misleading. Stan: What does this function do ? Name seems misleading. | |||||
Done Inline ActionsIt's just there to be specialised, and it pushes requests so I don't think so? wraitii: It's just there to be specialised, and it pushes requests so I don't think so? | |||||
Done Inline ActionsThat line doesn't pushes request does it ? Stan: That line doesn't pushes request does it ? | |||||
Done Inline ActionsIt inserts them at the end (by moving), so that's similar to pushing. wraitii: It inserts them at the end (by moving), so that's similar to pushing. | |||||
Done Inline Actionsstatic_assert(sizeof(T) == 0, "Only specializations can be used"); What part of that statement does that ? Oo There is only one line in that function Stan: static_assert(sizeof(T) == 0, "Only specializations can be used"); What part of that statement… | |||||
Done Inline ActionsAs said above, this is just the template function which gets specialised by L709/714. This base template cannot be used (it's not implemented but there's no clean way to write that because of issues in XCode/VS, see https://stackoverflow.com/a/37593094). wraitii: As said above, this is just the template function which gets specialised by L709/714. This base… | |||||
Not Done Inline ActionsAh thanks for the explanation :) Stan: Ah thanks for the explanation :) | |||||
{ | { | ||||
static_assert(sizeof(T) == 0, "Only specializations can be used"); | static_assert(sizeof(T) == 0, "Only specializations can be used"); | ||||
} | } | ||||
template<> void CCmpPathfinder::PathfinderWorker::PushRequests(std::vector<LongPathRequest>& from, ssize_t amount) | template<> void CCmpPathfinder::PathfinderWorker::PushRequests(std::vector<LongPathRequest>& from, ssize_t amount) | ||||
{ | { | ||||
m_LongRequests.insert(m_LongRequests.end(), std::make_move_iterator(from.end() - amount), std::make_move_iterator(from.end())); | m_LongRequests.insert(m_LongRequests.end(), std::make_move_iterator(from.end() - amount), std::make_move_iterator(from.end())); | ||||
} | } | ||||
template<> void CCmpPathfinder::PathfinderWorker::PushRequests(std::vector<ShortPathRequest>& from, ssize_t amount) | template<> void CCmpPathfinder::PathfinderWorker::PushRequests(std::vector<ShortPathRequest>& from, ssize_t amount) | ||||
{ | { | ||||
m_ShortRequests.insert(m_ShortRequests.end(), std::make_move_iterator(from.end() - amount), std::make_move_iterator(from.end())); | m_ShortRequests.insert(m_ShortRequests.end(), std::make_move_iterator(from.end() - amount), std::make_move_iterator(from.end())); | ||||
} | } | ||||
void CCmpPathfinder::PathfinderWorker::PrepareToKill() | |||||
{ | |||||
m_Kill = true; | |||||
} | |||||
void CCmpPathfinder::PathfinderWorker::WaitForWork(const CCmpPathfinder& pathfinder) | |||||
{ | |||||
while (true) | |||||
Not Done Inline ActionsWhy the scope ? Stan: Why the scope ? | |||||
Done Inline Actionslock gets release automatically at the end of the scope. wraitii: lock gets release automatically at the end of the scope. | |||||
Not Done Inline ActionsI guess one cannot check for m_kill there? Stan: I guess one cannot check for m_kill there? | |||||
{ | |||||
{ | |||||
std::unique_lock<std::mutex> lock(pathfinder.m_PathfinderMutex); | |||||
pathfinder.m_PathfinderConditionVariable.wait(lock, [this] { return m_Computing || m_Kill; }); | |||||
} | |||||
if (m_Kill) | |||||
return; | |||||
Not Done Inline Actionsbreak? Stan: break? | |||||
Done Inline ActionsMight as well return. wraitii: Might as well return. | |||||
Work(pathfinder); | |||||
// We must be the ones setting our m_Computing to false. | |||||
ENSURE(m_Computing); | |||||
m_Computing = false; | |||||
pathfinder.m_PathfinderFrontier.GoThrough(); | |||||
} | |||||
} | |||||
void CCmpPathfinder::PathfinderWorker::Work(const CCmpPathfinder& pathfinder) | void CCmpPathfinder::PathfinderWorker::Work(const CCmpPathfinder& pathfinder) | ||||
{ | { | ||||
while (!m_LongRequests.empty()) | while (!m_LongRequests.empty() && !m_Kill) | ||||
{ | { | ||||
const LongPathRequest& req = m_LongRequests.back(); | const LongPathRequest& req = m_LongRequests.back(); | ||||
WaypointPath path; | WaypointPath path; | ||||
pathfinder.m_LongPathfinder->ComputePath(*pathfinder.m_PathfinderHier, req.x0, req.z0, req.goal, req.passClass, path); | pathfinder.m_LongPathfinder->ComputePath(*pathfinder.m_PathfinderHier, req.x0, req.z0, req.goal, req.passClass, path); | ||||
m_Results.emplace_back(req.ticket, req.notify, path); | m_Results.emplace_back(req.ticket, req.notify, path); | ||||
m_LongRequests.pop_back(); | m_LongRequests.pop_back(); | ||||
} | } | ||||
while (!m_ShortRequests.empty()) | while (!m_ShortRequests.empty() && !m_Kill) | ||||
{ | { | ||||
const ShortPathRequest& req = m_ShortRequests.back(); | const ShortPathRequest& req = m_ShortRequests.back(); | ||||
WaypointPath path = pathfinder.m_VertexPathfinder->ComputeShortPath(req, CmpPtr<ICmpObstructionManager>(pathfinder.GetSystemEntity())); | WaypointPath path = m_VertexPathfinder->ComputeShortPath(req, CmpPtr<ICmpObstructionManager>(pathfinder.GetSystemEntity())); | ||||
m_Results.emplace_back(req.ticket, req.notify, path); | m_Results.emplace_back(req.ticket, req.notify, path); | ||||
m_ShortRequests.pop_back(); | m_ShortRequests.pop_back(); | ||||
} | } | ||||
} | } | ||||
u32 CCmpPathfinder::ComputePathAsync(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass, entity_id_t notify) | u32 CCmpPathfinder::ComputePathAsync(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass, entity_id_t notify) | ||||
{ | { | ||||
LongPathRequest req = { m_NextAsyncTicket++, x0, z0, goal, passClass, notify }; | LongPathRequest req = { m_NextAsyncTicket++, x0, z0, goal, passClass, notify }; | ||||
m_LongPathRequests.push_back(req); | m_LongPathRequests.push_back(req); | ||||
return req.ticket; | return req.ticket; | ||||
} | } | ||||
u32 CCmpPathfinder::ComputeShortPathAsync(entity_pos_t x0, entity_pos_t z0, entity_pos_t clearance, entity_pos_t range, | u32 CCmpPathfinder::ComputeShortPathAsync(entity_pos_t x0, entity_pos_t z0, entity_pos_t clearance, entity_pos_t range, | ||||
const PathGoal& goal, pass_class_t passClass, bool avoidMovingUnits, | const PathGoal& goal, pass_class_t passClass, bool avoidMovingUnits, | ||||
entity_id_t group, entity_id_t notify) | entity_id_t group, entity_id_t notify) | ||||
{ | { | ||||
ShortPathRequest req = { m_NextAsyncTicket++, x0, z0, clearance, range, goal, passClass, avoidMovingUnits, group, notify }; | ShortPathRequest req = { m_NextAsyncTicket++, x0, z0, clearance, range, goal, passClass, avoidMovingUnits, group, notify }; | ||||
m_ShortPathRequests.push_back(req); | m_ShortPathRequests.push_back(req); | ||||
return req.ticket; | return req.ticket; | ||||
} | } | ||||
Done Inline ActionsI don't think that the busy waiting is good here, why not to use std::condition_variable? vladislavbelov: I don't think that the busy waiting is good here, why not to use `std::condition_variable`? | |||||
Done Inline ActionsYou can't use a condition_variable alone to wait on multiple threads. Adding the proper synchronisation would require more code, which has a higher chance of mistakes now and down the line. wraitii: You can't use a `condition_variable` alone to [[ https://stackoverflow. | |||||
Done Inline ActionsYes, in a direct way, but you can wrap it. Like https://stackoverflow.com/questions/24465533/implementing-boostbarrier-in-c11/24465624#24465624. Or use something like boost::barrier.
It's not much more code and it's relatively simple. I'd prefer user's comfort instead of not saving few lines of code. In fact you may freeze the user machine (if it's not so good to predict busy-waiting). Also it may slow down other threads, I mean like renderer one, if it'd exist.
It's useful but it doesn't guarantee a good stable work. It may wake up every tick or sleep for a long time. Could you measure how many percents it and threads do busy waiting? vladislavbelov: Yes, in a direct way, but you can wrap it. Like https://stackoverflow. | |||||
Done Inline ActionsI've ran some benchmarks:
Overall I'll switch to using condition_variable. I think it's a complicated construct, but at the end of the day we can't occupy 100% of a process spin locking. wraitii: I've ran some benchmarks:
- Yield() isn't useful on OS X, and when spin looping hard we are… | |||||
void CCmpPathfinder::ComputePathImmediate(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass, WaypointPath& ret) const | void CCmpPathfinder::ComputePathImmediate(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass, WaypointPath& ret) const | ||||
{ | { | ||||
m_LongPathfinder->ComputePath(*m_PathfinderHier, x0, z0, goal, passClass, ret); | m_LongPathfinder->ComputePath(*m_PathfinderHier, x0, z0, goal, passClass, ret); | ||||
} | } | ||||
WaypointPath CCmpPathfinder::ComputeShortPathImmediate(const ShortPathRequest& request) const | WaypointPath CCmpPathfinder::ComputeShortPathImmediate(const ShortPathRequest& request) const | ||||
{ | { | ||||
return m_VertexPathfinder->ComputeShortPath(request, CmpPtr<ICmpObstructionManager>(GetSystemEntity())); | return m_VertexPathfinder->ComputeShortPath(request, CmpPtr<ICmpObstructionManager>(GetSystemEntity())); | ||||
Not Done Inline Actionsand here as well Silier: and here as well | |||||
} | } | ||||
void CCmpPathfinder::FetchAsyncResultsAndSendMessages() | void CCmpPathfinder::FetchAsyncResultsAndSendMessages() | ||||
{ | { | ||||
PROFILE2("FetchAsyncResults"); | PROFILE2("FetchAsyncResults"); | ||||
Not Done Inline ActionsI think you are missing unlock here, or consider std::lock_guard Silier: I think you are missing unlock here, or consider std::lock_guard | |||||
Not Done Inline Actionswell, ok it is unique lock and it unlocks itself when goes out of scope so it is fine I am just thinking that some unlocks could be removed as they are not needed for unique locks? Silier: well, ok it is unique lock and it unlocks itself when goes out of scope so it is fine
I am… | |||||
// TODO maybe: a possible improvement here would be to push results from workers whenever they are done, and not when all are done. | |||||
Not Done Inline ActionsIs it hard to do ? Stan: Is it hard to do ? | |||||
Done Inline ActionsIt's trickier, but it's also a micro-improvement that needs further testing. wraitii: It's trickier, but it's also a micro-improvement that needs further testing. | |||||
// Wait until all threads have finished computing. | |||||
m_PathfinderFrontier.Watch(); | |||||
GetSimContext().GetSynchronizationData().m_ComputingPaths = false; | |||||
// We may now clear existing requests. | // We may now clear existing requests. | ||||
m_ShortPathRequests.clear(); | m_ShortPathRequests.clear(); | ||||
m_LongPathRequests.clear(); | m_LongPathRequests.clear(); | ||||
// WARNING: the order in which moves are pulled must be consistent when using 1 or n workers. | // WARNING: the order in which moves are pulled must be consistent when using 1 or n workers. | ||||
// We fetch in the same order we inserted in, but we push moves backwards, so this works. | // We fetch in the same order we inserted in, but we push moves backwards, so this works. | ||||
std::vector<PathResult> results; | std::vector<PathResult> results; | ||||
for (PathfinderWorker& worker : m_Workers) | for (PathfinderWorker& worker : m_Workers) | ||||
{ | { | ||||
Done Inline ActionsRealised last night that I need to make sure the results are ordered consistently for threaded/non-threaded/different # of threads here. I think they are by accident right now, but and "ENSURE (is_sorted)" is probably worth adding. wraitii: Realised last night that I need to make sure the results are ordered consistently for… | |||||
results.insert(results.end(), std::make_move_iterator(worker.m_Results.begin()), std::make_move_iterator(worker.m_Results.end())); | results.insert(results.end(), std::make_move_iterator(worker.m_Results.begin()), std::make_move_iterator(worker.m_Results.end())); | ||||
Not Done Inline ActionsHow hard is it to do? + Profile? Stan: How hard is it to do? + Profile? | |||||
Done Inline ActionsIt's hard enough. There's a high chance of dumb bugs, and this whole diff is already stupidly faster than svn. wraitii: It's hard enough. There's a high chance of dumb bugs, and this whole diff is already stupidly… | |||||
worker.m_Results.clear(); | worker.m_Results.clear(); | ||||
} | } | ||||
{ | { | ||||
PROFILE2("PostMessages"); | PROFILE2("PostMessages"); | ||||
for (PathResult& path : results) | for (PathResult& path : results) | ||||
{ | { | ||||
CMessagePathResult msg(path.ticket, path.path); | CMessagePathResult msg(path.ticket, path.path); | ||||
GetSimContext().GetComponentManager().PostMessage(path.notify, msg); | GetSimContext().GetComponentManager().PostMessage(path.notify, msg); | ||||
} | } | ||||
} | } | ||||
} | } | ||||
void CCmpPathfinder::StartProcessingMoves(bool useMax) | void CCmpPathfinder::StartProcessingMoves(bool useMax) | ||||
Not Done Inline ActionsCan't you just move that out of the loop and check for !stop ? Stan: Can't you just move that out of the loop and check for !stop ? | |||||
Done Inline ActionsThis is another spin-lock: we continue looping until all threads are done computing, so it needs to be reset at the beginning of the loop anyways. Putting it outside the loop isn't much neater imo, but the whole thing might be rewritten in a slightly more functional way I think. wraitii: This is another spin-lock: we continue looping until all threads are done computing, so it… | |||||
{ | { | ||||
std::vector<LongPathRequest> longRequests = GetMovesToProcess(m_LongPathRequests, useMax, m_MaxSameTurnMoves); | std::vector<LongPathRequest> longRequests = GetMovesToProcess(m_LongPathRequests, useMax, m_MaxSameTurnMoves); | ||||
std::vector<ShortPathRequest> shortRequests = GetMovesToProcess(m_ShortPathRequests, useMax, m_MaxSameTurnMoves - longRequests.size()); | std::vector<ShortPathRequest> shortRequests = GetMovesToProcess(m_ShortPathRequests, useMax, m_MaxSameTurnMoves - longRequests.size()); | ||||
PushRequestsToWorkers(longRequests); | PushRequestsToWorkers(longRequests); | ||||
PushRequestsToWorkers(shortRequests); | PushRequestsToWorkers(shortRequests); | ||||
m_PathfinderFrontier.Reset(); | |||||
ENSURE(!GetSimContext().GetSynchronizationData().m_ComputingPaths); | |||||
GetSimContext().GetSynchronizationData().m_ComputingPaths = true; | |||||
if (!m_UseThreading) | |||||
{ | |||||
m_Workers.back().Work(*this); | |||||
return; | |||||
} | |||||
for (PathfinderWorker& worker : m_Workers) | for (PathfinderWorker& worker : m_Workers) | ||||
worker.Work(*this); | { | ||||
// Mark as computing to unblock. | |||||
ENSURE(!worker.m_Computing); | |||||
worker.m_Computing = true; | |||||
Done Inline Actionswe could lock/unlock only when using multithreads, branch is already here so ... Silier: we could lock/unlock only when using multithreads, branch is already here so ... | |||||
} | |||||
m_PathfinderConditionVariable.notify_all(); | |||||
Not Done Inline ActionsInvert, early return? Stan: Invert, early return? | |||||
} | } | ||||
template <typename T> | template <typename T> | ||||
std::vector<T> CCmpPathfinder::GetMovesToProcess(std::vector<T>& requests, bool useMax, size_t maxMoves) | std::vector<T> CCmpPathfinder::GetMovesToProcess(std::vector<T>& requests, bool useMax, size_t maxMoves) | ||||
{ | { | ||||
// Keep the original requests in which we need to serialize. | // Keep the original requests in which we need to serialize. | ||||
std::vector<T> copiedRequests; | std::vector<T> copiedRequests; | ||||
if (useMax) | if (useMax) | ||||
Show All 11 Lines | |||||
template <typename T> | template <typename T> | ||||
void CCmpPathfinder::PushRequestsToWorkers(std::vector<T>& from) | void CCmpPathfinder::PushRequestsToWorkers(std::vector<T>& from) | ||||
{ | { | ||||
if (from.empty()) | if (from.empty()) | ||||
return; | return; | ||||
// Trivial load-balancing, / rounds towards zero so add 1 to ensure we do push all requests. | // Trivial load-balancing, / rounds towards zero so add 1 to ensure we do push all requests. | ||||
size_t amount = from.size() / m_Workers.size() + 1; | size_t amount = from.size() / m_Workers.size() + 1; | ||||
Done Inline ActionsThis is a bit of a weird syntax in C++, but it's basically worker[into].insert() in JS. The alternative in C++ would be a macro or an explicit IF, but I think that's fine to use. wraitii: This is a bit of a weird syntax in C++, but it's basically `worker[into].insert()` in JS. The… | |||||
Done Inline ActionsToo unsafe for me. I'd replace the function by: template <typename T, typename U> void CCmpPathfinder::PushRequestsToWorkers(std::vector<T>& from, std::deque<U>& into) With probably a type validation like std::is_same<U, Async*PathRequest>::value. vladislavbelov: Too unsafe for me. I'd replace the function by:
```lang=cpp
template <typename T, typename U>… | |||||
Done Inline ActionsOne can't actually, because I need to call either the longRequest or the shortRequest of the worker in the inner loop. This is basically rewritable as CCmpPathfinder::PushRequestsToWorkers(std::vector<T>& from, bool toLongRequests) [...] if (toLongRequests) worker->m_LongRequests.insert([...]) else worker->m_ShortRequests.insert([...]) If I pass the worker, I lose the whole point of the function as the for-loop has to be outside the function. wraitii: One can't actually, because I need to call either the longRequest or the shortRequest of the… | |||||
Done Inline ActionsIf you wanna leave the function loop, I'd prefer to add more code but with an explicit access. Like: template<typename T> CCmpPathfinder::PushRequestsToWorkers(std::vector<T>& from) { // ... worker->AddRequests<T>( std::make_move_iterator(from.end() - amount), std::make_move_iterator(from.end()) ); // ... } template<typename T> void AsyncPathfinderWorkerThread::AddRequests(... it_begin, ... it_end) { auto& container = RequestsContainer<T>(); container.insert(container.begin(), std::make_move_iterator(it_begin), std::make_move_iterator(it_end)); } template<typename T> void AsyncPathfinderWorkerThread::RequestsContainer(); template<> std::deque<AsyncShortPathRequest>& RequestsContainer() { return m_ShortRequests; } // ... It'd be the same by performance, but safer I think. As you can't pass an arbitrary container member. vladislavbelov: If you wanna leave the function loop, I'd prefer to add more code but with an explicit access. | |||||
Done Inline ActionsI suppose ultimately this would be if-constexpr-ed. I'll go with your way as it does seem more readable and I didn't think of it. wraitii: I suppose ultimately this would be if-constexpr-ed. I'll go with your way as it does seem more… | |||||
Done Inline ActionsDone in a slightly simpler manner. wraitii: Done in a slightly simpler manner. | |||||
// WARNING: the order in which moves are pushed must be consistent when using 1 or n workers. | // WARNING: the order in which moves are pushed must be consistent when using 1 or n workers. | ||||
// In this instance, work is distributed in a strict LIFO order, effectively reversing tickets. | // In this instance, work is distributed in a strict LIFO order, effectively reversing tickets. | ||||
Not Done Inline Actionsrange loop ? Stan: range loop ? | |||||
Done Inline Actionssure :) Kuba386: sure :) | |||||
for (PathfinderWorker& worker : m_Workers) | for (PathfinderWorker& worker : m_Workers) | ||||
{ | { | ||||
// Prevent pushing requests when the worker is computing. | |||||
// Call FetchAsyncResultsAndSendMessages() before pushing new requests. | |||||
Not Done Inline Actionsshould not this comment be elsewhere ? Silier: should not this comment be elsewhere ? | |||||
Not Done Inline ActionsMy intention was that if people did something stupid, they would trigger the assert, and then that comment would tell them why this was a bad idea. wraitii: My intention was that if people did something stupid, they would trigger the assert, and then… | |||||
ENSURE(!worker.m_Computing); | |||||
Not Done Inline Actionsdo we want to just prevent pushing or really fail if some worker is computing ? Silier: do we want to just prevent pushing or really fail if some worker is computing ?
Is computing… | |||||
Not Done Inline ActionsAt this point, we are about to start computing pathes for new turn. Kuba386: At this point, we are about to start computing pathes for new turn.
Fact that we are starting… | |||||
Done Inline ActionsWell, not exaclty 'for new turn', because StartProcessingRequests is called three times during single turn. Kuba386: Well, not exaclty 'for new turn', because StartProcessingRequests is called three times during… | |||||
amount = std::min(amount, from.size()); // Since we are rounding up before, ensure we aren't pushing beyond the end. | amount = std::min(amount, from.size()); // Since we are rounding up before, ensure we aren't pushing beyond the end. | ||||
Not Done Inline ActionsCould not be here used atomic variable instead locking this? Silier: Could not be here used atomic variable instead locking this? | |||||
worker.PushRequests(from, amount); | worker.PushRequests(from, amount); | ||||
from.erase(from.end() - amount, from.end()); | from.erase(from.end() - amount, from.end()); | ||||
} | } | ||||
} | } | ||||
////////////////////////////////////////////////////////// | ////////////////////////////////////////////////////////// | ||||
bool CCmpPathfinder::IsGoalReachable(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass) | bool CCmpPathfinder::IsGoalReachable(entity_pos_t x0, entity_pos_t z0, const PathGoal& goal, pass_class_t passClass) | ||||
Show All 20 Lines | bool CCmpPathfinder::CheckMovement(const IObstructionTestFilter& filter, | ||||
CmpPtr<ICmpObstructionManager> cmpObstructionManager(GetSystemEntity()); | CmpPtr<ICmpObstructionManager> cmpObstructionManager(GetSystemEntity()); | ||||
if (!cmpObstructionManager || cmpObstructionManager->TestLine(filter, x0, z0, x1, z1, r, true)) | if (!cmpObstructionManager || cmpObstructionManager->TestLine(filter, x0, z0, x1, z1, r, true)) | ||||
return false; | return false; | ||||
// Then test against the terrain grid. This should not be necessary | // Then test against the terrain grid. This should not be necessary | ||||
// But in case we allow terrain to change it will become so. | // But in case we allow terrain to change it will become so. | ||||
return Pathfinding::CheckLineMovement(x0, z0, x1, z1, passClass, *m_TerrainOnlyGrid); | return Pathfinding::CheckLineMovement(x0, z0, x1, z1, passClass, *m_TerrainOnlyGrid); | ||||
} | } | ||||
Not Done Inline Actions? lyv: ? | |||||
Done Inline Actionsmutex spin-lock wraitii: mutex spin-lock | |||||
ICmpObstruction::EFoundationCheck CCmpPathfinder::CheckUnitPlacement(const IObstructionTestFilter& filter, | ICmpObstruction::EFoundationCheck CCmpPathfinder::CheckUnitPlacement(const IObstructionTestFilter& filter, | ||||
entity_pos_t x, entity_pos_t z, entity_pos_t r, pass_class_t passClass, bool UNUSED(onlyCenterPoint)) const | entity_pos_t x, entity_pos_t z, entity_pos_t r, pass_class_t passClass, bool UNUSED(onlyCenterPoint)) const | ||||
{ | { | ||||
// Check unit obstruction | // Check unit obstruction | ||||
CmpPtr<ICmpObstructionManager> cmpObstructionManager(GetSystemEntity()); | CmpPtr<ICmpObstructionManager> cmpObstructionManager(GetSystemEntity()); | ||||
if (!cmpObstructionManager) | if (!cmpObstructionManager) | ||||
return ICmpObstruction::FOUNDATION_CHECK_FAIL_ERROR; | return ICmpObstruction::FOUNDATION_CHECK_FAIL_ERROR; | ||||
if (cmpObstructionManager->TestUnitShape(filter, x, z, r, NULL)) | if (cmpObstructionManager->TestUnitShape(filter, x, z, r, NULL)) | ||||
Not Done Inline ActionsThis would throw a compiler warning. for (;;) is the usual standard for infinite loops. lyv: This would throw a compiler warning. `for (;;)` is the usual standard for infinite loops.
It… | |||||
return ICmpObstruction::FOUNDATION_CHECK_FAIL_OBSTRUCTS_FOUNDATION; | return ICmpObstruction::FOUNDATION_CHECK_FAIL_OBSTRUCTS_FOUNDATION; | ||||
// Test against terrain and static obstructions: | // Test against terrain and static obstructions: | ||||
Not Done Inline Actions(we use the actual type instead of auto, so that the reader doesn't have to guess) elexis: (we use the actual type instead of auto, so that the reader doesn't have to guess) | |||||
u16 i, j; | u16 i, j; | ||||
Pathfinding::NearestNavcell(x, z, i, j, m_MapSize*Pathfinding::NAVCELLS_PER_TILE, m_MapSize*Pathfinding::NAVCELLS_PER_TILE); | Pathfinding::NearestNavcell(x, z, i, j, m_MapSize*Pathfinding::NAVCELLS_PER_TILE, m_MapSize*Pathfinding::NAVCELLS_PER_TILE); | ||||
if (!IS_PASSABLE(m_Grid->get(i, j), passClass)) | if (!IS_PASSABLE(m_Grid->get(i, j), passClass)) | ||||
return ICmpObstruction::FOUNDATION_CHECK_FAIL_TERRAIN_CLASS; | return ICmpObstruction::FOUNDATION_CHECK_FAIL_TERRAIN_CLASS; | ||||
// (Static obstructions will be redundantly tested against in both the | // (Static obstructions will be redundantly tested against in both the | ||||
// obstruction-shape test and navcell-passability test, which is slightly | // obstruction-shape test and navcell-passability test, which is slightly | ||||
▲ Show 20 Lines • Show All 57 Lines • Show Last 20 Lines |
If you want to change this line replace the c style cast by a c++ cast :)