Removed reverse dependency on SOverlayTexturedLine from CTexturedLineRData; fixes dangling pointer dereference. Fixes #1495.

This was SVN commit r11979.

source/graphics/Overlay.cpp

@@ -17,9 +17,10 @@
 
 #include "precompiled.h"
 
-#include "ps/CStr.h"
 #include "Overlay.h"
 
+#include "ps/CStr.h"
+
 SOverlayTexturedLine::LineCapType SOverlayTexturedLine::StrToLineCapType(const std::wstring& str)
 {
 	if (str == L"round")
@@ -35,3 +36,4 @@ SOverlayTexturedLine::LineCapType SOverlayTexturedLine::StrToLineCapType(const s
 		return LINECAP_FLAT;
 	}
 }
+

source/graphics/Overlay.h

@@ -27,6 +27,7 @@
 
 class CTerrain;
 class CSimContext;
+class CTexturedLineRData;
 
 /**
  * Line-based overlay, with world-space coordinates, rendered in the world
@@ -50,8 +51,13 @@ struct SOverlayLine
 };
 
 /**
- * Textured line overlay, with world-space coordinates, rendered in the world
+ * Textured line overlay, with world-space coordinates, rendered in the world onto the terrain.
- * onto the terrain. Designed for territory borders.
+ * Designed for relatively static textured lines, i.e. territory borders, originally.
+ * 
+ * Once submitted for rendering, instances must not be copied afterwards. The reason is that they
+ * are assigned rendering data that is unique to the submitted instance, and non-transferable to
+ * any copies that would otherwise be made. Amongst others, this restraint includes that they must
+ * not be submitted by their address inside a std::vector storing them by value.
  */
 struct SOverlayTexturedLine
 {
@@ -71,8 +77,8 @@ struct SOverlayTexturedLine
 	};
 
 	SOverlayTexturedLine()
-		: m_SimContext(NULL), m_Thickness(1.0f), m_Closed(false), m_AlwaysVisible(false),
+		: m_Thickness(1.0f), m_Closed(false), m_AlwaysVisible(false),
-		  m_StartCapType(LINECAP_FLAT), m_EndCapType(LINECAP_FLAT)
+		  m_StartCapType(LINECAP_FLAT), m_EndCapType(LINECAP_FLAT), m_SimContext(NULL)
 	{ }
 
 	CTexturePtr m_TextureBase;
@@ -92,11 +98,21 @@ struct SOverlayTexturedLine
 	LineCapType m_StartCapType;
 	LineCapType m_EndCapType;
 
-	/// Simulation context applicable for this overlay line; used to obtain terrain information
+	/**
-	/// during automatic computation of Y coordinates.
+	 * Simulation context applicable for this overlay line; used to obtain terrain information
+	 * during automatic computation of Y coordinates.
+	 */
 	const CSimContext* m_SimContext;
-	/// Cached renderer data (shared_ptr so that copies/deletes are automatic).
+
-	shared_ptr<CRenderData> m_RenderData;
+	/**
+	 * Cached renderer data, because expensive to compute. Allocated by the renderer when necessary 
+	 * for rendering purposes.
+	 * 
+	 * Note: the rendering data may be shared between copies of this object to prevent having to 
+	 * recompute it, while at the same time maintaining copyability of this object (see also docs on 
+	 * CTexturedLineRData).
+	 */
+	shared_ptr<CTexturedLineRData> m_RenderData;
 
 	/**
 	 * Converts a string line cap type into its corresponding LineCap enum value, and returns

source/renderer/OverlayRenderer.cpp

@@ -254,8 +254,8 @@ void OverlayRenderer::PrepareForRendering()
 		SOverlayTexturedLine* line = m->texlines[i];
 		if (!line->m_RenderData)
 		{
-			line->m_RenderData = shared_ptr<CRenderData>(new CTexturedLineRData(line));
+			line->m_RenderData = shared_ptr<CTexturedLineRData>(new CTexturedLineRData());
-			static_cast<CTexturedLineRData*>(line->m_RenderData.get())->Update();
+			line->m_RenderData->Update(*line);
 			// We assume the overlay line will get replaced by the caller
 			// if terrain changes, so we don't need to detect that here and
 			// call Update again. Also we assume the caller won't change
@@ -464,10 +464,8 @@ void OverlayRenderer::RenderTexturedOverlayLines(CShaderProgramPtr shader, bool
 		if (!line->m_RenderData || line->m_AlwaysVisible != alwaysVisible)
 			continue;
 
-		CTexturedLineRData* rdata = static_cast<CTexturedLineRData*>(line->m_RenderData.get());
+		ENSURE(line->m_RenderData);
-		ENSURE(rdata);
+		line->m_RenderData->Render(*line, shader);
-
-		rdata->Render(shader);
 	}
 }
 

source/renderer/TexturedLineRData.cpp

@@ -32,7 +32,7 @@
  * because it allows you to work with variable amounts of vertices and indices more easily. New code should prefer
  * to use VertexArray where possible, though. */
 
-void CTexturedLineRData::Render(const CShaderProgramPtr& shader)
+void CTexturedLineRData::Render(const SOverlayTexturedLine& line, const CShaderProgramPtr& shader)
 {
 	if (!m_VB || !m_VBIndices)
 		return; // might have failed to allocate
@@ -41,9 +41,9 @@ void CTexturedLineRData::Render(const CShaderProgramPtr& shader)
 
 	const int streamFlags = shader->GetStreamFlags();
 
-	shader->BindTexture("baseTex", m_Line->m_TextureBase->GetHandle());
+	shader->BindTexture("baseTex", line.m_TextureBase->GetHandle());
-	shader->BindTexture("maskTex", m_Line->m_TextureMask->GetHandle());
+	shader->BindTexture("maskTex", line.m_TextureMask->GetHandle());
-	shader->Uniform("objectColor", m_Line->m_Color);
+	shader->Uniform("objectColor", line.m_Color);
 
 	GLsizei stride = sizeof(CTexturedLineRData::SVertex);
 	CTexturedLineRData::SVertex* vertexBase = reinterpret_cast<CTexturedLineRData::SVertex*>(m_VB->m_Owner->Bind());
@@ -66,7 +66,7 @@ void CTexturedLineRData::Render(const CShaderProgramPtr& shader)
 	g_Renderer.GetStats().m_OverlayTris += m_VBIndices->m_Count/3; 
 }
 
-void CTexturedLineRData::Update()
+void CTexturedLineRData::Update(const SOverlayTexturedLine& line)
 {
 	if (m_VB)
 	{
@@ -79,21 +79,21 @@ void CTexturedLineRData::Update()
 		m_VBIndices = NULL;
 	}
 
-	if (!m_Line->m_SimContext)
+	if (!line.m_SimContext)
 	{
 		debug_warn(L"[TexturedLineRData] No SimContext set for textured overlay line, cannot render (no terrain data)");
 		return;
 	}
 
-	const CTerrain& terrain = m_Line->m_SimContext->GetTerrain();
+	const CTerrain& terrain = line.m_SimContext->GetTerrain();
-	CmpPtr<ICmpWaterManager> cmpWaterManager(*m_Line->m_SimContext, SYSTEM_ENTITY);
+	CmpPtr<ICmpWaterManager> cmpWaterManager(*line.m_SimContext, SYSTEM_ENTITY);
 
 	float v = 0.f;
 	std::vector<SVertex> vertices;
 	std::vector<u16> indices;
 
-	size_t n = m_Line->m_Coords.size() / 2; // number of line points
+	size_t n = line.m_Coords.size() / 2; // number of line points
-	bool closed = m_Line->m_Closed;
+	bool closed = line.m_Closed;
 
 	ENSURE(n >= 2); // minimum needed to avoid errors (also minimum value to make sense, can't draw a line between 1 point)
 
@@ -102,12 +102,12 @@ void CTexturedLineRData::Update()
 	// recompute p2 at the end of each iteration.
 
 	CVector3D p0;
-	CVector3D p1(m_Line->m_Coords[0], 0, m_Line->m_Coords[1]);
+	CVector3D p1(line.m_Coords[0], 0, line.m_Coords[1]);
-	CVector3D p2(m_Line->m_Coords[(1 % n)*2], 0, m_Line->m_Coords[(1 % n)*2+1]);
+	CVector3D p2(line.m_Coords[(1 % n)*2], 0, line.m_Coords[(1 % n)*2+1]);
 
 	if (closed)
 		// grab the ending point so as to close the loop
-		p0 = CVector3D(m_Line->m_Coords[(n-1)*2], 0, m_Line->m_Coords[(n-1)*2+1]);
+		p0 = CVector3D(line.m_Coords[(n-1)*2], 0, line.m_Coords[(n-1)*2+1]);
 	else
 		// we don't want to loop around and use the direction towards the other end of the line, so create an artificial p0 that 
 		// extends the p2 -> p1 direction, and use that point instead
@@ -157,7 +157,7 @@ void CTexturedLineRData::Update()
 		// Adjust bisector length to match the line thickness, along the line's width
 		float l = b.Dot((p2 - p1).Normalized().Cross(norm));
 		if (fabs(l) > 0.000001f) // avoid unlikely divide-by-zero
-			b *= m_Line->m_Thickness / l;
+			b *= line.m_Thickness / l;
 
 		// Push vertices and indices for each quad in GL_TRIANGLES order. The two triangles of each quad are indexed using
 		// the winding orders (BR, BL, TR) and (TR, BL, TR) (where BR is bottom-right of this iteration's quad, TR top-right etc).
@@ -209,7 +209,7 @@ void CTexturedLineRData::Update()
 			// next iteration is the last point of the line, so create an artificial p2 that extends the p0 -> p1 direction
 			p2 = p1 + (p1 - p0);
 		else
-			p2 = CVector3D(m_Line->m_Coords[((i+2) % n)*2], 0, m_Line->m_Coords[((i+2) % n)*2+1]);
+			p2 = CVector3D(line.m_Coords[((i+2) % n)*2], 0, line.m_Coords[((i+2) % n)*2+1]);
 
 		p2.Y = terrain.GetExactGroundLevel(p2.X, p2.Z);
 		if (p2.Y < w)
@@ -242,15 +242,16 @@ void CTexturedLineRData::Update()
 
 		// create end cap
 		CreateLineCap(
+			line,
 			// the order of these vertices is important here, swapping them produces caps at the wrong side
 			vertices[vertices.size()-2].m_Position, // top-right vertex of last quad
 			vertices[vertices.size()-1].m_Position, // top-left vertex of last quad
 			// directional vector between centroids of last vertex pair and second-to-last vertex pair
 			(Centroid(vertices[vertices.size()-2], vertices[vertices.size()-1]) - Centroid(vertices[vertices.size()-4], vertices[vertices.size()-3])).Normalized(),
-			m_Line->m_EndCapType,
+			line.m_EndCapType,
 			capVertices,
 			capIndices
-			);
+		);
 
 		for (unsigned i = 0; i < capIndices.size(); i++)
 			capIndices[i] += vertices.size();
@@ -263,15 +264,16 @@ void CTexturedLineRData::Update()
 
 		// create start cap
 		CreateLineCap(
+			line,
 			// the order of these vertices is important here, swapping them produces caps at the wrong side
 			vertices[1].m_Position,
 			vertices[0].m_Position,
 			// directional vector between centroids of first vertex pair and second vertex pair
 			(Centroid(vertices[1], vertices[0]) - Centroid(vertices[3], vertices[2])).Normalized(),
-			m_Line->m_StartCapType,
+			line.m_StartCapType,
 			capVertices,
 			capIndices
-			);
+		);
 
 		for (unsigned i = 0; i < capIndices.size(); i++)
 			capIndices[i] += vertices.size();
@@ -297,8 +299,8 @@ void CTexturedLineRData::Update()
 
 }
 
-void CTexturedLineRData::CreateLineCap(const CVector3D& corner1, const CVector3D& corner2, const CVector3D& lineDirectionNormal,
+void CTexturedLineRData::CreateLineCap(const SOverlayTexturedLine& line, const CVector3D& corner1, const CVector3D& corner2,
-	SOverlayTexturedLine::LineCapType endCapType, std::vector<SVertex>& verticesOut,
+	const CVector3D& lineDirectionNormal, SOverlayTexturedLine::LineCapType endCapType, std::vector<SVertex>& verticesOut,
 	std::vector<u16>& indicesOut)
 {
 	if (endCapType == SOverlayTexturedLine::LINECAP_FLAT)
@@ -317,7 +319,7 @@ void CTexturedLineRData::CreateLineCap(const CVector3D& corner1, const CVector3D
 	//
 
 	int roundCapPoints = 8; // amount of points to sample along the semicircle for rounded caps (including corner points)
-	float radius = m_Line->m_Thickness;
+	float radius = line.m_Thickness;
 
 	CVector3D centerPoint = (corner1 + corner2) * 0.5f;
 	SVertex centerVertex(centerPoint, 0.5f, 0.5f);
@@ -397,8 +399,8 @@ void CTexturedLineRData::CreateLineCap(const CVector3D& corner1, const CVector3D
 			// are rendered only one-sided; the wrong order of vertices will make the cap visible only from the bottom.
 			verticesOut.push_back(centerVertex);
 			verticesOut.push_back(SVertex(corner2, 0.f, 0.f));
-			verticesOut.push_back(SVertex(corner2 + (lineDirectionNormal * (m_Line->m_Thickness)), 0.f, 0.33333f)); // extend butt corner point 2 along the normal vector 
+			verticesOut.push_back(SVertex(corner2 + (lineDirectionNormal * (line.m_Thickness)), 0.f, 0.33333f)); // extend butt corner point 2 along the normal vector 
-			verticesOut.push_back(SVertex(corner1 + (lineDirectionNormal * (m_Line->m_Thickness)), 0.f, 0.66666f)); // extend butt corner point 1 along the normal vector 
+			verticesOut.push_back(SVertex(corner1 + (lineDirectionNormal * (line.m_Thickness)), 0.f, 0.66666f)); // extend butt corner point 1 along the normal vector 
 			verticesOut.push_back(SVertex(corner1, 0.f, 1.0f)); // push butt corner point 1 
 
 			for (int i=1; i < 4; ++i) 

source/renderer/TexturedLineRData.h

@@ -25,17 +25,26 @@
 #include "renderer/VertexBufferManager.h"
 
 /**
- * Rendering data for a single textured overlay line. Used by the OverlayRenderer.
+ * Rendering data for an STexturedOverlayLine.
+ * 
+ * Note that instances may be shared amongst multiple copies of the same STexturedOverlayLine instance.
+ * The reason is that this rendering data is non-copyable, but we do wish to maintain copyability of
+ * SOverlayTexturedLineData to not limit its usage patterns too much (particularly the practice of storing
+ * them into containers).
+ * 
+ * For this reason, instead of storing a reverse pointer back to any single SOverlayTexturedLine, the methods
+ * in this class accept references to STexturedOverlayLines to work with. It is up to client code to pass in
+ * SOverlayTexturedLines to all methods that are consistently the same instance or non-modified copies of it.
  */
 class CTexturedLineRData : public CRenderData
 {
+	// we hold raw pointers to vertex buffer chunks that are handed out by the vertex buffer manager
+	// and can not be safely duplicated by us.
+	NONCOPYABLE(CTexturedLineRData);
+
 public:
 
-	/**
+	CTexturedLineRData() : m_VB(NULL), m_VBIndices(NULL) { }
-	 * @param line Overlay line to associate this render data with. Must not be null.
-	 */
-	CTexturedLineRData(SOverlayTexturedLine* line) : m_Line(line), m_VB(NULL), m_VBIndices(NULL)
-	{ ENSURE(m_Line && "Cannot create textured line render data for null overlay line"); }
 
 	~CTexturedLineRData()
 	{
@@ -45,9 +54,8 @@ public:
 			g_VBMan.Release(m_VBIndices);
 	}
 
-	void Update();
+	void Update(const SOverlayTexturedLine& line);
-
+	void Render(const SOverlayTexturedLine& line, const CShaderProgramPtr& shader);
-	void Render(const CShaderProgramPtr& shader);
 
 protected:
 
@@ -71,7 +79,7 @@ protected:
 	 * @param verticesOut Output vector of vertices for passing to the renderer.
 	 * @param indicesOut Output vector of vertex indices for passing to the renderer.
 	 */
-	void CreateLineCap(const CVector3D& corner1, const CVector3D& corner2, const CVector3D& normal,
+	void CreateLineCap(const SOverlayTexturedLine& line, const CVector3D& corner1, const CVector3D& corner2, const CVector3D& normal,
 		               SOverlayTexturedLine::LineCapType endCapType, std::vector<SVertex>& verticesOut, std::vector<u16>& indicesOut);
 
 	/// Small utility function; grabs the centroid of the positions of two vertices
@@ -80,7 +88,6 @@ protected:
 		return (v1.m_Position + v2.m_Position) * 0.5;
 	}
 
-	SOverlayTexturedLine* m_Line;
 	CVertexBuffer::VBChunk* m_VB;
 	CVertexBuffer::VBChunk* m_VBIndices;
 };