From 93807927f9e9d37e9455c2186e64681d8949fbda Mon Sep 17 00:00:00 2001 From: David Rose Date: Wed, 4 Apr 2007 21:21:23 +0000 Subject: [PATCH] add detect-graph-cycles --- panda/src/pgraph/config_pgraph.cxx | 6 ++ panda/src/pgraph/config_pgraph.h | 1 + panda/src/pgraph/findApproxLevelEntry.cxx | 2 + panda/src/pgraph/pandaNode.I | 22 ++++++++ panda/src/pgraph/pandaNode.cxx | 68 ++++++++++++++++++++++- panda/src/pgraph/pandaNode.h | 4 ++ panda/src/pgraph/workingNodePath.I | 1 + panda/src/pgraph/workingNodePath.cxx | 1 + 8 files changed, 104 insertions(+), 1 deletion(-) diff --git a/panda/src/pgraph/config_pgraph.cxx b/panda/src/pgraph/config_pgraph.cxx index 75b4b48468..7a63ed3c42 100644 --- a/panda/src/pgraph/config_pgraph.cxx +++ b/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 " diff --git a/panda/src/pgraph/config_pgraph.h b/panda/src/pgraph/config_pgraph.h index c2801c54e9..5b8623011a 100644 --- a/panda/src/pgraph/config_pgraph.h +++ b/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; diff --git a/panda/src/pgraph/findApproxLevelEntry.cxx b/panda/src/pgraph/findApproxLevelEntry.cxx index bb42668875..020523726e 100644 --- a/panda/src/pgraph/findApproxLevelEntry.cxx +++ b/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 diff --git a/panda/src/pgraph/pandaNode.I b/panda/src/pgraph/pandaNode.I index a8fd8349f1..95e9c69904 100644 --- a/panda/src/pgraph/pandaNode.I +++ b/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 diff --git a/panda/src/pgraph/pandaNode.cxx b/panda/src/pgraph/pandaNode.cxx index 9ee62137b4..3164ac13f3 100644 --- a/panda/src/pgraph/pandaNode.cxx +++ b/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) { diff --git a/panda/src/pgraph/pandaNode.h b/panda/src/pgraph/pandaNode.h index 17cbe7bb18..83461984ba 100644 --- a/panda/src/pgraph/pandaNode.h +++ b/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, diff --git a/panda/src/pgraph/workingNodePath.I b/panda/src/pgraph/workingNodePath.I index 24f01ff997..10270eebaf 100644 --- a/panda/src/pgraph/workingNodePath.I +++ b/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); } //////////////////////////////////////////////////////////////////// diff --git a/panda/src/pgraph/workingNodePath.cxx b/panda/src/pgraph/workingNodePath.cxx index 42ed7366af..ef31a7276a 100644 --- a/panda/src/pgraph/workingNodePath.cxx +++ b/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(); }