Merge pull request #2666 from yuriks/gl-cleanups

PicaShaderConfig cleanups

src/video_core/renderer_opengl/gl_rasterizer.cpp

@@ -20,7 +20,6 @@
 #include "video_core/regs_texturing.h"
 #include "video_core/renderer_opengl/gl_rasterizer.h"
 #include "video_core/renderer_opengl/gl_shader_gen.h"
-#include "video_core/renderer_opengl/gl_shader_util.h"
 #include "video_core/renderer_opengl/pica_to_gl.h"
 #include "video_core/renderer_opengl/renderer_opengl.h"
 
@@ -1005,7 +1004,7 @@ void RasterizerOpenGL::SamplerInfo::SyncWithConfig(
 }
 
 void RasterizerOpenGL::SetShader() {
-    PicaShaderConfig config = PicaShaderConfig::CurrentConfig();
+    auto config = GLShader::PicaShaderConfig::BuildFromRegs(Pica::g_state.regs);
     std::unique_ptr<PicaShader> shader = std::make_unique<PicaShader>();
 
     // Find (or generate) the GLSL shader for the current TEV state

src/video_core/renderer_opengl/gl_rasterizer.h

@@ -25,210 +25,13 @@
 #include "video_core/regs_texturing.h"
 #include "video_core/renderer_opengl/gl_rasterizer_cache.h"
 #include "video_core/renderer_opengl/gl_resource_manager.h"
+#include "video_core/renderer_opengl/gl_shader_gen.h"
 #include "video_core/renderer_opengl/gl_state.h"
 #include "video_core/renderer_opengl/pica_to_gl.h"
 #include "video_core/shader/shader.h"
 
 struct ScreenInfo;
 
-/**
- * This struct contains all state used to generate the GLSL shader program that emulates the current
- * Pica register configuration. This struct is used as a cache key for generated GLSL shader
- * programs. The functions in gl_shader_gen.cpp should retrieve state from this struct only, not by
- * directly accessing Pica registers. This should reduce the risk of bugs in shader generation where
- * Pica state is not being captured in the shader cache key, thereby resulting in (what should be)
- * two separate shaders sharing the same key.
- *
- * We use a union because "implicitly-defined copy/move constructor for a union X copies the object
- * representation of X." and "implicitly-defined copy assignment operator for a union X copies the
- * object representation (3.9) of X." = Bytewise copy instead of memberwise copy. This is important
- * because the padding bytes are included in the hash and comparison between objects.
- */
-union PicaShaderConfig {
-
-    /// Construct a PicaShaderConfig with the current Pica register configuration.
-    static PicaShaderConfig CurrentConfig() {
-        PicaShaderConfig res;
-
-        auto& state = res.state;
-        std::memset(&state, 0, sizeof(PicaShaderConfig::State));
-
-        const auto& regs = Pica::g_state.regs;
-
-        state.scissor_test_mode = regs.rasterizer.scissor_test.mode;
-
-        state.depthmap_enable = regs.rasterizer.depthmap_enable;
-
-        state.alpha_test_func = regs.framebuffer.output_merger.alpha_test.enable
-                                    ? regs.framebuffer.output_merger.alpha_test.func.Value()
-                                    : Pica::FramebufferRegs::CompareFunc::Always;
-
-        state.texture0_type = regs.texturing.texture0.type;
-
-        // Copy relevant tev stages fields.
-        // We don't sync const_color here because of the high variance, it is a
-        // shader uniform instead.
-        const auto& tev_stages = regs.texturing.GetTevStages();
-        DEBUG_ASSERT(state.tev_stages.size() == tev_stages.size());
-        for (size_t i = 0; i < tev_stages.size(); i++) {
-            const auto& tev_stage = tev_stages[i];
-            state.tev_stages[i].sources_raw = tev_stage.sources_raw;
-            state.tev_stages[i].modifiers_raw = tev_stage.modifiers_raw;
-            state.tev_stages[i].ops_raw = tev_stage.ops_raw;
-            state.tev_stages[i].scales_raw = tev_stage.scales_raw;
-        }
-
-        state.fog_mode = regs.texturing.fog_mode;
-        state.fog_flip = regs.texturing.fog_flip != 0;
-
-        state.combiner_buffer_input =
-            regs.texturing.tev_combiner_buffer_input.update_mask_rgb.Value() |
-            regs.texturing.tev_combiner_buffer_input.update_mask_a.Value() << 4;
-
-        // Fragment lighting
-
-        state.lighting.enable = !regs.lighting.disable;
-        state.lighting.src_num = regs.lighting.max_light_index + 1;
-
-        for (unsigned light_index = 0; light_index < state.lighting.src_num; ++light_index) {
-            unsigned num = regs.lighting.light_enable.GetNum(light_index);
-            const auto& light = regs.lighting.light[num];
-            state.lighting.light[light_index].num = num;
-            state.lighting.light[light_index].directional = light.config.directional != 0;
-            state.lighting.light[light_index].two_sided_diffuse =
-                light.config.two_sided_diffuse != 0;
-            state.lighting.light[light_index].dist_atten_enable =
-                !regs.lighting.IsDistAttenDisabled(num);
-        }
-
-        state.lighting.lut_d0.enable = regs.lighting.config1.disable_lut_d0 == 0;
-        state.lighting.lut_d0.abs_input = regs.lighting.abs_lut_input.disable_d0 == 0;
-        state.lighting.lut_d0.type = regs.lighting.lut_input.d0.Value();
-        state.lighting.lut_d0.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.d0);
-
-        state.lighting.lut_d1.enable = regs.lighting.config1.disable_lut_d1 == 0;
-        state.lighting.lut_d1.abs_input = regs.lighting.abs_lut_input.disable_d1 == 0;
-        state.lighting.lut_d1.type = regs.lighting.lut_input.d1.Value();
-        state.lighting.lut_d1.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.d1);
-
-        state.lighting.lut_fr.enable = regs.lighting.config1.disable_lut_fr == 0;
-        state.lighting.lut_fr.abs_input = regs.lighting.abs_lut_input.disable_fr == 0;
-        state.lighting.lut_fr.type = regs.lighting.lut_input.fr.Value();
-        state.lighting.lut_fr.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.fr);
-
-        state.lighting.lut_rr.enable = regs.lighting.config1.disable_lut_rr == 0;
-        state.lighting.lut_rr.abs_input = regs.lighting.abs_lut_input.disable_rr == 0;
-        state.lighting.lut_rr.type = regs.lighting.lut_input.rr.Value();
-        state.lighting.lut_rr.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.rr);
-
-        state.lighting.lut_rg.enable = regs.lighting.config1.disable_lut_rg == 0;
-        state.lighting.lut_rg.abs_input = regs.lighting.abs_lut_input.disable_rg == 0;
-        state.lighting.lut_rg.type = regs.lighting.lut_input.rg.Value();
-        state.lighting.lut_rg.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.rg);
-
-        state.lighting.lut_rb.enable = regs.lighting.config1.disable_lut_rb == 0;
-        state.lighting.lut_rb.abs_input = regs.lighting.abs_lut_input.disable_rb == 0;
-        state.lighting.lut_rb.type = regs.lighting.lut_input.rb.Value();
-        state.lighting.lut_rb.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.rb);
-
-        state.lighting.config = regs.lighting.config0.config;
-        state.lighting.fresnel_selector = regs.lighting.config0.fresnel_selector;
-        state.lighting.bump_mode = regs.lighting.config0.bump_mode;
-        state.lighting.bump_selector = regs.lighting.config0.bump_selector;
-        state.lighting.bump_renorm = regs.lighting.config0.disable_bump_renorm == 0;
-        state.lighting.clamp_highlights = regs.lighting.config0.clamp_highlights != 0;
-
-        return res;
-    }
-
-    bool TevStageUpdatesCombinerBufferColor(unsigned stage_index) const {
-        return (stage_index < 4) && (state.combiner_buffer_input & (1 << stage_index));
-    }
-
-    bool TevStageUpdatesCombinerBufferAlpha(unsigned stage_index) const {
-        return (stage_index < 4) && ((state.combiner_buffer_input >> 4) & (1 << stage_index));
-    }
-
-    bool operator==(const PicaShaderConfig& o) const {
-        return std::memcmp(&state, &o.state, sizeof(PicaShaderConfig::State)) == 0;
-    };
-
-    // NOTE: MSVC15 (Update 2) doesn't think `delete`'d constructors and operators are TC.
-    //       This makes BitField not TC when used in a union or struct so we have to resort
-    //       to this ugly hack.
-    //       Once that bug is fixed we can use Pica::Regs::TevStageConfig here.
-    //       Doesn't include const_color because we don't sync it, see comment in CurrentConfig()
-    struct TevStageConfigRaw {
-        u32 sources_raw;
-        u32 modifiers_raw;
-        u32 ops_raw;
-        u32 scales_raw;
-        explicit operator Pica::TexturingRegs::TevStageConfig() const noexcept {
-            Pica::TexturingRegs::TevStageConfig stage;
-            stage.sources_raw = sources_raw;
-            stage.modifiers_raw = modifiers_raw;
-            stage.ops_raw = ops_raw;
-            stage.const_color = 0;
-            stage.scales_raw = scales_raw;
-            return stage;
-        }
-    };
-
-    struct State {
-        Pica::FramebufferRegs::CompareFunc alpha_test_func;
-        Pica::RasterizerRegs::ScissorMode scissor_test_mode;
-        Pica::TexturingRegs::TextureConfig::TextureType texture0_type;
-        std::array<TevStageConfigRaw, 6> tev_stages;
-        u8 combiner_buffer_input;
-
-        Pica::RasterizerRegs::DepthBuffering depthmap_enable;
-        Pica::TexturingRegs::FogMode fog_mode;
-        bool fog_flip;
-
-        struct {
-            struct {
-                unsigned num;
-                bool directional;
-                bool two_sided_diffuse;
-                bool dist_atten_enable;
-            } light[8];
-
-            bool enable;
-            unsigned src_num;
-            Pica::LightingRegs::LightingBumpMode bump_mode;
-            unsigned bump_selector;
-            bool bump_renorm;
-            bool clamp_highlights;
-
-            Pica::LightingRegs::LightingConfig config;
-            Pica::LightingRegs::LightingFresnelSelector fresnel_selector;
-
-            struct {
-                bool enable;
-                bool abs_input;
-                Pica::LightingRegs::LightingLutInput type;
-                float scale;
-            } lut_d0, lut_d1, lut_fr, lut_rr, lut_rg, lut_rb;
-        } lighting;
-
-    } state;
-};
-#if (__GNUC__ >= 5) || defined(__clang__) || defined(_MSC_VER)
-static_assert(std::is_trivially_copyable<PicaShaderConfig::State>::value,
-              "PicaShaderConfig::State must be trivially copyable");
-#endif
-
-namespace std {
-
-template <>
-struct hash<PicaShaderConfig> {
-    size_t operator()(const PicaShaderConfig& k) const {
-        return Common::ComputeHash64(&k.state, sizeof(PicaShaderConfig::State));
-    }
-};
-
-} // namespace std
-
 class RasterizerOpenGL : public VideoCore::RasterizerInterface {
 public:
     RasterizerOpenGL();
@@ -437,7 +240,7 @@ private:
 
     std::vector<HardwareVertex> vertex_batch;
 
-    std::unordered_map<PicaShaderConfig, std::unique_ptr<PicaShader>> shader_cache;
+    std::unordered_map<GLShader::PicaShaderConfig, std::unique_ptr<PicaShader>> shader_cache;
     const PicaShader* current_shader = nullptr;
     bool shader_dirty;
 

src/video_core/renderer_opengl/gl_shader_gen.cpp

@@ -4,6 +4,7 @@
 
 #include <array>
 #include <cstddef>
+#include <cstring>
 #include "common/assert.h"
 #include "common/bit_field.h"
 #include "common/logging/log.h"
@@ -23,6 +24,97 @@ using TevStageConfig = TexturingRegs::TevStageConfig;
 
 namespace GLShader {
 
+PicaShaderConfig PicaShaderConfig::BuildFromRegs(const Pica::Regs& regs) {
+    PicaShaderConfig res;
+
+    auto& state = res.state;
+    std::memset(&state, 0, sizeof(PicaShaderConfig::State));
+
+    state.scissor_test_mode = regs.rasterizer.scissor_test.mode;
+
+    state.depthmap_enable = regs.rasterizer.depthmap_enable;
+
+    state.alpha_test_func = regs.framebuffer.output_merger.alpha_test.enable
+                                ? regs.framebuffer.output_merger.alpha_test.func.Value()
+                                : Pica::FramebufferRegs::CompareFunc::Always;
+
+    state.texture0_type = regs.texturing.texture0.type;
+
+    // Copy relevant tev stages fields.
+    // We don't sync const_color here because of the high variance, it is a
+    // shader uniform instead.
+    const auto& tev_stages = regs.texturing.GetTevStages();
+    DEBUG_ASSERT(state.tev_stages.size() == tev_stages.size());
+    for (size_t i = 0; i < tev_stages.size(); i++) {
+        const auto& tev_stage = tev_stages[i];
+        state.tev_stages[i].sources_raw = tev_stage.sources_raw;
+        state.tev_stages[i].modifiers_raw = tev_stage.modifiers_raw;
+        state.tev_stages[i].ops_raw = tev_stage.ops_raw;
+        state.tev_stages[i].scales_raw = tev_stage.scales_raw;
+    }
+
+    state.fog_mode = regs.texturing.fog_mode;
+    state.fog_flip = regs.texturing.fog_flip != 0;
+
+    state.combiner_buffer_input = regs.texturing.tev_combiner_buffer_input.update_mask_rgb.Value() |
+                                  regs.texturing.tev_combiner_buffer_input.update_mask_a.Value()
+                                      << 4;
+
+    // Fragment lighting
+
+    state.lighting.enable = !regs.lighting.disable;
+    state.lighting.src_num = regs.lighting.max_light_index + 1;
+
+    for (unsigned light_index = 0; light_index < state.lighting.src_num; ++light_index) {
+        unsigned num = regs.lighting.light_enable.GetNum(light_index);
+        const auto& light = regs.lighting.light[num];
+        state.lighting.light[light_index].num = num;
+        state.lighting.light[light_index].directional = light.config.directional != 0;
+        state.lighting.light[light_index].two_sided_diffuse = light.config.two_sided_diffuse != 0;
+        state.lighting.light[light_index].dist_atten_enable =
+            !regs.lighting.IsDistAttenDisabled(num);
+    }
+
+    state.lighting.lut_d0.enable = regs.lighting.config1.disable_lut_d0 == 0;
+    state.lighting.lut_d0.abs_input = regs.lighting.abs_lut_input.disable_d0 == 0;
+    state.lighting.lut_d0.type = regs.lighting.lut_input.d0.Value();
+    state.lighting.lut_d0.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.d0);
+
+    state.lighting.lut_d1.enable = regs.lighting.config1.disable_lut_d1 == 0;
+    state.lighting.lut_d1.abs_input = regs.lighting.abs_lut_input.disable_d1 == 0;
+    state.lighting.lut_d1.type = regs.lighting.lut_input.d1.Value();
+    state.lighting.lut_d1.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.d1);
+
+    state.lighting.lut_fr.enable = regs.lighting.config1.disable_lut_fr == 0;
+    state.lighting.lut_fr.abs_input = regs.lighting.abs_lut_input.disable_fr == 0;
+    state.lighting.lut_fr.type = regs.lighting.lut_input.fr.Value();
+    state.lighting.lut_fr.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.fr);
+
+    state.lighting.lut_rr.enable = regs.lighting.config1.disable_lut_rr == 0;
+    state.lighting.lut_rr.abs_input = regs.lighting.abs_lut_input.disable_rr == 0;
+    state.lighting.lut_rr.type = regs.lighting.lut_input.rr.Value();
+    state.lighting.lut_rr.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.rr);
+
+    state.lighting.lut_rg.enable = regs.lighting.config1.disable_lut_rg == 0;
+    state.lighting.lut_rg.abs_input = regs.lighting.abs_lut_input.disable_rg == 0;
+    state.lighting.lut_rg.type = regs.lighting.lut_input.rg.Value();
+    state.lighting.lut_rg.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.rg);
+
+    state.lighting.lut_rb.enable = regs.lighting.config1.disable_lut_rb == 0;
+    state.lighting.lut_rb.abs_input = regs.lighting.abs_lut_input.disable_rb == 0;
+    state.lighting.lut_rb.type = regs.lighting.lut_input.rb.Value();
+    state.lighting.lut_rb.scale = regs.lighting.lut_scale.GetScale(regs.lighting.lut_scale.rb);
+
+    state.lighting.config = regs.lighting.config0.config;
+    state.lighting.fresnel_selector = regs.lighting.config0.fresnel_selector;
+    state.lighting.bump_mode = regs.lighting.config0.bump_mode;
+    state.lighting.bump_selector = regs.lighting.config0.bump_selector;
+    state.lighting.bump_renorm = regs.lighting.config0.disable_bump_renorm == 0;
+    state.lighting.clamp_highlights = regs.lighting.config0.clamp_highlights != 0;
+
+    return res;
+}
+
 /// Detects if a TEV stage is configured to be skipped (to avoid generating unnecessary code)
 static bool IsPassThroughTevStage(const TevStageConfig& stage) {
     return (stage.color_op == TevStageConfig::Operation::Replace &&

src/video_core/renderer_opengl/gl_shader_gen.h

@@ -4,12 +4,121 @@
 
 #pragma once
 
+#include <array>
+#include <cstring>
+#include <functional>
 #include <string>
-
+#include <type_traits>
-union PicaShaderConfig;
+#include "video_core/regs.h"
 
 namespace GLShader {
 
+enum Attributes {
+    ATTRIBUTE_POSITION,
+    ATTRIBUTE_COLOR,
+    ATTRIBUTE_TEXCOORD0,
+    ATTRIBUTE_TEXCOORD1,
+    ATTRIBUTE_TEXCOORD2,
+    ATTRIBUTE_TEXCOORD0_W,
+    ATTRIBUTE_NORMQUAT,
+    ATTRIBUTE_VIEW,
+};
+
+/**
+ * This struct contains all state used to generate the GLSL shader program that emulates the current
+ * Pica register configuration. This struct is used as a cache key for generated GLSL shader
+ * programs. The functions in gl_shader_gen.cpp should retrieve state from this struct only, not by
+ * directly accessing Pica registers. This should reduce the risk of bugs in shader generation where
+ * Pica state is not being captured in the shader cache key, thereby resulting in (what should be)
+ * two separate shaders sharing the same key.
+ *
+ * We use a union because "implicitly-defined copy/move constructor for a union X copies the object
+ * representation of X." and "implicitly-defined copy assignment operator for a union X copies the
+ * object representation (3.9) of X." = Bytewise copy instead of memberwise copy. This is important
+ * because the padding bytes are included in the hash and comparison between objects.
+ */
+union PicaShaderConfig {
+
+    /// Construct a PicaShaderConfig with the given Pica register configuration.
+    static PicaShaderConfig BuildFromRegs(const Pica::Regs& regs);
+
+    bool TevStageUpdatesCombinerBufferColor(unsigned stage_index) const {
+        return (stage_index < 4) && (state.combiner_buffer_input & (1 << stage_index));
+    }
+
+    bool TevStageUpdatesCombinerBufferAlpha(unsigned stage_index) const {
+        return (stage_index < 4) && ((state.combiner_buffer_input >> 4) & (1 << stage_index));
+    }
+
+    bool operator==(const PicaShaderConfig& o) const {
+        return std::memcmp(&state, &o.state, sizeof(PicaShaderConfig::State)) == 0;
+    };
+
+    // NOTE: MSVC15 (Update 2) doesn't think `delete`'d constructors and operators are TC.
+    //       This makes BitField not TC when used in a union or struct so we have to resort
+    //       to this ugly hack.
+    //       Once that bug is fixed we can use Pica::Regs::TevStageConfig here.
+    //       Doesn't include const_color because we don't sync it, see comment in BuildFromRegs()
+    struct TevStageConfigRaw {
+        u32 sources_raw;
+        u32 modifiers_raw;
+        u32 ops_raw;
+        u32 scales_raw;
+        explicit operator Pica::TexturingRegs::TevStageConfig() const noexcept {
+            Pica::TexturingRegs::TevStageConfig stage;
+            stage.sources_raw = sources_raw;
+            stage.modifiers_raw = modifiers_raw;
+            stage.ops_raw = ops_raw;
+            stage.const_color = 0;
+            stage.scales_raw = scales_raw;
+            return stage;
+        }
+    };
+
+    struct State {
+        Pica::FramebufferRegs::CompareFunc alpha_test_func;
+        Pica::RasterizerRegs::ScissorMode scissor_test_mode;
+        Pica::TexturingRegs::TextureConfig::TextureType texture0_type;
+        std::array<TevStageConfigRaw, 6> tev_stages;
+        u8 combiner_buffer_input;
+
+        Pica::RasterizerRegs::DepthBuffering depthmap_enable;
+        Pica::TexturingRegs::FogMode fog_mode;
+        bool fog_flip;
+
+        struct {
+            struct {
+                unsigned num;
+                bool directional;
+                bool two_sided_diffuse;
+                bool dist_atten_enable;
+            } light[8];
+
+            bool enable;
+            unsigned src_num;
+            Pica::LightingRegs::LightingBumpMode bump_mode;
+            unsigned bump_selector;
+            bool bump_renorm;
+            bool clamp_highlights;
+
+            Pica::LightingRegs::LightingConfig config;
+            Pica::LightingRegs::LightingFresnelSelector fresnel_selector;
+
+            struct {
+                bool enable;
+                bool abs_input;
+                Pica::LightingRegs::LightingLutInput type;
+                float scale;
+            } lut_d0, lut_d1, lut_fr, lut_rr, lut_rg, lut_rb;
+        } lighting;
+
+    } state;
+};
+#if (__GNUC__ >= 5) || defined(__clang__) || defined(_MSC_VER)
+static_assert(std::is_trivially_copyable<PicaShaderConfig::State>::value,
+              "PicaShaderConfig::State must be trivially copyable");
+#endif
+
 /**
  * Generates the GLSL vertex shader program source code for the current Pica state
  * @returns String of the shader source code
@@ -25,3 +134,12 @@ std::string GenerateVertexShader();
 std::string GenerateFragmentShader(const PicaShaderConfig& config);
 
 } // namespace GLShader
+
+namespace std {
+template <>
+struct hash<GLShader::PicaShaderConfig> {
+    size_t operator()(const GLShader::PicaShaderConfig& k) const {
+        return Common::ComputeHash64(&k.state, sizeof(GLShader::PicaShaderConfig::State));
+    }
+};
+} // namespace std

src/video_core/renderer_opengl/gl_shader_util.h

@@ -8,17 +8,6 @@
 
 namespace GLShader {
 
-enum Attributes {
-    ATTRIBUTE_POSITION,
-    ATTRIBUTE_COLOR,
-    ATTRIBUTE_TEXCOORD0,
-    ATTRIBUTE_TEXCOORD1,
-    ATTRIBUTE_TEXCOORD2,
-    ATTRIBUTE_TEXCOORD0_W,
-    ATTRIBUTE_NORMQUAT,
-    ATTRIBUTE_VIEW,
-};
-
 /**
  * Utility function to create and compile an OpenGL GLSL shader program (vertex + fragment shader)
  * @param vertex_shader String of the GLSL vertex shader program