Page MenuHomeWildfire Games
Differential D1955 Diff 8373 source/simulation2/components/CCmpVisualActor.cpp

Changeset View

Standalone View

View Options

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"
StanUnsubmitted
Not Done
Inline Actions

@wraitii maybe forward class declaration ?

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;
vladislavbelovUnsubmitted
Done
Inline Actions

Useless 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;
vladislavbelovUnsubmitted
Done
Inline Actions

You 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)
vladislavbelovUnsubmitted
Done
Inline Actions

It'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}")`).
FreagarachAuthorUnsubmitted
Done
Inline Actions

Any advice on what to change then?

Freagarach: Any advice on what to change then?
vladislavbelovUnsubmitted
Done
Inline Actions

I'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 =…
StanUnsubmitted
Not Done
Inline Actions

I 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());
StanUnsubmitted
Done
Inline Actions

Close 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…
vladislavbelovUnsubmitted
Done
Inline Actions

We should avoid a hardcoded string in many places and magic numbers.

vladislavbelov: We should avoid a hardcoded string in many places and magic numbers.
FreagarachAuthorUnsubmitted
Done
Inline Actions

Any advice on what to change then?

Freagarach: Any advice on what to change then?
FreagarachAuthorUnsubmitted
Done
Inline Actions

Would 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.)
StanUnsubmitted
Done
Inline Actions

What'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.
vladislavbelovUnsubmitted
Done
Inline Actions

Actually 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`.
FreagarachAuthorUnsubmitted
Done
Inline Actions

If 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.
StanUnsubmitted
Done
Inline Actions

Maybe 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;
StanUnsubmitted
Done
Inline Actions

can'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;
wraitiiUnsubmitted
Done
Inline Actions

per @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
Wildfire Games · Phabricator