invalid collision handler shouldn't be fatal

panda/src/collide/collisionHandler.cxx

@@ -50,7 +50,13 @@ add_entry(CollisionEntry *) {
 //               all collision detections for this traversal.  It
 //               should do whatever finalization is required for the
 //               handler.
+//
+//               The return value is normally true, but if this
+//               returns value, the CollisionTraverser will remove the
+//               handler from its list, allowing the CollisionHandler
+//               itself to determine when it is no longer needed.
 ////////////////////////////////////////////////////////////////////
-void CollisionHandler::
+bool CollisionHandler::
 end_group() {
+  return true;
 }

panda/src/collide/collisionHandler.h

@@ -37,7 +37,7 @@ class EXPCL_PANDA CollisionHandler : public TypedReferenceCount {
 public:
   virtual void begin_group();
   virtual void add_entry(CollisionEntry *entry);
-  virtual void end_group();
+  virtual bool end_group();
 
 
 PUBLISHED:

panda/src/collide/collisionHandlerEvent.cxx

@@ -85,7 +85,7 @@ add_entry(CollisionEntry *entry) {
 //               should do whatever finalization is required for the
 //               handler.
 ////////////////////////////////////////////////////////////////////
-void CollisionHandlerEvent::
+bool CollisionHandlerEvent::
 end_group() {
   // Now compare the list of entries we collected this frame with
   // those we kept from the last time.  Each new entry represents a
@@ -139,6 +139,8 @@ end_group() {
     throw_event_pattern(_out_pattern, *cb);
     ++cb;
   }
+
+  return true;
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/collide/collisionHandlerEvent.h

@@ -43,7 +43,7 @@ PUBLISHED:
 public:
   virtual void begin_group();
   virtual void add_entry(CollisionEntry *entry);
-  virtual void end_group();
+  virtual bool end_group();
 
 PUBLISHED:
   INLINE void set_in_pattern(const string &pattern);

panda/src/collide/collisionHandlerFloor.cxx

@@ -53,77 +53,93 @@ CollisionHandlerFloor::
 //  Description: Called by the parent class after all collisions have
 //               been detected, this manages the various collisions
 //               and moves around the nodes as necessary.
+//
+//               The return value is normally true, but it may be
+//               false to indicate the CollisionTraverser should
+//               disable this handler from being called in the future.
 ////////////////////////////////////////////////////////////////////
-void CollisionHandlerFloor::
+bool CollisionHandlerFloor::
 handle_entries() {
+  bool okflag = true;
+
   FromEntries::const_iterator fi;
   for (fi = _from_entries.begin(); fi != _from_entries.end(); ++fi) {
     CollisionNode *from_node = (*fi).first;
-    nassertv(from_node != (CollisionNode *)NULL);
+    nassertr(from_node != (CollisionNode *)NULL, false);
     const Entries &entries = (*fi).second;
 
-    Colliders::const_iterator ci;
+    Colliders::iterator ci;
     ci = _colliders.find(from_node);
     if (ci == _colliders.end()) {
       // Hmm, someone added a CollisionNode to a traverser and gave
       // it this CollisionHandler pointer--but they didn't tell us
       // about the node.
       collide_cat.error()
-        << "CollisionHandlerFloor doesn't know about "
-        << *from_node << "\n";
+        << get_type() << " doesn't know about "
+        << *from_node << ", disabling.\n";
+      okflag = false;
 
     } else {
-      const ColliderDef &def = (*ci).second;
-
-      // Get the maximum height for all collisions with this node.
-      bool got_max = false;
-      float max_height = 0.0;
+      ColliderDef &def = (*ci).second;
+      if (!def.is_valid()) {
+        collide_cat.error()
+          << "Removing invalid collider " << *from_node << " from "
+          << get_type() << "\n";
+        _colliders.erase(ci);
 
-      Entries::const_iterator ei;
-      for (ei = entries.begin(); ei != entries.end(); ++ei) {
-        CollisionEntry *entry = (*ei);
-        nassertv(entry != (CollisionEntry *)NULL);
-        nassertv(from_node == entry->get_from_node());
-
-        if (entry->has_from_intersection_point()) {
-          LPoint3f point = entry->get_from_intersection_point();
+      } else {
+        // Get the maximum height for all collisions with this node.
+        bool got_max = false;
+        float max_height = 0.0;
+        
+        Entries::const_iterator ei;
+        for (ei = entries.begin(); ei != entries.end(); ++ei) {
+          CollisionEntry *entry = (*ei);
+          nassertr(entry != (CollisionEntry *)NULL, false);
+          nassertr(from_node == entry->get_from_node(), false);
+          
+          if (entry->has_from_intersection_point()) {
+            LPoint3f point = entry->get_from_intersection_point();
+            if (collide_cat.is_debug()) {
+              collide_cat.debug()
+                << "Intersection point detected at " << point << "\n";
+            }
+            
+            float height = point[2];
+            if (!got_max || height > max_height) {
+              got_max = true;
+              max_height = height;
+            }
+          }
+        }
+        
+        // Now set our height accordingly.
+        float adjust = max_height + _offset;
+        if (!IS_THRESHOLD_ZERO(adjust, 0.001)) {
           if (collide_cat.is_debug()) {
             collide_cat.debug()
-              << "Intersection point detected at " << point << "\n";
+              << "Adjusting height by " << adjust << "\n";
           }
-
-          float height = point[2];
-          if (!got_max || height > max_height) {
-            got_max = true;
-            max_height = height;
+          
+          if (adjust < 0.0 && _max_velocity != 0.0) {
+            float max_adjust =
+              _max_velocity * ClockObject::get_global_clock()->get_dt();
+            adjust = max(adjust, -max_adjust);
+          }
+          
+          LMatrix4f mat;
+          def.get_mat(mat);
+          mat(3, 2) += adjust;
+          def.set_mat(mat);
+        } else {
+          if (collide_cat.is_spam()) {
+            collide_cat.spam()
+              << "Leaving height unchanged.\n";
           }
-        }
-      }
-
-      // Now set our height accordingly.
-      float adjust = max_height + _offset;
-      if (!IS_THRESHOLD_ZERO(adjust, 0.001)) {
-        if (collide_cat.is_debug()) {
-          collide_cat.debug()
-            << "Adjusting height by " << adjust << "\n";
-        }
-
-        if (adjust < 0.0 && _max_velocity != 0.0) {
-          float max_adjust =
-            _max_velocity * ClockObject::get_global_clock()->get_dt();
-          adjust = max(adjust, -max_adjust);
-        }
-
-        LMatrix4f mat;
-        def.get_mat(mat);
-        mat(3, 2) += adjust;
-        def.set_mat(mat);
-      } else {
-        if (collide_cat.is_spam()) {
-          collide_cat.spam()
-            << "Leaving height unchanged.\n";
         }
       }
     }
   }
+
+  return okflag;
 }

panda/src/collide/collisionHandlerFloor.h

@@ -45,7 +45,7 @@ PUBLISHED:
   INLINE float get_max_velocity() const;
 
 protected:
-  virtual void handle_entries();
+  virtual bool handle_entries();
 
 private:
   float _offset;

panda/src/collide/collisionHandlerPhysical.I

@@ -37,3 +37,15 @@ set_arc(NodeRelation *arc) {
   _arc = arc;
   _drive_interface.clear();
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: CollisionHandlerPhysical::ColliderDef::is_valid
+//       Access: Public
+//  Description: Returns true if this ColliderDef is still valid;
+//               e.g. it refers to a valid arc or drive interface.
+////////////////////////////////////////////////////////////////////
+INLINE bool CollisionHandlerPhysical::ColliderDef::
+is_valid() const {
+  return (_arc != (NodeRelation *)NULL) ||
+    (_drive_interface != (DriveInterface *)NULL);
+}

panda/src/collide/collisionHandlerPhysical.cxx

@@ -58,9 +58,15 @@ get_mat(LMatrix4f &mat) const {
 //               indicated by the given transform.
 ////////////////////////////////////////////////////////////////////
 void CollisionHandlerPhysical::ColliderDef::
-set_mat(const LMatrix4f &mat) const {
+set_mat(const LMatrix4f &mat) {
   if (_arc != (NodeRelation *)NULL) {
-    _arc->set_transition(new TransformTransition(mat));
+    if (!_arc->is_attached()) {
+      collide_cat.error()
+        << "CollisionHandler is associated with an unattached arc.\n";
+      _arc = (NodeRelation *)NULL;
+    } else {
+      _arc->set_transition(new TransformTransition(mat));
+    }
 
   } else if (_drive_interface != (DriveInterface *)NULL) {
     _drive_interface->set_mat(mat);
@@ -129,11 +135,11 @@ add_entry(CollisionEntry *entry) {
 //               should do whatever finalization is required for the
 //               handler.
 ////////////////////////////////////////////////////////////////////
-void CollisionHandlerPhysical::
+bool CollisionHandlerPhysical::
 end_group() {
   CollisionHandlerEvent::end_group();
 
-  handle_entries();
+  return handle_entries();
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/collide/collisionHandlerPhysical.h

@@ -42,7 +42,7 @@ public:
 
   virtual void begin_group();
   virtual void add_entry(CollisionEntry *entry);
-  virtual void end_group();
+  virtual bool end_group();
 
 PUBLISHED:
   void add_collider(CollisionNode *node, DriveInterface *drive_interface);
@@ -52,7 +52,7 @@ PUBLISHED:
   void clear_colliders();
 
 protected:
-  virtual void handle_entries()=0;
+  virtual bool handle_entries()=0;
 
 protected:
   typedef pvector< PT(CollisionEntry) > Entries;
@@ -63,9 +63,10 @@ protected:
   public:
     INLINE void set_drive_interface(DriveInterface *drive_interface);
     INLINE void set_arc(NodeRelation *arc);
+    INLINE bool is_valid() const;
 
     void get_mat(LMatrix4f &mat) const;
-    void set_mat(const LMatrix4f &mat) const;
+    void set_mat(const LMatrix4f &mat);
 
     PT(DriveInterface) _drive_interface;
     PT_NodeRelation _arc;

panda/src/collide/collisionHandlerPusher.cxx

@@ -62,16 +62,22 @@ CollisionHandlerPusher::
 //  Description: Called by the parent class after all collisions have
 //               been detected, this manages the various collisions
 //               and moves around the nodes as necessary.
+//
+//               The return value is normally true, but it may be
+//               false to indicate the CollisionTraverser should
+//               disable this handler from being called in the future.
 ////////////////////////////////////////////////////////////////////
-void CollisionHandlerPusher::
+bool CollisionHandlerPusher::
 handle_entries() {
+  bool okflag = true;
+
   FromEntries::const_iterator fi;
   for (fi = _from_entries.begin(); fi != _from_entries.end(); ++fi) {
     CollisionNode *from_node = (*fi).first;
-    nassertv(from_node != (CollisionNode *)NULL);
+    nassertr(from_node != (CollisionNode *)NULL, false);
     const Entries &entries = (*fi).second;
 
-    Colliders::const_iterator ci;
+    Colliders::iterator ci;
     ci = _colliders.find(from_node);
     if (ci == _colliders.end()) {
       // Hmm, someone added a CollisionNode to a traverser and gave
@@ -79,113 +85,123 @@ handle_entries() {
       // about the node.
       collide_cat.error()
         << "CollisionHandlerPusher doesn't know about "
-        << *from_node << "\n";
+        << *from_node << ", disabling.\n";
+      okflag = false;
 
     } else {
-      const ColliderDef &def = (*ci).second;
-
-      // How to apply multiple shoves from different solids onto the
-      // same collider?  One's first intuition is to vector sum all
-      // the shoves.  However, this causes problems when two parallel
-      // walls shove on the collider, because we end up with a double
-      // shove.  We hack around this by testing if two shove vectors
-      // share nearly the same direction, and if so, we keep only the
-      // longer of the two.
-
-      typedef pvector<ShoveData> Shoves;
-      Shoves shoves;
-
-      Entries::const_iterator ei;
-      for (ei = entries.begin(); ei != entries.end(); ++ei) {
-        CollisionEntry *entry = (*ei);
-        nassertv(entry != (CollisionEntry *)NULL);
-        nassertv(from_node == entry->get_from_node());
-
-        if (!entry->has_into_surface_normal() ||
-            !entry->has_into_depth()) {
-          if (collide_cat.is_debug()) {
-            collide_cat.debug()
-              << "Cannot shove on " << *from_node << " for collision into "
-              << *entry->get_into_node() << "; no normal/depth information.\n";
-          }
-
-        } else {
-          // Shove it just enough to clear the volume.
-          if (entry->get_into_depth() != 0.0) {
-            ShoveData sd;
-            sd._shove =
-              entry->get_into_surface_normal() *
-              entry->get_into_depth();
-
+      ColliderDef &def = (*ci).second;
+      if (!def.is_valid()) {
+        collide_cat.error()
+          << "Removing invalid collider " << *from_node << " from "
+          << get_type() << "\n";
+        _colliders.erase(ci);
+
+      } else {
+        // How to apply multiple shoves from different solids onto the
+        // same collider?  One's first intuition is to vector sum all
+        // the shoves.  However, this causes problems when two parallel
+        // walls shove on the collider, because we end up with a double
+        // shove.  We hack around this by testing if two shove vectors
+        // share nearly the same direction, and if so, we keep only the
+        // longer of the two.
+        
+        typedef pvector<ShoveData> Shoves;
+        Shoves shoves;
+        
+        Entries::const_iterator ei;
+        for (ei = entries.begin(); ei != entries.end(); ++ei) {
+          CollisionEntry *entry = (*ei);
+          nassertr(entry != (CollisionEntry *)NULL, false);
+          nassertr(from_node == entry->get_from_node(), false);
+          
+          if (!entry->has_into_surface_normal() ||
+              !entry->has_into_depth()) {
             if (collide_cat.is_debug()) {
               collide_cat.debug()
-                << "Shove on " << *from_node << " from "
-                << *entry->get_into_node() << ": " << sd._shove << "\n";
+                << "Cannot shove on " << *from_node << " for collision into "
+                << *entry->get_into_node() << "; no normal/depth information.\n";
             }
-
-            sd._length = sd._shove.length();
-            sd._normalized_shove = sd._shove / sd._length;
-            sd._valid = true;
-
-            shoves.push_back(sd);
-          }
-        }
-      }
-
-      if (!shoves.empty()) {
-        // Now we combine any two shoves that shove in largely the
-        // same direction.  Hacky.
-
-        Shoves::iterator si;
-        for (si = shoves.begin(); si != shoves.end(); ++si) {
-          ShoveData &sd = (*si);
-          Shoves::iterator sj;
-          for (sj = shoves.begin(); sj != si; ++sj) {
-            ShoveData &sd2 = (*sj);
-            if (sd2._valid) {
-
-              float d = sd._normalized_shove.dot(sd2._normalized_shove);
+            
+          } else {
+            // Shove it just enough to clear the volume.
+            if (entry->get_into_depth() != 0.0) {
+              ShoveData sd;
+              sd._shove =
+                entry->get_into_surface_normal() *
+                entry->get_into_depth();
+              
               if (collide_cat.is_debug()) {
                 collide_cat.debug()
-                  << "Considering dot product " << d << "\n";
+                  << "Shove on " << *from_node << " from "
+                  << *entry->get_into_node() << ": " << sd._shove << "\n";
               }
-
-              if (d > 0.9) {
-                // These two shoves are largely in the same direction;
-                // save the larger of the two.
-                if (sd2._length < sd._length) {
-                  sd2._valid = false;
-                } else {
-                  sd._valid = false;
+              
+              sd._length = sd._shove.length();
+              sd._normalized_shove = sd._shove / sd._length;
+              sd._valid = true;
+              
+              shoves.push_back(sd);
+            }
+          }
+        }
+        
+        if (!shoves.empty()) {
+          // Now we combine any two shoves that shove in largely the
+          // same direction.  Hacky.
+          
+          Shoves::iterator si;
+          for (si = shoves.begin(); si != shoves.end(); ++si) {
+            ShoveData &sd = (*si);
+            Shoves::iterator sj;
+            for (sj = shoves.begin(); sj != si; ++sj) {
+              ShoveData &sd2 = (*sj);
+              if (sd2._valid) {
+                
+                float d = sd._normalized_shove.dot(sd2._normalized_shove);
+                if (collide_cat.is_debug()) {
+                  collide_cat.debug()
+                    << "Considering dot product " << d << "\n";
+                }
+                
+                if (d > 0.9) {
+                  // These two shoves are largely in the same direction;
+                  // save the larger of the two.
+                  if (sd2._length < sd._length) {
+                    sd2._valid = false;
+                  } else {
+                    sd._valid = false;
+                  }
                 }
               }
             }
           }
-        }
-
-        // Now we can determine the net shove.
-        LVector3f net_shove(0.0, 0.0, 0.0);
-        for (si = shoves.begin(); si != shoves.end(); ++si) {
-          const ShoveData &sd = (*si);
-          if (sd._valid) {
-            net_shove += sd._shove;
+          
+          // Now we can determine the net shove.
+          LVector3f net_shove(0.0, 0.0, 0.0);
+          for (si = shoves.begin(); si != shoves.end(); ++si) {
+            const ShoveData &sd = (*si);
+            if (sd._valid) {
+              net_shove += sd._shove;
+            }
           }
+          
+          if (_horizontal) {
+            net_shove[2] = 0.0;
+          }
+          
+          if (collide_cat.is_debug()) {
+            collide_cat.debug()
+              << "Net shove on " << *from_node << " is: "
+              << net_shove << "\n";
+          }
+          
+          LMatrix4f mat;
+          def.get_mat(mat);
+          def.set_mat(LMatrix4f::translate_mat(net_shove) * mat);
         }
-
-        if (_horizontal) {
-          net_shove[2] = 0.0;
-        }
-
-        if (collide_cat.is_debug()) {
-          collide_cat.debug()
-            << "Net shove on " << *from_node << " is: "
-            << net_shove << "\n";
-        }
-
-        LMatrix4f mat;
-        def.get_mat(mat);
-        def.set_mat(LMatrix4f::translate_mat(net_shove) * mat);
       }
     }
   }
+
+  return okflag;
 }

panda/src/collide/collisionHandlerPusher.h

@@ -40,7 +40,7 @@ PUBLISHED:
   INLINE bool get_horizontal() const;
 
 protected:
-  virtual void handle_entries();
+  virtual bool handle_entries();
 
 private:
   bool _horizontal;

panda/src/collide/collisionTraverser.cxx

@@ -257,8 +257,17 @@ traverse(const NodePath &root) {
   df_traverse(root.node(), *this, NullTransitionWrapper(),
               level_state, _graph_type);
 
-  for (hi = _handlers.begin(); hi != _handlers.end(); ++hi) {
-    (*hi).first->end_group();
+  hi = _handlers.begin();
+  while (hi != _handlers.end()) {
+    if (!(*hi).first->end_group()) {
+      // This handler wants to remove itself from the traversal list.
+      Handlers::iterator hnext = hi;
+      ++hnext;
+      _handlers.erase(hi);
+      hi = hnext;
+    } else {
+      ++hi;
+    }
   }
 }