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

@@ -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 "

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;

panda/src/gobj/material.I

@@ -32,8 +32,18 @@ Material(const std::string &name) : Namable(name) {
  *
  */
 INLINE Material::
-Material(const Material &copy) : Namable(copy) {
+Material(const Material &copy) :
-  operator = (copy);
+  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) {
+  if (has_ambient() && is_used_by_auto_shader()) {
-    nassertv(!is_attrib_locked());
+    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) {
+  if (has_diffuse() && is_used_by_auto_shader()) {
-    nassertv(!is_attrib_locked());
+    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) {
+  if (has_emission() && is_used_by_auto_shader()) {
-    nassertv(!is_attrib_locked());
+    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) {
+  if (is_used_by_auto_shader() && get_local() != local) {
-    nassertv(!is_attrib_locked());
+    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) {
+  if (is_used_by_auto_shader() && get_twoside() != twoside) {
-    nassertv(!is_attrib_locked());
+    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;
 }

panda/src/gobj/material.cxx

@@ -28,6 +28,11 @@ PT(Material) Material::_default;
 void Material::
 operator = (const Material &copy) {
   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 &copy) {
   _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 &copy) {
  */
 void Material::
 set_base_color(const LColor &color) {
-  if (enforce_attrib_lock) {
+  if (!has_base_color() && is_used_by_auto_shader()) {
-    if ((_flags & F_base_color) == 0) {
+    GraphicsStateGuardianBase::mark_rehash_generated_shaders();
-      nassertv(!is_attrib_locked());
-    }
   }
   _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) {
+  if (has_base_color() && is_used_by_auto_shader()) {
-    nassertv(!is_attrib_locked());
+    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 (!has_ambient() && is_used_by_auto_shader()) {
-    if ((_flags & F_ambient)==0) {
+    GraphicsStateGuardianBase::mark_rehash_generated_shaders();
-      nassertv(!is_attrib_locked());
-    }
   }
   _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 (!has_diffuse() && is_used_by_auto_shader()) {
-    if ((_flags & F_diffuse)==0) {
+    GraphicsStateGuardianBase::mark_rehash_generated_shaders();
-      nassertv(!is_attrib_locked());
-    }
   }
   _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 (!has_specular() && is_used_by_auto_shader()) {
-    if ((_flags & F_specular)==0) {
+    GraphicsStateGuardianBase::mark_rehash_generated_shaders();
-      nassertv(!is_attrib_locked());
-    }
   }
   _specular = color;
   _flags |= F_specular;
@@ -174,8 +171,8 @@ set_specular(const LColor &color) {
  */
 void Material::
 clear_specular() {
-  if (enforce_attrib_lock) {
+  if (has_specular() && is_used_by_auto_shader()) {
-    nassertv(!is_attrib_locked());
+    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 (!has_emission() && is_used_by_auto_shader()) {
-    if ((_flags & F_emission)==0) {
+    GraphicsStateGuardianBase::mark_rehash_generated_shaders();
-      nassertv(!is_attrib_locked());
-    }
   }
   _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();
+  }
 }

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:

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) {
+  Material *mat = material->get_material();
-    key._material_flags = material->get_material()->get_flags();
+  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.