Changes to make ColorAttrib behavior more consistent:

- T_off now actually properly disables vertex colours
- T_vertex is now the default, to preserve the previous behaviour
- ShaderGenerator behavior is now the same as in the FFP
- tests are updated to verify new behavior
- tests now properly use vertex colours, previously they accidentally only used flat colors
- With color-scale-via-lighting off and no color scale, color is no longer munged
- p3d_Color in GLSL shaders is now properly set to white instead of black with T_off mode
- In DX9 shaders will now sample white color for absent or disabled vertex color

Fixes #401
Also see #371

panda/src/display/graphicsStateGuardian.cxx

@@ -2722,7 +2722,7 @@ do_issue_color_scale() {
   }
 
   if (_alpha_scale_via_texture && !_has_scene_graph_color &&
-      target_color_scale->has_alpha_scale()) {
+      _vertex_colors_enabled && target_color_scale->has_alpha_scale()) {
     // This color scale will set a special texture--so again, clear the
     // texture.
     _state_mask.clear_bit(TextureAttrib::get_class_slot());
@@ -3168,6 +3168,17 @@ determine_light_color_scale() {
                                 _scene_graph_color[3] * _current_color_scale[3]);
     }
 
+  } else if (!_vertex_colors_enabled) {
+    // We don't have a scene graph color, but we don't want to enable vertex
+    // colors either, so we still need to force a white material color in
+    // absence of any other color.
+    _has_material_force_color = true;
+    _material_force_color.set(1.0f, 1.0f, 1.0f, 1.0f);
+    _light_color_scale.set(1.0f, 1.0f, 1.0f, 1.0f);
+    if (!_color_blend_involves_color_scale && _color_scale_enabled) {
+      _material_force_color.componentwise_mult(_current_color_scale);
+    }
+
   } else {
     // Otherise, leave the materials alone, but we might still scale the
     // lights.

panda/src/display/standardMunger.cxx

@@ -55,24 +55,10 @@ StandardMunger(GraphicsStateGuardianBase *gsg, const RenderState *state,
     const ColorScaleAttrib *color_scale_attrib;
 
     if (state->get_attrib(color_attrib) &&
-        color_attrib->get_color_type() == ColorAttrib::T_flat) {
+        color_attrib->get_color_type() != ColorAttrib::T_vertex) {
 
-      if (!get_gsg()->get_color_scale_via_lighting()) {
-        // We only need to munge the color directly if the GSG says it can't
-        // cheat the color via lighting (presumably, in this case, by applying
-        // a material).
-        _color = color_attrib->get_color();
-        if (state->get_attrib(color_scale_attrib) &&
-            color_scale_attrib->has_scale()) {
-          const LVecBase4 &cs = color_scale_attrib->get_scale();
-          _color.set(_color[0] * cs[0],
-                     _color[1] * cs[1],
-                     _color[2] * cs[2],
-                     _color[3] * cs[3]);
-        }
-        _munge_color = true;
-        _should_munge_state = true;
-      }
+      // In this case, we don't need to munge anything as we can apply the
+      // color and color scale via glColor4f.
 
     } else if (state->get_attrib(color_scale_attrib) &&
                color_scale_attrib->has_scale()) {

panda/src/display/standardMunger.h

@@ -51,12 +51,13 @@ private:
   NumericType _numeric_type;
   Contents _contents;
 
-  bool _munge_color;
-  bool _munge_color_scale;
   bool _auto_shader;
   bool _shader_skinning;
   bool _remove_material;
 
+protected:
+  bool _munge_color;
+  bool _munge_color_scale;
   LColor _color;
   LVecBase4 _color_scale;
 

panda/src/dxgsg9/dxGeomMunger9.I

@@ -10,34 +10,3 @@
  * @author drose
  * @date 2005-03-11
  */
-
-/**
- *
- */
-INLINE DXGeomMunger9::
-DXGeomMunger9(GraphicsStateGuardian *gsg, const RenderState *state) :
-  StandardMunger(gsg, state, 1, NT_packed_dabc, C_color),
-  _texture(nullptr),
-  _tex_gen(nullptr)
-{
-  const TextureAttrib *texture = nullptr;
-  const TexGenAttrib *tex_gen = nullptr;
-  state->get_attrib(texture);
-  state->get_attrib(tex_gen);
-  _texture = texture;
-  _tex_gen = tex_gen;
-
-  _filtered_texture = nullptr;
-  _reffed_filtered_texture = false;
-  if (texture != nullptr) {
-    _filtered_texture = texture->filter_to_max(gsg->get_max_texture_stages());
-    if (_filtered_texture != texture) {
-      _filtered_texture->ref();
-      _reffed_filtered_texture = true;
-    }
-  }
-  // Set a callback to unregister ourselves when either the Texture or the
-  // TexGen object gets deleted.
-  _texture.add_callback(this);
-  _tex_gen.add_callback(this);
-}

panda/src/dxgsg9/dxGeomMunger9.cxx

@@ -19,6 +19,66 @@
 GeomMunger *DXGeomMunger9::_deleted_chain = nullptr;
 TypeHandle DXGeomMunger9::_type_handle;
 
+/**
+ *
+ */
+DXGeomMunger9::
+DXGeomMunger9(GraphicsStateGuardian *gsg, const RenderState *state) :
+  StandardMunger(gsg, state, 1, NT_packed_dabc, C_color),
+  _texture(nullptr),
+  _tex_gen(nullptr)
+{
+  const TextureAttrib *texture = nullptr;
+  const TexGenAttrib *tex_gen = nullptr;
+  state->get_attrib(texture);
+  state->get_attrib(tex_gen);
+  _texture = texture;
+  _tex_gen = tex_gen;
+
+  if (!gsg->get_color_scale_via_lighting()) {
+    // We might need to munge the colors, if we are overriding the vertex
+    // colors and the GSG can't cheat the color via lighting.
+
+    const ColorAttrib *color_attrib;
+    const ShaderAttrib *shader_attrib;
+    state->get_attrib_def(shader_attrib);
+
+    if (!shader_attrib->auto_shader() &&
+        shader_attrib->get_shader() == nullptr &&
+        state->get_attrib(color_attrib) &&
+        color_attrib->get_color_type() != ColorAttrib::T_vertex) {
+
+      if (color_attrib->get_color_type() == ColorAttrib::T_off) {
+        _color.set(1, 1, 1, 1);
+      } else {
+        _color = color_attrib->get_color();
+      }
+
+      const ColorScaleAttrib *color_scale_attrib;
+      if (state->get_attrib(color_scale_attrib) &&
+          color_scale_attrib->has_scale()) {
+        _color.componentwise_mult(color_scale_attrib->get_scale());
+      }
+      _munge_color = true;
+      _should_munge_state = true;
+    }
+  }
+
+  _filtered_texture = nullptr;
+  _reffed_filtered_texture = false;
+  if (texture != nullptr) {
+    _filtered_texture = texture->filter_to_max(gsg->get_max_texture_stages());
+    if (_filtered_texture != texture) {
+      _filtered_texture->ref();
+      _reffed_filtered_texture = true;
+    }
+  }
+  // Set a callback to unregister ourselves when either the Texture or the
+  // TexGen object gets deleted.
+  _texture.add_callback(this);
+  _tex_gen.add_callback(this);
+}
+
 /**
  *
  */

panda/src/dxgsg9/dxGeomMunger9.h

@@ -28,7 +28,7 @@
  */
 class EXPCL_PANDADX DXGeomMunger9 : public StandardMunger, public WeakPointerCallback {
 public:
-  INLINE DXGeomMunger9(GraphicsStateGuardian *gsg, const RenderState *state);
+  DXGeomMunger9(GraphicsStateGuardian *gsg, const RenderState *state);
   virtual ~DXGeomMunger9();
   ALLOC_DELETED_CHAIN(DXGeomMunger9);
 

panda/src/dxgsg9/dxGraphicsStateGuardian9.cxx

@@ -140,6 +140,7 @@ DXGraphicsStateGuardian9(GraphicsEngine *engine, GraphicsPipe *pipe) :
 
   _last_fvf = 0;
   _num_bound_streams = 0;
+  _white_vbuffer = nullptr;
 
   _vertex_shader_version_major = 0;
   _vertex_shader_version_minor = 0;
@@ -4545,6 +4546,11 @@ reset_d3d_device(D3DPRESENT_PARAMETERS *presentation_params,
     release_all_vertex_buffers();
     release_all_index_buffers();
 
+    if (_white_vbuffer != nullptr) {
+      _white_vbuffer->Release();
+      _white_vbuffer = nullptr;
+    }
+
     // must be called before reset
     Thread *current_thread = Thread::get_current_thread();
     _prepared_objects->begin_frame(this, current_thread);
@@ -5404,6 +5410,40 @@ set_cg_device(LPDIRECT3DDEVICE9 cg_device) {
 #endif // HAVE_CG
 }
 
+/**
+ * Returns a vertex buffer containing only a full-white color.
+ */
+LPDIRECT3DVERTEXBUFFER9 DXGraphicsStateGuardian9::
+get_white_vbuffer() {
+  if (_white_vbuffer != nullptr) {
+    return _white_vbuffer;
+  }
+
+  LPDIRECT3DVERTEXBUFFER9 vbuffer;
+  HRESULT hr;
+  hr = _screen->_d3d_device->CreateVertexBuffer(sizeof(D3DCOLOR), D3DUSAGE_WRITEONLY, D3DFVF_DIFFUSE, D3DPOOL_DEFAULT, &vbuffer, nullptr);
+
+  if (FAILED(hr)) {
+    dxgsg9_cat.error()
+      << "CreateVertexBuffer failed" << D3DERRORSTRING(hr);
+    return nullptr;
+  }
+
+  D3DCOLOR *local_pointer;
+  hr = vbuffer->Lock(0, sizeof(D3DCOLOR), (void **) &local_pointer, D3DLOCK_DISCARD);
+  if (FAILED(hr)) {
+    dxgsg9_cat.error()
+      << "VertexBuffer::Lock failed" << D3DERRORSTRING(hr);
+    return false;
+  }
+
+  *local_pointer = D3DCOLOR_ARGB(255, 255, 255, 255);
+
+  vbuffer->Unlock();
+  _white_vbuffer = vbuffer;
+  return vbuffer;
+}
+
 typedef std::string KEY;
 
 typedef struct _KEY_ELEMENT

panda/src/dxgsg9/dxGraphicsStateGuardian9.h

@@ -168,6 +168,7 @@ public:
   static void set_cg_device(LPDIRECT3DDEVICE9 cg_device);
   virtual bool get_supports_cg_profile(const std::string &name) const;
 
+  LPDIRECT3DVERTEXBUFFER9 get_white_vbuffer();
 
 protected:
   void do_issue_transform();
@@ -274,12 +275,6 @@ protected:
 
   RenderBuffer::Type _cur_read_pixel_buffer;  // source for copy_pixel_buffer operation
 
-  PN_stdfloat _material_ambient;
-  PN_stdfloat _material_diffuse;
-  PN_stdfloat _material_specular;
-  PN_stdfloat _material_shininess;
-  PN_stdfloat _material_emission;
-
   enum DxgsgFogType {
     None,
     PerVertexFog=D3DRS_FOGVERTEXMODE,
@@ -320,6 +315,7 @@ protected:
 
   DWORD _last_fvf;
   int _num_bound_streams;
+  LPDIRECT3DVERTEXBUFFER9 _white_vbuffer;
 
   // Cache the data necessary to bind each particular light each frame, so if
   // we bind a given light multiple times, we only have to compute its data

panda/src/dxgsg9/dxShaderContext9.cxx

@@ -390,6 +390,8 @@ update_shader_vertex_arrays(DXShaderContext9 *prev, GSG *gsg, bool force) {
     // arrays ("streams"), and we repeatedly iterate the parameters to pull
     // out only those for a single stream.
 
+    bool apply_white_color = false;
+
     int number_of_arrays = gsg->_data_reader->get_num_arrays();
     for (int array_index = 0; array_index < number_of_arrays; ++array_index) {
       const GeomVertexArrayDataHandle* array_reader =
@@ -423,6 +425,11 @@ update_shader_vertex_arrays(DXShaderContext9 *prev, GSG *gsg, bool force) {
           }
         }
 
+        if (name == InternalName::get_color() && !gsg->_vertex_colors_enabled) {
+          apply_white_color = true;
+          continue;
+        }
+
         const GeomVertexArrayDataHandle *param_array_reader;
         Geom::NumericType numeric_type;
         int num_values, start, stride;
@@ -435,6 +442,9 @@ update_shader_vertex_arrays(DXShaderContext9 *prev, GSG *gsg, bool force) {
           // shader parameter, which can cause Bad Things to happen so I'd
           // like to at least get a hint as to what's gone wrong.
           dxgsg9_cat.info() << "Geometry contains no data for shader parameter " << *name << "\n";
+          if (name == InternalName::get_color()) {
+            apply_white_color = true;
+          }
           continue;
         }
 
@@ -564,6 +574,19 @@ update_shader_vertex_arrays(DXShaderContext9 *prev, GSG *gsg, bool force) {
 
     _num_bound_streams = number_of_arrays;
 
+    if (apply_white_color) {
+      // The shader needs a vertex color, but vertex colors are disabled.
+      // Bind a vertex buffer containing only one white colour.
+      int array_index = number_of_arrays;
+      LPDIRECT3DVERTEXBUFFER9 vbuffer = gsg->get_white_vbuffer();
+      hr = device->SetStreamSource(array_index, vbuffer, 0, 0);
+      if (FAILED(hr)) {
+        dxgsg9_cat.error() << "SetStreamSource failed" << D3DERRORSTRING(hr);
+      }
+      vertex_element_array->add_diffuse_color_vertex_element(array_index, 0);
+      ++_num_bound_streams;
+    }
+
     if (_vertex_element_array != nullptr &&
         _vertex_element_array->add_end_vertex_element()) {
       if (dxgsg9_cat.is_debug()) {

panda/src/glstuff/glGraphicsStateGuardian_src.cxx

@@ -4392,7 +4392,8 @@ update_standard_vertex_arrays(bool force) {
       GLPf(Color4)(1.0f, 1.0f, 1.0f, 1.0f);
     } else
 #endif // NDEBUG
-      if (_data_reader->get_color_info(array_reader, num_values, numeric_type,
+      if (_vertex_colors_enabled &&
+          _data_reader->get_color_info(array_reader, num_values, numeric_type,
                                        start, stride)) {
         if (!setup_array_data(client_pointer, array_reader, force)) {
           return false;
@@ -4409,7 +4410,13 @@ update_standard_vertex_arrays(bool force) {
         glDisableClientState(GL_COLOR_ARRAY);
 
         // Since we don't have per-vertex color, the implicit color is white.
-        GLPf(Color4)(1.0f, 1.0f, 1.0f, 1.0f);
+        if (_color_scale_via_lighting) {
+          GLPf(Color4)(1.0f, 1.0f, 1.0f, 1.0f);
+        } else {
+          LColor color = _scene_graph_color;
+          color.componentwise_mult(_current_color_scale);
+          GLPf(Color4)(color[0], color[1], color[2], color[3]);
+        }
       }
 
     // Now set up each of the active texture coordinate stages--or at least

panda/src/glstuff/glShaderContext_src.cxx

@@ -2440,9 +2440,9 @@ update_shader_vertex_arrays(ShaderContext *prev, bool force) {
         if (p == _color_attrib_index) {
           // Vertex colors are disabled or not present.  Apply flat color.
 #ifdef STDFLOAT_DOUBLE
-          _glgsg->_glVertexAttrib4dv(p, color_attrib->get_color().get_data());
+          _glgsg->_glVertexAttrib4dv(p, _glgsg->_scene_graph_color.get_data());
 #else
-          _glgsg->_glVertexAttrib4fv(p, color_attrib->get_color().get_data());
+          _glgsg->_glVertexAttrib4fv(p, _glgsg->_scene_graph_color.get_data());
 #endif
         }
       }

panda/src/pgraph/colorAttrib.cxx

@@ -68,7 +68,7 @@ make_off() {
  */
 CPT(RenderAttrib) ColorAttrib::
 make_default() {
-  return make_off();
+  return make_vertex();
 }
 
 /**

panda/src/pgraph/colorAttrib.h

@@ -88,7 +88,7 @@ public:
     register_type(_type_handle, "ColorAttrib",
                   RenderAttrib::get_class_type());
     _attrib_slot = register_slot(_type_handle, 100,
-      new ColorAttrib(T_off, LColor(1, 1, 1, 1)));
+      new ColorAttrib(T_vertex, LColor::zero()));
   }
   virtual TypeHandle get_type() const {
     return get_class_type();

tests/display/test_color_buffer.py

@@ -113,14 +113,38 @@ def render_color_pixel(region, state, vertex_color=None):
     camera.node().get_lens(0).set_near_far(1, 3)
     camera.node().set_cull_bounds(core.OmniBoundingVolume())
 
-    cm = core.CardMaker("card")
-    cm.set_frame(-1, 1, -1, 1)
+    if vertex_color is not None:
+        format = core.GeomVertexFormat.get_v3cp()
+    else:
+        format = core.GeomVertexFormat.get_v3()
+
+    vdata = core.GeomVertexData("card", format, core.Geom.UH_static)
+    vdata.unclean_set_num_rows(4)
+
+    vertex = core.GeomVertexWriter(vdata, "vertex")
+    vertex.set_data3(core.Vec3.rfu(-1, 0, 1))
+    vertex.set_data3(core.Vec3.rfu(-1, 0, -1))
+    vertex.set_data3(core.Vec3.rfu(1, 0, 1))
+    vertex.set_data3(core.Vec3.rfu(1, 0, -1))
 
     if vertex_color is not None:
-        cm.set_color(vertex_color)
+        color = core.GeomVertexWriter(vdata, "color")
+        color.set_data4(vertex_color)
+        color.set_data4(vertex_color)
+        color.set_data4(vertex_color)
+        color.set_data4(vertex_color)
 
-    card = scene.attach_new_node(cm.generate())
-    card.set_state(state)
+    strip = core.GeomTristrips(core.Geom.UH_static)
+    strip.set_shade_model(core.Geom.SM_uniform)
+    strip.add_next_vertices(4)
+    strip.close_primitive()
+
+    geom = core.Geom(vdata)
+    geom.add_primitive(strip)
+
+    gnode = core.GeomNode("card")
+    gnode.add_geom(geom, state)
+    card = scene.attach_new_node(gnode)
     card.set_pos(0, 2, 0)
     card.set_scale(60)
 
@@ -197,14 +221,13 @@ def test_color_empty_vertex(color_region, shader_attrib, material_attrib):
 
 
 def test_color_off_vertex(color_region, shader_attrib, material_attrib):
-    #XXX This behaviour is really odd.
     state = core.RenderState.make(
         core.ColorAttrib.make_off(),
         shader_attrib,
         material_attrib,
     )
     result = render_color_pixel(color_region, state, vertex_color=TEST_COLOR)
-    assert result.almost_equal(TEST_COLOR, FUZZ)
+    assert result == (1, 1, 1, 1)
 
 
 def test_scaled_color_empty(color_region, shader_attrib, material_attrib):
@@ -259,7 +282,6 @@ def test_scaled_color_empty_vertex(color_region, shader_attrib, material_attrib)
 
 
 def test_scaled_color_off_vertex(color_region, shader_attrib, material_attrib):
-    #XXX This behaviour is really odd.
     state = core.RenderState.make(
         core.ColorAttrib.make_off(),
         core.ColorScaleAttrib.make(TEST_COLOR_SCALE),
@@ -267,5 +289,5 @@ def test_scaled_color_off_vertex(color_region, shader_attrib, material_attrib):
         material_attrib,
     )
     result = render_color_pixel(color_region, state, vertex_color=TEST_COLOR)
-    assert result.almost_equal(TEST_SCALED_COLOR, FUZZ)
+    assert result.almost_equal(TEST_COLOR_SCALE, FUZZ)