Fix various GLSL bugs: attributes, p3d_Color range, and missing MatrixInverseTranspose stuff

panda/src/glstuff/glGraphicsStateGuardian_src.cxx

@@ -2507,8 +2507,8 @@ begin_draw_primitives(const GeomPipelineReader *geom_reader,
     return false;
   }
 #else
-  if (_current_shader_context == 0 || !_current_shader_context->uses_custom_vertex_arrays()) {
+  if (_current_shader_context == 0) {
-    // No shader, or a non-Cg shader.
+    // No shader.
     if (_vertex_array_shader_context != 0) {
       _vertex_array_shader_context->disable_shader_vertex_arrays(this);
     }
@@ -2516,17 +2516,27 @@ begin_draw_primitives(const GeomPipelineReader *geom_reader,
       return false;
     }
   } else {
-    // Cg shader.
+    // Shader.
-    if (_vertex_array_shader_context == 0) {
+    if (_vertex_array_shader_context == 0 || _vertex_array_shader_context->uses_standard_vertex_arrays()) {
-      disable_standard_vertex_arrays();
+      // Previous shader used standard arrays.
-      if (!_current_shader_context->update_shader_vertex_arrays(NULL, this, force)) {
+      if (_current_shader_context->uses_standard_vertex_arrays()) {
-        return false;
+        // So does the current, so update them.
+        if (!update_standard_vertex_arrays(force)) {
+          return false;
+        }
+      } else {
+        // The current shader does not, so disable them entirely.
+        disable_standard_vertex_arrays();
       }
-    } else {
+    }
+    if (_current_shader_context->uses_custom_vertex_arrays()) {
+      // The current shader also uses custom vertex arrays.
       if (!_current_shader_context->
           update_shader_vertex_arrays(_vertex_array_shader_context, this, force)) {
         return false;
       }
+    } else {
+      _vertex_array_shader_context->disable_shader_vertex_arrays(this);
     }
   }
 

panda/src/glstuff/glShaderContext_src.I

@@ -36,6 +36,18 @@ valid() {
   return false;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: GLShaderContext::uses_standard_vertex_arrays
+//       Access: Public
+//  Description: Returns true if the shader may need to access
+//               standard vertex attributes as passed by
+//               glVertexPointer and the like.
+////////////////////////////////////////////////////////////////////
+INLINE bool CLP(ShaderContext)::
+uses_standard_vertex_arrays() {
+  return _uses_standard_vertex_arrays;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: GLShaderContext::uses_custom_vertex_arrays
 //       Access: Public
@@ -43,11 +55,7 @@ valid() {
 ////////////////////////////////////////////////////////////////////
 INLINE bool CLP(ShaderContext)::
 uses_custom_vertex_arrays() {
-  #ifdef OPENGLES_2
+  return true;
-    return true;
-  #else
-    return (_shader->get_language() == Shader::SL_Cg);
-  #endif
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/glstuff/glShaderContext_src.cxx

@@ -219,6 +219,8 @@ CLP(ShaderContext)(Shader *s, GSG *gsg) : ShaderContext(s) {
   _glsl_gshader = 0;
   _glsl_tcshader = 0;
   _glsl_teshader = 0;
+  _uses_standard_vertex_arrays = false;
+
 #if defined(HAVE_CG) && !defined(OPENGLES)
   _cg_context = 0;
   if (s->get_language() == Shader::SL_Cg) {
@@ -284,7 +286,7 @@ CLP(ShaderContext)(Shader *s, GSG *gsg) : ShaderContext(s) {
       }
     }
     // Analyze the uniforms and put them in _glsl_parameter_map
-    if (s->_glsl_parameter_map.size() == 0) {
+    if (_glsl_parameter_map.size() == 0) {
       int seqno = 0, texunitno = 0;
       string noprefix;
       GLint param_count, param_maxlength, param_size;
@@ -300,53 +302,80 @@ CLP(ShaderContext)(Shader *s, GSG *gsg) : ShaderContext(s) {
           Shader::ShaderArgId arg_id;
           arg_id._name  = param_name;
           arg_id._seqno = seqno++;
-          s->_glsl_parameter_map.push_back(p);
+          _glsl_parameter_map.push_back(p);
-          if (param_name.substr(0, 4) == "p3d_" || param_name.substr(0, 3) == "gl_") {
+
-            if (param_name.substr(0, 3) == "gl_") {
+          // Check for inputs with p3d_ prefix.
-              noprefix = param_name.substr(3);
+          if (param_name.substr(0, 4) == "p3d_") {
-            } else {
+            noprefix = param_name.substr(4);
-              noprefix = param_name.substr(4);
+
+            // Check for matrix inputs.
+            bool transpose = false;
+            bool inverse = false;
+            string matrix_name (noprefix);
+            size_t size = matrix_name.size();
+
+            // Check for and chop off any "Transpose" or "Inverse" suffix.
+            if (size > 15 && matrix_name.compare(size - 9, 9, "Transpose") == 0) {
+              transpose = true;
+              matrix_name = matrix_name.substr(0, size - 9);
             }
-            
+            size = matrix_name.size();
-            if (noprefix == "ModelViewProjectionMatrix") {
+            if (size > 13 && matrix_name.compare(size - 7, 7, "Inverse") == 0) {
+              inverse = true;
+              matrix_name = matrix_name.substr(0, size - 7);
+            }
+            size = matrix_name.size();
+
+            // Now if the suffix that is left over is "Matrix",
+            // we know that it is supposed to be a matrix input.
+            if (size > 6 && matrix_name.compare(size - 6, 6, "Matrix") == 0) {
               Shader::ShaderMatSpec bind;
               bind._id = arg_id;
-              bind._piece = Shader::SMP_whole;
+              if (transpose) {
-              bind._func = Shader::SMF_compose;
+                bind._piece = Shader::SMP_transpose;
-              bind._part[0] = Shader::SMO_model_to_view;
+              } else {
+                bind._piece = Shader::SMP_whole;
+              }
               bind._arg[0] = NULL;
-              bind._dep[0] = Shader::SSD_general | Shader::SSD_transform;
-              bind._part[1] = Shader::SMO_view_to_apiclip;
               bind._arg[1] = NULL;
-              bind._dep[1] = Shader::SSD_general | Shader::SSD_transform;
+              
-              s->_mat_spec.push_back(bind);
+              if (matrix_name == "ModelViewProjectionMatrix") {
-              continue;
+                bind._func = Shader::SMF_compose;
-            }
+                if (inverse) {
-            if (noprefix == "ModelViewMatrix") {
+                  bind._part[0] = Shader::SMO_apiclip_to_view;
-              Shader::ShaderMatSpec bind;
+                  bind._part[1] = Shader::SMO_view_to_model;
-              bind._id = arg_id;
+                } else {
-              bind._piece = Shader::SMP_whole;
+                  bind._part[0] = Shader::SMO_model_to_view;
-              bind._func = Shader::SMF_first;
+                  bind._part[1] = Shader::SMO_view_to_apiclip;
-              bind._part[0] = Shader::SMO_model_to_view;
+                }
-              bind._arg[0] = NULL;
+                bind._dep[0] = Shader::SSD_general | Shader::SSD_transform;
-              bind._dep[0] = Shader::SSD_general | Shader::SSD_transform;
+                bind._dep[1] = Shader::SSD_general | Shader::SSD_transform;
-              bind._part[1] = Shader::SMO_identity;
+
-              bind._arg[1] = NULL;
+              } else if (matrix_name == "ModelViewMatrix") {
-              bind._dep[1] = Shader::SSD_NONE;
+                bind._func = Shader::SMF_first;
-              s->_mat_spec.push_back(bind);
+                if (inverse) {
-              continue;
+                  bind._part[0] = Shader::SMO_view_to_model;
-            }
+                } else {
-            if (noprefix == "ProjectionMatrix") {
+                  bind._part[0] = Shader::SMO_model_to_view;
-              Shader::ShaderMatSpec bind;
+                }
-              bind._id = arg_id;
+                bind._part[1] = Shader::SMO_identity;
-              bind._piece = Shader::SMP_whole;
+                bind._dep[0] = Shader::SSD_general | Shader::SSD_transform;
-              bind._func = Shader::SMF_first;
+                bind._dep[1] = Shader::SSD_NONE;
-              bind._part[0] = Shader::SMO_view_to_apiclip;
+
-              bind._arg[0] = NULL;
+              } else if (matrix_name == "ProjectionMatrix") {
-              bind._dep[0] = Shader::SSD_general | Shader::SSD_transform;
+                bind._func = Shader::SMF_first;
-              bind._part[1] = Shader::SMO_identity;
+                if (inverse) {
-              bind._arg[1] = NULL;
+                  bind._part[0] = Shader::SMO_apiclip_to_view;
-              bind._dep[1] = Shader::SSD_NONE;
+                } else {
+                  bind._part[0] = Shader::SMO_view_to_apiclip;
+                }
+                bind._part[1] = Shader::SMO_identity;
+                bind._dep[0] = Shader::SSD_general | Shader::SSD_transform;
+                bind._dep[1] = Shader::SSD_NONE;
+              } else {
+                GLCAT.error() << "Unrecognized uniform matrix name '" << matrix_name << "'!\n";
+                continue;
+              }
               s->_mat_spec.push_back(bind);
               continue;
             }
@@ -530,72 +559,80 @@ CLP(ShaderContext)(Shader *s, GSG *gsg) : ShaderContext(s) {
       gsg->_glGetProgramiv(_glsl_program, GL_ACTIVE_ATTRIBUTES, &param_count);
       gsg->_glGetProgramiv(_glsl_program, GL_ACTIVE_ATTRIBUTE_MAX_LENGTH, &param_maxlength);
       param_name_cstr = (char *)alloca(param_maxlength);
+
       for (int i = 0; i < param_count; ++i) {
         gsg->_glGetActiveAttrib(_glsl_program, i, param_maxlength, NULL, &param_size, &param_type, param_name_cstr);
         string param_name(param_name_cstr);
 
-        if (param_name.substr(0, 3) == "gl_") {
+        // Get the attrib location.
-          // We shouldn't bind anything to these.
+        GLint p = gsg->_glGetAttribLocation(_glsl_program, param_name_cstr);
 
+        GLCAT.error() <<
+          "Active attribute " << param_name << " is bound to location " << p << "\n";
+
+        if (p == -1) {
+          // A gl_ attribute such as gl_Vertex requires us to pass the
+          // standard vertex arrays as we would do without shader.
+          // This is not always the case, though -- see below.
+          _uses_standard_vertex_arrays = true;
+          continue;
+        }
+
+        Shader::ShaderArgId arg_id;
+        arg_id._name  = param_name;
+        arg_id._seqno = seqno++;
+        _glsl_parameter_map.push_back(p);
+
+        Shader::ShaderVarSpec bind;
+        bind._id = arg_id;
+        bind._name = NULL;
+        bind._append_uv = -1;
+
+        if (param_name.substr(0, 3) == "gl_") {
+          // Not all drivers return -1 in glGetAttribLocation
+          // for gl_ prefixed attributes.
+          _uses_standard_vertex_arrays = true;
+          continue;
         } else if (param_name.substr(0, 4) == "p3d_") {
           noprefix = param_name.substr(4);
+        } else {
+          noprefix = "";
+        }
 
-          Shader::ShaderVarSpec bind;
+        if (noprefix.empty()) {
-          bind._append_uv = -1;
+          // Arbitrarily named attribute.
+          bind._name = InternalName::make(param_name);
 
-          if (noprefix == "Vertex") {
+        } else if (noprefix == "Vertex") {
-            bind._name = InternalName::get_vertex();
+          bind._name = InternalName::get_vertex();
-            s->_var_spec.push_back(bind);
+
-            gsg->_glBindAttribLocation(_glsl_program, i, param_name_cstr);
+        } else if (noprefix == "Normal") {
-            continue;
+          bind._name = InternalName::get_normal();
+
+        } else if (noprefix == "Color") {
+          bind._name = InternalName::get_color();
+
+        } else if (noprefix.substr(0, 7) == "Tangent") {
+          bind._name = InternalName::get_tangent();
+          if (noprefix.size() > 7) {
+            bind._append_uv = atoi(noprefix.substr(7).c_str());
           }
-          if (noprefix == "Normal") {
+
-            bind._name = InternalName::get_normal();
+        } else if (noprefix.substr(0, 8) == "Binormal") {
-            s->_var_spec.push_back(bind);
+          bind._name = InternalName::get_binormal();
-            gsg->_glBindAttribLocation(_glsl_program, i, param_name_cstr);
+          if (noprefix.size() > 8) {
-            continue;
+            bind._append_uv = atoi(noprefix.substr(8).c_str());
           }
-          if (noprefix == "Color") {
+
-            bind._name = InternalName::get_color();
+        } else if (noprefix.substr(0, 13) == "MultiTexCoord") {
-            s->_var_spec.push_back(bind);
+          bind._name = InternalName::get_texcoord();
-            gsg->_glBindAttribLocation(_glsl_program, i, param_name_cstr);
+          bind._append_uv = atoi(noprefix.substr(13).c_str());
-            continue;
-          }
-          if (noprefix.substr(0, 7)  == "Tangent") {
-            bind._name = InternalName::get_tangent();
-            if (noprefix.size() > 7) {
-              bind._append_uv = atoi(noprefix.substr(7).c_str());
-            }
-            s->_var_spec.push_back(bind);
-            gsg->_glBindAttribLocation(_glsl_program, i, param_name_cstr);
-            continue;
-          }
-          if (noprefix.substr(0, 8)  == "Binormal") {
-            bind._name = InternalName::get_binormal();
-            if (noprefix.size() > 8) {
-              bind._append_uv = atoi(noprefix.substr(8).c_str());
-            }
-            s->_var_spec.push_back(bind);
-            gsg->_glBindAttribLocation(_glsl_program, i, param_name_cstr);
-            continue;
-          }
-          if (noprefix.substr(0, 13)  == "MultiTexCoord") {
-            bind._name = InternalName::get_texcoord();
-            bind._append_uv = atoi(noprefix.substr(13).c_str());
-            s->_var_spec.push_back(bind);
-            gsg->_glBindAttribLocation(_glsl_program, i, param_name_cstr);
-            continue;
-          }
-          GLCAT.error() << "Unrecognized vertex attrib '" << param_name << "'!\n";
-          continue;
 
         } else {
-          Shader::ShaderVarSpec bind;
+          GLCAT.error() << "Unrecognized vertex attrib '" << param_name << "'!\n";
-          bind._name = InternalName::make(param_name);
+          continue;
-          bind._append_uv = -1;
-          s->_var_spec.push_back(bind);
-          gsg->_glBindAttribLocation(_glsl_program, i, param_name_cstr);
         }
+        s->_var_spec.push_back(bind);
       }
     }
     
@@ -810,7 +847,7 @@ issue_parameters(GSG *gsg, int altered) {
           release_resources(gsg);
           return;
         }
-        GLint p = _shader->_glsl_parameter_map[_shader->_ptr_spec[i]._id._seqno];
+        GLint p = _glsl_parameter_map[_shader->_ptr_spec[i]._id._seqno];
         switch(_ptr._dim[1]) {
           case 1: gsg->_glUniform4fv(p, _ptr._dim[0], (float*)_ptr_data->_ptr); continue;
           case 2: gsg->_glUniform4fv(p, _ptr._dim[0], (float*)_ptr_data->_ptr); continue;
@@ -925,7 +962,7 @@ issue_parameters(GSG *gsg, int altered) {
 #endif
 
       if (_shader->get_language() == Shader::SL_GLSL) {
-        GLint p = _shader->_glsl_parameter_map[_shader->_mat_spec[i]._id._seqno];
+        GLint p = _glsl_parameter_map[_shader->_mat_spec[i]._id._seqno];
         switch (_shader->_mat_spec[i]._piece) {
         case Shader::SMP_whole: gsg->_glUniformMatrix4fv(p, 1, false, data); continue;
         case Shader::SMP_transpose: gsg->_glUniformMatrix4fv(p, 1, true, data); continue;
@@ -1035,7 +1072,7 @@ update_shader_vertex_arrays(CLP(ShaderContext) *prev, GSG *gsg,
     Geom::NumericType numeric_type;
     int start, stride, num_values;
     int nvarying = _shader->_var_spec.size();
-    for (int i=0; i<nvarying; i++) {
+    for (int i = 0; i < nvarying; ++i) {
 #if defined(HAVE_CG) && !defined(OPENGLES)
       if (_shader->get_language() == Shader::SL_Cg) {
         if (_cg_parameter_map[_shader->_var_spec[i]._id._seqno] == 0) {
@@ -1062,16 +1099,13 @@ update_shader_vertex_arrays(CLP(ShaderContext) *prev, GSG *gsg,
         if (!gsg->setup_array_data(client_pointer, array_reader, force)) {
           return false;
         }
+
         if (_shader->get_language() == Shader::SL_GLSL) {
-#ifndef OPENGLES_2
+          const GLint p = _glsl_parameter_map[_shader->_var_spec[i]._id._seqno];
-          glEnableClientState(GL_VERTEX_ARRAY);
+          gsg->_glEnableVertexAttribArray(p);
-#endif
+          gsg->_glVertexAttribPointer(p, num_values, gsg->get_numeric_type(numeric_type),
-          gsg->_glEnableVertexAttribArray(i);
+                                      GL_TRUE, stride, client_pointer + start);
-          gsg->_glVertexAttribPointer(i, num_values, gsg->get_numeric_type(numeric_type),
+
-                                GL_FALSE, stride, client_pointer + start);
-#ifndef OPENGLES_2
-          glDisableClientState(GL_VERTEX_ARRAY);
-#endif
 #if defined(HAVE_CG) && !defined(OPENGLES)
         } else if (_shader->get_language() == Shader::SL_Cg) {
           CGparameter p = _cg_parameter_map[_shader->_var_spec[i]._id._seqno];
@@ -1245,7 +1279,7 @@ update_shader_texture_bindings(CLP(ShaderContext) *prev, GSG *gsg) {
     gsg->apply_texture(tc);
 
     if (_shader->get_language() == Shader::SL_GLSL) {
-      GLint p = _shader->_glsl_parameter_map[_shader->_tex_spec[i]._id._seqno];
+      GLint p = _glsl_parameter_map[_shader->_tex_spec[i]._id._seqno];
       gsg->_glUniform1i(p, texunit);
     }
 

panda/src/glstuff/glShaderContext_src.h

@@ -46,6 +46,7 @@ public:
   void disable_shader_texture_bindings(GSG *gsg);
   void update_shader_texture_bindings(CLP(ShaderContext) *prev, GSG *gsg);
 
+  INLINE bool uses_standard_vertex_arrays(void);
   INLINE bool uses_custom_vertex_arrays(void);
   INLINE bool uses_custom_texture_bindings(void);
 
@@ -67,11 +68,15 @@ private:
   GLuint _glsl_tcshader;
   GLuint _glsl_teshader;
 
+  pvector <GLint> _glsl_parameter_map;
+
   int _stage_offset;
   // Avoid using this! It merely exists so the
   // destructor has access to the extension functions.
   WPT(GSG) _last_gsg;
 
+  bool _uses_standard_vertex_arrays;
+
   void glsl_report_shader_errors(GSG *gsg, unsigned int shader);
   void glsl_report_program_errors(GSG *gsg, unsigned int program);
   unsigned int glsl_compile_entry_point(GSG *gsg, Shader::ShaderType type);