Differential D14 Diff 17948 source/simulation2/components/CCmpPathfinder_Common.h

Changeset View

Standalone View

View Options

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/TaskManager.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

#include "simulation2/helpers/Grid.h"

+ #include <atomic>

#include <vector>

class HierarchicalPathfinder;

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:

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

StanUnsubmitted

Not Done

Probably 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)

{ {

vladislavbelovUnsubmitted

Not Done

Why do you need std::atomic if it's private?

vladislavbelov: Why do you need `std::atomic` if it's private?

wraitiiAuthorUnsubmitted

Done

It'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 =…

StanUnsubmitted

Not Done

#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

StanUnsubmitted

Not Done

#include <memory>

Stan: ``` #include <memory> ```

componentManager.SubscribeToMessageType(MT_TerrainChanged); componentManager.SubscribeToMessageType(MT_TerrainChanged);

vladislavbelovUnsubmitted

Done

Sections 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);

} }

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

~CCmpPathfinder(); ~CCmpPathfinder();

DEFAULT_COMPONENT_ALLOCATOR(Pathfinder) DEFAULT_COMPONENT_ALLOCATOR(Pathfinder)

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

// Template state: // Template state:

std::map<std::string, pass_class_t> m_PassClassMasks; std::map<std::string, pass_class_t> m_PassClassMasks;

vladislavbelovUnsubmitted

Done

Why 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…

wraitiiAuthorUnsubmitted

Done

Earlier 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).
Again, I probably need a std::this_thread::yield() here, but I think the busy-waiting on an atomic variable is simpler and safer.

wraitii: Earlier version of the code used a condition_variable, I've changed it since. The thing is I…

StanUnsubmitted

Done

Should or is ?

Stan: Should or is ?

wraitiiAuthorUnsubmitted

Done

Should. 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_GridSize; // Navcells per side of the map. u16 m_GridSize; // Navcells per side of the map.

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;

// One per live asynchronous path computing task.

std::vector<Future<void>> m_Futures;

template<typename T> template<typename T>

StanUnsubmitted

Not Done

Process worker maybe ?

Stan: Process worker maybe ?

class PathRequests { class PathRequests {

public: public:

StanUnsubmitted

Not Done

#include <mutex>

Stan: ``` #include <mutex> ```

std::vector<T> m_Requests; std::vector<T> m_Requests;

StanUnsubmitted

Not Done

#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;

StanUnsubmitted

Not Done

Stan: <atomic>

// This is false until all scheduled paths have been computed. // This is false until all scheduled paths have been computed.

wraitiiAuthorUnsubmitted

Done

Forgot 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())

StanUnsubmitted

Done

Initialization in Init ?

Stan: Initialization in Init ?

wraitiiAuthorUnsubmitted

Done

I 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;

m_NextPathToCompute = 0; m_NextPathToCompute = 0;

m_Results.resize(n); m_Results.resize(n);

vladislavbelovUnsubmitted

Not Done

It'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…

wraitiiAuthorUnsubmitted

Done

These 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_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);

}; };

PathRequests<LongPathRequest> m_LongPathRequests; PathRequests<LongPathRequest> m_LongPathRequests;

PathRequests<ShortPathRequest> m_ShortPathRequests; PathRequests<ShortPathRequest> m_ShortPathRequests;

▲ Show 20 Lines • Show All 157 Lines • Show Last 20 Lines