saner treatment of ColorAttrib

panda/src/pgraph/colorAttrib.I

@@ -40,10 +40,7 @@ ColorAttrib(ColorAttrib::Type type, const Colorf &color) :
 //                 ColorAttrib for all geometry.  You can get this
 //                 color via get_color().
 //
-//                 T_off - do not issue any color commands at all.
-//                 This is generally used only in contexts where the
-//                 color is meaningless, e.g. when drawing directly to
-//                 the depth buffer.
+//                 T_off - use the color white.
 ////////////////////////////////////////////////////////////////////
 INLINE ColorAttrib::Type ColorAttrib::
 get_color_type() const {
@@ -53,9 +50,9 @@ get_color_type() const {
 ////////////////////////////////////////////////////////////////////
 //     Function: ColorAttrib::get_color
 //       Access: Published
-//  Description: If the type is T_flat, this returns the color that
-//               will be applied to geometry.  If the type is anything
-//               else, this is meaningless.
+//  Description: If the type is T_flat or T_off, this returns the
+//               color that will be applied to geometry.  If the type
+//               is T_vertex, this is meaningless.
 ////////////////////////////////////////////////////////////////////
 INLINE const Colorf &ColorAttrib::
 get_color() const {

panda/src/pgraph/colorAttrib.cxx

@@ -37,7 +37,7 @@ make_vertex() {
   if (_vertex != 0) {
     return _vertex;
   }
-  ColorAttrib *attrib = new ColorAttrib(T_vertex);
+  ColorAttrib *attrib = new ColorAttrib(T_vertex, Colorf::zero());
   _vertex = return_new(attrib);
   return _vertex;
 }
@@ -58,15 +58,14 @@ make_flat(const Colorf &color) {
 //     Function: ColorAttrib::make_off
 //       Access: Published, Static
 //  Description: Constructs a new ColorAttrib object that indicates
-//               geometry should be rendered without any color
-//               commands at all.
+//               geometry should be rendered in white.
 ////////////////////////////////////////////////////////////////////
 CPT(RenderAttrib) ColorAttrib::
 make_off() {
   if (_off != 0) {
     return _off;
   }
-  ColorAttrib *attrib = new ColorAttrib(T_off);
+  ColorAttrib *attrib = new ColorAttrib(T_off, Colorf(1.0f, 1.0f, 1.0f, 1.0f));
   _off = return_new(attrib);
   return _off;
 }
@@ -80,7 +79,7 @@ make_off() {
 ////////////////////////////////////////////////////////////////////
 CPT(RenderAttrib) ColorAttrib::
 make_default() {
-  return make_flat(Colorf(1.0f, 1.0f, 1.0f, 1.0f));
+  return make_off();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -143,10 +142,22 @@ compare_to_impl(const RenderAttrib *other) const {
 ////////////////////////////////////////////////////////////////////
 void ColorAttrib::
 quantize_color() {
-  _color[0] = cfloor(_color[0] * 1000.0f + 0.5f) * 0.001f;
-  _color[1] = cfloor(_color[1] * 1000.0f + 0.5f) * 0.001f;
-  _color[2] = cfloor(_color[2] * 1000.0f + 0.5f) * 0.001f;
-  _color[3] = cfloor(_color[3] * 1000.0f + 0.5f) * 0.001f;
+  switch (_type) {
+  case T_flat:
+    _color[0] = cfloor(_color[0] * 1000.0f + 0.5f) * 0.001f;
+    _color[1] = cfloor(_color[1] * 1000.0f + 0.5f) * 0.001f;
+    _color[2] = cfloor(_color[2] * 1000.0f + 0.5f) * 0.001f;
+    _color[3] = cfloor(_color[3] * 1000.0f + 0.5f) * 0.001f;
+    break;
+
+  case T_off:
+    _color.set(1.0f, 1.0f, 1.0f, 1.0f);
+    break;
+
+  case T_vertex:
+    _color.set(0.0f, 0.0f, 0.0f, 0.0f);
+    break;
+  }
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -184,7 +195,7 @@ write_datagram(BamWriter *manager, Datagram &dg) {
 ////////////////////////////////////////////////////////////////////
 TypedWritable *ColorAttrib::
 make_from_bam(const FactoryParams &params) {
-  ColorAttrib *attrib = new ColorAttrib;
+  ColorAttrib *attrib = new ColorAttrib(T_off, Colorf::zero());
   DatagramIterator scan;
   BamReader *manager;
 

panda/src/pgraph/colorAttrib.h

@@ -34,8 +34,7 @@ PUBLISHED:
   };
 
 private:
-  INLINE ColorAttrib(Type type = T_vertex, 
-                     const Colorf &color = Colorf(0.0f, 0.0f, 0.0f, 1.0f));
+  INLINE ColorAttrib(Type type, const Colorf &color);
 
 PUBLISHED:
   static CPT(RenderAttrib) make_vertex();

panda/src/pgraph/geomNode.cxx

@@ -134,17 +134,12 @@ apply_attribs_to_vertices(const AccumulatedAttribs &attribs, int attrib_types,
 
       if ((attrib_types & SceneGraphReducer::TT_color) != 0) {
         CPT(RenderAttrib) ra = geom_attribs._color;
-        int override = geom_attribs._color_override;
-        if (ra == (const RenderAttrib *)NULL) {
-          // If we don't have a color attrib, implicitly apply the
-          // "off" attrib.  But use an override of -1, so we don't
-          // replace a color attrib already on the Geom state.
-          ra = ColorAttrib::make_off();
-          override = -1;
+        if (ra != (const RenderAttrib *)NULL) {
+          int override = geom_attribs._color_override;
+          entry->_state = entry->_state->add_attrib(ra, override);
         }
-        entry->_state = entry->_state->add_attrib(ra, override);
 
-        ra = entry->_state->get_attrib(ColorAttrib::get_class_slot());
+        ra = entry->_state->get_attrib_def(ColorAttrib::get_class_slot());
         const ColorAttrib *ca = DCAST(ColorAttrib, ra);
         if (ca->get_color_type() != ColorAttrib::T_vertex) {
           if (transformer.remove_column(new_geom, InternalName::get_color())) {
@@ -160,30 +155,28 @@ apply_attribs_to_vertices(const AccumulatedAttribs &attribs, int attrib_types,
             // Now, if we have an "off" or "flat" color attribute, we
             // simply modify the color attribute, and leave the
             // vertices alone.
-            const RenderAttrib *ra = entry->_state->get_attrib(ColorAttrib::get_class_slot());
-            if (ra != (const RenderAttrib *)NULL) {
-              const ColorAttrib *ca = DCAST(ColorAttrib, ra);
-              if (ca->get_color_type() == ColorAttrib::T_off) {
-                entry->_state = entry->_state->set_attrib(ColorAttrib::make_vertex());
-                // ColorAttrib::T_off means the color scale becomes
-                // the new color.
-                entry->_state = entry->_state->set_attrib(ColorAttrib::make_flat(csa->get_scale()));
-
-              } else if (ca->get_color_type() == ColorAttrib::T_flat) {
-                // ColorAttrib::T_flat means the color scale modulates
-                // the specified color to produce a new color.
-                const Colorf &c1 = ca->get_color();
-                const LVecBase4f &c2 = csa->get_scale();
-                Colorf color(c1[0] * c2[0], c1[1] * c2[1], 
-                             c1[2] * c2[2], c1[3] * c2[3]);
-                entry->_state = entry->_state->set_attrib(ColorAttrib::make_flat(color));
-
-              } else {
-                // Otherwise, we have vertex color, and we just scale
-                // it normally.
-                if (transformer.transform_colors(new_geom, csa->get_scale())) {
-                  any_changed = true;
-                }
+            const RenderAttrib *ra = entry->_state->get_attrib_def(ColorAttrib::get_class_slot());
+            const ColorAttrib *ca = DCAST(ColorAttrib, ra);
+            if (ca->get_color_type() == ColorAttrib::T_off) {
+              entry->_state = entry->_state->set_attrib(ColorAttrib::make_vertex());
+              // ColorAttrib::T_off means the color scale becomes
+              // the new color.
+              entry->_state = entry->_state->set_attrib(ColorAttrib::make_flat(csa->get_scale()));
+              
+            } else if (ca->get_color_type() == ColorAttrib::T_flat) {
+              // ColorAttrib::T_flat means the color scale modulates
+              // the specified color to produce a new color.
+              const Colorf &c1 = ca->get_color();
+              const LVecBase4f &c2 = csa->get_scale();
+              Colorf color(c1[0] * c2[0], c1[1] * c2[1], 
+                           c1[2] * c2[2], c1[3] * c2[3]);
+              entry->_state = entry->_state->set_attrib(ColorAttrib::make_flat(color));
+              
+            } else {
+              // Otherwise, we have vertex color, and we just scale
+              // it normally.
+              if (transformer.transform_colors(new_geom, csa->get_scale())) {
+                any_changed = true;
               }
             }
           }

panda/src/pgraph/geomTransformer.cxx

@@ -819,26 +819,6 @@ remove_column(GeomNode *node, const InternalName *column) {
 //               RenderStates the same.  It does this by
 //               canonicalizing the ColorAttribs, and in the future,
 //               possibly other attribs.
-//
-//               This implementation is not very smart yet.  It 
-//               unnecessarily canonicalizes ColorAttribs even if 
-//               this will not yield compatible RenderStates.  A
-//               better algorithm would:
-//
-//               - each geom already starts with an original
-//                 RenderState.  In addition to this, calculate for
-//                 each geom a canonical RenderState.
-//
-//               - maintain a table mapping canonical RenderState
-//                 to a list of geoms.
-//
-//               - for each group of geoms with the same 
-//                 canonical renderstate, see if they already have
-//                 matching RenderStates.
-//
-//               - If they have differing RenderStates, then
-//                 actually canonicalize the geoms.
-//               
 ////////////////////////////////////////////////////////////////////
 bool GeomTransformer::
 make_compatible_state(GeomNode *node) {
@@ -850,15 +830,16 @@ make_compatible_state(GeomNode *node) {
   GeomNode::GeomList::iterator gi;
   PT(GeomNode::GeomList) geoms = cdata->modify_geoms();
 
-  // For each geom, calculate a canonicalized RenderState, and 
-  // classify all the geoms according to that.
+  // For each geom, calculate a canonicalized RenderState, and
+  // classify all the geoms according to that.  By "canonicalize"
+  // here, we simply mean removing the ColorAttrib.
   
   typedef pmap <CPT(RenderState), pvector<int> > StateTable;
   StateTable state_table;
   
   for (int i = 0; i < (int)geoms->size(); i++) {
     GeomNode::GeomEntry &entry = (*geoms)[i];
-    CPT(RenderState) canon = entry._state->add_attrib(ColorAttrib::make_vertex(), -1);
+    CPT(RenderState) canon = entry._state->remove_attrib(ColorAttrib::get_class_slot());
     state_table[canon].push_back(i);
   }
 
@@ -871,7 +852,7 @@ make_compatible_state(GeomNode *node) {
     // If the geoms in the group already have the same RenderStates,
     // then nothing needs to be done to this group.
     
-    pvector<int> &indices = (*si).second;
+    const pvector<int> &indices = (*si).second;
     bool mismatch = false;
     for (int i = 1; i < (int)indices.size(); i++) {
       if ((*geoms)[indices[i]]._state != (*geoms)[indices[0]]._state) {
@@ -884,15 +865,13 @@ make_compatible_state(GeomNode *node) {
     }
     
     // The geoms do not have the same RenderState, but they could,
-    // since their canonicalized states are the same.  Canonicalize them.
+    // since their canonicalized states are the same.  Canonicalize
+    // them, by applying the colors to the vertices.
     
     const RenderState *canon_state = (*si).first;
     for (int i = 0; i < (int)indices.size(); i++) {
       GeomNode::GeomEntry &entry = (*geoms)[indices[i]];
-      const RenderAttrib *ra = entry._state->get_attrib(ColorAttrib::get_class_slot());
-      if (ra == (RenderAttrib *)NULL) {
-        ra = ColorAttrib::make_off();
-      }
+      const RenderAttrib *ra = entry._state->get_attrib_def(ColorAttrib::get_class_slot());
       const ColorAttrib *ca = DCAST(ColorAttrib, ra);
       if (ca->get_color_type() == ColorAttrib::T_vertex) {
         // All we need to do is ensure that the geom has a color column.
@@ -900,21 +879,19 @@ make_compatible_state(GeomNode *node) {
           PT(Geom) new_geom = entry._geom.get_read_pointer()->make_copy();
           if (set_color(new_geom, Colorf(1,1,1,1))) {
             entry._geom = new_geom;
-            any_changed = true;
           }
         }
       } else {
-        Colorf c(1,1,1,1);
-        if (ca->get_color_type() == ColorAttrib::T_flat) {
-          c = ca->get_color();
-        }
+        // A flat color (or "off", which is white).  Set the vertices
+        // to the indicated flat color.
+        Colorf c = ca->get_color();
         PT(Geom) new_geom = entry._geom.get_read_pointer()->make_copy();
         if (set_color(new_geom, c)) {
           entry._geom = new_geom;
-          any_changed = true;
         }
       }
-      entry._state = canon_state;
+      entry._state = canon_state->add_attrib(ColorAttrib::make_vertex());
+      any_changed = true;
     }
   }