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

Changeset View

Standalone View

source/simulation2/components/CCmpPathfinder_Common.h

* This file is part of 0 A.D. * This file is part of 0 A.D.

* *

* 0 A.D. is free software: you can redistribute it and/or modify * 0 A.D. is free software: you can redistribute it and/or modify

* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by

* the Free Software Foundation, either version 2 of the License, or * the Free Software Foundation, either version 2 of the License, or

* (at your option) any later version. * (at your option) any later version.

* *

* 0 A.D. is distributed in the hope that it will be useful, * 0 A.D. is distributed in the hope that it will be useful,

Show All 20 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/ThreadFrontier.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 <thread>

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

Show All 9 Lines

{ {

protected: protected:

class PathfinderWorker class PathfinderWorker

{ {

friend CCmpPathfinder; friend CCmpPathfinder;

public: public:

PathfinderWorker(); PathfinderWorker();

/**

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

* Implement a noexcept move constructor for std::vector that actually does nothing.

PathfinderWorker(PathfinderWorker&&) noexcept

{

ENSURE(!m_Thread.joinable());

}

~PathfinderWorker();

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

/**

* Create the std::thread and call InitThread

void Start(const CCmpPathfinder& pathfinder, size_t index);

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

void PrepareToKill();

/**

* Will loop until a condition_variable notifies us, and call Work().

void WaitForWork(const CCmpPathfinder& pathfinder);

// Process path requests, checking if we should stop before each new one. /**

* Process path requests, checking if we should stop before each new one.

* Should be callable both synchronously and asynchronously.

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

void Work(const CCmpPathfinder& pathfinder); void Work(const CCmpPathfinder& pathfinder);

private: private:

// Insert requests in m_[Long/Short]Requests depending on from. /**

StanUnsubmitted

Not Done

Doxygen ?

Stan: Doxygen ?

// This could be removed when we may use if-constexpr in CCmpPathfinder::PushRequestsToWorkers * Takes care of what needs to be called to initialise the thread before calling WaitForWork().

void InitThread(const CCmpPathfinder& pathfinder, size_t index);

/**

* Insert requests in m_[Long/Short]Requests depending on from.

* This could be removed when we may use if-constexpr in CCmpPathfinder::PushRequestsToWorkers

template<typename T> template<typename T>

void PushRequests(std::vector<T>& from, ssize_t amount); void PushRequests(std::vector<T>& from, ssize_t amount);

// Stores our results, the main thread will fetch this. // Stores our results, the main thread will fetch this.

std::vector<PathResult> m_Results; std::vector<PathResult> m_Results;

std::thread m_Thread;

std::atomic<bool> m_Kill;

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> ```

std::atomic<bool> m_Computing;

std::vector<LongPathRequest> m_LongRequests; std::vector<LongPathRequest> m_LongRequests;

StanUnsubmitted

Not Done

Probably needs

#include <vector>

(likely included in its deps)

Stan: Probably needs ``` #include <vector> ``` (likely included in its deps)

std::vector<ShortPathRequest> m_ShortRequests; std::vector<ShortPathRequest> m_ShortRequests;

// The vertex pathfinder has local caches that cannot be shared across threads

// Since it otherwise has no particular state, we can freely have one for each pathfinder worker.

// (this data could be thread_local, but the performance of that is terrible)

std::unique_ptr<VertexPathfinder> m_VertexPathfinder;

StanUnsubmitted

Not Done

#include <memory>

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

// The same is not true of the long-range pathfinder, which uses the grid, but has no particular thread-local state.

}; };

// Allow the workers to access our private variables // Allow the workers to access our private variables

friend class PathfinderWorker; friend class PathfinderWorker;

public: public:

static void ClassInit(CComponentManager& componentManager) static void ClassInit(CComponentManager& componentManager)

{ {

componentManager.SubscribeToMessageType(MT_Deserialized); componentManager.SubscribeToMessageType(MT_Deserialized);

componentManager.SubscribeToMessageType(MT_RenderSubmit); // for debug overlays componentManager.SubscribeToMessageType(MT_RenderSubmit); // for debug overlays

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

} }

~CCmpPathfinder(); ~CCmpPathfinder();

DEFAULT_COMPONENT_ALLOCATOR(Pathfinder) DEFAULT_COMPONENT_ALLOCATOR(Pathfinder)

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

// Dynamic state: // Dynamic state:

std::vector<LongPathRequest> m_LongPathRequests; std::vector<LongPathRequest> m_LongPathRequests;

std::vector<ShortPathRequest> m_ShortPathRequests; std::vector<ShortPathRequest> m_ShortPathRequests;

u32 m_NextAsyncTicket; // Unique IDs for asynchronous path requests. u32 m_NextAsyncTicket; // Unique IDs for asynchronous path requests.

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.

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

// If threading is used, this will only be used for synchronous calls.

std::unique_ptr<VertexPathfinder> m_VertexPathfinder; std::unique_ptr<VertexPathfinder> m_VertexPathfinder;

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…

std::unique_ptr<HierarchicalPathfinder> m_PathfinderHier; std::unique_ptr<HierarchicalPathfinder> m_PathfinderHier;

std::unique_ptr<LongPathfinder> m_LongPathfinder; std::unique_ptr<LongPathfinder> m_LongPathfinder;

// Workers process pathing requests. // Workers process pathing requests.

StanUnsubmitted

Not Done

Process worker maybe ?

Stan: Process worker maybe ?

StanUnsubmitted

Not Done

Stan: <atomic>

std::vector<PathfinderWorker> m_Workers; std::vector<PathfinderWorker> m_Workers;

bool m_UseThreading = false;

mutable std::mutex m_PathfinderMutex;

StanUnsubmitted

Not Done

#include <mutex>

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

mutable std::condition_variable m_PathfinderConditionVariable;

StanUnsubmitted

Not Done

#include <condition_variable>

Stan: ``` #include <condition_variable> ```

mutable ThreadFrontier m_PathfinderFrontier;

AtlasOverlay* m_AtlasOverlay; AtlasOverlay* m_AtlasOverlay;

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.

static std::string GetSchema() static std::string GetSchema()

{ {

return "<a:component type='system'/><empty/>"; return "<a:component type='system'/><empty/>";

} }

virtual void Init(const CParamNode& paramNode); virtual void Init(const CParamNode& paramNode);

virtual void Deinit(); virtual void Deinit();

template<typename S> template<typename S>

void SerializeCommon(S& serialize); void SerializeCommon(S& serialize);

virtual void Serialize(ISerializer& serialize); virtual void Serialize(ISerializer& serialize);

virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize); virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize);

virtual void HandleMessage(const CMessage& msg, bool global); virtual void HandleMessage(const CMessage& msg, bool global);

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…

virtual pass_class_t GetPassabilityClass(const std::string& name) const; virtual pass_class_t GetPassabilityClass(const std::string& name) const;

virtual void GetPassabilityClasses(std::map<std::string, pass_class_t>& passClasses) const; virtual void GetPassabilityClasses(std::map<std::string, pass_class_t>& passClasses) const;

virtual void GetPassabilityClasses( virtual void GetPassabilityClasses(

std::map<std::string, pass_class_t>& nonPathfindingPassClasses, std::map<std::string, pass_class_t>& nonPathfindingPassClasses,

std::map<std::string, pass_class_t>& pathfindingPassClasses) const; std::map<std::string, pass_class_t>& pathfindingPassClasses) const;

const PathfinderPassability* GetPassabilityFromMask(pass_class_t passClass) const; const PathfinderPassability* GetPassabilityFromMask(pass_class_t passClass) const;

▲ Show 20 Lines • Show All 126 Lines • Show Last 20 Lines