add detect-graph-cycles

panda/src/pgraph/config_pgraph.cxx

@@ -128,6 +128,12 @@ ConfigVariableBool unambiguous_graph
           "assertion failure instead of just a warning (which can then be "
           "trapped with assert-abort)."));
 
+ConfigVariableBool detect_graph_cycles
+("detect-graph-cycles", true,
+ PRC_DESC("Set this true to attempt to detect cycles in the scene graph "
+          "(e.g. a node which is its own parent) as soon as they are "
+          "made.  This has no effect in NDEBUG mode."));
+
 ConfigVariableBool no_unsupported_copy
 ("no-unsupported-copy", false,
  PRC_DESC("Set this true to make an attempt to copy an unsupported type "

panda/src/pgraph/config_pgraph.h

@@ -37,6 +37,7 @@ NotifyCategoryDecl(portal, EXPCL_PANDA, EXPTP_PANDA);
 extern ConfigVariableBool fake_view_frustum_cull;
 extern ConfigVariableBool allow_portal_cull;
 extern ConfigVariableBool unambiguous_graph;
+extern ConfigVariableBool detect_graph_cycles;
 extern ConfigVariableBool no_unsupported_copy;
 extern ConfigVariableBool allow_unrelated_wrt;
 extern ConfigVariableBool paranoid_compose;

panda/src/pgraph/findApproxLevelEntry.cxx

@@ -147,6 +147,8 @@ consider_node(NodePathCollection &result, FindApproxLevelEntry *&next_level,
 void FindApproxLevelEntry::
 consider_next_step(PandaNode *child_node, FindApproxLevelEntry *&next_level, 
                    int increment) const {
+  nassertv(child_node != _node_path.node());
+
   if (!_approx_path.return_hidden() && child_node->is_overall_hidden()) {
     // If the approx path does not allow us to return hidden nodes,
     // and this node has indeed been completely hidden, then stop

panda/src/pgraph/pandaNode.I

@@ -762,6 +762,28 @@ do_find_parent(PandaNode *node, const CData *cdata) const {
   return ui - up.begin();
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: PandaNode::verify_child_no_cycles
+//       Access: Private
+//  Description: Ensures that attaching the indicated child node to
+//               this node would not introduce a cycle in the graph.
+//               Returns true if the attachment is valid, false
+//               otherwise.
+////////////////////////////////////////////////////////////////////
+INLINE bool PandaNode::
+verify_child_no_cycles(PandaNode *child_node) {
+#ifndef NDEBUG
+  if (detect_graph_cycles) {
+    if (!find_node_above(child_node)) {
+      return true;
+    }
+    report_cycle(child_node);
+    return false;
+  }
+#endif  // NDEBUG
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: PandaNode::set_dirty_prev_transform
 //       Access: Private

panda/src/pgraph/pandaNode.cxx

@@ -586,6 +586,13 @@ copy_subgraph(Thread *current_thread) const {
 void PandaNode::
 add_child(PandaNode *child_node, int sort, Thread *current_thread) {
   nassertv(child_node != (PandaNode *)NULL);
+
+  if (!verify_child_no_cycles(child_node)) {
+    // Whoops, adding this child node would introduce a cycle in the
+    // scene graph.
+    return;
+  }
+
   // Ensure the child_node is not deleted while we do this.
   PT(PandaNode) keep_child = child_node;
   remove_child(child_node);
@@ -607,6 +614,7 @@ add_child(PandaNode *child_node, int sort, Thread *current_thread) {
   CLOSE_ITERATE_CURRENT_AND_UPSTREAM_NOLOCK(_cycler);
 
   force_bounds_stale();
+
   children_changed();
   child_node->parents_changed();
 }
@@ -685,7 +693,8 @@ remove_child(PandaNode *child_node, Thread *current_thread) {
 //  Description: Searches for the orig_child node in the node's list
 //               of children, and replaces it with the new_child
 //               instead.  Returns true if the replacement is made, or
-//               false if the node is not a child.
+//               false if the node is not a child or if there is some
+//               other problem.
 ////////////////////////////////////////////////////////////////////
 bool PandaNode::
 replace_child(PandaNode *orig_child, PandaNode *new_child,
@@ -697,6 +706,12 @@ replace_child(PandaNode *orig_child, PandaNode *new_child,
     // Trivial no-op.
     return true;
   }
+
+  if (!verify_child_no_cycles(new_child)) {
+    // Whoops, adding this child node would introduce a cycle in the
+    // scene graph.
+    return false;
+  }
   
   // Make sure the orig_child node is not destructed during the
   // execution of this method.
@@ -830,6 +845,12 @@ add_stashed(PandaNode *child_node, int sort, Thread *current_thread) {
   int pipeline_stage = current_thread->get_pipeline_stage();
   nassertv(pipeline_stage == 0);
 
+  if (!verify_child_no_cycles(child_node)) {
+    // Whoops, adding this child node would introduce a cycle in the
+    // scene graph.
+    return;
+  }
+
   // Ensure the child_node is not deleted while we do this.
   PT(PandaNode) keep_child = child_node;
   remove_child(child_node);
@@ -2652,6 +2673,44 @@ stage_replace_child(PandaNode *orig_child, PandaNode *new_child,
   return true;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: PandaNode::report_cycle
+//       Access: Private
+//  Description: Raises an assertion when a graph cycle attempt is
+//               detected (and aborted).
+////////////////////////////////////////////////////////////////////
+void PandaNode::
+report_cycle(PandaNode *child_node) {
+  ostringstream strm;
+  strm << "Detected attempt to create a cycle in the scene graph: " 
+       << NodePath::any_path(this) << " : " << *child_node;
+  nassert_raise(strm.str());
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: PandaNode::find_node_above
+//       Access: Private
+//  Description: Returns true if the indicated node is this node, or
+//               any ancestor of this node; or false if it is not in
+//               this node's ancestry.
+////////////////////////////////////////////////////////////////////
+bool PandaNode::
+find_node_above(PandaNode *node) {
+  if (node == this) {
+    return true;
+  }
+
+  Parents parents = get_parents();
+  for (int i = 0; i < parents.get_num_parents(); ++i) {
+    PandaNode *parent = parents.get_parent(i);
+    if (parent->find_node_above(node)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: PandaNode::attach
 //       Access: Private, Static
@@ -2794,6 +2853,13 @@ reparent(NodePathComponent *new_parent, NodePathComponent *child, int sort,
          bool as_stashed, int pipeline_stage, Thread *current_thread) {
   bool any_ok = false;
 
+  if (new_parent != (NodePathComponent *)NULL &&
+      !new_parent->get_node()->verify_child_no_cycles(child->get_node())) {
+    // Whoops, adding this child node would introduce a cycle in the
+    // scene graph.
+    return false;
+  }
+
   for (int pipeline_stage_i = pipeline_stage;
        pipeline_stage_i >= 0; 
        --pipeline_stage_i) {

panda/src/pgraph/pandaNode.h

@@ -305,6 +305,10 @@ private:
   bool stage_replace_child(PandaNode *orig_child, PandaNode *new_child,
                            int pipeline_stage, Thread *current_thread);
 
+  INLINE bool verify_child_no_cycles(PandaNode *child_node);
+  void report_cycle(PandaNode *node);
+  bool find_node_above(PandaNode *node);
+
   // parent-child manipulation for NodePath support.  Don't try to
   // call these directly.
   static PT(NodePathComponent) attach(NodePathComponent *parent, 

panda/src/pgraph/workingNodePath.I

@@ -61,6 +61,7 @@ WorkingNodePath(const WorkingNodePath &parent, PandaNode *child) {
   _next = &parent;
   _start = (NodePathComponent *)NULL;
   _node = child;
+  nassertv(_node != _next->_node);
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/pgraph/workingNodePath.cxx

@@ -34,6 +34,7 @@ is_valid() const {
     return (_start != (NodePathComponent *)NULL);
   }
 
+  nassertr(_node != _next->_node, false);
   return _next->is_valid();
 }