Changeset View
Standalone View
source/simulation2/components/CCmpPathfinder_Common.h
Show All 29 Lines | |||||||||||
#include "simulation2/system/Component.h" | #include "simulation2/system/Component.h" | ||||||||||
#include "ICmpPathfinder.h" | #include "ICmpPathfinder.h" | ||||||||||
#include "graphics/Overlay.h" | #include "graphics/Overlay.h" | ||||||||||
#include "graphics/Terrain.h" | #include "graphics/Terrain.h" | ||||||||||
#include "maths/MathUtil.h" | #include "maths/MathUtil.h" | ||||||||||
#include "ps/CLogger.h" | #include "ps/CLogger.h" | ||||||||||
#include "ps/ThreadPool.h" | |||||||||||
#include "renderer/TerrainOverlay.h" | #include "renderer/TerrainOverlay.h" | ||||||||||
#include "simulation2/components/ICmpObstructionManager.h" | #include "simulation2/components/ICmpObstructionManager.h" | ||||||||||
#include "simulation2/helpers/Grid.h" | #include "simulation2/helpers/Grid.h" | ||||||||||
#include <vector> | #include <vector> | ||||||||||
StanUnsubmitted Not Done Inline Actions
Stan: | |||||||||||
class HierarchicalPathfinder; | class HierarchicalPathfinder; | ||||||||||
class LongPathfinder; | class LongPathfinder; | ||||||||||
class VertexPathfinder; | class VertexPathfinder; | ||||||||||
class SceneCollector; | class SceneCollector; | ||||||||||
class AtlasOverlay; | class AtlasOverlay; | ||||||||||
#ifdef NDEBUG | #ifdef NDEBUG | ||||||||||
#define PATHFIND_DEBUG 0 | #define PATHFIND_DEBUG 0 | ||||||||||
#else | #else | ||||||||||
#define PATHFIND_DEBUG 1 | #define PATHFIND_DEBUG 1 | ||||||||||
#endif | #endif | ||||||||||
/** | /** | ||||||||||
* Implementation of ICmpPathfinder | * Implementation of ICmpPathfinder | ||||||||||
*/ | */ | ||||||||||
class CCmpPathfinder final : public ICmpPathfinder | class CCmpPathfinder final : public ICmpPathfinder | ||||||||||
{ | { | ||||||||||
public: | public: | ||||||||||
Not Done Inline ActionsDoxygen ? Stan: Doxygen ? | |||||||||||
Not Done Inline ActionsDoxygen ? Stan: Doxygen ? | |||||||||||
Not Done Inline ActionsDoxygen ? Stan: Doxygen ? | |||||||||||
Not Done Inline ActionsProbably needs #include <vector> (likely included in its deps) Stan: Probably needs
```
#include <vector>
```
(likely included in its deps) | |||||||||||
static void ClassInit(CComponentManager& componentManager) | static void ClassInit(CComponentManager& componentManager) | ||||||||||
{ | { | ||||||||||
Not Done Inline ActionsWhy do you need std::atomic if it's private? vladislavbelov: Why do you need `std::atomic` if it's private? | |||||||||||
Done Inline ActionsIt's still called from 2 threads, as the main thread calls PrepareToKill(), which sets m_Kill = true, and it's checked for in the worker thread. wraitii: It's still called from 2 threads, as the main thread calls PrepareToKill(), which sets m_Kill =… | |||||||||||
Not Done Inline Actions#include <atomic> Stan: ```
#include <atomic>
``` | |||||||||||
componentManager.SubscribeToMessageType(MT_Deserialized); | componentManager.SubscribeToMessageType(MT_Deserialized); | ||||||||||
componentManager.SubscribeToMessageType(MT_RenderSubmit); // for debug overlays | componentManager.SubscribeToMessageType(MT_RenderSubmit); // for debug overlays | ||||||||||
Not Done Inline Actions#include <memory> Stan: ```
#include <memory>
``` | |||||||||||
componentManager.SubscribeToMessageType(MT_TerrainChanged); | componentManager.SubscribeToMessageType(MT_TerrainChanged); | ||||||||||
Done Inline ActionsSections should be in order: public > protected > private. vladislavbelov: Sections should be in order: public > protected > private. | |||||||||||
componentManager.SubscribeToMessageType(MT_WaterChanged); | componentManager.SubscribeToMessageType(MT_WaterChanged); | ||||||||||
componentManager.SubscribeToMessageType(MT_ObstructionMapShapeChanged); | componentManager.SubscribeToMessageType(MT_ObstructionMapShapeChanged); | ||||||||||
} | } | ||||||||||
Not Done Inline ActionsDoxygen ? Stan: Doxygen ? | |||||||||||
~CCmpPathfinder(); | ~CCmpPathfinder(); | ||||||||||
DEFAULT_COMPONENT_ALLOCATOR(Pathfinder) | DEFAULT_COMPONENT_ALLOCATOR(Pathfinder) | ||||||||||
Not Done Inline ActionsDoxygen ? Stan: Doxygen ? | |||||||||||
// Template state: | // Template state: | ||||||||||
std::map<std::string, pass_class_t> m_PassClassMasks; | std::map<std::string, pass_class_t> m_PassClassMasks; | ||||||||||
Done Inline ActionsWhy comments states that the function uses conditional_variable but in fact it uses a simple busy waiting. vladislavbelov: Why comments states that the function uses `conditional_variable` but in fact it uses a simple… | |||||||||||
Done Inline ActionsEarlier version of the code used a condition_variable, I've changed it since. The thing is I need a m_Computing predicate anyways for the condition-variable, and possibly a mutex (I had issues which look a bit like this). wraitii: Earlier version of the code used a condition_variable, I've changed it since. The thing is I… | |||||||||||
Done Inline ActionsShould or is ? Stan: Should or is ? | |||||||||||
Done Inline ActionsShould. It also is, but it should be and remain so in the future. wraitii: Should. It also is, but it should be and remain so in the future. | |||||||||||
std::vector<PathfinderPassability> m_PassClasses; | std::vector<PathfinderPassability> m_PassClasses; | ||||||||||
u16 m_MaxSameTurnMoves; // Compute only this many paths when useMax is true in StartProcessingMoves. | u16 m_MaxSameTurnMoves; // Compute only this many paths when useMax is true in StartProcessingMoves. | ||||||||||
// Dynamic state: | // Dynamic state: | ||||||||||
// Lazily-constructed dynamic state (not serialized): | // Lazily-constructed dynamic state (not serialized): | ||||||||||
u16 m_MapSize; // tiles per side | u16 m_MapSize; // tiles per side | ||||||||||
Grid<NavcellData>* m_Grid; // terrain/passability information | Grid<NavcellData>* m_Grid; // terrain/passability information | ||||||||||
Grid<NavcellData>* m_TerrainOnlyGrid; // same as m_Grid, but only with terrain, to avoid some recomputations | Grid<NavcellData>* m_TerrainOnlyGrid; // same as m_Grid, but only with terrain, to avoid some recomputations | ||||||||||
// Keep clever updates in memory to avoid memory fragmentation from the grid. | // Keep clever updates in memory to avoid memory fragmentation from the grid. | ||||||||||
// This should be used only in UpdateGrid(), there is no guarantee the data is properly initialized anywhere else. | // This should be used only in UpdateGrid(), there is no guarantee the data is properly initialized anywhere else. | ||||||||||
GridUpdateInformation m_DirtinessInformation; | GridUpdateInformation m_DirtinessInformation; | ||||||||||
// The data from clever updates is stored for the AI manager | // The data from clever updates is stored for the AI manager | ||||||||||
GridUpdateInformation m_AIPathfinderDirtinessInformation; | GridUpdateInformation m_AIPathfinderDirtinessInformation; | ||||||||||
bool m_TerrainDirty; | bool m_TerrainDirty; | ||||||||||
std::unique_ptr<VertexPathfinder> m_VertexPathfinder; | std::vector<VertexPathfinder> m_VertexPathfinders; | ||||||||||
std::unique_ptr<HierarchicalPathfinder> m_PathfinderHier; | std::unique_ptr<HierarchicalPathfinder> m_PathfinderHier; | ||||||||||
std::unique_ptr<LongPathfinder> m_LongPathfinder; | std::unique_ptr<LongPathfinder> m_LongPathfinder; | ||||||||||
mutable std::mutex m_PathfinderMutex; | |||||||||||
mutable std::condition_variable m_PathfinderConditionVariable; | |||||||||||
// One per live asynchronous path computing task. | |||||||||||
std::vector<Future<void>> m_Futures; | |||||||||||
template<typename T> | template<typename T> | ||||||||||
Not Done Inline ActionsProcess worker maybe ? Stan: Process worker maybe ? | |||||||||||
class PathRequests { | class PathRequests { | ||||||||||
public: | public: | ||||||||||
Not Done Inline Actions#include <mutex> Stan: ```
#include <mutex>
``` | |||||||||||
std::vector<T> m_Requests; | std::vector<T> m_Requests; | ||||||||||
Not Done Inline Actions#include <condition_variable> Stan: ```
#include <condition_variable>
``` | |||||||||||
std::vector<PathResult> m_Results; | std::vector<PathResult> m_Results; | ||||||||||
// This is the array index of the next path to compute. | // This is the array index of the next path to compute. | ||||||||||
size_t m_NextPathToCompute = 0; | std::atomic<size_t> m_NextPathToCompute = 0; | ||||||||||
Not Done Inline Actions<atomic> Stan: <atomic> | |||||||||||
// This is false until all scheduled paths have been computed. | // This is false until all scheduled paths have been computed. | ||||||||||
Done Inline ActionsForgot to use that in this diff, but it'll be in the next. wraitii: Forgot to use that in this diff, but it'll be in the next. | |||||||||||
bool m_ComputeDone = true; | std::atomic<bool> m_ComputeDone = true; | ||||||||||
void ClearComputed() | void ClearComputed() | ||||||||||
{ | { | ||||||||||
if (m_Results.size() == m_Requests.size()) | if (m_Results.size() == m_Requests.size()) | ||||||||||
Done Inline ActionsInitialization in Init ? Stan: Initialization in Init ? | |||||||||||
Done Inline ActionsI think it's better to init in the definition whenever possible going forward. Not sure if that's a convention we have or not? wraitii: I think it's better to init in the definition whenever possible going forward. Not sure if… | |||||||||||
m_Requests.clear(); | m_Requests.clear(); | ||||||||||
else | else | ||||||||||
m_Requests.erase(m_Requests.end() - m_Results.size(), m_Requests.end()); | m_Requests.erase(m_Requests.end() - m_Results.size(), m_Requests.end()); | ||||||||||
m_Results.clear(); | m_Results.clear(); | ||||||||||
} | } | ||||||||||
/** | /** | ||||||||||
* @param max - if non-zero, how many paths to process. | * @param max - if non-zero, how many paths to process. | ||||||||||
*/ | */ | ||||||||||
void PrepareForComputation(u16 max) | void PrepareForComputation(u16 max) | ||||||||||
{ | { | ||||||||||
size_t n = m_Requests.size(); | size_t n = m_Requests.size(); | ||||||||||
if (max && n > max) | if (max && n > max) | ||||||||||
n = max; | n = max; | ||||||||||
Not Done Inline ActionsIt'd be good to fix names (to follow CC). Also as it'd be good to use "cache" word as the variables are used inside const methods. vladislavbelov: It'd be good to fix names (to follow CC). Also as it'd be good to use "cache" word as the… | |||||||||||
Done Inline ActionsThese were simply moved, and I think I'll refactor this in a VertexPathfinder class anyways that this calls (like the LongPathfinder) instead of what I've done here. wraitii: These were simply moved, and I think I'll refactor this in a VertexPathfinder class anyways… | |||||||||||
m_NextPathToCompute = 0; | m_NextPathToCompute = 0; | ||||||||||
m_Results.resize(n); | m_Results.resize(n); | ||||||||||
m_ComputeDone = n == 0; | m_ComputeDone = n == 0; | ||||||||||
} | } | ||||||||||
template<typename U> | template<typename U> | ||||||||||
void Compute(const CCmpPathfinder& cmpPathfinder, const U& pathfinder); | void Compute(const CCmpPathfinder& cmpPathfinder, const U& pathfinder); | ||||||||||
}; | }; | ||||||||||
▲ Show 20 Lines • Show All 159 Lines • Show Last 20 Lines |