Refactors models and materials, part 2, replaces raw pointer by unique_ptr and adds test.

Comments By: phosit
Differential Revision: https://code.wildfiregames.com/D5128
This was SVN commit r27846.

source/graphics/Decal.cpp

@@ -1,4 +1,4 @@
-/* Copyright (C) 2022 Wildfire Games.
+/* Copyright (C) 2023 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -23,10 +23,9 @@
 #include "maths/MathUtil.h"
 #include "ps/CStrInternStatic.h"
 
-CModelAbstract* CModelDecal::Clone() const
+std::unique_ptr<CModelAbstract> CModelDecal::Clone() const
 {
-	CModelDecal* clone = new CModelDecal(m_Terrain, m_Decal);
-	return clone;
+	return std::make_unique<CModelDecal>(m_Terrain, m_Decal);
 }
 
 void CModelDecal::CalcVertexExtents(ssize_t& i0, ssize_t& j0, ssize_t& i1, ssize_t& j1)

source/graphics/Decal.h

@@ -1,4 +1,4 @@
-/* Copyright (C) 2022 Wildfire Games.
+/* Copyright (C) 2023 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -61,7 +61,7 @@ public:
 		return this;
 	}
 
-	virtual CModelAbstract* Clone() const;
+	virtual std::unique_ptr<CModelAbstract> Clone() const;
 
 	virtual void SetTerrainDirty(ssize_t i0, ssize_t j0, ssize_t i1, ssize_t j1);
 

source/graphics/Model.cpp

@@ -60,9 +60,6 @@ CModel::CModel(const CSimulation2& simulation, const CMaterial& material, const
 CModel::~CModel()
 {
 	rtl_FreeAligned(m_BoneMatrices);
-
-	for (Prop& prop : m_Props)
-		delete prop.m_Model;
 }
 
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -385,7 +382,7 @@ void CModel::CopyAnimationFrom(CModel* source)
 
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////
 // AddProp: add a prop to the model on the given point
-void CModel::AddProp(const SPropPoint* point, CModelAbstract* model, CObjectEntry* objectentry, float minHeight, float maxHeight, bool selectable)
+void CModel::AddProp(const SPropPoint* point, std::unique_ptr<CModelAbstract> model, CObjectEntry* objectentry, float minHeight, float maxHeight, bool selectable)
 {
 	// position model according to prop point position
 
@@ -395,17 +392,17 @@ void CModel::AddProp(const SPropPoint* point, CModelAbstract* model, CObjectEntr
 
 	Prop prop;
 	prop.m_Point = point;
-	prop.m_Model = model;
+	prop.m_Model = std::move(model);
 	prop.m_ObjectEntry = objectentry;
 	prop.m_MinHeight = minHeight;
 	prop.m_MaxHeight = maxHeight;
 	prop.m_Selectable = selectable;
-	m_Props.push_back(prop);
+	m_Props.push_back(std::move(prop));
 }
 
-void CModel::AddAmmoProp(const SPropPoint* point, CModelAbstract* model, CObjectEntry* objectentry)
+void CModel::AddAmmoProp(const SPropPoint* point, std::unique_ptr<CModelAbstract> model, CObjectEntry* objectentry)
 {
-	AddProp(point, model, objectentry);
+	AddProp(point, std::move(model), objectentry);
 	m_AmmoPropPoint = point;
 	m_AmmoLoadedProp = m_Props.size() - 1;
 	m_Props[m_AmmoLoadedProp].m_Hidden = true;
@@ -448,7 +445,7 @@ void CModel::HideAmmoProp()
 CModelAbstract* CModel::FindFirstAmmoProp()
 {
 	if (m_AmmoPropPoint)
-		return m_Props[m_AmmoLoadedProp].m_Model;
+		return m_Props[m_AmmoLoadedProp].m_Model.get();
 
 	for (size_t i = 0; i < m_Props.size(); ++i)
 	{
@@ -466,9 +463,9 @@ CModelAbstract* CModel::FindFirstAmmoProp()
 
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////
 // Clone: return a clone of this model
-CModelAbstract* CModel::Clone() const
+std::unique_ptr<CModelAbstract> CModel::Clone() const
 {
-	CModel* clone = new CModel(m_Simulation, m_Material, m_pModelDef);
+	std::unique_ptr<CModel> clone = std::make_unique<CModel>(m_Simulation, m_Material, m_pModelDef);
 	clone->m_ObjectBounds = m_ObjectBounds;
 	clone->SetAnimation(m_Anim);
 	clone->SetFlags(m_Flags);

source/graphics/Model.h

@@ -49,28 +49,26 @@ class CModel : public CModelAbstract
 public:
 	struct Prop
 	{
-		Prop() : m_MinHeight(0.f), m_MaxHeight(0.f), m_Point(0), m_Model(0), m_ObjectEntry(0), m_Hidden(false), m_Selectable(true) {}
-
-		float m_MinHeight;
-		float m_MaxHeight;
+		float m_MinHeight{0.0f};
+		float m_MaxHeight{0.0f};
 
 		/**
 		 * Location of the prop point within its parent model, relative to either a bone in the parent model or to the
 		 * parent model's origin. See the documentation for @ref SPropPoint for more details.
 		 * @see SPropPoint
 		 */
-		const SPropPoint* m_Point;
+		const SPropPoint* m_Point{nullptr};
 
 		/**
 		 * Pointer to the model associated with this prop. Note that the transform matrix held by this model is the full object-to-world
 		 * space transform, taking into account all parent model positioning (see @ref CModel::ValidatePosition for positioning logic).
 		 * @see CModel::ValidatePosition
 		 */
-		CModelAbstract* m_Model;
-		CObjectEntry* m_ObjectEntry;
+		std::unique_ptr<CModelAbstract> m_Model;
+		CObjectEntry* m_ObjectEntry{nullptr};
 
-		bool m_Hidden; ///< Should this prop be temporarily removed from rendering?
-		bool m_Selectable; /// < should this prop count in the selection size?
+		bool m_Hidden{false}; ///< Should this prop be temporarily removed from rendering?
+		bool m_Selectable{true}; /// < should this prop count in the selection size?
 	};
 
 public:
@@ -182,13 +180,13 @@ public:
 	/**
 	 * Add a prop to the model on the given point.
 	 */
-	void AddProp(const SPropPoint* point, CModelAbstract* model, CObjectEntry* objectentry, float minHeight = 0.f, float maxHeight = 0.f, bool selectable = true);
+	void AddProp(const SPropPoint* point, std::unique_ptr<CModelAbstract> model, CObjectEntry* objectentry, float minHeight = 0.f, float maxHeight = 0.f, bool selectable = true);
 
 	/**
 	 * Add a prop to the model on the given point, and treat it as the ammo prop.
 	 * The prop will be hidden by default.
 	 */
-	void AddAmmoProp(const SPropPoint* point, CModelAbstract* model, CObjectEntry* objectentry);
+	void AddAmmoProp(const SPropPoint* point, std::unique_ptr<CModelAbstract> model, CObjectEntry* objectentry);
 
 	/**
 	 * Show the ammo prop (if any), and hide any other props on that prop point.
@@ -210,7 +208,7 @@ public:
 	const std::vector<Prop>& GetProps() const { return m_Props; }
 
 	// return a clone of this model
-	virtual CModelAbstract* Clone() const;
+	virtual std::unique_ptr<CModelAbstract> Clone() const;
 
 	/**
 	 * Ensure that both the transformation and the bone

source/graphics/ModelAbstract.h

@@ -1,4 +1,4 @@
-/* Copyright (C) 2022 Wildfire Games.
+/* Copyright (C) 2023 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -23,6 +23,8 @@
 #include "maths/BoundingBoxOriented.h"
 #include "simulation2/helpers/Player.h"
 
+#include <memory>
+
 class CModelDummy;
 class CModel;
 class CModelDecal;
@@ -71,7 +73,7 @@ public:
 		delete m_CustomSelectionShape; // allocated and set externally by CCmpVisualActor, but our responsibility to clean up
 	}
 
-	virtual CModelAbstract* Clone() const = 0;
+	virtual std::unique_ptr<CModelAbstract> Clone() const = 0;
 
 	/// Dynamic cast
 	virtual CModelDummy* ToCModelDummy() { return nullptr; }

source/graphics/ModelDummy.h

@@ -1,4 +1,4 @@
-/* Copyright (C) 2022 Wildfire Games.
+/* Copyright (C) 2023 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -32,13 +32,13 @@ public:
 	CModelDummy() = default;
 	virtual ~CModelDummy() = default;
 
-	virtual CModelAbstract* Clone() const { return new CModelDummy(); }
+	virtual std::unique_ptr<CModelAbstract> Clone() const { return std::make_unique<CModelDummy>(); }
 	virtual CModelDummy* ToCModelDummy() { return this; }
 
-	virtual void CalcBounds() {};
+	virtual void CalcBounds() {}
 	virtual void SetTerrainDirty(ssize_t, ssize_t, ssize_t, ssize_t) {}
-	virtual void ValidatePosition() {};
-	virtual void InvalidatePosition() {};
+	virtual void ValidatePosition() {}
+	virtual void InvalidatePosition() {}
 };
 
 #endif // INCLUDED_MODELDUMMY

source/graphics/ObjectEntry.cpp

@@ -245,13 +245,13 @@ bool CObjectEntry::BuildVariation(const std::vector<const std::set<CStr>*>& comp
 		const SPropPoint* proppoint = modeldef->FindPropPoint(ppn.c_str());
 		if (proppoint)
 		{
-			CModelAbstract* propmodel = oe->m_Model->Clone();
-			if (isAmmo)
-				model->AddAmmoProp(proppoint, propmodel, oe);
-			else
-				model->AddProp(proppoint, propmodel, oe, prop.m_minHeight, prop.m_maxHeight, prop.m_selectable);
+			std::unique_ptr<CModelAbstract> propmodel = oe->m_Model->Clone();
 			if (propmodel->ToCModel())
 				propmodel->ToCModel()->SetAnimation(oe->GetRandomAnimation("idle"));
+			if (isAmmo)
+				model->AddAmmoProp(proppoint, std::move(propmodel), oe);
+			else
+				model->AddProp(proppoint, std::move(propmodel), oe, prop.m_minHeight, prop.m_maxHeight, prop.m_selectable);
 		}
 		else
 			LOGERROR("Failed to find matching prop point called \"%s\" in model \"%s\" for actor \"%s\"", ppn, m_ModelName.string8(), m_Base->GetIdentifier());

source/graphics/ParticleEmitter.cpp

@@ -281,9 +281,9 @@ void CModelParticleEmitter::SetEntityVariable(const std::string& name, float val
 	m_Emitter->SetEntityVariable(name, value);
 }
 
-CModelAbstract* CModelParticleEmitter::Clone() const
+std::unique_ptr<CModelAbstract> CModelParticleEmitter::Clone() const
 {
-	return new CModelParticleEmitter(m_Type);
+	return std::make_unique<CModelParticleEmitter>(m_Type);
 }
 
 void CModelParticleEmitter::CalcBounds()

source/graphics/ParticleEmitter.h

@@ -197,7 +197,7 @@ public:
 		return this;
 	}
 
-	virtual CModelAbstract* Clone() const;
+	virtual std::unique_ptr<CModelAbstract> Clone() const;
 
 	virtual void SetTerrainDirty(ssize_t UNUSED(i0), ssize_t UNUSED(j0), ssize_t UNUSED(i1), ssize_t UNUSED(j1))
 	{

source/graphics/Unit.cpp

@@ -45,7 +45,6 @@ CUnit::CUnit(CObjectManager& objectManager, const CActorDef& actor, const entity
 CUnit::~CUnit()
 {
 	delete m_Animation;
-	delete m_Model;
 }
 
 CUnit* CUnit::Create(const CStrW& actorName, const entity_id_t id, const uint32_t seed, CObjectManager& objectManager)
@@ -128,7 +127,7 @@ void CUnit::ReloadObject()
 	else if (m_Object && newObject != m_Object)
 	{
 		// Clone the new object's base (non-instance) model
-		CModelAbstract* newModel = newObject->m_Model->Clone();
+		std::unique_ptr<CModelAbstract> newModel = newObject->m_Model->Clone();
 
 		// Copy the old instance-specific settings from the old model to the new instance
 		newModel->SetTransform(m_Model->GetTransform());
@@ -142,8 +141,7 @@ void CUnit::ReloadObject()
 			newModel->ToCModel()->AddFlagsRec(instanceFlags);
 		}
 
-		delete m_Model;
-		m_Model = newModel;
+		m_Model = std::move(newModel);
 		m_Object = newObject;
 
 		if (m_Model->ToCModel())

source/graphics/Unit.h

@@ -22,6 +22,7 @@
 #include "simulation2/system/Entity.h"	// entity_id_t
 
 #include <map>
+#include <memory>
 #include <set>
 
 class CActorDef;
@@ -83,8 +84,8 @@ private:
 	const CActorDef& m_Actor;
 	// object from which unit was created; never NULL once fully created.
 	CObjectEntry* m_Object = nullptr;
-	// object model representation; never NULL once fully created.
-	CModelAbstract* m_Model = nullptr;
+	// object model representation; never nullptr once fully created.
+	std::unique_ptr<CModelAbstract> m_Model;
 
 	CUnitAnimation* m_Animation = nullptr;
 

source/graphics/tests/test_Model.h

@@ -0,0 +1,83 @@
+/* Copyright (C) 2023 Wildfire Games.
+ * This file is part of 0 A.D.
+ *
+ * 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
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 0 A.D. is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * 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/>.
+ */
+
+#include "lib/self_test.h"
+
+#include "graphics/Material.h"
+#include "graphics/Model.h"
+#include "graphics/ModelDef.h"
+#include "graphics/ShaderDefines.h"
+#include "ps/CStrInternStatic.h"
+#include "scriptinterface/ScriptInterface.h"
+#include "simulation2/Simulation2.h"
+
+#include <memory>
+
+class TestModel : public CxxTest::TestSuite
+{
+public:
+	bool HasShaderDefine(const CShaderDefines& defines, CStrIntern define)
+	{
+		const auto& map = defines.GetMap();
+		const auto it = map.find(define);
+		return it != map.end() && it->second == str_1;
+	}
+
+	bool HasMaterialDefine(CModel* model, CStrIntern define)
+	{
+		return HasShaderDefine(model->GetMaterial().GetShaderDefines(), define);
+	}
+
+	void test_model_with_flags()
+	{
+		CMaterial material{};
+		CSimulation2 simulation{nullptr, g_ScriptContext, nullptr};
+
+		// TODO: load a proper mock for modeldef.
+		CModelDefPtr modeldef = std::make_shared<CModelDef>();
+
+		std::unique_ptr<CModel> model = std::make_unique<CModel>(simulation, material, modeldef);
+
+		SPropPoint propPoint{};
+		model->AddProp(&propPoint, std::make_unique<CModel>(simulation, material, modeldef), nullptr);
+
+		model->AddFlagsRec(MODELFLAG_IGNORE_LOS);
+		model->RemoveShadowsRec();
+
+		TS_ASSERT(HasMaterialDefine(model.get(), str_DISABLE_RECEIVE_SHADOWS));
+		TS_ASSERT(HasMaterialDefine(model.get(), str_IGNORE_LOS));
+		for (const CModel::Prop& prop : model->GetProps())
+		{
+			TS_ASSERT(prop.m_Model->ToCModel());
+			TS_ASSERT(HasMaterialDefine(prop.m_Model->ToCModel(), str_DISABLE_RECEIVE_SHADOWS));
+			TS_ASSERT(HasMaterialDefine(prop.m_Model->ToCModel(), str_IGNORE_LOS));
+		}
+
+		std::unique_ptr<CModelAbstract> clonedModel = model->Clone();
+		TS_ASSERT(clonedModel->ToCModel());
+		TS_ASSERT(HasMaterialDefine(clonedModel->ToCModel(), str_DISABLE_RECEIVE_SHADOWS));
+		TS_ASSERT(HasMaterialDefine(clonedModel->ToCModel(), str_IGNORE_LOS));
+
+		TS_ASSERT_EQUALS(model->GetProps().size(), clonedModel->ToCModel()->GetProps().size());
+		for (const CModel::Prop& prop : clonedModel->ToCModel()->GetProps())
+		{
+			TS_ASSERT(prop.m_Model->ToCModel());
+			TS_ASSERT(HasMaterialDefine(prop.m_Model->ToCModel(), str_DISABLE_RECEIVE_SHADOWS));
+			TS_ASSERT(HasMaterialDefine(prop.m_Model->ToCModel(), str_IGNORE_LOS));
+		}
+	}
+};

source/renderer/Scene.cpp

@@ -1,4 +1,4 @@
-/* Copyright (C) 2021 Wildfire Games.
+/* Copyright (C) 2023 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -35,7 +35,7 @@ void SceneCollector::SubmitRecursive(CModelAbstract* model)
 		for (size_t i = 0; i < props.size(); i++)
 		{
 			if (!props[i].m_Hidden)
-				SubmitRecursive(props[i].m_Model);
+				SubmitRecursive(props[i].m_Model.get());
 		}
 	}
 	else if (model->ToCModelDecal())

source/tools/atlas/GameInterface/ActorViewer.cpp

@@ -1,4 +1,4 @@
-/* Copyright (C) 2022 Wildfire Games.
+/* Copyright (C) 2023 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -118,7 +118,8 @@ public:
 
 	SOverlayLine SelectionBoxOverlay;
 	SOverlayLine AxesMarkerOverlays[3];
-	std::vector<CModel::Prop> Props;
+
+	std::vector<CModelAbstract*> PropModels;
 	std::vector<SOverlayLine> PropPointOverlays;
 
 	// Simplistic implementation of the Scene interface
@@ -159,16 +160,16 @@ public:
 			}
 
 			// add prop point overlays
-			if (PropPointsMode > 0 && Props.size() > 0)
+			if (PropPointsMode > 0 && PropModels.size() > 0)
 			{
 				PropPointOverlays.clear(); // doesn't clear capacity, but should be ok since the number of prop points is usually pretty limited
-				for (size_t i = 0; i < Props.size(); ++i)
+				for (size_t i = 0; i < PropModels.size(); ++i)
 				{
-					CModel::Prop& prop = Props[i];
-					if (prop.m_Model) // should always be the case
+					CModelAbstract* model = PropModels[i];
+					if (model) // should always be the case
 					{
 						// prop point positions are automatically updated during animations etc. by CModel::ValidatePosition
-						const CMatrix3D& propCoordSystem = prop.m_Model->GetTransform();
+						const CMatrix3D& propCoordSystem = model->GetTransform();
 
 						SOverlayLine pointGimbal;
 						pointGimbal.m_Color = CColor(1.f, 0.f, 1.f, 1.f);
@@ -231,7 +232,7 @@ public:
 
 void ActorViewerImpl::UpdatePropList()
 {
-	Props.clear();
+	PropModels.clear();
 
 	CmpPtr<ICmpVisual> cmpVisual(Simulation2, Entity);
 	if (cmpVisual)
@@ -255,9 +256,9 @@ void ActorViewerImpl::UpdatePropListRecursive(CModelAbstract* modelAbstract)
 		std::vector<CModel::Prop>& modelProps = model->GetProps();
 		for (CModel::Prop& modelProp : modelProps)
 		{
-			Props.push_back(modelProp);
+			PropModels.push_back(modelProp.m_Model.get());
 			if (modelProp.m_Model)
-				UpdatePropListRecursive(modelProp.m_Model);
+				UpdatePropListRecursive(modelProp.m_Model.get());
 		}
 	}
 }