Page MenuHomeWildfire Games
Differential D754 Diff 4065 source/simulation2/components/CCmpTerritoryManager.cpp

Changeset View

Standalone View

View Options

source/simulation2/components/CCmpTerritoryManager.cpp

Show First 20 Lines • Show All 150 Lines • ▼ Show 20 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);
elexisUnsubmitted
Not Done
Inline Actions

Here we see the evidence that no proposed change causes an OOS in the TerritoryManager component.

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(); // Color change is not a trigger event
SAFE_DELETE(m_Territories);
++m_DirtyID;
m_BoundaryLinesDirty = true;
break; break;
elexisUnsubmitted
Not Done
Inline Actions

Isn't that an unrelated diff?
trigger event? Sounds like trigger script, can you clarify?

elexis: Isn't that an unrelated diff? trigger event? Sounds like trigger script, can you clarify?
templeAuthorUnsubmitted
Not Done
Inline Actions

The 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.
Since the territory doesn't actually change, only changes colors, we shouldn't be sending a message anyway.
(Actually m_TriggerEvent itself is serialized, but it resets after updates, so I guess it's rarer to get an oos from just that.)

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;
void MakeDirty() void MakeDirty()
elexisUnsubmitted
Not Done
Inline Actions

Examining 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…
{ {
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) const
StanUnsubmitted
Not Done
Inline Actions

Any 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…
templeAuthorUnsubmitted
Not Done
Inline Actions

Here 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) if (*dirtyID != m_DirtyID)
{ {
*dirtyID = m_DirtyID; *dirtyID = m_DirtyID;
return true; return true;
} }
return false; 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;
} }
elexisUnsubmitted
Not Done
Inline Actions

Does this function need to check for the player color too?
Their purpose is not commented in the header file, a small defect in the curent code.
A rename to distinguish them better (NeedUpdateTexture NeedUpdateSomethingInThesimulation) might satisfy too.

elexis: Does this function need to check for the player color too? Their purpose is not commented in…
templeAuthorUnsubmitted
Not Done
Inline Actions

Rename sounds good. (Added in D910.)

temple: Rename sounds good. (Added in D910.)
elexisUnsubmitted
Not Done
Inline Actions

Reading 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.
But that you add the playercolor condition to exactly one of them indeed makes it reasonable to rename it - otherwise it became a trap and some sim component might check for UpdateState and expect something simulation safe and trigger an OOS.

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();
▲ Show 20 Lines • Show All 325 Lines • ▼ Show 20 Lines for (size_t i = 0; i < boundaries.size(); ++i)
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->GetColor();
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().color = color; m_BoundaryLines.back().color = color;
elexisUnsubmitted
Not Done
Inline Actions

Offtopic, 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.
I assume we want an STL initialization for this, the code comes from back in the day when rP9929 was committed.

elexis: Offtopic, but a bit awkward to push an empty struct and then change the property of the back of…
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;
SimRender::SmoothPointsAverage(boundaries[i].points, m_BoundaryLines.back().overlay.m_Closed); SimRender::SmoothPointsAverage(boundaries[i].points, m_BoundaryLines.back().overlay.m_Closed);
Show All 30 Linesvoid 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)
elexisUnsubmitted
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]);
elexisUnsubmitted
Not Done
Inline Actions

same

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 Linesbool 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;
} }
TerritoryOverlay::TerritoryOverlay(CCmpTerritoryManager& manager) : TerritoryOverlay::TerritoryOverlay(CCmpTerritoryManager& manager) :
elexisUnsubmitted
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…
templeAuthorUnsubmitted
Not Done
Inline Actions

Yeah, 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.
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)
{ {
elexisUnsubmitted
Not Done
Inline Actions

Could 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…
for (size_t i = 0; i < w; ++i) for (size_t i = 0; i < w; ++i)
{ {
SColor4ub color; SColor4ub color;
elexisUnsubmitted
Done
Inline Actions

maybe continue

elexis: maybe continue
u8 id = (m_TerritoryManager.m_Territories->get((int)i, (int)j) & ICmpTerritoryManager::TERRITORY_PLAYER_MASK); u8 id = (m_TerritoryManager.m_Territories->get((int)i, (int)j) & ICmpTerritoryManager::TERRITORY_PLAYER_MASK);
templeAuthorUnsubmitted
Done
Inline Actions

I guess this should be written in two lines rather than one?

temple: I guess this should be written in two lines rather than one?
color = GetColor(id, 64); color = GetColor(id, 64);
*data++ = color.R; *data++ = color.R;
*data++ = color.G; *data++ = color.G;
*data++ = color.B; *data++ = color.B;
*data++ = color.A; *data++ = color.A;
elexisUnsubmitted
Not Done
Inline Actions

Function 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…
} }
} }
} }
#undef FLOODFILL #undef FLOODFILL
Wildfire Games · Phabricator