Changeset View
Standalone View
source/simulation2/components/CCmpVisualActor.cpp
/* Copyright (C) 2018 Wildfire Games. | /* Copyright (C) 2019 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, | ||||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||
* GNU General Public License for more details. | * GNU General Public License for more details. | ||||
* | * | ||||
* You should have received a copy of the GNU General Public License | * You should have received a copy of the GNU General Public License | ||||
* along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. | * along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. | ||||
*/ | */ | ||||
#include "precompiled.h" | #include "precompiled.h" | ||||
#include "simulation2/system/Component.h" | #include "simulation2/system/Component.h" | ||||
#include "ICmpVisual.h" | #include "ICmpVisual.h" | ||||
#include "simulation2/MessageTypes.h" | #include "simulation2/MessageTypes.h" | ||||
#include "ICmpFootprint.h" | #include "ICmpFootprint.h" | ||||
#include "ICmpIdentity.h" | |||||
Stan: @wraitii maybe forward class declaration ? | |||||
#include "ICmpUnitRenderer.h" | #include "ICmpUnitRenderer.h" | ||||
#include "ICmpOwnership.h" | #include "ICmpOwnership.h" | ||||
#include "ICmpPosition.h" | #include "ICmpPosition.h" | ||||
#include "ICmpTemplateManager.h" | #include "ICmpTemplateManager.h" | ||||
#include "ICmpTerrain.h" | #include "ICmpTerrain.h" | ||||
#include "ICmpUnitMotion.h" | #include "ICmpUnitMotion.h" | ||||
#include "ICmpValueModificationManager.h" | #include "ICmpValueModificationManager.h" | ||||
#include "ICmpVisibility.h" | #include "ICmpVisibility.h" | ||||
Show All 27 Lines | static void ClassInit(CComponentManager& componentManager) | ||||
componentManager.SubscribeToMessageType(MT_ValueModification); | componentManager.SubscribeToMessageType(MT_ValueModification); | ||||
componentManager.SubscribeToMessageType(MT_TerrainChanged); | componentManager.SubscribeToMessageType(MT_TerrainChanged); | ||||
componentManager.SubscribeToMessageType(MT_Destroy); | componentManager.SubscribeToMessageType(MT_Destroy); | ||||
} | } | ||||
DEFAULT_COMPONENT_ALLOCATOR(VisualActor) | DEFAULT_COMPONENT_ALLOCATOR(VisualActor) | ||||
private: | private: | ||||
std::wstring m_BaseActorName, m_ActorName; | std::wstring m_BaseActorName, m_ActorName, m_BaseActorString; | ||||
Done Inline ActionsUseless added member, should be a local variable. vladislavbelov: Useless added member, should be a local variable. | |||||
bool m_IsFoundationActor; | bool m_IsFoundationActor; | ||||
// Not initialized in non-visual mode | // Not initialized in non-visual mode | ||||
CUnit* m_Unit; | CUnit* m_Unit; | ||||
fixed m_R, m_G, m_B; // shading color | fixed m_R, m_G, m_B; // shading color | ||||
std::map<std::string, std::string> m_AnimOverride; | std::map<std::string, std::string> m_AnimOverride; | ||||
▲ Show 20 Lines • Show All 108 Lines • ▼ Show 20 Lines | return | ||||
"</optional>" | "</optional>" | ||||
"<element name='VisibleInAtlasOnly'>" | "<element name='VisibleInAtlasOnly'>" | ||||
"<data type='boolean'/>" | "<data type='boolean'/>" | ||||
"</element>"; | "</element>"; | ||||
} | } | ||||
virtual void Init(const CParamNode& paramNode) | virtual void Init(const CParamNode& paramNode) | ||||
{ | { | ||||
m_Unit = NULL; | m_Unit = NULL; | ||||
Done Inline ActionsYou don't need the variable for the whole scope of the function, put it in the usage scope. Also we don't use m_ prefix for local variables. vladislavbelov: You don't need the variable for the whole scope of the function, put it in the usage scope. | |||||
m_R = m_G = m_B = fixed::FromInt(1); | m_R = m_G = m_B = fixed::FromInt(1); | ||||
m_ConstructionPreview = paramNode.GetChild("ConstructionPreview").IsOk(); | m_ConstructionPreview = paramNode.GetChild("ConstructionPreview").IsOk(); | ||||
m_Seed = GetEntityId(); | m_Seed = GetEntityId(); | ||||
m_IsFoundationActor = paramNode.GetChild("Foundation").IsOk() && paramNode.GetChild("FoundationActor").IsOk(); | m_IsFoundationActor = paramNode.GetChild("Foundation").IsOk() && paramNode.GetChild("FoundationActor").IsOk(); | ||||
if (m_IsFoundationActor) | if (m_IsFoundationActor) | ||||
m_BaseActorName = m_ActorName = paramNode.GetChild("FoundationActor").ToString(); | m_BaseActorName = m_ActorName = paramNode.GetChild("FoundationActor").ToString(); | ||||
else | else | ||||
m_BaseActorName = m_ActorName = paramNode.GetChild("Actor").ToString(); | { | ||||
m_BaseActorString = paramNode.GetChild("Actor").ToString(); | |||||
CmpPtr<ICmpIdentity> cmpIdentity(GetEntityHandle()); | |||||
if (m_BaseActorString.find(L"{gender}") != std::wstring::npos) | |||||
Done Inline ActionsIt's not a good practice to repeat a search (m_BaseActorString.find(L"{gender}")). vladislavbelov: It's not a good practice to repeat a search (`m_BaseActorString.find(L"{gender}")`). | |||||
Done Inline ActionsAny advice on what to change then? Freagarach: Any advice on what to change then? | |||||
Done Inline ActionsI'd write it like: const std::wstring pattern = L"{phenotype}"; auto patternPos = baseActorString .find(pattern); if (patternPos != std::wstring::npos) baseActorString.replace(patternPos, pattern.length(), cmpIdentity->GetPhenotype()); Also I think that the name may content more than one pattern, then this code won't work. So: #include <boost/algorithm/string.hpp> // ... boost::replace_all(baseActorString , "{phenotype}", cmpIdentity->GetPhenotype()); vladislavbelov: I'd write it like:
```lang=cpp
const std::wstring pattern = L"{phenotype}";
auto patternPos =… | |||||
Not Done Inline ActionsI wonder if the "{phenotype}" string could not be a constant in the js component instead ? Stan: I wonder if the "{phenotype}" string could not be a constant in the js component instead ?
| |||||
m_BaseActorString.replace(m_BaseActorString.find(L"{gender}"), 8, cmpIdentity->GetGender()); | |||||
Done Inline ActionsClose enough :) m_BaseActorString.replace(m_BaseActorString.find(L"{gender}"), 8, cmpIdentity->GetGender()); Stan: Close enough :)
```lang=cpp
m_BaseActorString.replace(m_BaseActorString.find(L"{gender}"), 8… | |||||
Done Inline ActionsWe should avoid a hardcoded string in many places and magic numbers. vladislavbelov: We should avoid a hardcoded string in many places and magic numbers. | |||||
Done Inline ActionsAny advice on what to change then? Freagarach: Any advice on what to change then? | |||||
Done Inline ActionsWould you suggest using a "VisualActor.js" component in simulation then? (For this and above.) Freagarach: Would you suggest using a "VisualActor.js" component in simulation then? (For this and above.) | |||||
Done Inline ActionsWhat's the type ? We usually avoid auto not sure why @vladislavbelov suggested it there. Stan: What's the type ? We usually avoid auto not sure why @vladislavbelov suggested it there. | |||||
Done Inline ActionsActually we don't need the patternPos at all if we use boost::replace_all. vladislavbelov: Actually we don't need the `patternPos` at all if we use `boost::replace_all`. | |||||
Done Inline ActionsIf I don't use "if (patternPos != std::wstring::npos)" I get a segfault upon map creation. Freagarach: If I don't use "if (patternPos != std::wstring::npos)" I get a segfault upon map creation. | |||||
Done Inline ActionsMaybe it segfaults because cmpIdentity is not defined ? You do not check whether it's here :) Stan: Maybe it segfaults because cmpIdentity is not defined ? You do not check whether it's here :) | |||||
m_BaseActorName = m_ActorName = m_BaseActorString; | |||||
Done Inline Actionscan't you replace "{phenotype}" by pattern ? Stan: can't you replace "{phenotype}" by pattern ? | |||||
} | |||||
m_VisibleInAtlasOnly = paramNode.GetChild("VisibleInAtlasOnly").ToBool(); | m_VisibleInAtlasOnly = paramNode.GetChild("VisibleInAtlasOnly").ToBool(); | ||||
m_IsActorOnly = paramNode.GetChild("ActorOnly").IsOk(); | m_IsActorOnly = paramNode.GetChild("ActorOnly").IsOk(); | ||||
InitModel(paramNode); | InitModel(paramNode); | ||||
SelectAnimation("idle"); | SelectAnimation("idle"); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 351 Lines • ▼ Show 20 Lines | |||||
// ------------------------------------------------------------------------------------------------------------------ | // ------------------------------------------------------------------------------------------------------------------ | ||||
void CCmpVisualActor::InitModel(const CParamNode& paramNode) | void CCmpVisualActor::InitModel(const CParamNode& paramNode) | ||||
{ | { | ||||
if (!GetSimContext().HasUnitManager()) | if (!GetSimContext().HasUnitManager()) | ||||
return; | return; | ||||
std::set<CStr> selections; | std::set<CStr> selections; | ||||
Done Inline Actionsper @elexis suggestion that we ought not use boost here, I'd replace this with: size_t pos = base.find(pattern, pos); while (pos != std::string::npos) { base.replace(pos, pos + 11, cmpIdentity->GetPhenotype()); pos = base.find(pattern, pos + 11); } (needs some testing, but ought to work and be optimised enough) wraitii: per @elexis suggestion that we ought not use boost here, I'd replace this with:
```
size_t… | |||||
std::wstring actorName = m_ActorName; | std::wstring actorName = m_ActorName; | ||||
if (actorName.find(L".xml") == std::wstring::npos) | if (actorName.find(L".xml") == std::wstring::npos) | ||||
actorName += L".xml"; | actorName += L".xml"; | ||||
m_Unit = GetSimContext().GetUnitManager().CreateUnit(actorName, GetActorSeed(), selections); | m_Unit = GetSimContext().GetUnitManager().CreateUnit(actorName, GetActorSeed(), selections); | ||||
if (!m_Unit) | if (!m_Unit) | ||||
return; | return; | ||||
CModelAbstract& model = m_Unit->GetModel(); | CModelAbstract& model = m_Unit->GetModel(); | ||||
▲ Show 20 Lines • Show All 227 Lines • Show Last 20 Lines |
@wraitii maybe forward class declaration ?