From 821cfe8862a93d405e7dc50db71293e17ba257c0 Mon Sep 17 00:00:00 2001 From: vts Date: Sun, 17 Jun 2012 03:40:35 +0000 Subject: [PATCH] Removed reverse dependency on SOverlayTexturedLine from CTexturedLineRData; fixes dangling pointer dereference. Fixes #1495. This was SVN commit r11979. --- source/graphics/Overlay.cpp | 4 ++- source/graphics/Overlay.h | 32 ++++++++++++----- source/renderer/OverlayRenderer.cpp | 10 +++--- source/renderer/TexturedLineRData.cpp | 50 ++++++++++++++------------- source/renderer/TexturedLineRData.h | 29 ++++++++++------ 5 files changed, 75 insertions(+), 50 deletions(-) diff --git a/source/graphics/Overlay.cpp b/source/graphics/Overlay.cpp index 7e520632fe..eaea0e37b2 100644 --- a/source/graphics/Overlay.cpp +++ b/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; } } + diff --git a/source/graphics/Overlay.h b/source/graphics/Overlay.h index a197bb41d2..f304bdce9c 100644 --- a/source/graphics/Overlay.h +++ b/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 - * onto the terrain. Designed for territory borders. + * Textured line overlay, with world-space coordinates, rendered in the world onto the terrain. + * 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_StartCapType(LINECAP_FLAT), m_EndCapType(LINECAP_FLAT) + : m_Thickness(1.0f), m_Closed(false), m_AlwaysVisible(false), + 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 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 m_RenderData; /** * Converts a string line cap type into its corresponding LineCap enum value, and returns diff --git a/source/renderer/OverlayRenderer.cpp b/source/renderer/OverlayRenderer.cpp index be36f08ee8..72b34b4bb9 100644 --- a/source/renderer/OverlayRenderer.cpp +++ b/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(new CTexturedLineRData(line)); - static_cast(line->m_RenderData.get())->Update(); + line->m_RenderData = shared_ptr(new CTexturedLineRData()); + 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(line->m_RenderData.get()); - ENSURE(rdata); - - rdata->Render(shader); + ENSURE(line->m_RenderData); + line->m_RenderData->Render(*line, shader); } } diff --git a/source/renderer/TexturedLineRData.cpp b/source/renderer/TexturedLineRData.cpp index 357c2ef818..2c2cdcffd4 100644 --- a/source/renderer/TexturedLineRData.cpp +++ b/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("maskTex", m_Line->m_TextureMask->GetHandle()); - shader->Uniform("objectColor", m_Line->m_Color); + shader->BindTexture("baseTex", line.m_TextureBase->GetHandle()); + shader->BindTexture("maskTex", line.m_TextureMask->GetHandle()); + shader->Uniform("objectColor", line.m_Color); GLsizei stride = sizeof(CTexturedLineRData::SVertex); CTexturedLineRData::SVertex* vertexBase = reinterpret_cast(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(); - CmpPtr cmpWaterManager(*m_Line->m_SimContext, SYSTEM_ENTITY); + const CTerrain& terrain = line.m_SimContext->GetTerrain(); + CmpPtr cmpWaterManager(*line.m_SimContext, SYSTEM_ENTITY); float v = 0.f; std::vector vertices; std::vector indices; - size_t n = m_Line->m_Coords.size() / 2; // number of line points - bool closed = m_Line->m_Closed; + size_t n = line.m_Coords.size() / 2; // number of line points + 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 p2(m_Line->m_Coords[(1 % n)*2], 0, m_Line->m_Coords[(1 % n)*2+1]); + CVector3D p1(line.m_Coords[0], 0, line.m_Coords[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, - SOverlayTexturedLine::LineCapType endCapType, std::vector& verticesOut, +void CTexturedLineRData::CreateLineCap(const SOverlayTexturedLine& line, const CVector3D& corner1, const CVector3D& corner2, + const CVector3D& lineDirectionNormal, SOverlayTexturedLine::LineCapType endCapType, std::vector& verticesOut, std::vector& 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(corner1 + (lineDirectionNormal * (m_Line->m_Thickness)), 0.f, 0.66666f)); // extend butt corner point 1 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 * (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) diff --git a/source/renderer/TexturedLineRData.h b/source/renderer/TexturedLineRData.h index 2a50cd596a..a6e57a0166 100644 --- a/source/renderer/TexturedLineRData.h +++ b/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: - /** - * @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() : m_VB(NULL), m_VBIndices(NULL) { } ~CTexturedLineRData() { @@ -45,9 +54,8 @@ public: g_VBMan.Release(m_VBIndices); } - void Update(); - - void Render(const CShaderProgramPtr& shader); + void Update(const SOverlayTexturedLine& line); + void Render(const SOverlayTexturedLine& line, 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& verticesOut, std::vector& 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; };