fix nodepath collapsing problems

panda/src/pgraph/pandaNode.cxx

@@ -431,21 +431,6 @@ make_copy() const {
   return new PandaNode(*this);
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: PandaNode::copy_subgraph
-//       Access: Public
-//  Description: Allocates and returns a complete copy of this
-//               PandaNode and the entire scene graph rooted at this
-//               PandaNode.  Some data may still be shared from the
-//               original (e.g. vertex index tables), but nothing that
-//               will impede normal use of the PandaNode.
-////////////////////////////////////////////////////////////////////
-PT(PandaNode) PandaNode::
-copy_subgraph() const {
-  InstanceMap inst_map;
-  return r_copy_subgraph(inst_map);
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: PandaNode::safe_to_flatten
 //       Access: Public, Virtual
@@ -646,6 +631,21 @@ get_next_visible_child(int n) const {
   return n + 1;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: PandaNode::copy_subgraph
+//       Access: Published
+//  Description: Allocates and returns a complete copy of this
+//               PandaNode and the entire scene graph rooted at this
+//               PandaNode.  Some data may still be shared from the
+//               original (e.g. vertex index tables), but nothing that
+//               will impede normal use of the PandaNode.
+////////////////////////////////////////////////////////////////////
+PT(PandaNode) PandaNode::
+copy_subgraph() const {
+  InstanceMap inst_map;
+  return r_copy_subgraph(inst_map);
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: PandaNode::find_child
 //       Access: Published
@@ -1384,28 +1384,9 @@ detach(qpNodePathComponent *child) {
   // sort of thing.
   nassertv(child_node->find_parent(parent_node) >= 0);
   
-  // Break the qpNodePathComponent connection.
-  child->set_top_node();
-  
   CDWriter cdata_child(child_node->_cycler);
   CDWriter cdata_parent(parent_node->_cycler);
   
-  // Any other components in the same child_node that previously
-  // referenced the same parent has now become invalid and must be
-  // collapsed into this one and removed from the paths set.
-  Paths::iterator pi;
-  pi = cdata_child->_paths.begin();
-  while (pi != cdata_child->_paths.end()) {
-    Paths::iterator pnext = pi;
-    ++pnext;
-    if ((*pi) != child && !(*pi)->is_top_node() && 
-        (*pi)->get_next()->get_node() == parent_node) {
-      (*pi)->collapse_with(child);
-      cdata_child->_paths.erase(pi);
-    }
-    pi = pnext;
-  }
-  
   // Now look for the child and break the actual connection.
   
   // First, look for and remove the parent node from the child's up
@@ -1413,8 +1394,6 @@ detach(qpNodePathComponent *child) {
   int num_erased = cdata_child->_up.erase(UpConnection(parent_node));
   nassertv(num_erased == 1);
   
-  child_node->fix_path_lengths(cdata_child);
-  
   // Now, look for and remove the child node from the parent's down
   // list.  We also check in the stashed list, in case the child node
   // has been stashed.
@@ -1438,6 +1417,9 @@ detach(qpNodePathComponent *child) {
   }
   nassertv(found);
 
+  // Finally, break the NodePathComponent connection.
+  sever_connection(parent_node, child_node);
+
   // Mark the bounding volumes stale.
   parent_node->force_bound_stale();
 
@@ -1581,13 +1563,15 @@ get_top_component(PandaNode *child_node, bool force) {
 //     Function: PandaNode::get_generic_component
 //       Access: Private
 //  Description: Returns a qpNodePathComponent referencing this node as
-//               a path from the root.  It is only valid to call this
-//               if there is an unambiguous path from the root;
-//               otherwise, a warning will be issued and one path will
-//               be chosen arbitrarily.
+//               a path from the root.  
+//
+//               Unless accept_ambiguity is true, it is only valid to
+//               call this if there is an unambiguous path from the
+//               root; otherwise, a warning will be issued and one
+//               path will be chosen arbitrarily.
 ////////////////////////////////////////////////////////////////////
 PT(qpNodePathComponent) PandaNode::
-get_generic_component() {
+get_generic_component(bool accept_ambiguity) {
   int num_parents = get_num_parents();
   if (num_parents == 0) {
     // No parents; no ambiguity.  This is the root.
@@ -1597,18 +1581,22 @@ get_generic_component() {
   PT(qpNodePathComponent) result;
   if (num_parents == 1) {
     // Only one parent; no ambiguity.
-    PT(qpNodePathComponent) parent = get_parent(0)->get_generic_component();
+    PT(qpNodePathComponent) parent = 
+      get_parent(0)->get_generic_component(accept_ambiguity);
     result = get_component(parent, this);
 
   } else {
     // Oops, multiple parents; the NodePath is ambiguous.
-    pgraph_cat.warning()
-      << *this << " has " << num_parents
-      << " parents; choosing arbitrary path to root.\n";
+    if (!accept_ambiguity) {
+      pgraph_cat.warning()
+        << *this << " has " << num_parents
+        << " parents; choosing arbitrary path to root.\n";
+    }
   
-    PT(qpNodePathComponent) parent = get_parent(0)->get_generic_component();
+    PT(qpNodePathComponent) parent = 
+      get_parent(0)->get_generic_component(accept_ambiguity);
     result = get_component(parent, this);
-    nassertr(!unambiguous_graph, result);
+    nassertr(accept_ambiguity || !unambiguous_graph, result);
   }
 
   return result;
@@ -1658,7 +1646,7 @@ delete_component(qpNodePathComponent *component) {
 void PandaNode::
 sever_connection(PandaNode *parent_node, PandaNode *child_node) {
   CDWriter cdata_child(child_node->_cycler);
-  qpNodePathComponent *collapsed = (qpNodePathComponent *)NULL;
+  PT(qpNodePathComponent) collapsed = (qpNodePathComponent *)NULL;
 
   Paths::iterator pi;
   pi = cdata_child->_paths.begin();
@@ -1679,10 +1667,11 @@ sever_connection(PandaNode *parent_node, PandaNode *child_node) {
           
         } else {
           // If the node still has one parent, all of its paths that
-          // referenced the old parent will be combined with the remaining
-          // parent.  If there are multiple parents, choose one arbitrarily
-          // to combine with.
-          collapsed = child_node->get_generic_component();
+          // referenced the old parent will be combined with the
+          // remaining parent.  If there are multiple parents, choose
+          // the first parent arbitrarily to combine with, and don't
+          // complain if there's ambiguity.
+          collapsed = child_node->get_generic_component(true);
         }
 
         if (collapsed == (qpNodePathComponent *)NULL) {
@@ -1732,7 +1721,7 @@ new_connection(PandaNode *parent_node, PandaNode *child_node) {
        pi != cdata_child->_paths.end();
        ++pi) {
     if ((*pi)->is_top_node()) {
-      (*pi)->set_next(parent_node->get_generic_component());
+      (*pi)->set_next(parent_node->get_generic_component(false));
     }
   }
   child_node->fix_path_lengths(cdata_child);

panda/src/pgraph/pandaNode.h

@@ -64,7 +64,6 @@ private:
 
 public:
   virtual PandaNode *make_copy() const;
-  PT(PandaNode) copy_subgraph() const;
 
   virtual bool safe_to_flatten() const;
   virtual bool safe_to_transform() const;
@@ -80,6 +79,8 @@ public:
   virtual int get_next_visible_child(int n) const;
 
 PUBLISHED:
+  PT(PandaNode) copy_subgraph() const;
+
   INLINE int get_num_parents() const;
   INLINE PandaNode *get_parent(int n) const;
   INLINE int find_parent(PandaNode *node) const;
@@ -198,7 +199,7 @@ private:
                                               PandaNode *child);
   static PT(qpNodePathComponent) get_top_component(PandaNode *child,
                                                    bool force);
-  PT(qpNodePathComponent) get_generic_component();
+  PT(qpNodePathComponent) get_generic_component(bool accept_ambiguity);
   void delete_component(qpNodePathComponent *component);
   static void sever_connection(PandaNode *parent_node, PandaNode *child_node);
   static void new_connection(PandaNode *parent_node, PandaNode *child_node);

panda/src/pgraph/qpnodePath.I

@@ -44,7 +44,7 @@ qpNodePath(const string &top_node_name) :
   _error_type(ET_ok)
 {
   PandaNode *top_node = new PandaNode(top_node_name);
-  _head = top_node->get_generic_component();
+  _head = top_node->get_generic_component(false);
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -62,7 +62,7 @@ qpNodePath(PandaNode *top_node) :
   _error_type(ET_ok)
 {
   if (top_node != (PandaNode *)NULL) {
-    _head = top_node->get_generic_component();
+    _head = top_node->get_generic_component(false);
   }
 }
 
@@ -232,7 +232,7 @@ get_error_type() const {
 ////////////////////////////////////////////////////////////////////
 INLINE PandaNode *qpNodePath::
 node() const {
-  nassertr(!is_empty(), (PandaNode *)NULL);
+  nassertr_always(!is_empty(), (PandaNode *)NULL);
   return _head->get_node();
 }
 

panda/src/pgraph/qpnodePath.cxx

@@ -207,6 +207,7 @@ find_all_paths_to(PandaNode *node) const {
 ////////////////////////////////////////////////////////////////////
 void qpNodePath::
 reparent_to(const qpNodePath &other, int sort) {
+  nassertv(verify_complete());
   nassertv(other.verify_complete());
   nassertv_always(!is_empty());
   nassertv_always(!other.is_empty());
@@ -227,6 +228,7 @@ reparent_to(const qpNodePath &other, int sort) {
 ////////////////////////////////////////////////////////////////////
 void qpNodePath::
 wrt_reparent_to(const qpNodePath &other, int sort) {
+  nassertv(verify_complete());
   nassertv(other.verify_complete());
   nassertv_always(!is_empty());
   nassertv_always(!other.is_empty());
@@ -255,6 +257,7 @@ wrt_reparent_to(const qpNodePath &other, int sort) {
 qpNodePath qpNodePath::
 instance_to(const qpNodePath &other, int sort) const {
   nassertr(verify_complete(), qpNodePath::fail());
+  nassertr(other.verify_complete(), qpNodePath::fail());
   nassertr_always(!is_empty(), qpNodePath::fail());
   nassertr(!other.is_empty(), qpNodePath::fail());
 
@@ -279,7 +282,11 @@ instance_to(const qpNodePath &other, int sort) const {
 qpNodePath qpNodePath::
 instance_under_node(const qpNodePath &other, const string &name, int sort) const {
   qpNodePath new_node = other.attach_new_node(name, sort);
-  instance_to(new_node);
+  qpNodePath instance = instance_to(new_node);
+  if (instance.is_empty()) {
+    new_node.remove_node();
+    return instance;
+  }
   return new_node;
 }
 
@@ -295,6 +302,7 @@ instance_under_node(const qpNodePath &other, const string &name, int sort) const
 qpNodePath qpNodePath::
 copy_to(const qpNodePath &other, int sort) const {
   nassertr(verify_complete(), fail());
+  nassertr(other.verify_complete(), fail());
   nassertr_always(!is_empty(), fail());
   nassertr(!other.is_empty(), fail());
 
@@ -2326,10 +2334,17 @@ verify_complete() const {
     nassertr(next_node != (const PandaNode *)NULL, false);
 
     if (node->find_parent(next_node) < 0) {
+      pgraph_cat.warning()
+        << *this << " is incomplete; " << *node << " is not a child of "
+        << *next_node << "\n";
       return false;
     }
 
     if (comp->get_length() != length) {
+      pgraph_cat.warning()
+        << *this << " is incomplete; length at " << *next_node
+        << " indicates " << comp->get_length() << " while length at "
+        << *node << " indicates " << length << "\n";
       return false;
     }
 
@@ -2339,7 +2354,14 @@ verify_complete() const {
   }
 
   // Now that we've reached the top, we should have no parents.
-  return length == 0 && node->get_num_parents() == 0;
+  if (length == 0 && node->get_num_parents() == 0) {
+    return true;
+  }
+
+  pgraph_cat.warning()
+    << *this << " is incomplete; top node " << *node << " indicates length "
+    << length << " with " << node->get_num_parents() << " parents.\n";
+  return false;
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/pgraph/qpnodePathComponent.I

@@ -101,43 +101,13 @@ INLINE qpNodePathComponent::
 ////////////////////////////////////////////////////////////////////
 INLINE PandaNode *qpNodePathComponent::
 get_node() const {
+  // We don't have to bother checking if the component has been
+  // collapsed here, since the _node pointer will still be the same
+  // even if it has.
   nassertr(_node != (PandaNode *)NULL, _node);
   return _node;
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: qpNodePathComponent::get_key
-//       Access: Public
-//  Description: Returns an index number that is guaranteed to be
-//               unique for this particular NodePathComponent, and not
-//               to be reused for the lifetime of the application
-//               (barring integer overflow).
-////////////////////////////////////////////////////////////////////
-INLINE int qpNodePathComponent::
-get_key() const {
-  if (_key == 0) {
-    // The first time someone asks for a particular component's key,
-    // we make it up on the spot.  This helps keep us from wasting
-    // index numbers generating a unique number for *every* component
-    // in the world (we only have 4.2 billion 32-bit integers, after
-    // all)
-    ((qpNodePathComponent *)this)->_key = _next_key++;
-  }
-  return _key;
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: qpNodePathComponent::is_top_node
-//       Access: Public
-//  Description: Returns true if this component represents the top
-//               node in the path.
-////////////////////////////////////////////////////////////////////
-INLINE bool qpNodePathComponent::
-is_top_node() const {
-  CDReader cdata(_cycler);
-  return (cdata->_next == (qpNodePathComponent *)NULL);
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: qpNodePathComponent::is_collapsed
 //       Access: Public
@@ -152,18 +122,6 @@ is_collapsed() const {
   return (cdata->_length == 0);
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: qpNodePathComponent::get_length
-//       Access: Public
-//  Description: Returns the length of the path.
-////////////////////////////////////////////////////////////////////
-INLINE int qpNodePathComponent::
-get_length() const {
-  nassertr(!is_collapsed(), 0);
-  CDReader cdata(_cycler);
-  return cdata->_length;
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: qpNodePathComponent::get_collapsed
 //       Access: Public
@@ -178,32 +136,6 @@ get_collapsed() const {
   return cdata->_next;
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: qpNodePathComponent::set_next
-//       Access: Private
-//  Description: Sets the next pointer in the path.
-////////////////////////////////////////////////////////////////////
-INLINE void qpNodePathComponent::
-set_next(qpNodePathComponent *next) {
-  nassertv(!is_collapsed());
-  nassertv(next != (qpNodePathComponent *)NULL);
-  CDWriter cdata(_cycler);
-  cdata->_next = next;
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: qpNodePathComponent::set_top_node
-//       Access: Private
-//  Description: Severs any connection to the next pointer in the
-//               path and makes this component a top node.
-////////////////////////////////////////////////////////////////////
-INLINE void qpNodePathComponent::
-set_top_node() {
-  nassertv(!is_collapsed());
-  CDWriter cdata(_cycler);
-  cdata->_next = (qpNodePathComponent *)NULL;
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: qpNodePathComponent::collapse_with
 //       Access: Private

panda/src/pgraph/qpnodePathComponent.cxx

@@ -35,6 +35,59 @@ make_copy() const {
   return new CData(*this);
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: qpNodePathComponent::get_key
+//       Access: Public
+//  Description: Returns an index number that is guaranteed to be
+//               unique for this particular NodePathComponent, and not
+//               to be reused for the lifetime of the application
+//               (barring integer overflow).
+////////////////////////////////////////////////////////////////////
+int qpNodePathComponent::
+get_key() const {
+  if (is_collapsed()) {
+    return get_collapsed()->get_key();
+  }
+  if (_key == 0) {
+    // The first time someone asks for a particular component's key,
+    // we make it up on the spot.  This helps keep us from wasting
+    // index numbers generating a unique number for *every* component
+    // in the world (we only have 4.2 billion 32-bit integers, after
+    // all)
+    ((qpNodePathComponent *)this)->_key = _next_key++;
+  }
+  return _key;
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: qpNodePathComponent::is_top_node
+//       Access: Public
+//  Description: Returns true if this component represents the top
+//               node in the path.
+////////////////////////////////////////////////////////////////////
+bool qpNodePathComponent::
+is_top_node() const {
+  if (is_collapsed()) {
+    return get_collapsed()->is_top_node();
+  }
+  CDReader cdata(_cycler);
+  return (cdata->_next == (qpNodePathComponent *)NULL);
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: qpNodePathComponent::get_length
+//       Access: Public
+//  Description: Returns the length of the path to this node.
+////////////////////////////////////////////////////////////////////
+int qpNodePathComponent::
+get_length() const {
+  if (is_collapsed()) {
+    return get_collapsed()->get_length();
+  }
+  CDReader cdata(_cycler);
+  return cdata->_length;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: qpNodePathComponent::get_next
 //       Access: Public
@@ -42,20 +95,24 @@ make_copy() const {
 ////////////////////////////////////////////////////////////////////
 qpNodePathComponent *qpNodePathComponent::
 get_next() const {
-  CDReader cdata(_cycler);
-  nassertr(!is_collapsed(), (qpNodePathComponent *)NULL);
+  if (is_collapsed()) {
+    return get_collapsed()->get_next();
+  }
 
+  CDReader cdata(_cycler);
   qpNodePathComponent *next = cdata->_next;
-
+  
   // If the next component has been collapsed, transparently update
   // the pointer to get the actual node, and store the new pointer,
   // before we return.  Collapsing can happen at any time to any
   // component in the path and we have to deal with it.
   if (next != (qpNodePathComponent *)NULL && next->is_collapsed()) {
     next = next->uncollapse();
-    ((qpNodePathComponent *)this)->set_next(next);
-  }
 
+    CDWriter cdata_w(((qpNodePathComponent *)this)->_cycler, cdata);
+    cdata_w->_next = next;
+  }
+  
   return next;
 }
 
@@ -104,3 +161,35 @@ uncollapse() {
 
   return comp;
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: qpNodePathComponent::set_next
+//       Access: Private
+//  Description: Sets the next pointer in the path.
+////////////////////////////////////////////////////////////////////
+void qpNodePathComponent::
+set_next(qpNodePathComponent *next) {
+  if (is_collapsed()) {
+    get_collapsed()->set_next(next);
+  } else {
+    nassertv(next != (qpNodePathComponent *)NULL);
+    CDWriter cdata(_cycler);
+    cdata->_next = next;
+  }
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: qpNodePathComponent::set_top_node
+//       Access: Private
+//  Description: Severs any connection to the next pointer in the
+//               path and makes this component a top node.
+////////////////////////////////////////////////////////////////////
+void qpNodePathComponent::
+set_top_node() {
+  if (is_collapsed()) {
+    get_collapsed()->set_top_node();
+  } else {
+    CDWriter cdata(_cycler);
+    cdata->_next = (qpNodePathComponent *)NULL;
+  }
+}

panda/src/pgraph/qpnodePathComponent.h

@@ -54,22 +54,26 @@ public:
   INLINE ~qpNodePathComponent();
   
   INLINE PandaNode *get_node() const;
-  INLINE int get_key() const;
-  INLINE bool is_top_node() const;
+  int get_key() const;
+  bool is_top_node() const;
   INLINE bool is_collapsed() const;
   
   qpNodePathComponent *get_next() const;
-  INLINE int get_length() const;
+  int get_length() const;
   INLINE qpNodePathComponent *get_collapsed() const;
 
   bool fix_length();
   qpNodePathComponent *uncollapse();
   
 private:
-  INLINE void set_next(qpNodePathComponent *next);
-  INLINE void set_top_node();
+  void set_next(qpNodePathComponent *next);
+  void set_top_node();
   INLINE void collapse_with(qpNodePathComponent *next);
 
+  // We don't have to cycle the _node and _key elements, since these
+  // are permanent properties of this object.  (Well, the _key is
+  // semi-permanent: it becomes permanent after it has been set the
+  // first time.)
   PT(PandaNode) _node;
   int _key;
 

panda/src/pgraph/test_pgraph.cxx

@@ -18,77 +18,21 @@
 
 #include "pandaNode.h"
 #include "qpnodePath.h"
-#include "textureAttrib.h"
-#include "colorAttrib.h"
-#include "transformState.h"
-#include "texture.h"
-#include "qpgeomNode.h"
-#include "geomTristrip.h"
-#include "geomTrifan.h"
-#include "qpcullTraverser.h"
-#include "cullHandler.h"
-
-void
-list_hierarchy(PandaNode *node, int indent_level) {
-  node->write(cerr, indent_level);
-  PandaNode::Children cr = node->get_children();
-  int num_children = cr.get_num_children();
-  for (int i = 0; i < num_children; i++) {
-    list_hierarchy(cr.get_child(i), indent_level + 2);
-  }
-}
 
 int 
 main(int argc, char *argv[]) {
-  PT(Texture) tex = new Texture;
-  tex->set_name("tex");
-
-  PT(PandaNode) root = new PandaNode("root");
-  root->set_attrib(TextureAttrib::make_off());
-  root->set_attrib(ColorAttrib::make_flat(Colorf(1, 0, 0, 1)));
-
-  PandaNode *a = new PandaNode("a");
-  root->add_child(a);
-  a->set_attrib(TextureAttrib::make(tex));
-
-  PandaNode *b = new PandaNode("b");
-  root->add_child(b);
-  b->set_attrib(ColorAttrib::make_vertex());
-
-  PandaNode *a1 = new PandaNode("a1");
-  a->add_child(a1);
-
-  qpGeomNode *g1 = new qpGeomNode("g1");
-  a1->add_child(g1);
-
-  Geom *geom1 = new GeomTristrip;
-  g1->add_geom(geom1, RenderState::make_empty());
-  Geom *geom2 = new GeomTrifan;
-  g1->add_geom(geom2, b->get_state());
-
-  qpGeomNode *g2 = new qpGeomNode("g2");
-  b->add_child(g2);
-  g2->add_geom(geom1, b->get_state());
-  g2->set_transform(TransformState::make_mat(LMatrix4f::translate_mat(10, 0, 0)));
-
-  cerr << "\n";
-  list_hierarchy(root, 0);
-
-  qpNodePath ch_g1(g1);
-  cerr << ch_g1 << "\n";
-
-  qpNodePath ch_g2(g2);
-  cerr << ch_g2 << "\n";
+  qpNodePath h("h");
+  qpNodePath n1("n1");
+  qpNodePath n2("n2");
 
-  cerr << *ch_g1.get_transform(ch_g2) << "\n";
-  cerr << *ch_g2.get_transform(ch_g1) << "\n";
+  PT(PandaNode) node = new PandaNode("node");
 
-  cerr << *ch_g1.get_state(ch_g2) << "\n";
-  cerr << *ch_g2.get_state(ch_g1) << "\n";
+  qpNodePath t1 = h.attach_new_node(node);
+  t1.reparent_to(n1);
 
-  cerr << *ch_g2.get_transform(ch_g2) << "\n";
-  cerr << *ch_g2.get_state(ch_g2) << "\n";
+  t1 = qpNodePath();
 
-  cerr << "\n";
+  qpNodePath t2 = h.attach_new_node(node);
+  t2.reparent_to(n2);
   return 0;
 }