change logic for uniqueness of NodePaths w.r.t. instances

panda/src/collide/collisionTraverser.h

@@ -108,12 +108,6 @@ private:
   PT(CollisionHandler) _default_handler;
   TypeHandle _graph_type;
 
-  // A map of NodePaths is slightly dangerous, since there is a
-  // (small) possiblity that a particular NodePath's sorting order
-  // relative to other NodePaths will spontaneously change.  This can
-  // only happen if two NodePaths get collapsed together due to a
-  // removal of a certain kind of instanced node; see the comments in
-  // NodePath.cxx.
   typedef pmap<NodePath,  PT(CollisionHandler) > Colliders;
   Colliders _colliders;
   typedef pvector<NodePath> OrderedColliders;

panda/src/pgraph/nodePath.I

@@ -112,7 +112,6 @@ NodePath(const NodePath &copy) :
   _head(copy._head),
   _error_type(copy._error_type)
 {
-  uncollapse_head();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -124,7 +123,6 @@ INLINE void NodePath::
 operator = (const NodePath &copy) {
   _head = copy._head;
   _error_type = copy._error_type;
-  uncollapse_head();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -212,7 +210,6 @@ get_max_search_depth() {
 ////////////////////////////////////////////////////////////////////
 INLINE bool NodePath::
 is_empty() const {
-  uncollapse_head();
   return (_head == (NodePathComponent *)NULL);
 }
 
@@ -224,7 +221,6 @@ is_empty() const {
 ////////////////////////////////////////////////////////////////////
 INLINE bool NodePath::
 is_singleton() const {
-  uncollapse_head();
   return (_head != (NodePathComponent *)NULL && _head->is_top_node());
 }
 
@@ -429,7 +425,6 @@ set_state(const RenderState *state) {
 ////////////////////////////////////////////////////////////////////
 INLINE CPT(RenderState) NodePath::
 get_net_state() const {
-  uncollapse_head();
   return r_get_net_state(_head);
 }
 
@@ -462,7 +457,6 @@ set_transform(const TransformState *transform) {
 ////////////////////////////////////////////////////////////////////
 INLINE CPT(TransformState) NodePath::
 get_net_transform() const {
-  uncollapse_head();
   return r_get_net_transform(_head);
 }
 
@@ -501,7 +495,6 @@ set_prev_transform(const TransformState *transform) {
 ////////////////////////////////////////////////////////////////////
 INLINE CPT(TransformState) NodePath::
 get_net_prev_transform() const {
-  uncollapse_head();
   return r_get_net_prev_transform(_head);
 }
 
@@ -1149,51 +1142,6 @@ is_hidden(DrawMask camera_mask) const {
   return !get_hidden_ancestor(camera_mask).is_empty();
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: NodePath::unstash
-//       Access: Published
-//  Description: Undoes the effect of a previous stash() on this
-//               node: makes the referenced node (and the entire
-//               subgraph below this node) once again part of the
-//               scene graph.  Returns true if the node is unstashed,
-//               or false if it wasn't stashed to begin with.
-////////////////////////////////////////////////////////////////////
-INLINE bool NodePath::
-unstash() {
-  nassertr(!is_singleton(), false);
-  PandaNode *parent_node = get_parent().node();
-  PandaNode *this_node = node();
-  nassertr(parent_node != (PandaNode *)NULL && this_node != (PandaNode *)NULL, false);
-  return parent_node->unstash_child(this_node);
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: NodePath::stash
-//       Access: Published
-//  Description: Removes the referenced node (and the entire subgraph
-//               below this node) from the scene graph in any normal
-//               sense.  The node will no longer be visible and is not
-//               tested for collisions; furthermore, no normal scene
-//               graph traversal will visit the node.  The node's
-//               bounding volume no longer contributes to its parent's
-//               bounding volume.
-//
-//               A stashed node cannot be located by a normal find()
-//               operation (although a special find string can still
-//               retrieve it).
-//
-//               Returns true if the node is successfully stashed, or
-//               false if it was already stashed.
-////////////////////////////////////////////////////////////////////
-INLINE bool NodePath::
-stash() {
-  nassertr(!is_singleton(), false);
-  PandaNode *parent_node = get_parent().node();
-  PandaNode *this_node = node();
-  nassertr(parent_node != (PandaNode *)NULL && this_node != (PandaNode *)NULL, false);
-  return parent_node->stash_child(this_node);
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: NodePath::is_stashed
 //       Access: Published
@@ -1335,6 +1283,28 @@ has_net_tag(const string &key) const {
   return find_net_tag(key).has_tag(key);
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: NodePath::set_name
+//       Access: Published
+//  Description: Changes the name of the referenced node.
+////////////////////////////////////////////////////////////////////
+INLINE void NodePath::
+set_name(const string &name) {
+  nassertv_always(!is_empty());
+  node()->set_name(name);
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: NodePath::get_name
+//       Access: Published
+//  Description: Returns the name of the referenced node.
+////////////////////////////////////////////////////////////////////
+INLINE string NodePath::
+get_name() const {
+  nassertr_always(!is_empty(), string());
+  return node()->get_name();
+}
+
 
 INLINE ostream &operator << (ostream &out, const NodePath &node_path) {
   node_path.output(out);

panda/src/pgraph/nodePath.cxx

@@ -64,7 +64,6 @@ get_num_nodes() const {
   if (is_empty()) {
     return 0;
   }
-  uncollapse_head();
   return _head->get_length();
 }
 
@@ -80,7 +79,6 @@ PandaNode *NodePath::
 get_node(int index) const {
   nassertr(index >= 0 && index < get_num_nodes(), NULL);
 
-  uncollapse_head();
   NodePathComponent *comp = _head;
   while (index > 0) {
     // If this assertion fails, the index was out of range; the
@@ -108,7 +106,6 @@ get_top_node() const {
     return (PandaNode *)NULL;
   }
 
-  uncollapse_head();
   NodePathComponent *comp = _head;
   while (!comp->is_top_node()) {
     comp = comp->get_next();
@@ -186,9 +183,17 @@ get_sort() const {
   PandaNode *child = node();
   nassertr(parent != (PandaNode *)NULL && child != (PandaNode *)NULL, 0);
   int child_index = parent->find_child(child);
-  nassertr(child_index != -1, 0);
+  if (child_index != -1) {
+    return parent->get_child_sort(child_index);
+  }
+
+  child_index = parent->find_stashed(child);
+  if (child_index != -1) {
+    return parent->get_stashed_sort(child_index);
+  }
 
-  return parent->get_child_sort(child_index);
+  nassertr(false, 0);
+  return 0;
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -286,14 +291,12 @@ reparent_to(const NodePath &other, int sort) {
   nassertv(verify_complete());
   nassertv(other.verify_complete());
   nassertv_always(!is_empty());
-  nassertv_always(!other.is_empty());
-
-  uncollapse_head();
-  other.uncollapse_head();
-  PandaNode::reparent(other._head, _head, sort);
 
   // Reparenting implicitly resents the delta vector.
   node()->reset_prev_transform();
+
+  bool reparented = PandaNode::reparent(other._head, _head, sort, false);
+  nassertv(reparented);
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -310,7 +313,6 @@ wrt_reparent_to(const NodePath &other, int sort) {
   nassertv(verify_complete());
   nassertv(other.verify_complete());
   nassertv_always(!is_empty());
-  nassertv_always(!other.is_empty());
 
   if (get_transform() == get_prev_transform()) {
     set_transform(get_transform(other));
@@ -345,13 +347,17 @@ instance_to(const NodePath &other, int sort) const {
   nassertr(verify_complete(), NodePath::fail());
   nassertr(other.verify_complete(), NodePath::fail());
   nassertr_always(!is_empty(), NodePath::fail());
-  nassertr(!other.is_empty(), NodePath::fail());
-
-  uncollapse_head();
-  other.uncollapse_head();
 
   NodePath new_instance;
-  new_instance._head = PandaNode::attach(other._head, node(), sort);
+
+  // First, we'll attach to NULL, to guarantee we get a brand new
+  // instance.
+  new_instance._head = PandaNode::attach(NULL, node(), sort);
+
+  // Now, we'll reparent the new instance to the target node.
+  bool reparented = PandaNode::reparent(other._head, new_instance._head,
+                                        sort, false);
+  nassertr(reparented, new_instance);
 
   // instance_to() doesn't reset the velocity delta, unlike most of
   // the other reparenting operations.  The reasoning is that
@@ -414,6 +420,9 @@ copy_to(const NodePath &other, int sort) const {
 //               of this NodePath.  This is the preferred way to add
 //               nodes to the graph.
 //
+//               If the node was already a child of the parent, this
+//               returns a NodePath to the existing child.
+//
 //               This does *not* automatically extend the current
 //               NodePath to reflect the attachment; however, a
 //               NodePath that does reflect this extension is
@@ -425,7 +434,6 @@ attach_new_node(PandaNode *node, int sort) const {
   nassertr_always(!is_empty(), NodePath());
   nassertr(node != (PandaNode *)NULL, NodePath());
 
-  uncollapse_head();
   NodePath new_path(*this);
   new_path._head = PandaNode::attach(_head, node, sort);
   return new_path;
@@ -462,7 +470,6 @@ remove_node() {
   // NodePath is clear.
   if (!is_empty() && !is_singleton()) {
     node()->reset_prev_transform();
-    uncollapse_head();
     PandaNode::detach(_head);
   }
 
@@ -493,7 +500,6 @@ detach_node() {
   nassertv(_error_type != ET_not_found);
   if (!is_empty() && !is_singleton()) {
     node()->reset_prev_transform();
-    uncollapse_head();
     PandaNode::detach(_head);
   }
 }
@@ -506,8 +512,6 @@ detach_node() {
 ////////////////////////////////////////////////////////////////////
 void NodePath::
 output(ostream &out) const {
-  uncollapse_head();
-
   switch (_error_type) {
   case ET_not_found:
     out << "**not found**";
@@ -3025,6 +3029,59 @@ get_hidden_ancestor(DrawMask camera_mask) const {
   return not_found();
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: NodePath::stash
+//       Access: Published
+//  Description: Removes the referenced node (and the entire subgraph
+//               below this node) from the scene graph in any normal
+//               sense.  The node will no longer be visible and is not
+//               tested for collisions; furthermore, no normal scene
+//               graph traversal will visit the node.  The node's
+//               bounding volume no longer contributes to its parent's
+//               bounding volume.
+//
+//               A stashed node cannot be located by a normal find()
+//               operation (although a special find string can still
+//               retrieve it).
+////////////////////////////////////////////////////////////////////
+void NodePath::
+stash(int sort) {
+  nassertv_always(!is_singleton() && !is_empty());
+  nassertv(verify_complete());
+
+  bool reparented = PandaNode::reparent(_head->get_next(), _head, sort, true);
+  nassertv(reparented);
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: NodePath::unstash
+//       Access: Published
+//  Description: Undoes the effect of a previous stash() on this
+//               node: makes the referenced node (and the entire
+//               subgraph below this node) once again part of the
+//               scene graph.
+////////////////////////////////////////////////////////////////////
+void NodePath::
+unstash(int sort) {
+  nassertv_always(!is_singleton() && !is_empty());
+  nassertv(verify_complete());
+
+  bool reparented = PandaNode::reparent(_head->get_next(), _head, sort, false);
+  nassertv(reparented);
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: NodePath::unstash_all
+//       Access: Published
+//  Description: Unstashes this node and all stashed child nodes.
+////////////////////////////////////////////////////////////////////
+void NodePath::
+unstash_all() {
+  NodePathCollection stashed_descendents = find_all_matches("**/@@*");
+  stashed_descendents.unstash();
+  unstash();
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: NodePath::get_stashed_ancestor
 //       Access: Published
@@ -3074,9 +3131,6 @@ get_stashed_ancestor() const {
 ////////////////////////////////////////////////////////////////////
 int NodePath::
 compare_to(const NodePath &other) const {
-  uncollapse_head();
-  other.uncollapse_head();
-
   // Nowadays, the NodePathComponents at the head are pointerwise
   // equivalent if and only if the NodePaths are equivalent.  So we
   // only have to compare pointers.
@@ -3090,8 +3144,7 @@ compare_to(const NodePath &other) const {
 //     Function: NodePath::verify_complete
 //       Access: Published
 //  Description: Returns true if all of the nodes described in the
-//               NodePath are connected and the top node is the top
-//               of the graph, or false otherwise.
+//               NodePath are connected, or false otherwise.
 ////////////////////////////////////////////////////////////////////
 bool NodePath::
 verify_complete() const {
@@ -3099,7 +3152,6 @@ verify_complete() const {
     return true;
   }
 
-  uncollapse_head();
   const NodePathComponent *comp = _head;
   nassertr(comp != (const NodePathComponent *)NULL, false);
 
@@ -3133,15 +3185,7 @@ verify_complete() const {
     length--;
   }
 
-  // Now that we've reached the top, we should have no parents.
-  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;
+  return true;
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -3409,22 +3453,6 @@ write_bam_file(const string &filename) const {
   return okflag;
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: NodePath::uncollapse_head
-//       Access: Private
-//  Description: Quietly and transparently uncollapses the _head
-//               pointer if it needs it.  This can happen only when
-//               two distinct NodePaths are collapsed into the same
-//               path after the removal of an instance somewhere
-//               higher up the chain.
-////////////////////////////////////////////////////////////////////
-void NodePath::
-uncollapse_head() const {
-  if (_head != (NodePathComponent *)NULL && _head->is_collapsed()) {
-    ((NodePath *)this)->_head = _head->uncollapse();
-  }
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: NodePath::find_common_ancestor
 //       Access: Private, Static
@@ -3441,9 +3469,6 @@ NodePathComponent *NodePath::
 find_common_ancestor(const NodePath &a, const NodePath &b,
                      int &a_count, int &b_count) {
   nassertr(!a.is_empty() && !b.is_empty(), NULL);
-  a.uncollapse_head();
-  b.uncollapse_head();
-
   NodePathComponent *ac = a._head;
   NodePathComponent *bc = b._head;
   a_count = 0;

panda/src/pgraph/nodePath.h

@@ -552,8 +552,9 @@ PUBLISHED:
   INLINE bool is_hidden(DrawMask camera_mask = DrawMask::all_on()) const;
   NodePath get_hidden_ancestor(DrawMask camera_mask = DrawMask::all_on()) const;
 
-  INLINE bool stash();
-  INLINE bool unstash();
+  void stash(int sort = 0);
+  void unstash(int sort = 0);
+  void unstash_all();
   INLINE bool is_stashed() const;
   NodePath get_stashed_ancestor() const;
 
@@ -587,10 +588,12 @@ PUBLISHED:
   INLINE bool has_net_tag(const string &key) const;
   NodePath find_net_tag(const string &key) const;
 
+  INLINE void set_name(const string &name);
+  INLINE string get_name() const;
+
   bool write_bam_file(const string &filename) const;
 
 private:
-  void uncollapse_head() const;
   static NodePathComponent *
   find_common_ancestor(const NodePath &a, const NodePath &b,
                        int &a_count, int &b_count);

panda/src/pgraph/nodePathComponent.I

@@ -101,41 +101,10 @@ INLINE NodePathComponent::
 ////////////////////////////////////////////////////////////////////
 INLINE PandaNode *NodePathComponent::
 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: NodePathComponent::is_collapsed
-//       Access: Public
-//  Description: Returns true if this component has been collapsed
-//               with another component.  In this case, the component
-//               itself is invalid, and the collapsed component should
-//               be used instead.
-////////////////////////////////////////////////////////////////////
-INLINE bool NodePathComponent::
-is_collapsed() const {
-  CDReader cdata(_cycler);
-  return (cdata->_length == 0);
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: NodePathComponent::get_collapsed
-//       Access: Public
-//  Description: If is_collapsed() returns true, this is the component
-//               that this one has been collapsed with, and should be
-//               replaced with.
-////////////////////////////////////////////////////////////////////
-INLINE NodePathComponent *NodePathComponent::
-get_collapsed() const {
-  nassertr(is_collapsed(), (NodePathComponent *)NULL);
-  CDReader cdata(_cycler);
-  return cdata->_next;
-}
-
 INLINE ostream &operator << (ostream &out, const NodePathComponent &comp) {
   comp.output(out);
   return out;

panda/src/pgraph/nodePathComponent.cxx

@@ -45,9 +45,6 @@ make_copy() const {
 ////////////////////////////////////////////////////////////////////
 int NodePathComponent::
 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
@@ -67,9 +64,6 @@ get_key() const {
 ////////////////////////////////////////////////////////////////////
 bool NodePathComponent::
 is_top_node() const {
-  if (is_collapsed()) {
-    return get_collapsed()->is_top_node();
-  }
   CDReader cdata(_cycler);
   return (cdata->_next == (NodePathComponent *)NULL);
 }
@@ -81,9 +75,6 @@ is_top_node() const {
 ////////////////////////////////////////////////////////////////////
 int NodePathComponent::
 get_length() const {
-  if (is_collapsed()) {
-    return get_collapsed()->get_length();
-  }
   CDReader cdata(_cycler);
   return cdata->_length;
 }
@@ -95,24 +86,9 @@ get_length() const {
 ////////////////////////////////////////////////////////////////////
 NodePathComponent *NodePathComponent::
 get_next() const {
-  if (is_collapsed()) {
-    return get_collapsed()->get_next();
-  }
-
   CDReader cdata(_cycler);
   NodePathComponent *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 != (NodePathComponent *)NULL && next->is_collapsed()) {
-    next = next->uncollapse();
-
-    CDWriter cdata_w(((NodePathComponent *)this)->_cycler, cdata);
-    cdata_w->_next = next;
-  }
-  
   return next;
 }
 
@@ -139,29 +115,6 @@ fix_length() {
   return true;
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: NodePathComponent::uncollapse
-//       Access: Public
-//  Description: Returns this component pointer if the component is
-//               not collapsed; or if it has been collapsed, returns
-//               the pointer it has been collapsed into.
-//
-//               Collapsing can happen at any time to any component in
-//               the path and we have to deal with it.  It happens
-//               when a node is removed further up the path that
-//               results in two instances becoming the same thing.
-////////////////////////////////////////////////////////////////////
-NodePathComponent *NodePathComponent::
-uncollapse() {
-  NodePathComponent *comp = this;
-
-  while (comp->is_collapsed()) {
-    comp = comp->get_collapsed();
-  }
-
-  return comp;
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: NodePathComponent::output
 //       Access: Public
@@ -206,13 +159,9 @@ output(ostream &out) const {
 ////////////////////////////////////////////////////////////////////
 void NodePathComponent::
 set_next(NodePathComponent *next) {
-  if (is_collapsed()) {
-    get_collapsed()->set_next(next);
-  } else {
-    nassertv(next != (NodePathComponent *)NULL);
-    CDWriter cdata(_cycler);
-    cdata->_next = next;
-  }
+  nassertv(next != (NodePathComponent *)NULL);
+  CDWriter cdata(_cycler);
+  cdata->_next = next;
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -223,37 +172,6 @@ set_next(NodePathComponent *next) {
 ////////////////////////////////////////////////////////////////////
 void NodePathComponent::
 set_top_node() {
-  if (is_collapsed()) {
-    get_collapsed()->set_top_node();
-  } else {
-    CDWriter cdata(_cycler);
-    cdata->_next = (NodePathComponent *)NULL;
-  }
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: NodePathComponent::collapse_with
-//       Access: Private
-//  Description: Indicates that this component pointer is no longer
-//               valid, and that the indicated component should be
-//               used instead.  This is done whenever two
-//               NodePathComponents have been collapsed together due
-//               to an instance being removed higher up in the graph.
-////////////////////////////////////////////////////////////////////
-void NodePathComponent::
-collapse_with(NodePathComponent *next) {
-  nassertv(!is_collapsed());
-  nassertv(next != (NodePathComponent *)NULL);
   CDWriter cdata(_cycler);
-
-  // We indicate a component has been collapsed by setting its length
-  // to zero.
-  cdata->_next = next;
-  cdata->_length = 0;
-
-  if (_key != 0 && next->_key == 0) {
-    // If we had a key set and the other one didn't, it inherits our
-    // key.  Otherwise, we inherit the other's key.
-    next->_key = _key;
-  }
+  cdata->_next = (NodePathComponent *)NULL;
 }

panda/src/pgraph/nodePathComponent.h

@@ -56,21 +56,17 @@ public:
   INLINE PandaNode *get_node() const;
   int get_key() const;
   bool is_top_node() const;
-  INLINE bool is_collapsed() const;
   
   NodePathComponent *get_next() const;
   int get_length() const;
-  INLINE NodePathComponent *get_collapsed() const;
 
   bool fix_length();
-  NodePathComponent *uncollapse();
 
   void output(ostream &out) const;
   
 private:
   void set_next(NodePathComponent *next);
   void set_top_node();
-  void collapse_with(NodePathComponent *next);
 
   // We don't have to cycle the _node and _key elements, since these
   // are permanent properties of this object.  (Well, the _key is

panda/src/pgraph/pandaNode.cxx

@@ -29,68 +29,15 @@
 
 TypeHandle PandaNode::_type_handle;
 
-// 
-// We go through some considerable effort in this class to ensure that
-// NodePaths are kept consistent as we attach and detach nodes.  We
-// must enforce the following rules:
-//
-//   1) Each NodePath (i.e. chain of NodePathComponents) represents a
-//   complete unbroken chain from a PandaNode to the root of the
-//   graph.
-//
-//   2) Each NodePathComponent chain is unique.  There are no two
-//   different NodePathComponents that reference the same path to the
-//   root.
-//
-// The following rules all follow from rules (1) and (2):
-//
-//   3) If a PandaNode with no parents is attached to a new parent,
-//   all NodePaths that previously indicated this node as the root of
-//   graph must now be updated to include the complete chain to the
-//   new root.
-//
-//   4) If a PandaNode with other parents is attached to a new parent,
-//   any previously existing NodePaths are not affected.
-//
-//   5) If a PandaNode is disconnected from its parent, and it has no
-//   other parents, all NodePaths that previously passed through this
-//   node to the old parent must now be updated to indicate this node
-//   is now the root.
-//
-//   6) If a PandaNode is disconnected from its parent, and it has at
-//   least one other parent, all NodePaths that previously passed
-//   through this node to the old parent must now be updated to pass
-//   through one of the other parents instead.
-//
-// Rules (5) and (6) can especially complicate things because they
-// introduce the possibility that two formerly distinct NodePaths are
-// now equivalent, which violates rule (2).  For example, if A is the
-// top of the graph with children B and C, and D is instanced to both
-// B and C, and E is a child of D, there might be two different
-// NodePaths to D: A/B/D/E and A/C/D/E.  If you then break the
-// connection between D and E, both NodePaths must now become just the
-// singleton E.
-//
-// Unfortunately, we cannot simply remove one of the extra
-// NodePathComponents, because there may be any number of NodePath
-// objects that reference them.  Instead, we define the concept of
-// "collapsed" NodePathComponents, which means one NodePathComponent
-// can be "collapsed" into a different one so that any attempt to
-// reference the first actually retrieves the second, rather like a
-// symbolic link in the file system.  When the NodePath traverses its
-// component chain, it will pass right over the collapsed component in
-// favor of the one it has been collapsed into.
-//
-
-
 //
 // There are two different interfaces here for making and breaking
 // parent-child connections: the fundamental PandaNode interface, via
 // add_child() and remove_child() (and related functions), and the
 // NodePath support interface, via attach(), detach(), and reparent().
 // They both do essentially the same thing, but with slightly
-// different inputs.  Both are responsible for keeping all NodePaths
-// up-to-date according to the above rules.
+// different inputs.  The PandaNode interfaces try to guess which
+// NodePaths should be updated as a result of the scene graph change,
+// while the NodePath interfaces already know.
 //
 // The NodePath support interface functions are strictly called from
 // within the NodePath class, and are used to implement
@@ -1607,7 +1554,15 @@ r_copy_children(const PandaNode *from, PandaNode::InstanceMap &inst_map) {
 ////////////////////////////////////////////////////////////////////
 PT(NodePathComponent) PandaNode::
 attach(NodePathComponent *parent, PandaNode *child_node, int sort) {
-  nassertr(parent != (NodePathComponent *)NULL, (NodePathComponent *)NULL);
+  if (parent == (NodePathComponent *)NULL) {
+    // Attaching to NULL means to create a new "instance" with no
+    // attachments, and no questions asked.
+    PT(NodePathComponent) child = 
+      new NodePathComponent(child_node, (NodePathComponent *)NULL);
+    CDWriter cdata_child(child_node->_cycler);
+    cdata_child->_paths.insert(child);
+    return child;
+  }
 
   // See if the child was already attached to the parent.  If it was,
   // we'll use that same NodePathComponent.
@@ -1619,7 +1574,7 @@ attach(NodePathComponent *parent, PandaNode *child_node, int sort) {
     child = get_top_component(child_node, true);
   }
 
-  reparent(parent, child, sort);
+  reparent(parent, child, sort, false);
   return child;
 }
 
@@ -1688,39 +1643,58 @@ detach(NodePathComponent *child) {
 ////////////////////////////////////////////////////////////////////
 //     Function: PandaNode::reparent
 //       Access: Private, Static
-//  Description: Switches a node from one parent to another.
+//  Description: Switches a node from one parent to another.  Returns
+//               true if the new connection is allowed, or false if it
+//               conflicts with another instance (that is, another
+//               instance of the child is already attached to the
+//               indicated parent).
 ////////////////////////////////////////////////////////////////////
-void PandaNode::
-reparent(NodePathComponent *new_parent, NodePathComponent *child, int sort) {
-  nassertv(new_parent != (NodePathComponent *)NULL);
-  nassertv(child != (NodePathComponent *)NULL);
+bool PandaNode::
+reparent(NodePathComponent *new_parent, NodePathComponent *child, int sort,
+         bool as_stashed) {
+  nassertr(child != (NodePathComponent *)NULL, false);
 
   if (!child->is_top_node()) {
     detach(child);
   }
 
-  // Adjust the NodePathComponents.
-  child->set_next(new_parent);
+  if (new_parent != (NodePathComponent *)NULL) {
+    PandaNode *child_node = child->get_node();
+    PandaNode *parent_node = new_parent->get_node();
 
-  PandaNode *child_node = child->get_node();
-  PandaNode *parent_node = new_parent->get_node();
+    if (child_node->find_parent(parent_node) >= 0) {
+      // Whoops, there's already another instance of the child there.
+      return false;
+    }
 
-  // Now reattach at the indicated sort position.
-  CDWriter cdata_parent(parent_node->_cycler);
-  CDWriter cdata_child(child_node->_cycler);
-  
-  cdata_parent->_down.insert(DownConnection(child_node, sort));
-  cdata_child->_up.insert(UpConnection(parent_node));
-  
-  cdata_child->_paths.insert(child);
-  child_node->fix_path_lengths(cdata_child);
+    // Redirect the connection to the indicated new parent.
+    child->set_next(new_parent);
+    
+    // Now reattach the child node at the indicated sort position.
+    CDWriter cdata_parent(parent_node->_cycler);
+    CDWriter cdata_child(child_node->_cycler);
 
-  // Mark the bounding volumes stale.
-  parent_node->force_bound_stale();
+    if (as_stashed) {
+      cdata_parent->_stashed.insert(DownConnection(child_node, sort));
+    } else {
+      cdata_parent->_down.insert(DownConnection(child_node, sort));
+    }
+    cdata_child->_up.insert(UpConnection(parent_node));
+    
+    cdata_child->_paths.insert(child);
+    child_node->fix_path_lengths(cdata_child);
+    
+    // Mark the bounding volumes stale.
+    if (!as_stashed) {
+      parent_node->force_bound_stale();
+    }
 
-  // Call callback hooks.
-  parent_node->children_changed();
-  child_node->parents_changed();
+    // Call callback hooks.
+    parent_node->children_changed();
+    child_node->parents_changed();
+  }
+
+  return true;
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -1904,8 +1878,7 @@ delete_component(NodePathComponent *component) {
     }
   }
 
-  // This may legitimately be zero if we are deleting a collapsed NodePath.
-  //  nassertv(max_num_erased == 1);
+  nassertv(max_num_erased == 1);
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -1916,65 +1889,25 @@ delete_component(NodePathComponent *component) {
 //               that reflected this connection.
 //
 //               It severs any NodePathComponents on the child node
-//               that reference the indicated parent node.  If the
-//               child node has additional parents, chooses one of
-//               them arbitrarily instead.  Collapses together
-//               instances below this node that used to differentiate
-//               on some instance above the old parent.
+//               that reference the indicated parent node.  These
+//               components remain unattached; there may therefore be
+//               multiple "instances" of a node that all have no
+//               parent, even while there are other instances that do
+//               have parents.
 ////////////////////////////////////////////////////////////////////
 void PandaNode::
 sever_connection(PandaNode *parent_node, PandaNode *child_node) {
   CDWriter cdata_child(child_node->_cycler);
-  PT(NodePathComponent) collapsed = (NodePathComponent *)NULL;
 
   Paths::iterator pi;
-  pi = cdata_child->_paths.begin();
-  while (pi != cdata_child->_paths.end()) {
-    Paths::iterator pnext = pi;
-    ++pnext;
+  for (pi = cdata_child->_paths.begin();
+       pi != cdata_child->_paths.end();
+       ++pi) {
     if (!(*pi)->is_top_node() && 
         (*pi)->get_next()->get_node() == parent_node) {
-      if (collapsed == (NodePathComponent *)NULL) {
-
-        // This is the first component we've found that references
-        // this node.  Should we sever it or reattach it?
-        if (child_node->get_num_parents() == 0) {
-          // If the node no longer has any parents, all of its paths will be
-          // severed here.  Collapse them all with the existing top
-          // component if there is one.
-          collapsed = get_top_component(child_node, false);
-          
-        } 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
-          // the first parent arbitrarily to combine with, and don't
-          // complain if there's ambiguity.
-          collapsed = child_node->get_generic_component(true);
-        }
-
-        if (collapsed == (NodePathComponent *)NULL) {
-          // Sever the component here; there's nothing to attach it
-          // to.
-          (*pi)->set_top_node();
-          collapsed = (*pi);
-
-        } else {
-          // Collapse the new component with the pre-existing
-          // component.
-          (*pi)->collapse_with(collapsed);
-          cdata_child->_paths.erase(pi);
-        }
-
-      } else {
-        // This is the second (or later) component we've found that
-        // references this node.  We should collapse it with the first
-        // one.
-        (*pi)->collapse_with(collapsed);
-        cdata_child->_paths.erase(pi);
-      }
+      // Sever the component here.
+      (*pi)->set_top_node();
     }
-    pi = pnext;
   }
   child_node->fix_path_lengths(cdata_child);
 }
@@ -1983,7 +1916,7 @@ sever_connection(PandaNode *parent_node, PandaNode *child_node) {
 //     Function: PandaNode::new_connection
 //       Access: Private, Static
 //  Description: This is called internally when a parent-child
-//               connection is establshed to update the
+//               connection is established to update the
 //               NodePathComponents that might be involved.
 //
 //               It adjusts any NodePathComponents the child has that

panda/src/pgraph/pandaNode.h

@@ -221,8 +221,8 @@ private:
   static PT(NodePathComponent) attach(NodePathComponent *parent, 
                                        PandaNode *child, int sort);
   static void detach(NodePathComponent *child);
-  static void reparent(NodePathComponent *new_parent,
-                       NodePathComponent *child, int sort);
+  static bool reparent(NodePathComponent *new_parent,
+                       NodePathComponent *child, int sort, bool as_stashed);
   static PT(NodePathComponent) get_component(NodePathComponent *parent,
                                               PandaNode *child);
   static PT(NodePathComponent) get_top_component(PandaNode *child,