From 9ba2d7242e6ed91da981829b27f9842943cf8329 Mon Sep 17 00:00:00 2001 From: rdb Date: Fri, 3 Aug 2018 21:27:58 +0200 Subject: [PATCH] gobj: get rid of enforce-attrib-lock, just DTRT instead This setting causes asserts when modifying certain material flags after they are assigned to a node, in case a shader has been generated that uses the materials, which cannot efficiently detect whether the material has changed. However, we already sort of have a (inefficient, but effective) solution for this in TextureStage (see #178) that we could just apply here as well: changing the material attributes after such a shader has been generated could call GraphicsStateGuardianBase::mark_rehash_generated_shaders(). In the future, we should replace the material model with one that does not require shader regeneration for such seemingly trivial property changes. Fixes #370 --- panda/src/gobj/config_gobj.cxx | 12 ----- panda/src/gobj/config_gobj.h | 1 - panda/src/gobj/material.I | 58 ++++++++++++++++------ panda/src/gobj/material.cxx | 59 ++++++++++------------- panda/src/gobj/material.h | 6 +++ panda/src/pgraphnodes/shaderGenerator.cxx | 8 ++- 6 files changed, 80 insertions(+), 64 deletions(-) diff --git a/panda/src/gobj/config_gobj.cxx b/panda/src/gobj/config_gobj.cxx index b7d0e02eb5..7da3b1d31e 100644 --- a/panda/src/gobj/config_gobj.cxx +++ b/panda/src/gobj/config_gobj.cxx @@ -260,18 +260,6 @@ ConfigVariableBool cache_generated_shaders PRC_DESC("Set this true to cause all generated shaders to be cached in " "memory. This is useful to prevent unnecessary recompilation.")); -ConfigVariableBool enforce_attrib_lock -("enforce-attrib-lock", true, - PRC_DESC("When a MaterialAttrib, TextureAttrib, or LightAttrib is " - "constructed, the corresponding Material, Texture, or Light " - "is 'attrib locked.' The attrib lock prevents qualitative " - "changes to the object. This makes it possible to hardwire " - "information about material, light, and texture properties " - "into generated shaders. This config variable can disable " - "the attrib lock. Disabling the lock will break the shader " - "generator, but doing so may be necessary for backward " - "compatibility with old code.")); - ConfigVariableBool vertices_float64 ("vertices-float64", false, PRC_DESC("When this is true, the default float format for vertices " diff --git a/panda/src/gobj/config_gobj.h b/panda/src/gobj/config_gobj.h index d3b651d3d4..7e769c0f45 100644 --- a/panda/src/gobj/config_gobj.h +++ b/panda/src/gobj/config_gobj.h @@ -51,7 +51,6 @@ extern EXPCL_PANDA_GOBJ ConfigVariableBool connect_triangle_strips; extern EXPCL_PANDA_GOBJ ConfigVariableBool preserve_triangle_strips; extern EXPCL_PANDA_GOBJ ConfigVariableBool dump_generated_shaders; extern EXPCL_PANDA_GOBJ ConfigVariableBool cache_generated_shaders; -extern EXPCL_PANDA_GOBJ ConfigVariableBool enforce_attrib_lock; extern EXPCL_PANDA_GOBJ ConfigVariableBool vertices_float64; extern EXPCL_PANDA_GOBJ ConfigVariableInt vertex_column_alignment; extern EXPCL_PANDA_GOBJ ConfigVariableBool vertex_animation_align_16; diff --git a/panda/src/gobj/material.I b/panda/src/gobj/material.I index b9e30ba262..dcaef86980 100644 --- a/panda/src/gobj/material.I +++ b/panda/src/gobj/material.I @@ -32,8 +32,18 @@ Material(const std::string &name) : Namable(name) { * */ INLINE Material:: -Material(const Material ©) : Namable(copy) { - operator = (copy); +Material(const Material ©) : + Namable(copy) , + _base_color(copy._base_color), + _ambient(copy._ambient), + _diffuse(copy._diffuse), + _specular(copy._specular), + _emission(copy._emission), + _shininess(copy._shininess), + _roughness(copy._roughness), + _metallic(copy._metallic), + _refractive_index(copy._refractive_index), + _flags(copy._flags & ~(F_attrib_lock | F_used_by_auto_shader)) { } /** @@ -99,8 +109,8 @@ get_ambient() const { */ INLINE void Material:: clear_ambient() { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); + if (has_ambient() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _flags &= ~F_ambient; _ambient = _base_color; @@ -129,8 +139,8 @@ get_diffuse() const { */ INLINE void Material:: clear_diffuse() { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); + if (has_diffuse() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _flags &= ~F_diffuse; _diffuse = _base_color * (1 - _metallic); @@ -177,8 +187,8 @@ get_emission() const { */ INLINE void Material:: clear_emission() { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); + if (has_emission() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _flags &= ~F_emission; _emission.set(0.0f, 0.0f, 0.0f, 0.0f); @@ -253,8 +263,8 @@ get_local() const { */ INLINE void Material:: set_local(bool local) { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); + if (is_used_by_auto_shader() && get_local() != local) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } if (local) { _flags |= F_local; @@ -278,8 +288,8 @@ get_twoside() const { */ INLINE void Material:: set_twoside(bool twoside) { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); + if (is_used_by_auto_shader() && get_twoside() != twoside) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } if (twoside) { _flags |= F_twoside; @@ -313,7 +323,7 @@ operator < (const Material &other) const { } /** - * + * @deprecated This no longer has any meaning in 1.10. */ INLINE bool Material:: is_attrib_locked() const { @@ -321,17 +331,35 @@ is_attrib_locked() const { } /** - * + * @deprecated This no longer has any meaning in 1.10. */ INLINE void Material:: set_attrib_lock() { _flags |= F_attrib_lock; } +/** + * Internal. Returns true if a shader has been generated that uses this. + */ +INLINE bool Material:: +is_used_by_auto_shader() const { + return (_flags & F_attrib_lock) != 0; +} + +/** + * Called by the shader generator to indicate that a shader has been generated + * that uses this material. + */ +INLINE void Material:: +mark_used_by_auto_shader() { + _flags |= F_used_by_auto_shader; +} + /** * */ INLINE int Material:: get_flags() const { - return _flags; + // F_used_by_auto_shader is an internal flag, ignore it. + return _flags & ~F_used_by_auto_shader; } diff --git a/panda/src/gobj/material.cxx b/panda/src/gobj/material.cxx index 1952066649..24bc0fb590 100644 --- a/panda/src/gobj/material.cxx +++ b/panda/src/gobj/material.cxx @@ -28,6 +28,11 @@ PT(Material) Material::_default; void Material:: operator = (const Material ©) { Namable::operator = (copy); + + if (is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); + } + _base_color = copy._base_color; _ambient = copy._ambient; _diffuse = copy._diffuse; @@ -37,7 +42,7 @@ operator = (const Material ©) { _roughness = copy._roughness; _metallic = copy._metallic; _refractive_index = copy._refractive_index; - _flags = copy._flags & (~F_attrib_lock); + _flags = (copy._flags & ~(F_attrib_lock | F_used_by_auto_shader)) | (_flags & (F_attrib_lock | F_used_by_auto_shader)); } /** @@ -53,10 +58,8 @@ operator = (const Material ©) { */ void Material:: set_base_color(const LColor &color) { - if (enforce_attrib_lock) { - if ((_flags & F_base_color) == 0) { - nassertv(!is_attrib_locked()); - } + if (!has_base_color() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _base_color = color; _flags |= F_base_color | F_metallic; @@ -81,8 +84,8 @@ set_base_color(const LColor &color) { */ void Material:: clear_base_color() { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); + if (has_base_color() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _flags &= ~F_base_color; _base_color.set(0.0f, 0.0f, 0.0f, 0.0f); @@ -116,10 +119,8 @@ clear_base_color() { */ void Material:: set_ambient(const LColor &color) { - if (enforce_attrib_lock) { - if ((_flags & F_ambient)==0) { - nassertv(!is_attrib_locked()); - } + if (!has_ambient() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _ambient = color; _flags |= F_ambient; @@ -137,10 +138,8 @@ set_ambient(const LColor &color) { */ void Material:: set_diffuse(const LColor &color) { - if (enforce_attrib_lock) { - if ((_flags & F_diffuse)==0) { - nassertv(!is_attrib_locked()); - } + if (!has_diffuse() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _diffuse = color; _flags |= F_diffuse; @@ -160,10 +159,8 @@ set_diffuse(const LColor &color) { */ void Material:: set_specular(const LColor &color) { - if (enforce_attrib_lock) { - if ((_flags & F_specular)==0) { - nassertv(!is_attrib_locked()); - } + if (!has_specular() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _specular = color; _flags |= F_specular; @@ -174,8 +171,8 @@ set_specular(const LColor &color) { */ void Material:: clear_specular() { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); + if (has_specular() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _flags &= ~F_specular; @@ -201,10 +198,8 @@ clear_specular() { */ void Material:: set_emission(const LColor &color) { - if (enforce_attrib_lock) { - if ((_flags & F_emission)==0) { - nassertv(!is_attrib_locked()); - } + if (!has_emission() && is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); } _emission = color; _flags |= F_emission; @@ -275,11 +270,6 @@ set_roughness(PN_stdfloat roughness) { */ void Material:: set_metallic(PN_stdfloat metallic) { - if (enforce_attrib_lock) { - if ((_flags & F_metallic) == 0) { - nassertv(!is_attrib_locked()); - } - } _metallic = metallic; _flags |= F_metallic; @@ -305,9 +295,6 @@ set_metallic(PN_stdfloat metallic) { */ void Material:: clear_metallic() { - if (enforce_attrib_lock) { - nassertv(!is_attrib_locked()); - } _flags &= ~F_metallic; _metallic = 0; @@ -482,7 +469,7 @@ write_datagram(BamWriter *manager, Datagram &me) { me.add_string(get_name()); if (manager->get_file_minor_ver() >= 39) { - me.add_int32(_flags); + me.add_int32(_flags & ~F_used_by_auto_shader); if (_flags & F_metallic) { // Metalness workflow. @@ -570,4 +557,8 @@ fillin(DatagramIterator &scan, BamReader *manager) { set_roughness(_shininess); } } + + if (is_used_by_auto_shader()) { + GraphicsStateGuardianBase::mark_rehash_generated_shaders(); + } } diff --git a/panda/src/gobj/material.h b/panda/src/gobj/material.h index 5835a81ae3..259d5d6874 100644 --- a/panda/src/gobj/material.h +++ b/panda/src/gobj/material.h @@ -21,6 +21,7 @@ #include "luse.h" #include "numeric_types.h" #include "config_gobj.h" +#include "graphicsStateGuardianBase.h" class FactoryParams; @@ -127,7 +128,11 @@ PUBLISHED: MAKE_PROPERTY(local, get_local, set_local); MAKE_PROPERTY(twoside, get_twoside, set_twoside); +protected: + INLINE bool is_used_by_auto_shader() const; + public: + INLINE void mark_used_by_auto_shader(); INLINE int get_flags() const; enum Flags { @@ -142,6 +147,7 @@ public: F_metallic = 0x100, F_base_color = 0x200, F_refractive_index = 0x400, + F_used_by_auto_shader = 0x800, }; private: diff --git a/panda/src/pgraphnodes/shaderGenerator.cxx b/panda/src/pgraphnodes/shaderGenerator.cxx index 71379ddbac..1673b82358 100644 --- a/panda/src/pgraphnodes/shaderGenerator.cxx +++ b/panda/src/pgraphnodes/shaderGenerator.cxx @@ -252,8 +252,12 @@ analyze_renderstate(ShaderKey &key, const RenderState *rs) { // Store the material flags (not the material values itself). const MaterialAttrib *material; rs->get_attrib_def(material); - if (material->get_material() != nullptr) { - key._material_flags = material->get_material()->get_flags(); + Material *mat = material->get_material(); + if (mat != nullptr) { + // The next time the Material flags change, the Material should cause the + // states to be rehashed. + mat->mark_used_by_auto_shader(); + key._material_flags = mat->get_flags(); } // Break out the lights by type.