More efficient light handling; sort lights by highest priority first

panda/src/display/graphicsStateGuardian.cxx

@@ -1092,23 +1092,12 @@ fetch_specified_part(Shader::ShaderMatInput part, InternalName *name,
     const LightAttrib *target_light = (const LightAttrib *)
       _target_rs->get_attrib_def(LightAttrib::get_class_slot());
 
-    int num_on_lights = target_light->get_num_on_lights();
-    if (num_on_lights == 0) {
+    if (!target_light->has_any_on_light()) {
       // There are no lights at all.  This means, to follow the fixed-
       // function model, we pretend there is an all-white ambient light.
       t.set_row(3, LVecBase4(1, 1, 1, 1));
     } else {
-      for (int li = 0; li < num_on_lights; li++) {
-        NodePath light = target_light->get_on_light(li);
-        nassertr(!light.is_empty(), &LMatrix4::zeros_mat());
-        Light *light_obj = light.node()->as_light();
-        nassertr(light_obj != (Light *)NULL, &LMatrix4::zeros_mat());
-
-        if (light_obj->is_ambient_light()) {
-          cur_ambient_light += light_obj->get_color();
-        }
-      }
-      t.set_row(3, cur_ambient_light);
+      t.set_row(3, target_light->get_ambient_contribution());
     }
     return &t;
   }
@@ -1374,28 +1363,20 @@ fetch_specified_part(Shader::ShaderMatInput part, InternalName *name,
     const LightAttrib *target_light;
     _target_rs->get_attrib_def(target_light);
 
-    // We want to ignore ambient lights.  To that effect, iterate through the
-    // list of lights.  In the future, we will improve this system, by also
-    // filtering down to the number of lights specified by the shader.
-    int i = 0;
-
-    int num_on_lights = target_light->get_num_on_lights();
-    for (int li = 0; li < num_on_lights; li++) {
-      NodePath light = target_light->get_on_light(li);
+    // We don't count ambient lights, which would be pretty silly to handle
+    // via this mechanism.
+    size_t num_lights = target_light->get_num_non_ambient_lights();
+    if (index >= 0 && (size_t)index < num_lights) {
+      NodePath light = target_light->get_on_light((size_t)index);
       nassertr(!light.is_empty(), &LMatrix4::ident_mat());
       Light *light_obj = light.node()->as_light();
-      nassertr(light_obj != (Light *)NULL, &LMatrix4::ident_mat());
+      nassertr(light_obj != nullptr, &LMatrix4::ident_mat());
 
-      if (!light_obj->is_ambient_light()) {
-        if (i++ == index) {
-          return fetch_specified_member(light, name, t);
-        }
-      }
-    }
+      return fetch_specified_member(light, name, t);
 
-    // Apply the default OpenGL lights otherwise.
-    // Special exception for light 0, which defaults to white.
-    if (index == 0) {
+    } else if (index == 0) {
+      // Apply the default OpenGL lights otherwise.
+      // Special exception for light 0, which defaults to white.
       string basename = name->get_basename();
       if (basename == "color" || basename == "diffuse") {
         t.set_row(3, _light_color_scale);
@@ -1776,35 +1757,28 @@ fetch_specified_texture(Shader::ShaderTexSpec &spec, SamplerState &sampler,
       const LightAttrib *target_light;
       _target_rs->get_attrib_def(target_light);
 
-      // We want to ignore ambient lights.  To that effect, iterate through
-      // the list of lights.  In the future, we will improve this system, by
-      // also filtering down to the number of lights specified by the shader.
-      int i = 0;
-
-      int num_on_lights = target_light->get_num_on_lights();
-      for (int li = 0; li < num_on_lights; li++) {
-        NodePath light = target_light->get_on_light(li);
-        nassertr(!light.is_empty(), NULL);
+      // We don't count ambient lights, which would be pretty silly to handle
+      // via this mechanism.
+      size_t num_lights = target_light->get_num_non_ambient_lights();
+      if (spec._stage >= 0 && (size_t)spec._stage < num_lights) {
+        NodePath light = target_light->get_on_light((size_t)spec._stage);
+        nassertr(!light.is_empty(), nullptr);
         Light *light_obj = light.node()->as_light();
-        nassertr(light_obj != (Light *)NULL, NULL);
-
-        if (!light_obj->is_ambient_light()) {
-          if (i++ == spec._stage) {
-            PT(Texture) tex = get_shadow_map(light);
-            if (tex != (Texture *)NULL) {
-              sampler = tex->get_default_sampler();
-            }
-            return tex;
-          }
-        }
-      }
+        nassertr(light_obj != nullptr, nullptr);
 
-      // There is no such light assigned.  Bind a dummy shadow map.
-      PT(Texture) tex = get_dummy_shadow_map((Texture::TextureType)spec._desired_type);
-      if (tex != nullptr) {
-        sampler = tex->get_default_sampler();
+        PT(Texture) tex = get_shadow_map(light);
+        if (tex != nullptr) {
+          sampler = tex->get_default_sampler();
+        }
+        return tex;
+      } else {
+        // There is no such light assigned.  Bind a dummy shadow map.
+        PT(Texture) tex = get_dummy_shadow_map((Texture::TextureType)spec._desired_type);
+        if (tex != nullptr) {
+          sampler = tex->get_default_sampler();
+        }
+        return tex;
       }
-      return tex;
     }
     break;
 
@@ -2645,7 +2619,7 @@ do_issue_light() {
   int i;
 
   int num_enabled = 0;
-  int num_on_lights = 0;
+  bool any_on_lights = false;
 
   const LightAttrib *target_light;
   _target_rs->get_attrib_def(target_light);
@@ -2654,18 +2628,16 @@ do_issue_light() {
     display_cat.spam()
       << "do_issue_light: " << target_light << "\n";
   }
-  if (target_light != (LightAttrib *)NULL) {
-    CPT(LightAttrib) new_light = target_light->filter_to_max(_max_lights);
-    if (display_cat.is_spam()) {
-      new_light->write(display_cat.spam(false), 2);
-    }
-
-    num_on_lights = new_light->get_num_on_lights();
-    for (int li = 0; li < num_on_lights; li++) {
-      NodePath light = new_light->get_on_light(li);
+  if (target_light != nullptr) {
+    // LightAttrib guarantees that the on lights are sorted, and that
+    // non-ambient lights come before ambient lights.
+    any_on_lights = target_light->has_any_on_light();
+    size_t filtered_lights = min((size_t)_max_lights, target_light->get_num_non_ambient_lights());
+    for (size_t li = 0; li < filtered_lights; ++li) {
+      NodePath light = target_light->get_on_light(li);
       nassertv(!light.is_empty());
       Light *light_obj = light.node()->as_light();
-      nassertv(light_obj != (Light *)NULL);
+      nassertv(light_obj != nullptr);
 
       // Lighting should be enabled before we apply any lights.
       if (!_lighting_enabled) {
@@ -2673,23 +2645,16 @@ do_issue_light() {
         _lighting_enabled = true;
       }
 
-      if (light_obj->is_ambient_light()) {
-        // Ambient lights don't require specific light ids; simply add in the
-        // ambient contribution to the current total
-        cur_ambient_light += light_obj->get_color();
-
-      } else {
-        const LColor &color = light_obj->get_color();
-        // Don't bother binding the light if it has no color to contribute.
-        if (color[0] != 0.0 || color[1] != 0.0 || color[2] != 0.0) {
-          enable_light(num_enabled, true);
-          if (num_enabled == 0) {
-            begin_bind_lights();
-          }
-
-          light_obj->bind(this, light, num_enabled);
-          num_enabled++;
+      const LColor &color = light_obj->get_color();
+      // Don't bother binding the light if it has no color to contribute.
+      if (color[0] != 0.0 || color[1] != 0.0 || color[2] != 0.0) {
+        enable_light(num_enabled, true);
+        if (num_enabled == 0) {
+          begin_bind_lights();
         }
+
+        light_obj->bind(this, light, num_enabled);
+        num_enabled++;
       }
     }
   }
@@ -2700,7 +2665,7 @@ do_issue_light() {
   _num_lights_enabled = num_enabled;
 
   // If no lights were set, disable lighting
-  if (num_on_lights == 0) {
+  if (!any_on_lights) {
     if (_color_scale_via_lighting && (_has_material_force_color || _light_color_scale != LVecBase4(1.0f, 1.0f, 1.0f, 1.0f))) {
       // Unless we need lighting anyway to apply a color or color scale.
       if (!_lighting_enabled) {
@@ -2717,7 +2682,7 @@ do_issue_light() {
     }
 
   } else {
-    set_ambient_light(cur_ambient_light);
+    set_ambient_light(target_light->get_ambient_contribution());
   }
 
   if (num_enabled != 0) {

panda/src/pgraph/lightAttrib.I

@@ -15,8 +15,7 @@
  * Use LightAttrib::make() to construct a new LightAttrib object.
  */
 INLINE LightAttrib::
-LightAttrib() {
-  _off_all_lights = false;
+LightAttrib() : _off_all_lights(false), _num_non_ambient_lights(0) {
 }
 
 /**
@@ -27,25 +26,37 @@ INLINE LightAttrib::
 LightAttrib(const LightAttrib &copy) :
   _on_lights(copy._on_lights),
   _off_lights(copy._off_lights),
-  _off_all_lights(copy._off_all_lights)
+  _off_all_lights(copy._off_all_lights),
+  _sort_seq(UpdateSeq::old())
 {
 }
 
 /**
  * Returns the number of lights that are turned on by the attribute.
  */
-INLINE int LightAttrib::
+INLINE size_t LightAttrib::
 get_num_on_lights() const {
-  return _on_lights.size();
+  check_sorted();
+  return _sorted_on_lights.size();
+}
+
+/**
+ * Returns the number of non-ambient lights that are turned on by this
+ * attribute.
+ */
+INLINE size_t LightAttrib::
+get_num_non_ambient_lights() const {
+  check_sorted();
+  return _num_non_ambient_lights;
 }
 
 /**
  * Returns the nth light turned on by the attribute, sorted in render order.
  */
 INLINE NodePath LightAttrib::
-get_on_light(int n) const {
-  nassertr(n >= 0 && n < (int)_on_lights.size(), NodePath::fail());
-  return _on_lights[n];
+get_on_light(size_t n) const {
+  nassertr(n < _sorted_on_lights.size(), NodePath::fail());
+  return _sorted_on_lights[n];
 }
 
 /**
@@ -57,10 +68,18 @@ has_on_light(const NodePath &light) const {
   return _on_lights.find(light) != _on_lights.end();
 }
 
+/**
+ * Returns true if any light is turned on by the attrib, false otherwise.
+ */
+INLINE bool LightAttrib::
+has_any_on_light() const {
+  return !_on_lights.empty();
+}
+
 /**
  * Returns the number of lights that are turned off by the attribute.
  */
-INLINE int LightAttrib::
+INLINE size_t LightAttrib::
 get_num_off_lights() const {
   return _off_lights.size();
 }
@@ -70,8 +89,8 @@ get_num_off_lights() const {
  * (pointer) order.
  */
 INLINE NodePath LightAttrib::
-get_off_light(int n) const {
-  nassertr(n >= 0 && n < (int)_off_lights.size(), NodePath::fail());
+get_off_light(size_t n) const {
+  nassertr(n < _off_lights.size(), NodePath::fail());
   return _off_lights[n];
 }
 
@@ -104,13 +123,11 @@ is_identity() const {
 }
 
 /**
- * Confirms whether the _filtered table is still valid.  It may become invalid
- * if someone calls Light::set_priority().
- *
- * If the table is invalid, transparently empties it before returning.
+ * Makes sure that the on lights are still sorted by priority.  It may become
+ * invalid if someone calls Light::set_priority().
  */
 INLINE void LightAttrib::
-check_filtered() const {
+check_sorted() const {
   if (_sort_seq != Light::get_sort_seq()) {
     ((LightAttrib *)this)->sort_on_lights();
   }

panda/src/pgraph/lightAttrib.cxx

@@ -29,7 +29,7 @@ int LightAttrib::_attrib_slot;
 CPT(RenderAttrib) LightAttrib::_all_off_attrib;
 TypeHandle LightAttrib::_type_handle;
 
-// This STL Function object is used in filter_to_max(), below, to sort a list
+// This STL Function object is used in sort_on_lights(), below, to sort a list
 // of Lights in reverse order by priority.  In the case of two lights with
 // equal priority, the class priority is compared.
 class CompareLightPriorities {
@@ -431,102 +431,41 @@ remove_off_light(const NodePath &light) const {
 }
 
 /**
- * Returns a new LightAttrib, very much like this one, but with the number of
- * on_lights reduced to be no more than max_lights.  The number of off_lights
- * in the new LightAttrib is undefined.
- *
- * The number of AmbientLights is not included in the count.  All
- * AmbientLights in the original attrib are always included in the result,
- * regardless of the value of max_lights.
+ * Returns the most important light (that is, the light with the highest
+ * priority) in the LightAttrib, excluding any ambient lights.  Returns an
+ * empty NodePath if no non-ambient lights are found.
  */
-CPT(LightAttrib) LightAttrib::
-filter_to_max(int max_lights) const {
-  if (max_lights < 0 || (int)_on_lights.size() <= max_lights) {
-    // Trivial case: this LightAttrib qualifies.
-    return this;
-  }
-
-  // Since check_filtered() will clear the _filtered list if we are out of
-  // date, we should call it first.
-  check_filtered();
-
-  Filtered::const_iterator fi;
-  fi = _filtered.find(max_lights);
-  if (fi != _filtered.end()) {
-    // Easy case: we have already computed this for this particular
-    // LightAttrib.
-    return (*fi).second;
-  }
-
-  // Harder case: we have to compute it now.  We must choose the n lights with
-  // the highest priority in our list of lights.
-  Lights priority_lights, ambient_lights;
-
-  // Separate the list of lights into ambient lights and other lights.
-  Lights::const_iterator li;
-  for (li = _on_lights.begin(); li != _on_lights.end(); ++li) {
-    const NodePath &np = (*li);
-    nassertr(!np.is_empty() && np.node()->as_light() != (Light *)NULL, this);
-    if (np.node()->is_ambient_light()) {
-      ambient_lights.push_back(np);
-    } else {
-      priority_lights.push_back(np);
-    }
-  }
-
-  // This sort function uses the STL function object defined above.
-  sort(priority_lights.begin(), priority_lights.end(),
-       CompareLightPriorities());
-
-  // Now lop off all of the lights after the first max_lights.
-  if ((int)priority_lights.size() > max_lights) {
-    priority_lights.erase(priority_lights.begin() + max_lights,
-                          priority_lights.end());
-  }
+NodePath LightAttrib::
+get_most_important_light() const {
+  check_sorted();
 
-  // Put the ambient lights back into the list.
-  for (li = ambient_lights.begin(); li != ambient_lights.end(); ++li) {
-    priority_lights.push_back(*li);
+  if (_num_non_ambient_lights > 0) {
+    return _sorted_on_lights[0];
+  } else {
+    return NodePath();
   }
-
-  // And re-sort the ov_set into its proper order.
-  priority_lights.sort();
-
-  // Now create a new attrib reflecting these lights.
-  PT(LightAttrib) attrib = new LightAttrib;
-  attrib->_on_lights.swap(priority_lights);
-
-  CPT(RenderAttrib) new_attrib = return_new(attrib);
-
-  // Finally, record this newly-created attrib in the map for next time.
-  CPT(LightAttrib) light_attrib = (const LightAttrib *)new_attrib.p();
-  ((LightAttrib *)this)->_filtered[max_lights] = light_attrib;
-  return light_attrib;
 }
 
 /**
- * Returns the most important light (that is, the light with the highest
- * priority) in the LightAttrib, excluding any ambient lights.  Returns an
- * empty NodePath if no non-ambient lights are found.
+ * Returns the total contribution of all the ambient lights.
  */
-NodePath LightAttrib::
-get_most_important_light() const {
-  NodePath best;
+LColor LightAttrib::
+get_ambient_contribution() const {
+  check_sorted();
 
-  CompareLightPriorities compare;
+  LVecBase4 total(0);
 
   Lights::const_iterator li;
-  for (li = _on_lights.begin(); li != _on_lights.end(); ++li) {
+  li = _sorted_on_lights.begin() + _num_non_ambient_lights;
+  for (; li != _sorted_on_lights.end(); ++li) {
     const NodePath &np = (*li);
-    nassertr(!np.is_empty() && np.node()->as_light() != (Light *)NULL, NodePath());
-    if (!np.node()->is_ambient_light()) {
-      if (best.is_empty() || compare(np, best)) {
-        best = np;
-      }
-    }
+    Light *light = np.node()->as_light();
+    nassertd(light != nullptr && light->is_ambient_light()) continue;
+
+    total += light->get_color();
   }
 
-  return best;
+  return total;
 }
 
 /**
@@ -869,13 +808,42 @@ get_auto_shader_attrib_impl(const RenderState *state) const {
 }
 
 /**
- * This is patterned after TextureAttrib::sort_on_stages(), but since lights
- * don't actually require sorting, this only empties the _filtered map.
+ * Makes sure the lights are sorted in order of priority.  Also counts the
+ * number of non-ambient lights.
  */
 void LightAttrib::
 sort_on_lights() {
   _sort_seq = Light::get_sort_seq();
-  _filtered.clear();
+
+  // Separate the list of lights into ambient lights and other lights.
+  _sorted_on_lights.clear();
+  OrderedLights ambient_lights;
+
+  Lights::const_iterator li;
+  for (li = _on_lights.begin(); li != _on_lights.end(); ++li) {
+    const NodePath &np = (*li);
+    nassertd(!np.is_empty() && np.node()->as_light() != nullptr) continue;
+
+    if (!np.node()->is_ambient_light()) {
+      _sorted_on_lights.push_back(np);
+    } else {
+      ambient_lights.push_back(np);
+    }
+  }
+
+  // Remember how many lights were non-ambient lights, which makes it easier
+  // to traverse through the list of non-ambient lights.
+  _num_non_ambient_lights = _sorted_on_lights.size();
+
+  // This sort function uses the STL function object defined above.
+  sort(_sorted_on_lights.begin(), _sorted_on_lights.end(),
+       CompareLightPriorities());
+
+  // Now insert the ambient lights back at the end.  We don't really care
+  // about their relative priorities, because their contribution will simply
+  // be summed up in the end anyway.
+  _sorted_on_lights.insert(_sorted_on_lights.end(),
+                           ambient_lights.begin(), ambient_lights.end());
 }
 
 /**
@@ -1085,4 +1053,7 @@ fillin(DatagramIterator &scan, BamReader *manager) {
     aux->_num_on_lights = scan.get_uint16();
     manager->read_pointers(scan, aux->_num_on_lights);
   }
+
+  _sorted_on_lights.clear();
+  _sort_seq = UpdateSeq::old();
 }

panda/src/pgraph/lightAttrib.h

@@ -67,13 +67,15 @@ PUBLISHED:
   static CPT(RenderAttrib) make();
   static CPT(RenderAttrib) make_all_off();
 
-  INLINE int get_num_on_lights() const;
-  INLINE NodePath get_on_light(int n) const;
+  INLINE size_t get_num_on_lights() const;
+  INLINE size_t get_num_non_ambient_lights() const;
+  INLINE NodePath get_on_light(size_t n) const;
   MAKE_SEQ(get_on_lights, get_num_on_lights, get_on_light);
   INLINE bool has_on_light(const NodePath &light) const;
+  INLINE bool has_any_on_light() const;
 
-  INLINE int get_num_off_lights() const;
-  INLINE NodePath get_off_light(int n) const;
+  INLINE size_t get_num_off_lights() const;
+  INLINE NodePath get_off_light(size_t n) const;
   MAKE_SEQ(get_off_lights, get_num_off_lights, get_off_light);
   INLINE bool has_off_light(const NodePath &light) const;
   INLINE bool has_all_off() const;
@@ -85,8 +87,11 @@ PUBLISHED:
   CPT(RenderAttrib) add_off_light(const NodePath &light) const;
   CPT(RenderAttrib) remove_off_light(const NodePath &light) const;
 
-  CPT(LightAttrib) filter_to_max(int max_lights) const;
   NodePath get_most_important_light() const;
+  LColor get_ambient_contribution() const;
+
+  MAKE_SEQ_PROPERTY(on_lights, get_num_on_lights, get_on_light);
+  MAKE_SEQ_PROPERTY(off_lights, get_num_off_lights, get_off_light);
 
 public:
   virtual void output(ostream &out) const;
@@ -100,7 +105,7 @@ protected:
   virtual CPT(RenderAttrib) get_auto_shader_attrib_impl(const RenderState *state) const;
 
 private:
-  INLINE void check_filtered() const;
+  INLINE void check_sorted() const;
   void sort_on_lights();
 
 private:
@@ -108,8 +113,11 @@ private:
   Lights _on_lights, _off_lights;
   bool _off_all_lights;
 
-  typedef pmap< int, CPT(LightAttrib) > Filtered;
-  Filtered _filtered;
+  // These are sorted in descending order of priority, with the ambient lights
+  // sorted last.
+  typedef pvector<NodePath> OrderedLights;
+  OrderedLights _sorted_on_lights;
+  size_t _num_non_ambient_lights;
 
   UpdateSeq _sort_seq;
 

panda/src/tinydisplay/tinyGraphicsStateGuardian.cxx

@@ -1734,11 +1734,7 @@ release_texture(TextureContext *tc) {
  */
 void TinyGraphicsStateGuardian::
 do_issue_light() {
-  // Initialize the current ambient light total and newly enabled light list
-  LColor cur_ambient_light(0.0f, 0.0f, 0.0f, 0.0f);
-
   int num_enabled = 0;
-  int num_on_lights = 0;
 
   const LightAttrib *target_light = DCAST(LightAttrib, _target_rs->get_attrib_def(LightAttrib::get_class_slot()));
   if (display_cat.is_spam()) {
@@ -1750,43 +1746,35 @@ do_issue_light() {
   clear_light_state();
 
   // Now, assign new lights.
-  if (target_light != (LightAttrib *)NULL) {
-    CPT(LightAttrib) new_light = target_light->filter_to_max(_max_lights);
-    if (display_cat.is_spam()) {
-      new_light->write(display_cat.spam(false), 2);
+  if (target_light != nullptr) {
+    if (target_light->has_any_on_light()) {
+      _lighting_enabled = true;
+      _c->lighting_enabled = true;
     }
 
-    num_on_lights = new_light->get_num_on_lights();
-    for (int li = 0; li < num_on_lights; li++) {
-      NodePath light = new_light->get_on_light(li);
+    size_t filtered_lights = min((size_t)_max_lights, target_light->get_num_non_ambient_lights());
+    for (size_t li = 0; li < filtered_lights; ++li) {
+      NodePath light = target_light->get_on_light(li);
       nassertv(!light.is_empty());
       Light *light_obj = light.node()->as_light();
-      nassertv(light_obj != (Light *)NULL);
-
-      _lighting_enabled = true;
-      _c->lighting_enabled = true;
-
-      if (light_obj->get_type() == AmbientLight::get_class_type()) {
-        // Accumulate all of the ambient lights together into one.
-        cur_ambient_light += light_obj->get_color();
-
-      } else {
-        // Other kinds of lights each get their own GLLight object.
-        light_obj->bind(this, light, num_enabled);
-        num_enabled++;
-
-        // Handle the diffuse color here, since all lights have this property.
-        GLLight *gl_light = _c->first_light;
-        nassertv(gl_light != NULL);
-        const LColor &diffuse = light_obj->get_color();
-        gl_light->diffuse.v[0] = diffuse[0];
-        gl_light->diffuse.v[1] = diffuse[1];
-        gl_light->diffuse.v[2] = diffuse[2];
-        gl_light->diffuse.v[3] = diffuse[3];
-      }
+      nassertv(light_obj != nullptr);
+
+      // Other kinds of lights each get their own GLLight object.
+      light_obj->bind(this, light, num_enabled);
+      num_enabled++;
+
+      // Handle the diffuse color here, since all lights have this property.
+      GLLight *gl_light = _c->first_light;
+      nassertv(gl_light != NULL);
+      const LColor &diffuse = light_obj->get_color();
+      gl_light->diffuse.v[0] = diffuse[0];
+      gl_light->diffuse.v[1] = diffuse[1];
+      gl_light->diffuse.v[2] = diffuse[2];
+      gl_light->diffuse.v[3] = diffuse[3];
     }
   }
 
+  LColor cur_ambient_light = target_light->get_ambient_contribution();
   _c->ambient_light_model.v[0] = cur_ambient_light[0];
   _c->ambient_light_model.v[1] = cur_ambient_light[1];
   _c->ambient_light_model.v[2] = cur_ambient_light[2];