PR feedback

CMakeLists.txt

@@ -108,8 +108,10 @@ set(PLATFORM_QT_GL OpenGL)
 
 if (USE_GLES)
   add_definitions(-DUSE_GLES)
+  add_definitions(-DGPU_POINTER_STORAGE_SHARED)
   set(PLATFORM_GL_BACKEND gpu-gl-common gpu-gles)
 else()
+  add_definitions(-DGPU_POINTER_STORAGE_RAW)
   set(PLATFORM_GL_BACKEND gpu-gl-common gpu-gl)
 endif()
 

interface/src/Application.cpp

@@ -3962,7 +3962,7 @@ void Application::keyPressEvent(QKeyEvent* event) {
                 break;
 
             case Qt::Key_G:
-                if (isShifted && isMeta) {
+                if (isShifted && isMeta && Menu::getInstance() && Menu::getInstance()->getMenu("Developer")->isVisible()) {
                     static const QString HIFI_FRAMES_FOLDER_VAR = "HIFI_FRAMES_FOLDER";
                     static const QString GPU_FRAME_FOLDER = QProcessEnvironment::systemEnvironment().contains(HIFI_FRAMES_FOLDER_VAR)
                         ? QProcessEnvironment::systemEnvironment().value(HIFI_FRAMES_FOLDER_VAR)

libraries/gpu-gl-common/src/gpu/gl/GLBackend.h

@@ -54,28 +54,6 @@
 #define GPU_STEREO_CAMERA_BUFFER
 #endif
 
-//
-// GL Backend pointer storage mechanism
-// One of the following three defines must be defined.
-// GPU_POINTER_STORAGE_SHARED
-
-// The platonic ideal, use references to smart pointers.
-// However, this produces artifacts because there are too many places in the code right now that
-// create temporary values (undesirable smart pointer duplications) and then those temp variables
-// get passed on and have their reference taken, and then invalidated
-// GPU_POINTER_STORAGE_REF
-
-// Raw pointer manipulation.  Seems more dangerous than the reference wrappers,
-// but in practice, the danger of grabbing a reference to a temporary variable
-// is causing issues
-// GPU_POINTER_STORAGE_RAW
-
-#if defined(USE_GLES)
-#define GPU_POINTER_STORAGE_SHARED
-#else
-#define GPU_POINTER_STORAGE_RAW
-#endif
-
 namespace gpu { namespace gl {
 
 class GLBackend : public Backend, public std::enable_shared_from_this<GLBackend> {

libraries/gpu-gl-common/src/gpu/gl/GLState.cpp

@@ -44,7 +44,7 @@ const GLState::Commands makeResetStateCommands() {
     // and we have a 50/50 chance that State::DEFAULT is not yet initialized.
     // Since State::DEFAULT = State::Data() it is much easier to not use the actual State::DEFAULT
     // but another State::Data object with a default initialization.
-    const State::Data& DEFAULT = State::DEFAULT;
+    const State::Data DEFAULT = State::Data();
 
     auto depthBiasCommand = std::make_shared<CommandDepthBias>(&GLBackend::do_setStateDepthBias,
         Vec2(DEFAULT.depthBias, DEFAULT.depthBiasSlopeScale));

libraries/gpu/src/gpu/Context.h

@@ -24,157 +24,12 @@
 #include "Pipeline.h"
 #include "Framebuffer.h"
 #include "Frame.h"
+#include "PointerStorage.h"
 
 class QImage;
 
 namespace gpu {
 
-//
-// GL Backend pointer storage mechanism
-// One of the following three defines must be defined.
-// GPU_POINTER_STORAGE_SHARED
-
-// The platonic ideal, use references to smart pointers.
-// However, this produces artifacts because there are too many places in the code right now that
-// create temporary values (undesirable smart pointer duplications) and then those temp variables
-// get passed on and have their reference taken, and then invalidated
-// GPU_POINTER_STORAGE_REF
-
-// Raw pointer manipulation.  Seems more dangerous than the reference wrappers,
-// but in practice, the danger of grabbing a reference to a temporary variable
-// is causing issues
-// GPU_POINTER_STORAGE_RAW
-
-#if defined(USE_GLES)
-#define GPU_POINTER_STORAGE_SHARED
-#else
-#define GPU_POINTER_STORAGE_RAW
-#endif
-
-#if defined(GPU_POINTER_STORAGE_SHARED)
-template <typename T>
-static inline bool compare(const std::shared_ptr<T>& a, const std::shared_ptr<T>& b) {
-    return a == b;
-}
-
-template <typename T>
-static inline T* acquire(const std::shared_ptr<T>& pointer) {
-    return pointer.get();
-}
-
-template <typename T>
-static inline void reset(std::shared_ptr<T>& pointer) {
-    return pointer.reset();
-}
-
-template <typename T>
-static inline bool valid(const std::shared_ptr<T>& pointer) {
-    return pointer.operator bool();
-}
-
-template <typename T>
-static inline void assign(std::shared_ptr<T>& pointer, const std::shared_ptr<T>& source) {
-    pointer = source;
-}
-
-using BufferReference = BufferPointer;
-using TextureReference = TexturePointer;
-using FramebufferReference = FramebufferPointer;
-using FormatReference = Stream::FormatPointer;
-using PipelineReference = PipelinePointer;
-
-#define GPU_REFERENCE_INIT_VALUE nullptr
-
-#elif defined(GPU_POINTER_STORAGE_REF)
-
-template <typename T>
-class PointerReferenceWrapper : public std::reference_wrapper<const std::shared_ptr<T>> {
-    using Parent = std::reference_wrapper<const std::shared_ptr<T>>;
-
-public:
-    using Pointer = std::shared_ptr<T>;
-    PointerReferenceWrapper() : Parent(EMPTY()) {}
-    PointerReferenceWrapper(const Pointer& pointer) : Parent(pointer) {}
-    void clear() { *this = EMPTY(); }
-
-private:
-    static const Pointer& EMPTY() {
-        static const Pointer EMPTY_VALUE;
-        return EMPTY_VALUE;
-    };
-};
-
-template <typename T>
-static bool compare(const PointerReferenceWrapper<T>& reference, const std::shared_ptr<T>& pointer) {
-    return reference.get() == pointer;
-}
-
-template <typename T>
-static inline T* acquire(const PointerReferenceWrapper<T>& reference) {
-    return reference.get().get();
-}
-
-template <typename T>
-static void assign(PointerReferenceWrapper<T>& reference, const std::shared_ptr<T>& pointer) {
-    reference = pointer;
-}
-
-template <typename T>
-static bool valid(const PointerReferenceWrapper<T>& reference) {
-    return reference.get().operator bool();
-}
-
-template <typename T>
-static inline void reset(PointerReferenceWrapper<T>& reference) {
-    return reference.clear();
-}
-
-using BufferReference = PointerReferenceWrapper<Buffer>;
-using TextureReference = PointerReferenceWrapper<Texture>;
-using FramebufferReference = PointerReferenceWrapper<Framebuffer>;
-using FormatReference = PointerReferenceWrapper<Stream::Format>;
-using PipelineReference = PointerReferenceWrapper<Pipeline>;
-
-#define GPU_REFERENCE_INIT_VALUE
-
-#elif defined(GPU_POINTER_STORAGE_RAW)
-
-template <typename T>
-static bool compare(const T* const& rawPointer, const std::shared_ptr<T>& pointer) {
-    return rawPointer == pointer.get();
-}
-
-template <typename T>
-static inline T* acquire(T* const& rawPointer) {
-    return rawPointer;
-}
-
-template <typename T>
-static inline bool valid(const T* const& rawPointer) {
-    return rawPointer;
-}
-
-template <typename T>
-static inline void reset(T*& rawPointer) {
-    rawPointer = nullptr;
-}
-
-template <typename T>
-static inline void assign(T*& rawPointer, const std::shared_ptr<T>& pointer) {
-    rawPointer = pointer.get();
-}
-
-using BufferReference = Buffer*;
-using TextureReference = Texture*;
-using FramebufferReference = Framebuffer*;
-using FormatReference = Stream::Format*;
-using PipelineReference = Pipeline*;
-
-#define GPU_REFERENCE_INIT_VALUE nullptr
-
-#endif
-
-
 struct ContextStats {
 public:
     int _ISNumFormatChanges = 0;

libraries/gpu/src/gpu/PointerStorage.h

@@ -0,0 +1,159 @@
+//
+//  Created by Bradley Austin Davis on 2019/01/25
+//  Copyright 2013-2019 High Fidelity, Inc.
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+#pragma once
+#ifndef hifi_gpu_PointerStorage_h
+#define hifi_gpu_PointerStorage_h
+
+namespace gpu {
+
+//
+// GL Backend pointer storage mechanism
+// One of the following three defines must be defined.
+// GPU_POINTER_STORAGE_SHARED
+
+// The platonic ideal, use references to smart pointers.
+// However, this produces artifacts because there are too many places in the code right now that
+// create temporary values (undesirable smart pointer duplications) and then those temp variables
+// get passed on and have their reference taken, and then invalidated
+// GPU_POINTER_STORAGE_REF
+
+// Raw pointer manipulation.  Seems more dangerous than the reference wrappers,
+// but in practice, the danger of grabbing a reference to a temporary variable
+// is causing issues
+// GPU_POINTER_STORAGE_RAW
+
+#if defined(GPU_POINTER_STORAGE_SHARED)
+template <typename T>
+static inline bool compare(const std::shared_ptr<T>& a, const std::shared_ptr<T>& b) {
+    return a == b;
+}
+
+template <typename T>
+static inline T* acquire(const std::shared_ptr<T>& pointer) {
+    return pointer.get();
+}
+
+template <typename T>
+static inline void reset(std::shared_ptr<T>& pointer) {
+    return pointer.reset();
+}
+
+template <typename T>
+static inline bool valid(const std::shared_ptr<T>& pointer) {
+    return pointer.operator bool();
+}
+
+template <typename T>
+static inline void assign(std::shared_ptr<T>& pointer, const std::shared_ptr<T>& source) {
+    pointer = source;
+}
+
+using BufferReference = BufferPointer;
+using TextureReference = TexturePointer;
+using FramebufferReference = FramebufferPointer;
+using FormatReference = Stream::FormatPointer;
+using PipelineReference = PipelinePointer;
+
+#define GPU_REFERENCE_INIT_VALUE nullptr
+
+#elif defined(GPU_POINTER_STORAGE_REF)
+
+template <typename T>
+class PointerReferenceWrapper : public std::reference_wrapper<const std::shared_ptr<T>> {
+    using Parent = std::reference_wrapper<const std::shared_ptr<T>>;
+
+public:
+    using Pointer = std::shared_ptr<T>;
+    PointerReferenceWrapper() : Parent(EMPTY()) {}
+    PointerReferenceWrapper(const Pointer& pointer) : Parent(pointer) {}
+    void clear() { *this = EMPTY(); }
+
+private:
+    static const Pointer& EMPTY() {
+        static const Pointer EMPTY_VALUE;
+        return EMPTY_VALUE;
+    };
+};
+
+template <typename T>
+static bool compare(const PointerReferenceWrapper<T>& reference, const std::shared_ptr<T>& pointer) {
+    return reference.get() == pointer;
+}
+
+template <typename T>
+static inline T* acquire(const PointerReferenceWrapper<T>& reference) {
+    return reference.get().get();
+}
+
+template <typename T>
+static void assign(PointerReferenceWrapper<T>& reference, const std::shared_ptr<T>& pointer) {
+    reference = pointer;
+}
+
+template <typename T>
+static bool valid(const PointerReferenceWrapper<T>& reference) {
+    return reference.get().operator bool();
+}
+
+template <typename T>
+static inline void reset(PointerReferenceWrapper<T>& reference) {
+    return reference.clear();
+}
+
+using BufferReference = PointerReferenceWrapper<Buffer>;
+using TextureReference = PointerReferenceWrapper<Texture>;
+using FramebufferReference = PointerReferenceWrapper<Framebuffer>;
+using FormatReference = PointerReferenceWrapper<Stream::Format>;
+using PipelineReference = PointerReferenceWrapper<Pipeline>;
+
+#define GPU_REFERENCE_INIT_VALUE
+
+#elif defined(GPU_POINTER_STORAGE_RAW)
+
+template <typename T>
+static bool compare(const T* const& rawPointer, const std::shared_ptr<T>& pointer) {
+    return rawPointer == pointer.get();
+}
+
+template <typename T>
+static inline T* acquire(T* const& rawPointer) {
+    return rawPointer;
+}
+
+template <typename T>
+static inline bool valid(const T* const& rawPointer) {
+    return rawPointer;
+}
+
+template <typename T>
+static inline void reset(T*& rawPointer) {
+    rawPointer = nullptr;
+}
+
+template <typename T>
+static inline void assign(T*& rawPointer, const std::shared_ptr<T>& pointer) {
+    rawPointer = pointer.get();
+}
+
+using BufferReference = Buffer*;
+using TextureReference = Texture*;
+using FramebufferReference = Framebuffer*;
+using FormatReference = Stream::Format*;
+using PipelineReference = Pipeline*;
+
+#define GPU_REFERENCE_INIT_VALUE nullptr
+
+#else
+
+#error "No GPU pointer storage scheme defined"
+
+#endif
+
+}
+
+#endif // hifi_gpu_PointerStorage_h

libraries/gpu/src/gpu/State.cpp

@@ -105,7 +105,6 @@ std::string State::getKey() const {
     key += ":" + hex(_values.stencilTestBack.getRaw());
     key += ":" + hex(_values.blendFunction.getRaw());
     key += ":" + hex(_values.sampleMask);
-    key += ":" + hex(_values.sampleMask);
     // fillMode, cullMode, colorMaskWrite and the flags consume 32 bits alltogether
     static_assert(0 == offsetof(State::Data, fillMode) % 4, "Validate fillMode offset");
     key += ":" + hex(*(int*)&_values.fillMode);

libraries/gpu/src/gpu/State.h

@@ -262,7 +262,7 @@ public:
 
     static_assert(sizeof(Flags) == sizeof(uint8), "Flags size check");
 
-        // The Data class is the full explicit description of the State class fields value.
+    // The Data class is the full explicit description of the State class fields value.
     // Useful for having one const static called Default for reference or for the gpu::Backend to keep track of the current value
     class Data {
     public:
@@ -273,13 +273,11 @@ public:
         StencilActivation stencilActivation;
         StencilTest stencilTestFront;
         StencilTest stencilTestBack;
-        BlendFunction blendFunction;
-
         uint32 sampleMask = 0xFFFFFFFF;
-
-        FillMode fillMode = FILL_FACE;
-        CullMode cullMode = CULL_NONE;
-        ColorMask colorWriteMask = WRITE_ALL;
+        BlendFunction blendFunction;
+        FillMode fillMode{ FILL_FACE };
+        CullMode cullMode{ CULL_NONE };
+        ColorMask colorWriteMask{ WRITE_ALL };
 
         Flags flags;
     };
@@ -290,15 +288,14 @@ public:
     static_assert(offsetof(Data, stencilActivation) == 12, "Data offsets");
     static_assert(offsetof(Data, stencilTestFront) == 16, "Data offsets");
     static_assert(offsetof(Data, stencilTestBack) == 20, "Data offsets");
-    static_assert(offsetof(Data, blendFunction) == 24, "Data offsets");
-    static_assert(offsetof(Data, sampleMask) == 28, "Data offsets");
+    static_assert(offsetof(Data, sampleMask) == 24, "Data offsets");
+    static_assert(offsetof(Data, blendFunction) == 28, "Data offsets");
     static_assert(offsetof(Data, fillMode) == 32, "Data offsets");
     static_assert(offsetof(Data, cullMode) == 33, "Data offsets");
     static_assert(offsetof(Data, colorWriteMask) == 34, "Data offsets");
     static_assert(offsetof(Data, flags) == 35, "Data offsets");
     static_assert(sizeof(Data) == 36, "Data Size Check");
 
-
     std::string getKey() const;
 
     // The unique default values for all the fields
@@ -310,7 +307,7 @@ public:
     CullMode getCullMode() const { return _values.cullMode; }
 
     const Flags& getFlags() const { return _values.flags; }
-    
+
     void setFrontFaceClockwise(bool isClockwise) { SET_FIELD(FRONT_FACE_CLOCKWISE, flags.frontFaceClockwise, isClockwise); }
     bool isFrontFaceClockwise() const { return _values.flags.frontFaceClockwise; }