From 8da027c56f383abfb140d90437b0de09ad7195ef Mon Sep 17 00:00:00 2001
From: Atlante45 <clement.brisset@gmail.com>
Date: Wed, 13 Jan 2016 12:15:57 -0800
Subject: [PATCH 1/3] Encapsulate GPUObject in a safer way

---
 libraries/gpu/src/gpu/Context.h     | 66 +++--------------------------
 libraries/gpu/src/gpu/Format.h      | 19 ++++++++-
 libraries/gpu/src/gpu/Framebuffer.h |  8 +---
 libraries/gpu/src/gpu/Pipeline.h    |  8 +---
 libraries/gpu/src/gpu/Query.h       | 10 +----
 libraries/gpu/src/gpu/Resource.cpp  | 13 ++----
 libraries/gpu/src/gpu/Resource.h    |  9 +---
 libraries/gpu/src/gpu/Shader.h      |  8 +---
 libraries/gpu/src/gpu/State.h       |  8 +---
 libraries/gpu/src/gpu/Texture.h     |  9 +---
 10 files changed, 34 insertions(+), 124 deletions(-)

diff --git a/libraries/gpu/src/gpu/Context.h b/libraries/gpu/src/gpu/Context.h
index ad21d71427..260aff72d7 100644
--- a/libraries/gpu/src/gpu/Context.h
+++ b/libraries/gpu/src/gpu/Context.h
@@ -97,68 +97,16 @@ public:
         TransformCamera getEyeCamera(int eye, const StereoState& stereo) const;
     };
 
-    template< typename T >
-    static void setGPUObject(const Buffer& buffer, T* object) {
-        buffer.setGPUObject(object);
+
+    template<typename T>
+    static void setGPUObject(const GPUObjectWrapper& wrapper, T* object) {
+        wrapper.setGPUObject(object);
     }
-    template< typename T >
-    static T* getGPUObject(const Buffer& buffer) {
-        return reinterpret_cast<T*>(buffer.getGPUObject());
+    template<typename T>
+    static T* getGPUObject(const GPUObjectWrapper& wrapper) {
+        return reinterpret_cast<T*>(wrapper.getGPUObject());
     }
 
-    template< typename T >
-    static void setGPUObject(const Texture& texture, T* object) {
-        texture.setGPUObject(object);
-    }
-    template< typename T >
-    static T* getGPUObject(const Texture& texture) {
-        return reinterpret_cast<T*>(texture.getGPUObject());
-    }
-    
-    template< typename T >
-    static void setGPUObject(const Shader& shader, T* object) {
-        shader.setGPUObject(object);
-    }
-    template< typename T >
-    static T* getGPUObject(const Shader& shader) {
-        return reinterpret_cast<T*>(shader.getGPUObject());
-    }
-
-    template< typename T >
-    static void setGPUObject(const Pipeline& pipeline, T* object) {
-        pipeline.setGPUObject(object);
-    }
-    template< typename T >
-    static T* getGPUObject(const Pipeline& pipeline) {
-        return reinterpret_cast<T*>(pipeline.getGPUObject());
-    }
-
-    template< typename T >
-    static void setGPUObject(const State& state, T* object) {
-        state.setGPUObject(object);
-    }
-    template< typename T >
-    static T* getGPUObject(const State& state) {
-        return reinterpret_cast<T*>(state.getGPUObject());
-    }
-
-    template< typename T >
-    static void setGPUObject(const Framebuffer& framebuffer, T* object) {
-        framebuffer.setGPUObject(object);
-    }
-    template< typename T >
-    static T* getGPUObject(const Framebuffer& framebuffer) {
-        return reinterpret_cast<T*>(framebuffer.getGPUObject());
-    }
-
-    template< typename T >
-    static void setGPUObject(const Query& query, T* object) {
-        query.setGPUObject(object);
-    }
-    template< typename T >
-    static T* getGPUObject(const Query& query) {
-        return reinterpret_cast<T*>(query.getGPUObject());
-    }
 
 protected:
     StereoState  _stereo;
diff --git a/libraries/gpu/src/gpu/Format.h b/libraries/gpu/src/gpu/Format.h
index 41a95e2578..1ae579d9f5 100644
--- a/libraries/gpu/src/gpu/Format.h
+++ b/libraries/gpu/src/gpu/Format.h
@@ -16,10 +16,25 @@
 
 namespace gpu {
 
+class Backend;
+
 class GPUObject {
 public:
-    GPUObject() {}
-    virtual ~GPUObject() {}
+    virtual ~GPUObject() = default;
+};
+
+class GPUObjectWrapper {
+public:
+    virtual ~GPUObjectWrapper() { delete _gpuObject; }
+
+private:
+    // This shouldn't be used by anything else than the Backend class with the proper casting.
+    // TODO: Consider using std::unique_ptr to get rid of dtor and ensure correct destruction of GPU objects
+    mutable GPUObject* _gpuObject { nullptr };
+    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
+    GPUObject* getGPUObject() const { return _gpuObject; }
+
+    friend class Backend;
 };
 
 typedef int  Stamp;
diff --git a/libraries/gpu/src/gpu/Framebuffer.h b/libraries/gpu/src/gpu/Framebuffer.h
index 807047a56e..5fbc2e1487 100755
--- a/libraries/gpu/src/gpu/Framebuffer.h
+++ b/libraries/gpu/src/gpu/Framebuffer.h
@@ -64,7 +64,7 @@ protected:
 typedef std::shared_ptr<Swapchain> SwapchainPointer;
 
 
-class Framebuffer {
+class Framebuffer : public GPUObjectWrapper {
 public:
     enum BufferMask {
         BUFFER_COLOR0 = 1,
@@ -153,12 +153,6 @@ protected:
     // Non exposed
     Framebuffer(const Framebuffer& framebuffer) = delete;
     Framebuffer() {}
-    
-    // This shouldn't be used by anything else than the Backend class with the proper casting.
-    mutable GPUObject* _gpuObject = NULL;
-    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-    GPUObject* getGPUObject() const { return _gpuObject; }
-    friend class Backend;
 };
 typedef std::shared_ptr<Framebuffer> FramebufferPointer;
 
diff --git a/libraries/gpu/src/gpu/Pipeline.h b/libraries/gpu/src/gpu/Pipeline.h
index adc65a0c66..f062873869 100755
--- a/libraries/gpu/src/gpu/Pipeline.h
+++ b/libraries/gpu/src/gpu/Pipeline.h
@@ -20,7 +20,7 @@
  
 namespace gpu {
 
-class Pipeline {
+class Pipeline : public GPUObjectWrapper {
 public:
     using Pointer = std::shared_ptr< Pipeline >;
 
@@ -38,12 +38,6 @@ protected:
     Pipeline();
     Pipeline(const Pipeline& pipeline); // deep copy of the sysmem shader
     Pipeline& operator=(const Pipeline& pipeline); // deep copy of the sysmem texture
-
-    // This shouldn't be used by anything else than the Backend class with the proper casting.
-    mutable GPUObject* _gpuObject = nullptr;
-    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-    GPUObject* getGPUObject() const { return _gpuObject; }
-    friend class Backend;
 };
 
 typedef Pipeline::Pointer PipelinePointer;
diff --git a/libraries/gpu/src/gpu/Query.h b/libraries/gpu/src/gpu/Query.h
index d9c3185d9e..82f17c71ae 100644
--- a/libraries/gpu/src/gpu/Query.h
+++ b/libraries/gpu/src/gpu/Query.h
@@ -19,7 +19,7 @@
 
 namespace gpu {
 
-    class Query {
+    class Query : public GPUObjectWrapper {
     public:
         Query();
         ~Query();
@@ -27,14 +27,6 @@ namespace gpu {
         uint32 queryResult;
 
         double getElapsedTime();
-
-    protected:
-        
-        // This shouldn't be used by anything else than the Backend class with the proper casting.
-        mutable GPUObject* _gpuObject = NULL;
-        void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-        GPUObject* getGPUObject() const { return _gpuObject; }
-        friend class Backend; 
     };
     
     typedef std::shared_ptr<Query> QueryPointer;
diff --git a/libraries/gpu/src/gpu/Resource.cpp b/libraries/gpu/src/gpu/Resource.cpp
index c162b38b93..1788f6ece5 100644
--- a/libraries/gpu/src/gpu/Resource.cpp
+++ b/libraries/gpu/src/gpu/Resource.cpp
@@ -170,20 +170,17 @@ Resource::Size Resource::Sysmem::append(Size size, const Byte* bytes) {
 
 Buffer::Buffer() :
     Resource(),
-    _sysmem(new Sysmem()),
-    _gpuObject(NULL) {
+    _sysmem(new Sysmem()) {
 }
 
 Buffer::Buffer(Size size, const Byte* bytes) :
     Resource(),
-    _sysmem(new Sysmem(size, bytes)),
-    _gpuObject(NULL) {
+    _sysmem(new Sysmem(size, bytes)) {
 }
 
 Buffer::Buffer(const Buffer& buf) :
     Resource(),
-    _sysmem(new Sysmem(buf.getSysmem())),
-    _gpuObject(NULL) {
+    _sysmem(new Sysmem(buf.getSysmem())) {
 }
 
 Buffer& Buffer::operator=(const Buffer& buf) {
@@ -196,10 +193,6 @@ Buffer::~Buffer() {
         delete _sysmem;
         _sysmem = NULL;
     }
-    if (_gpuObject) {
-        delete _gpuObject;
-        _gpuObject = NULL;
-    }
 }
 
 Buffer::Size Buffer::resize(Size size) {
diff --git a/libraries/gpu/src/gpu/Resource.h b/libraries/gpu/src/gpu/Resource.h
index 794ee680f4..817deaae86 100644
--- a/libraries/gpu/src/gpu/Resource.h
+++ b/libraries/gpu/src/gpu/Resource.h
@@ -108,7 +108,7 @@ protected:
 
 };
 
-class Buffer : public Resource {
+class Buffer : public Resource, public GPUObjectWrapper  {
 public:
 
     Buffer();
@@ -156,13 +156,6 @@ public:
 protected:
 
     Sysmem* _sysmem = NULL;
-
-
-    // This shouldn't be used by anything else than the Backend class with the proper casting.
-    mutable GPUObject* _gpuObject = NULL;
-    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-    GPUObject* getGPUObject() const { return _gpuObject; }
-    friend class Backend;
 };
 
 typedef std::shared_ptr<Buffer> BufferPointer;
diff --git a/libraries/gpu/src/gpu/Shader.h b/libraries/gpu/src/gpu/Shader.h
index b737a42e12..908e57e06b 100755
--- a/libraries/gpu/src/gpu/Shader.h
+++ b/libraries/gpu/src/gpu/Shader.h
@@ -20,7 +20,7 @@
  
 namespace gpu {
 
-class Shader {
+class Shader : public GPUObjectWrapper {
 public:
 
     typedef std::shared_ptr< Shader > Pointer;
@@ -178,12 +178,6 @@ protected:
 
     // The type of the shader, the master key
     Type _type;
-
-    // This shouldn't be used by anything else than the Backend class with the proper casting.
-    mutable GPUObject* _gpuObject = NULL;
-    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-    GPUObject* getGPUObject() const { return _gpuObject; }
-    friend class Backend;
 };
 
 typedef Shader::Pointer ShaderPointer;
diff --git a/libraries/gpu/src/gpu/State.h b/libraries/gpu/src/gpu/State.h
index 7740506bce..e468b01171 100755
--- a/libraries/gpu/src/gpu/State.h
+++ b/libraries/gpu/src/gpu/State.h
@@ -36,7 +36,7 @@ namespace gpu {
 
 class GPUObject;
 
-class State {
+class State : public GPUObjectWrapper {
 public:
     State();
     virtual ~State();
@@ -392,12 +392,6 @@ protected:
     Data _values;
     Signature _signature{0};
     Stamp _stamp{0};
-
-    // This shouldn't be used by anything else than the Backend class with the proper casting.
-    mutable GPUObject* _gpuObject = nullptr;
-    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-    GPUObject* getGPUObject() const { return _gpuObject; }
-    friend class Backend;
 };
 
 typedef std::shared_ptr< State > StatePointer;
diff --git a/libraries/gpu/src/gpu/Texture.h b/libraries/gpu/src/gpu/Texture.h
index 6e8eb10380..e68b65a15e 100755
--- a/libraries/gpu/src/gpu/Texture.h
+++ b/libraries/gpu/src/gpu/Texture.h
@@ -136,7 +136,7 @@ protected:
     Desc _desc;
 };
 
-class Texture : public Resource {
+class Texture : public Resource, public GPUObjectWrapper {
 public:
 
     class Pixels {
@@ -386,13 +386,6 @@ protected:
     static Texture* create(Type type, const Element& texelFormat, uint16 width, uint16 height, uint16 depth, uint16 numSamples, uint16 numSlices, const Sampler& sampler);
 
     Size resize(Type type, const Element& texelFormat, uint16 width, uint16 height, uint16 depth, uint16 numSamples, uint16 numSlices);
-
-    // This shouldn't be used by anything else than the Backend class with the proper casting.
-    mutable GPUObject* _gpuObject = NULL;
-    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-    GPUObject* getGPUObject() const { return _gpuObject; }
-
-    friend class Backend;
 };
 
 typedef std::shared_ptr<Texture> TexturePointer;

From 504939f1937fae3f2d1212e7dc6f2e7af45adab7 Mon Sep 17 00:00:00 2001
From: Atlante45 <clement.brisset@gmail.com>
Date: Wed, 13 Jan 2016 14:07:48 -0800
Subject: [PATCH 2/3] Use a unique_ptr to track gpu objects

---
 libraries/gpu/src/gpu/Format.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/libraries/gpu/src/gpu/Format.h b/libraries/gpu/src/gpu/Format.h
index 1ae579d9f5..61fb1e847e 100644
--- a/libraries/gpu/src/gpu/Format.h
+++ b/libraries/gpu/src/gpu/Format.h
@@ -11,8 +11,10 @@
 #ifndef hifi_gpu_Format_h
 #define hifi_gpu_Format_h
 
-#include <glm/glm.hpp>
 #include <assert.h>
+#include <memory>
+
+#include <glm/glm.hpp>
 
 namespace gpu {
 
@@ -25,14 +27,15 @@ public:
 
 class GPUObjectWrapper {
 public:
-    virtual ~GPUObjectWrapper() { delete _gpuObject; }
+    virtual ~GPUObjectWrapper() = default;
 
 private:
+    using GPUObjectPointer = std::unique_ptr<GPUObject>;
+
     // This shouldn't be used by anything else than the Backend class with the proper casting.
-    // TODO: Consider using std::unique_ptr to get rid of dtor and ensure correct destruction of GPU objects
-    mutable GPUObject* _gpuObject { nullptr };
-    void setGPUObject(GPUObject* gpuObject) const { _gpuObject = gpuObject; }
-    GPUObject* getGPUObject() const { return _gpuObject; }
+    mutable GPUObjectPointer _gpuObject;
+    void setGPUObject(GPUObject* gpuObject) const { _gpuObject.reset(gpuObject); }
+    GPUObject* getGPUObject() const { return _gpuObject.get(); }
 
     friend class Backend;
 };

From ef5af45acdc6a5902232f4e1d79d1f12aa021f5c Mon Sep 17 00:00:00 2001
From: Atlante45 <clement.brisset@gmail.com>
Date: Wed, 13 Jan 2016 14:55:49 -0800
Subject: [PATCH 3/3] Replce GPUObjectWrapper by public const member

---
 libraries/gpu/src/gpu/Context.h     | 12 ++++++------
 libraries/gpu/src/gpu/Format.h      |  9 +++------
 libraries/gpu/src/gpu/Framebuffer.h |  4 +++-
 libraries/gpu/src/gpu/Pipeline.h    |  4 +++-
 libraries/gpu/src/gpu/Query.h       |  4 +++-
 libraries/gpu/src/gpu/Resource.h    |  4 +++-
 libraries/gpu/src/gpu/Shader.h      |  4 +++-
 libraries/gpu/src/gpu/State.h       |  4 +++-
 libraries/gpu/src/gpu/Texture.h     |  4 +++-
 9 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/libraries/gpu/src/gpu/Context.h b/libraries/gpu/src/gpu/Context.h
index 260aff72d7..ed2afe91eb 100644
--- a/libraries/gpu/src/gpu/Context.h
+++ b/libraries/gpu/src/gpu/Context.h
@@ -98,13 +98,13 @@ public:
     };
 
 
-    template<typename T>
-    static void setGPUObject(const GPUObjectWrapper& wrapper, T* object) {
-        wrapper.setGPUObject(object);
+    template<typename T, typename U>
+    static void setGPUObject(const U& object, T* gpuObject) {
+        object.gpuObject.setGPUObject(gpuObject);
     }
-    template<typename T>
-    static T* getGPUObject(const GPUObjectWrapper& wrapper) {
-        return reinterpret_cast<T*>(wrapper.getGPUObject());
+    template<typename T, typename U>
+    static T* getGPUObject(const U& object) {
+        return reinterpret_cast<T*>(object.gpuObject.getGPUObject());
     }
 
 
diff --git a/libraries/gpu/src/gpu/Format.h b/libraries/gpu/src/gpu/Format.h
index 61fb1e847e..54d40c3e12 100644
--- a/libraries/gpu/src/gpu/Format.h
+++ b/libraries/gpu/src/gpu/Format.h
@@ -25,15 +25,12 @@ public:
     virtual ~GPUObject() = default;
 };
 
-class GPUObjectWrapper {
-public:
-    virtual ~GPUObjectWrapper() = default;
-
+class GPUObjectPointer {
 private:
-    using GPUObjectPointer = std::unique_ptr<GPUObject>;
+    using GPUObjectUniquePointer = std::unique_ptr<GPUObject>;
 
     // This shouldn't be used by anything else than the Backend class with the proper casting.
-    mutable GPUObjectPointer _gpuObject;
+    mutable GPUObjectUniquePointer _gpuObject;
     void setGPUObject(GPUObject* gpuObject) const { _gpuObject.reset(gpuObject); }
     GPUObject* getGPUObject() const { return _gpuObject.get(); }
 
diff --git a/libraries/gpu/src/gpu/Framebuffer.h b/libraries/gpu/src/gpu/Framebuffer.h
index 5fbc2e1487..e986e4a481 100755
--- a/libraries/gpu/src/gpu/Framebuffer.h
+++ b/libraries/gpu/src/gpu/Framebuffer.h
@@ -64,7 +64,7 @@ protected:
 typedef std::shared_ptr<Swapchain> SwapchainPointer;
 
 
-class Framebuffer : public GPUObjectWrapper {
+class Framebuffer {
 public:
     enum BufferMask {
         BUFFER_COLOR0 = 1,
@@ -134,6 +134,8 @@ public:
     static const uint32 MAX_NUM_RENDER_BUFFERS = 8; 
     static uint32 getMaxNumRenderBuffers() { return MAX_NUM_RENDER_BUFFERS; }
 
+    const GPUObjectPointer gpuObject {};
+    
 protected:
     SwapchainPointer _swapchain;
 
diff --git a/libraries/gpu/src/gpu/Pipeline.h b/libraries/gpu/src/gpu/Pipeline.h
index f062873869..28f7fe106e 100755
--- a/libraries/gpu/src/gpu/Pipeline.h
+++ b/libraries/gpu/src/gpu/Pipeline.h
@@ -20,7 +20,7 @@
  
 namespace gpu {
 
-class Pipeline : public GPUObjectWrapper {
+class Pipeline {
 public:
     using Pointer = std::shared_ptr< Pipeline >;
 
@@ -31,6 +31,8 @@ public:
 
     const StatePointer& getState() const { return _state; }
 
+    const GPUObjectPointer gpuObject {};
+    
 protected:
     ShaderPointer _program;
     StatePointer _state;
diff --git a/libraries/gpu/src/gpu/Query.h b/libraries/gpu/src/gpu/Query.h
index 82f17c71ae..25897c5c91 100644
--- a/libraries/gpu/src/gpu/Query.h
+++ b/libraries/gpu/src/gpu/Query.h
@@ -19,7 +19,7 @@
 
 namespace gpu {
 
-    class Query : public GPUObjectWrapper {
+    class Query {
     public:
         Query();
         ~Query();
@@ -27,6 +27,8 @@ namespace gpu {
         uint32 queryResult;
 
         double getElapsedTime();
+
+        const GPUObjectPointer gpuObject {};
     };
     
     typedef std::shared_ptr<Query> QueryPointer;
diff --git a/libraries/gpu/src/gpu/Resource.h b/libraries/gpu/src/gpu/Resource.h
index 817deaae86..3517b67203 100644
--- a/libraries/gpu/src/gpu/Resource.h
+++ b/libraries/gpu/src/gpu/Resource.h
@@ -108,7 +108,7 @@ protected:
 
 };
 
-class Buffer : public Resource, public GPUObjectWrapper  {
+class Buffer : public Resource {
 public:
 
     Buffer();
@@ -153,6 +153,8 @@ public:
     const Sysmem& getSysmem() const { assert(_sysmem); return (*_sysmem); }
     Sysmem& editSysmem() { assert(_sysmem); return (*_sysmem); }
 
+    const GPUObjectPointer gpuObject {};
+    
 protected:
 
     Sysmem* _sysmem = NULL;
diff --git a/libraries/gpu/src/gpu/Shader.h b/libraries/gpu/src/gpu/Shader.h
index 908e57e06b..59c6401150 100755
--- a/libraries/gpu/src/gpu/Shader.h
+++ b/libraries/gpu/src/gpu/Shader.h
@@ -20,7 +20,7 @@
  
 namespace gpu {
 
-class Shader : public GPUObjectWrapper {
+class Shader {
 public:
 
     typedef std::shared_ptr< Shader > Pointer;
@@ -155,6 +155,8 @@ public:
     // independant of the graphics api in use underneath (looking at you opengl & vulkan).
     static bool makeProgram(Shader& shader, const Shader::BindingSet& bindings = Shader::BindingSet());
 
+    const GPUObjectPointer gpuObject {};
+    
 protected:
     Shader(Type type, const Source& source);
     Shader(Type type, const Pointer& vertex, const Pointer& pixel);
diff --git a/libraries/gpu/src/gpu/State.h b/libraries/gpu/src/gpu/State.h
index e468b01171..7e32a7280a 100755
--- a/libraries/gpu/src/gpu/State.h
+++ b/libraries/gpu/src/gpu/State.h
@@ -36,7 +36,7 @@ namespace gpu {
 
 class GPUObject;
 
-class State : public GPUObjectWrapper {
+class State {
 public:
     State();
     virtual ~State();
@@ -385,6 +385,8 @@ public:
     State(const Data& values);
     const Data& getValues() const { return _values; }
 
+    const GPUObjectPointer gpuObject {};
+
 protected:
     State(const State& state);
     State& operator=(const State& state);
diff --git a/libraries/gpu/src/gpu/Texture.h b/libraries/gpu/src/gpu/Texture.h
index e68b65a15e..378f49c2f4 100755
--- a/libraries/gpu/src/gpu/Texture.h
+++ b/libraries/gpu/src/gpu/Texture.h
@@ -136,7 +136,7 @@ protected:
     Desc _desc;
 };
 
-class Texture : public Resource, public GPUObjectWrapper {
+class Texture : public Resource {
 public:
 
     class Pixels {
@@ -356,6 +356,8 @@ public:
     // Only callable by the Backend
     void notifyMipFaceGPULoaded(uint16 level, uint8 face) const { return _storage->notifyMipFaceGPULoaded(level, face); }
 
+    const GPUObjectPointer gpuObject {};
+
 protected:
     std::unique_ptr< Storage > _storage;