Changeset View
Standalone View
source/simulation2/components/CCmpTerritoryManager.cpp
/* Copyright (C) 2017 Wildfire Games. | /* Copyright (C) 2018 Wildfire Games. | ||||
* 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 20 Lines • Show All 87 Lines • ▼ Show 20 Lines | public: | ||||
// Set to true when territories change; will send a TerritoriesChanged message | // Set to true when territories change; will send a TerritoriesChanged message | ||||
// during the Update phase | // during the Update phase | ||||
bool m_TriggerEvent; | bool m_TriggerEvent; | ||||
struct SBoundaryLine | struct SBoundaryLine | ||||
{ | { | ||||
bool blinking; | bool blinking; | ||||
player_id_t owner; | |||||
CColor color; | CColor color; | ||||
SOverlayTexturedLine overlay; | SOverlayTexturedLine overlay; | ||||
}; | }; | ||||
std::vector<SBoundaryLine> m_BoundaryLines; | std::vector<SBoundaryLine> m_BoundaryLines; | ||||
bool m_BoundaryLinesDirty; | bool m_BoundaryLinesDirty; | ||||
double m_AnimTime; // time since start of rendering, in seconds | double m_AnimTime; // time since start of rendering, in seconds | ||||
Show All 10 Lines | virtual void Init(const CParamNode& UNUSED(paramNode)) | ||||
m_DebugOverlay = NULL; | m_DebugOverlay = NULL; | ||||
// m_DebugOverlay = new TerritoryOverlay(*this); | // m_DebugOverlay = new TerritoryOverlay(*this); | ||||
m_BoundaryLinesDirty = true; | m_BoundaryLinesDirty = true; | ||||
m_TriggerEvent = true; | m_TriggerEvent = true; | ||||
m_EnableLineDebugOverlays = false; | m_EnableLineDebugOverlays = false; | ||||
m_DirtyID = 1; | m_DirtyID = 1; | ||||
m_DirtyBlinkingID = 1; | m_DirtyBlinkingID = 1; | ||||
m_Visible = true; | m_Visible = true; | ||||
m_ColorChanged = false; | |||||
m_AnimTime = 0.0; | m_AnimTime = 0.0; | ||||
m_TerritoryTotalPassableCellCount = 0; | m_TerritoryTotalPassableCellCount = 0; | ||||
// Register Relax NG validator | // Register Relax NG validator | ||||
CXeromyces::AddValidator(g_VFS, "territorymanager", "simulation/data/territorymanager.rng"); | CXeromyces::AddValidator(g_VFS, "territorymanager", "simulation/data/territorymanager.rng"); | ||||
Show All 12 Lines | virtual void Deinit() | ||||
SAFE_DELETE(m_Territories); | SAFE_DELETE(m_Territories); | ||||
SAFE_DELETE(m_CostGrid); | SAFE_DELETE(m_CostGrid); | ||||
SAFE_DELETE(m_DebugOverlay); | SAFE_DELETE(m_DebugOverlay); | ||||
} | } | ||||
virtual void Serialize(ISerializer& serialize) | virtual void Serialize(ISerializer& serialize) | ||||
{ | { | ||||
// Territory state can be recomputed as required, so we don't need to serialize any of it. | // Territory state can be recomputed as required, so we don't need to serialize any of it. | ||||
serialize.Bool("trigger event", m_TriggerEvent); | serialize.Bool("trigger event", m_TriggerEvent); | ||||
elexis: Here we see the evidence that no proposed change causes an OOS in the TerritoryManager… | |||||
} | } | ||||
virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize) | virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize) | ||||
{ | { | ||||
Init(paramNode); | Init(paramNode); | ||||
deserialize.Bool("trigger event", m_TriggerEvent); | deserialize.Bool("trigger event", m_TriggerEvent); | ||||
} | } | ||||
virtual void HandleMessage(const CMessage& msg, bool UNUSED(global)) | virtual void HandleMessage(const CMessage& msg, bool UNUSED(global)) | ||||
{ | { | ||||
switch (msg.GetType()) | switch (msg.GetType()) | ||||
{ | { | ||||
case MT_OwnershipChanged: | case MT_OwnershipChanged: | ||||
{ | { | ||||
const CMessageOwnershipChanged& msgData = static_cast<const CMessageOwnershipChanged&> (msg); | const CMessageOwnershipChanged& msgData = static_cast<const CMessageOwnershipChanged&> (msg); | ||||
MakeDirtyIfRelevantEntity(msgData.entity); | MakeDirtyIfRelevantEntity(msgData.entity); | ||||
break; | break; | ||||
} | } | ||||
case MT_PlayerColorChanged: | case MT_PlayerColorChanged: | ||||
{ | { | ||||
MakeDirty(); | MakeDirty(); | ||||
break; | break; | ||||
Not Done Inline ActionsIsn't that an unrelated diff? elexis: Isn't that an unrelated diff?
trigger event? Sounds like trigger script, can you clarify? | |||||
Not Done Inline ActionsThe difference between the above lines and MakeDirty() is the latter says m_TriggerEvent = true;. In that case MT_Update sends a territory changed message, which is used in AIInterface.js and serialized there. Since it's sent by the player who toggled diplomacy colors but not by the player who didn't, they'll go out of sync. temple: The difference between the above lines and `MakeDirty()` is the latter says `m_TriggerEvent =… | |||||
} | } | ||||
case MT_PositionChanged: | case MT_PositionChanged: | ||||
{ | { | ||||
const CMessagePositionChanged& msgData = static_cast<const CMessagePositionChanged&> (msg); | const CMessagePositionChanged& msgData = static_cast<const CMessagePositionChanged&> (msg); | ||||
MakeDirtyIfRelevantEntity(msgData.entity); | MakeDirtyIfRelevantEntity(msgData.entity); | ||||
break; | break; | ||||
} | } | ||||
case MT_ValueModification: | case MT_ValueModification: | ||||
▲ Show 20 Lines • Show All 63 Lines • ▼ Show 20 Lines | // m_DebugOverlay = new TerritoryOverlay(*this); | ||||
// To support lazy updates of territory render data, | // To support lazy updates of territory render data, | ||||
// we maintain a DirtyID here and increment it whenever territories change; | // we maintain a DirtyID here and increment it whenever territories change; | ||||
// if a caller has a lower DirtyID then it needs to be updated. | // if a caller has a lower DirtyID then it needs to be updated. | ||||
// We also do the same thing for blinking updates using DirtyBlinkingID. | // We also do the same thing for blinking updates using DirtyBlinkingID. | ||||
size_t m_DirtyID; | size_t m_DirtyID; | ||||
size_t m_DirtyBlinkingID; | size_t m_DirtyBlinkingID; | ||||
bool m_ColorChanged; | |||||
Not Done Inline ActionsExamining the correctness of this line, the only deduction from assuming this to be wrong line of code I could find is the assumption that it's unneded. Reading the surroudning code we notice MakeDirty which might already satisfy the components need to update the territory texture color. No need to estimate if it's a critical performance improvement, because MakeDirty sends a simulation message and that's something we have to avoid. So all good. elexis: Examining the correctness of this line, the only deduction from assuming this to be wrong line… | |||||
void MakeDirty() | void MakeDirty() | ||||
{ | { | ||||
SAFE_DELETE(m_Territories); | SAFE_DELETE(m_Territories); | ||||
++m_DirtyID; | ++m_DirtyID; | ||||
m_BoundaryLinesDirty = true; | m_BoundaryLinesDirty = true; | ||||
m_TriggerEvent = true; | m_TriggerEvent = true; | ||||
} | } | ||||
virtual bool NeedUpdate(size_t* dirtyID) const | virtual bool NeedUpdate(size_t* dirtyID) | ||||
Not Done Inline ActionsAny reason for the const; removal ? I'm don't really know about those things is it because you do not return .kewords but an actual variable ? Stan: Any reason for the const; removal ? I'm don't really know about those things is it because you… | |||||
Not Done Inline ActionsHere const means that the function shouldn't change any of the class data, in this case m_DirtyID or m_ColorChanged. We don't change the value of m_DirtyID, but we do change the value of m_ColorChanged, so that means that the function can't be const. temple: Here const means that the function shouldn't change any of the class data, in this case… | |||||
{ | |||||
if (*dirtyID != m_DirtyID) | |||||
{ | { | ||||
bool ret = *dirtyID != m_DirtyID || m_ColorChanged; | |||||
*dirtyID = m_DirtyID; | *dirtyID = m_DirtyID; | ||||
return true; | m_ColorChanged = false; | ||||
} | return ret; | ||||
return false; | |||||
} | } | ||||
virtual bool NeedUpdate(size_t* dirtyID, size_t* dirtyBlinkingID) const | virtual bool NeedUpdate(size_t* dirtyID, size_t* dirtyBlinkingID) const | ||||
{ | { | ||||
if (*dirtyID != m_DirtyID || *dirtyBlinkingID != m_DirtyBlinkingID) | if (*dirtyID != m_DirtyID || *dirtyBlinkingID != m_DirtyBlinkingID) | ||||
{ | { | ||||
*dirtyID = m_DirtyID; | *dirtyID = m_DirtyID; | ||||
*dirtyBlinkingID = m_DirtyBlinkingID; | *dirtyBlinkingID = m_DirtyBlinkingID; | ||||
return true; | return true; | ||||
} | } | ||||
return false; | return false; | ||||
} | } | ||||
Not Done Inline ActionsDoes this function need to check for the player color too? elexis: Does this function need to check for the player color too?
Their purpose is not commented in… | |||||
Not Done Inline ActionsRename sounds good. (Added in D910.) temple: Rename sounds good. (Added in D910.) | |||||
Not Done Inline ActionsReading the calls to this a bit more, it seems that the purpose of both functions is that other components and graphics files can be kept in sync with this. So the previous function names are actually ok in the previous code. The "could also have other consequences in the simulation" part of D910 makes me think it could be relevant for the simulation in general too. Could go for NeedUpdateState or leave as is, since the other function name is distinguished with the Texture word. (Adding the non-serialized playercolor condition to the one function is correct as it's only used from graphics/). (Perhaps some knucklehead will try to use the texture function in the sim and expect it to be sim-safe. That might be avoided by just adding getters for the three variables and deleting the two above functions, but who cares now. Commit your patch please X) ) elexis: Reading the calls to this a bit more, it seems that the purpose of both functions is that other… | |||||
void CalculateCostGrid(); | void CalculateCostGrid(); | ||||
void CalculateTerritories(); | void CalculateTerritories(); | ||||
u8 GetTerritoryPercentage(player_id_t player); | u8 GetTerritoryPercentage(player_id_t player); | ||||
std::vector<STerritoryBoundary> ComputeBoundaries(); | std::vector<STerritoryBoundary> ComputeBoundaries(); | ||||
void UpdateBoundaryLines(); | void UpdateBoundaryLines(); | ||||
void Interpolate(float frameTime, float frameOffset); | void Interpolate(float frameTime, float frameOffset); | ||||
void RenderSubmit(SceneCollector& collector); | void RenderSubmit(SceneCollector& collector); | ||||
void SetVisibility(bool visible) | void SetVisibility(bool visible) | ||||
{ | { | ||||
m_Visible = visible; | m_Visible = visible; | ||||
} | } | ||||
void UpdateColors(); | |||||
private: | private: | ||||
bool m_Visible; | bool m_Visible; | ||||
}; | }; | ||||
REGISTER_COMPONENT_TYPE(TerritoryManager) | REGISTER_COMPONENT_TYPE(TerritoryManager) | ||||
// Tile data type, for easier accessing of coordinates | // Tile data type, for easier accessing of coordinates | ||||
▲ Show 20 Lines • Show All 301 Lines • ▼ Show 20 Lines | void CCmpTerritoryManager::UpdateBoundaryLines() | ||||
for (size_t i = 0; i < boundaries.size(); ++i) | for (size_t i = 0; i < boundaries.size(); ++i) | ||||
{ | { | ||||
if (boundaries[i].points.empty()) | if (boundaries[i].points.empty()) | ||||
continue; | continue; | ||||
CColor color(1, 0, 1, 1); | CColor color(1, 0, 1, 1); | ||||
CmpPtr<ICmpPlayer> cmpPlayer(GetSimContext(), cmpPlayerManager->GetPlayerByID(boundaries[i].owner)); | CmpPtr<ICmpPlayer> cmpPlayer(GetSimContext(), cmpPlayerManager->GetPlayerByID(boundaries[i].owner)); | ||||
if (cmpPlayer) | if (cmpPlayer) | ||||
color = cmpPlayer->GetColor(); | color = cmpPlayer->GetDisplayedColor(); | ||||
m_BoundaryLines.push_back(SBoundaryLine()); | m_BoundaryLines.push_back(SBoundaryLine()); | ||||
m_BoundaryLines.back().blinking = boundaries[i].blinking; | m_BoundaryLines.back().blinking = boundaries[i].blinking; | ||||
m_BoundaryLines.back().owner = boundaries[i].owner; | |||||
Not Done Inline ActionsOfftopic, but a bit awkward to push an empty struct and then change the property of the back of the vector, rather than initializing a struct fully and pushing it when it's done. elexis: Offtopic, but a bit awkward to push an empty struct and then change the property of the back of… | |||||
m_BoundaryLines.back().color = color; | m_BoundaryLines.back().color = color; | ||||
m_BoundaryLines.back().overlay.m_SimContext = &GetSimContext(); | m_BoundaryLines.back().overlay.m_SimContext = &GetSimContext(); | ||||
m_BoundaryLines.back().overlay.m_TextureBase = textureBase; | m_BoundaryLines.back().overlay.m_TextureBase = textureBase; | ||||
m_BoundaryLines.back().overlay.m_TextureMask = textureMask; | m_BoundaryLines.back().overlay.m_TextureMask = textureMask; | ||||
m_BoundaryLines.back().overlay.m_Color = color; | m_BoundaryLines.back().overlay.m_Color = color; | ||||
m_BoundaryLines.back().overlay.m_Thickness = m_BorderThickness; | m_BoundaryLines.back().overlay.m_Thickness = m_BorderThickness; | ||||
m_BoundaryLines.back().overlay.m_Closed = true; | m_BoundaryLines.back().overlay.m_Closed = true; | ||||
Show All 31 Lines | void CCmpTerritoryManager::Interpolate(float frameTime, float UNUSED(frameOffset)) | ||||
m_AnimTime += frameTime; | m_AnimTime += frameTime; | ||||
if (m_BoundaryLinesDirty) | if (m_BoundaryLinesDirty) | ||||
{ | { | ||||
UpdateBoundaryLines(); | UpdateBoundaryLines(); | ||||
m_BoundaryLinesDirty = false; | m_BoundaryLinesDirty = false; | ||||
} | } | ||||
for (size_t i = 0; i < m_BoundaryLines.size(); ++i) | for (size_t i = 0; i < m_BoundaryLines.size(); ++i) | ||||
Not Done Inline Actions(could use range-based loop) elexis: (could use range-based loop) | |||||
{ | { | ||||
if (m_BoundaryLines[i].blinking) | if (m_BoundaryLines[i].blinking) | ||||
{ | { | ||||
CColor c = m_BoundaryLines[i].color; | CColor c = m_BoundaryLines[i].color; | ||||
c.a *= 0.2f + 0.8f * fabsf((float)cos(m_AnimTime * M_PI)); // TODO: should let artists tweak this | c.a *= 0.2f + 0.8f * fabsf((float)cos(m_AnimTime * M_PI)); // TODO: should let artists tweak this | ||||
m_BoundaryLines[i].overlay.m_Color = c; | m_BoundaryLines[i].overlay.m_Color = c; | ||||
} | } | ||||
} | } | ||||
} | } | ||||
void CCmpTerritoryManager::RenderSubmit(SceneCollector& collector) | void CCmpTerritoryManager::RenderSubmit(SceneCollector& collector) | ||||
{ | { | ||||
if (!m_Visible) | if (!m_Visible) | ||||
return; | return; | ||||
for (size_t i = 0; i < m_BoundaryLines.size(); ++i) | for (size_t i = 0; i < m_BoundaryLines.size(); ++i) | ||||
collector.Submit(&m_BoundaryLines[i].overlay); | collector.Submit(&m_BoundaryLines[i].overlay); | ||||
for (size_t i = 0; i < m_DebugBoundaryLineNodes.size(); ++i) | for (size_t i = 0; i < m_DebugBoundaryLineNodes.size(); ++i) | ||||
collector.Submit(&m_DebugBoundaryLineNodes[i]); | collector.Submit(&m_DebugBoundaryLineNodes[i]); | ||||
Not Done Inline Actionssame elexis: same | |||||
} | } | ||||
player_id_t CCmpTerritoryManager::GetOwner(entity_pos_t x, entity_pos_t z) | player_id_t CCmpTerritoryManager::GetOwner(entity_pos_t x, entity_pos_t z) | ||||
{ | { | ||||
u16 i, j; | u16 i, j; | ||||
if (!m_Territories) | if (!m_Territories) | ||||
{ | { | ||||
▲ Show 20 Lines • Show All 94 Lines • ▼ Show 20 Lines | bool CCmpTerritoryManager::IsTerritoryBlinking(entity_pos_t x, entity_pos_t z) | ||||
if (!m_Territories) | if (!m_Territories) | ||||
return false; | return false; | ||||
u16 i, j; | u16 i, j; | ||||
NearestTerritoryTile(x, z, i, j, m_Territories->m_W, m_Territories->m_H); | NearestTerritoryTile(x, z, i, j, m_Territories->m_W, m_Territories->m_H); | ||||
return (m_Territories->get(i, j) & TERRITORY_BLINKING_MASK) != 0; | return (m_Territories->get(i, j) & TERRITORY_BLINKING_MASK) != 0; | ||||
} | } | ||||
void CCmpTerritoryManager::UpdateColors() | |||||
Not Done Inline Actions(UpdateColor would be consistent with the other components, but the other components are entity components while this one is a system component, so maybe its good, whatever ? ) elexis: (UpdateColor would be consistent with the other components, but the other components are entity… | |||||
Not Done Inline ActionsYeah, this one deals with more than one color so I went for the plural. temple: Yeah, this one deals with more than one color so I went for the plural. | |||||
{ | |||||
m_ColorChanged = true; | |||||
CmpPtr<ICmpPlayerManager> cmpPlayerManager(GetSystemEntity()); | |||||
if (!cmpPlayerManager) | |||||
return; | |||||
for (SBoundaryLine& boundaryLine : m_BoundaryLines) | |||||
Not Done Inline ActionsCould use a range based loop for (const something& boundaryLine : m_BoundaryLines), possibly some other loops in the diff too elexis: Could use a range based loop `for (const something& boundaryLine : m_BoundaryLines)`, possibly… | |||||
{ | |||||
CmpPtr<ICmpPlayer> cmpPlayer(GetSimContext(), cmpPlayerManager->GetPlayerByID(boundaryLine.owner)); | |||||
if (cmpPlayer) | |||||
Done Inline Actionsmaybe continue elexis: maybe continue | |||||
{ | |||||
Done Inline ActionsI guess this should be written in two lines rather than one? temple: I guess this should be written in two lines rather than one? | |||||
boundaryLine.color = cmpPlayer->GetDisplayedColor(); | |||||
boundaryLine.overlay.m_Color = boundaryLine.color; | |||||
} | |||||
} | |||||
} | |||||
Not Done Inline ActionsFunction correct, RenderSubmit pulls the overlays each frame, so we don't need to do any further calls to apply the new color as far as I see. elexis: Function correct, `RenderSubmit` pulls the overlays each frame, so we don't need to do any… | |||||
TerritoryOverlay::TerritoryOverlay(CCmpTerritoryManager& manager) : | TerritoryOverlay::TerritoryOverlay(CCmpTerritoryManager& manager) : | ||||
TerrainTextureOverlay((float)Pathfinding::NAVCELLS_PER_TILE / ICmpTerritoryManager::NAVCELLS_PER_TERRITORY_TILE), | TerrainTextureOverlay((float)Pathfinding::NAVCELLS_PER_TILE / ICmpTerritoryManager::NAVCELLS_PER_TERRITORY_TILE), | ||||
m_TerritoryManager(manager) | m_TerritoryManager(manager) | ||||
{ } | { } | ||||
void TerritoryOverlay::BuildTextureRGBA(u8* data, size_t w, size_t h) | void TerritoryOverlay::BuildTextureRGBA(u8* data, size_t w, size_t h) | ||||
{ | { | ||||
for (size_t j = 0; j < h; ++j) | for (size_t j = 0; j < h; ++j) | ||||
Show All 15 Lines |
Here we see the evidence that no proposed change causes an OOS in the TerritoryManager component.