#5584: More refactoring to move the shader-specific update routines t…

…o the base class. Subclasses should only care about vertices and indices at best.

libs/render/RenderableGeometry.h

@@ -17,7 +17,7 @@ namespace render
 class RenderableGeometry :
     public OpenGLRenderable
 {
-protected:
+private:
     ShaderPtr _shader;
     IGeometryRenderer::Slot _surfaceSlot;
 
@@ -27,16 +27,38 @@ class RenderableGeometry :
     {}
 
 public:
-    // Removes any geometry attached to a shader
-    virtual void clear()
+    // Noncopyable
+    RenderableGeometry(const RenderableGeometry& other) = delete;
+    RenderableGeometry& operator=(const RenderableGeometry& other) = delete;
+
+    // (Non-virtual) update method handling any possible shader change
+    // The geometry is withdrawn from the given shader if it turns out
+    // to be different from the last update.
+    void update(const ShaderPtr& shader)
     {
-        if (_shader && _surfaceSlot != IGeometryRenderer::InvalidSlot)
+        bool shaderChanged = _shader != shader;
+
+        if (shaderChanged)
         {
-            _shader->removeGeometry(_surfaceSlot);
+            clear();
         }
 
+        // Update our local shader reference
+        _shader = shader;
+
+        if (_shader)
+        {
+            // Invoke the virtual method to run needed updates in the subclass
+            updateGeometry();
+        }
+    }
+
+    // Removes the geometry and clears the shader reference
+    virtual void clear()
+    {
+        removeGeometry();
+
         _shader.reset();
-        _surfaceSlot = IGeometryRenderer::InvalidSlot;
     }
 
     virtual void render(const RenderInfo& info) const override
@@ -48,17 +70,36 @@ class RenderableGeometry :
     }
 
 protected:
-    virtual void addOrUpdateGeometry(const ShaderPtr& shader, GeometryType type,
+    // Removes the geometry from the attached shader. Does nothing if no geometry has been added.
+    void removeGeometry()
+    {
+        if (_shader && _surfaceSlot != IGeometryRenderer::InvalidSlot)
+        {
+            _shader->removeGeometry(_surfaceSlot);
+        }
+
+        _surfaceSlot = IGeometryRenderer::InvalidSlot;
+    }
+
+    // Sub-class specific geometry update. Should check whether any of the
+    // vertex data needs to be added or updated to the shader, in which case
+    // the implementation should invoke addOrUpdateGeometry()
+    virtual void updateGeometry() = 0;
+
+    // Submits the given geometry to the known _shader reference
+    // This method is supposed to be called from within updateGeometry()
+    // to ensure that the _shader reference is already up to date.
+    virtual void addOrUpdateGeometry(GeometryType type,
         const std::vector<ArbitraryMeshVertex>& vertices,
         const std::vector<unsigned int>& indices)
     {
         if (_surfaceSlot == IGeometryRenderer::InvalidSlot)
         {
-            _surfaceSlot = shader->addGeometry(type, vertices, indices);
+            _surfaceSlot = _shader->addGeometry(type, vertices, indices);
         }
         else
         {
-            shader->updateGeometry(_surfaceSlot, vertices, indices);
+            _shader->updateGeometry(_surfaceSlot, vertices, indices);
         }
     }
 };

radiantcore/entity/target/RenderableTargetLines.h

@@ -28,15 +28,14 @@ class RenderableTargetLines :
     public render::RenderableGeometry
 {
 private:
-	const TargetKeyCollection& _targetKeys;
+    const TargetKeyCollection& _targetKeys;
 
-    bool _needsUpdate;
+    Vector3 _worldPosition;
     std::size_t _numVisibleLines;
 
 public:
     RenderableTargetLines(const TargetKeyCollection& targetKeys) :
         _targetKeys(targetKeys),
-        _needsUpdate(true),
         _numVisibleLines(0)
     {}
 
@@ -45,11 +44,6 @@ class RenderableTargetLines :
         return !_targetKeys.empty();
     }
 
-    void queueUpdate()
-    {
-        _needsUpdate = true;
-    }
-
     void clear() override
     {
         RenderableGeometry::clear();
@@ -58,13 +52,20 @@ class RenderableTargetLines :
     }
 
     void update(const ShaderPtr& shader, const Vector3& worldPosition)
+    {
+        // Store the new world position for use in updateGeometry()
+        _worldPosition = worldPosition;
+
+        // Tell the base class to run the rest of the update routine
+        RenderableGeometry::update(shader);
+    }
+
+protected:
+    void updateGeometry() override
     {
         // Target lines are visible if both their start and end entities are visible
         // This is hard to track in the scope of this class, so we fall back to doing 
-        // an update on the renderable geometry every frame
-
-        auto shaderChanged = _shader != shader;
-        _needsUpdate = false;
+        // an update on the renderable geometry every time we're asked to
 
         // Collect vertex and index data
         std::vector<ArbitraryMeshVertex> vertices;
@@ -85,19 +86,17 @@ class RenderableTargetLines :
             numVisibleLines++;
             auto targetPosition = target->getPosition();
 
-            addTargetLine(worldPosition, targetPosition, vertices, indices);
+            addTargetLine(_worldPosition, targetPosition, vertices, indices);
         });
 
-        // Size or shader changes both require detaching our geometry first
-        if (_shader && _surfaceSlot != render::IGeometryRenderer::InvalidSlot && (shaderChanged || numVisibleLines != _numVisibleLines))
+        // Size changes requires detaching our geometry first
+        if (numVisibleLines != _numVisibleLines)
         {
-            clear();
+            removeGeometry();
+            _numVisibleLines = numVisibleLines;
         }
 
-        _shader = shader;
-        _numVisibleLines = numVisibleLines;
-
-        addOrUpdateGeometry(shader, render::GeometryType::Lines, vertices, indices);
+        addOrUpdateGeometry(render::GeometryType::Lines, vertices, indices);
     }
 
 private:

radiantcore/entity/target/TargetLineNode.cpp

@@ -33,7 +33,7 @@ void TargetLineNode::onInsertIntoScene(scene::IMapRootNode& root)
 {
     Node::onInsertIntoScene(root);
 
-    _targetLines.queueUpdate();
+    _targetLines.clear();
 }
 
 void TargetLineNode::onRemoveFromScene(scene::IMapRootNode& root)
@@ -72,7 +72,6 @@ void TargetLineNode::renderHighlights(IRenderableCollector& collector, const Vol
 void TargetLineNode::onRenderSystemChanged()
 {
     _targetLines.clear();
-    _targetLines.queueUpdate();
 }
 
 void TargetLineNode::onVisibilityChanged(bool visible)
@@ -82,13 +81,10 @@ void TargetLineNode::onVisibilityChanged(bool visible)
     if (!visible)
     {
         // Disconnect our renderable when the node is hidden
+        // Once this node is shown again, the onPreRender method will
+        // call RenderableTargetLines::update()
         _targetLines.clear();
     }
-    else
-    {
-        // Update the vertex buffers next time we need to render
-        _targetLines.queueUpdate();
-    }
 }
 
 std::size_t TargetLineNode::getHighlightFlags()

radiantcore/patch/PatchRenderables.h

@@ -187,29 +187,26 @@ class RenderablePatchTesselation :
         _needsUpdate = true;
     }
 
-    void update(const ShaderPtr& shader)
+protected:
+    void updateGeometry() override
     {
-        bool shaderChanged = _shader != shader;
-
-        if (!_needsUpdate && !shaderChanged) return;
+        if (!_needsUpdate) return;
 
         _needsUpdate = false;
-        auto sizeChanged = _tess.vertices.size() != _size;
 
-        if (_shader && _surfaceSlot != render::IGeometryRenderer::InvalidSlot && (shaderChanged || sizeChanged))
+        // Tesselation size change requires to remove the geometry first
+        if (_tess.vertices.size() != _size)
         {
-            clear();
+            removeGeometry();
+            _size = _tess.vertices.size();
         }
 
-        _shader = shader;
-        _size = _tess.vertices.size();
-
-        // Generate the index array
+        // Generate the new index array
         std::vector<unsigned int> indices;
         indices.reserve(_indexer.getNumIndices(_tess));
 
         _indexer.generateIndices(_tess, std::back_inserter(indices));
 
-        addOrUpdateGeometry(shader, _indexer.getType(), _tess.vertices, indices);
+        addOrUpdateGeometry(_indexer.getType(), _tess.vertices, indices);
     }
 };