From bd42de5fea718ed6bf680dddbb8d823dd6760b68 Mon Sep 17 00:00:00 2001 From: lawnjelly Date: Sat, 29 May 2021 18:03:43 +0100 Subject: [PATCH] BVH - fix stale current_tree in deactivate function [4.x] Changes passing of current_tree from a member variable to a function argument, making bugs due to stale state less likely. Fix a bug in deactivate where current_tree variable was stale. This may have resulted in visual anomalies. --- core/math/bvh_logic.inc | 14 ++++++------ core/math/bvh_public.inc | 46 ++++++++++++++++++++------------------- core/math/bvh_refit.inc | 4 ++-- core/math/bvh_structs.inc | 1 - core/math/bvh_tree.h | 15 ++++++------- 5 files changed, 40 insertions(+), 40 deletions(-) diff --git a/core/math/bvh_logic.inc b/core/math/bvh_logic.inc index d84c3f78300..afab08f151a 100644 --- a/core/math/bvh_logic.inc +++ b/core/math/bvh_logic.inc @@ -20,17 +20,17 @@ void _logic_item_remove_and_reinsert(uint32_t p_ref_id) { // some overlay elaborate way to find out which tree the node is in! BVHHandle temp_handle; temp_handle.set_id(p_ref_id); - _current_tree = _handle_get_tree_id(temp_handle); + uint32_t tree_id = _handle_get_tree_id(temp_handle); // remove and reinsert BVHABB_CLASS abb; - node_remove_item(p_ref_id, &abb); + node_remove_item(p_ref_id, tree_id, &abb); // we must choose where to add to tree - ref.tnode_id = _logic_choose_item_add_node(_root_node_id[_current_tree], abb); + ref.tnode_id = _logic_choose_item_add_node(_root_node_id[tree_id], abb); _node_add_item(ref.tnode_id, p_ref_id, abb); - refit_upward_and_balance(ref.tnode_id); + refit_upward_and_balance(ref.tnode_id, tree_id); } // from randy gaul balance function @@ -66,7 +66,7 @@ BVHABB_CLASS _logic_abb_merge(const BVHABB_CLASS &a, const BVHABB_CLASS &b) { // https://github.com/RandyGaul/qu3e // It is MODIFIED from qu3e version. // This is the only function used (and _logic_abb_merge helper function). -int32_t _logic_balance(int32_t iA) { +int32_t _logic_balance(int32_t iA, uint32_t p_tree_id) { // return iA; // uncomment this to bypass balance TNode *A = &_nodes[iA]; @@ -107,7 +107,7 @@ int32_t _logic_balance(int32_t iA) { } } else { // check this .. seems dodgy - change_root_node(iC); + change_root_node(iC, p_tree_id); } // Swap A and C @@ -159,7 +159,7 @@ int32_t _logic_balance(int32_t iA) { else { // check this .. seems dodgy - change_root_node(iB); + change_root_node(iB, p_tree_id); } // Swap A and B diff --git a/core/math/bvh_public.inc b/core/math/bvh_public.inc index f1b6d6b1bff..2c1e4067126 100644 --- a/core/math/bvh_public.inc +++ b/core/math/bvh_public.inc @@ -53,16 +53,16 @@ BVHHandle item_add(T *p_userdata, bool p_active, const Bounds &p_aabb, int32_t p // assign to handle to return handle.set_id(ref_id); - _current_tree = 0; + uint32_t tree_id = 0; if (p_pairable) { - _current_tree = 1; + tree_id = 1; } - create_root_node(_current_tree); + create_root_node(tree_id); // we must choose where to add to tree if (p_active) { - ref->tnode_id = _logic_choose_item_add_node(_root_node_id[_current_tree], abb); + ref->tnode_id = _logic_choose_item_add_node(_root_node_id[tree_id], abb); bool refit = _node_add_item(ref->tnode_id, ref_id, abb); @@ -70,7 +70,7 @@ BVHHandle item_add(T *p_userdata, bool p_active, const Bounds &p_aabb, int32_t p // only need to refit from the parent const TNode &add_node = _nodes[ref->tnode_id]; if (add_node.parent_id != BVHCommon::INVALID) { - refit_upward_and_balance(add_node.parent_id); + refit_upward_and_balance(add_node.parent_id, tree_id); } } } else { @@ -139,13 +139,13 @@ bool item_move(BVHHandle p_handle, const Bounds &p_aabb) { return true; } - _current_tree = _handle_get_tree_id(p_handle); + uint32_t tree_id = _handle_get_tree_id(p_handle); // remove and reinsert - node_remove_item(ref_id); + node_remove_item(ref_id, tree_id); // we must choose where to add to tree - ref.tnode_id = _logic_choose_item_add_node(_root_node_id[_current_tree], abb); + ref.tnode_id = _logic_choose_item_add_node(_root_node_id[tree_id], abb); // add to the tree bool needs_refit = _node_add_item(ref.tnode_id, ref_id, abb); @@ -167,7 +167,7 @@ bool item_move(BVHHandle p_handle, const Bounds &p_aabb) { void item_remove(BVHHandle p_handle) { uint32_t ref_id = p_handle.id(); - _current_tree = _handle_get_tree_id(p_handle); + uint32_t tree_id = _handle_get_tree_id(p_handle); VERBOSE_PRINT("item_remove [" + itos(ref_id) + "] "); @@ -187,7 +187,7 @@ void item_remove(BVHHandle p_handle) { // remove the item from the node (only if active) if (_refs[ref_id].is_active()) { - node_remove_item(ref_id); + node_remove_item(ref_id, tree_id); } // remove the item reference @@ -198,10 +198,10 @@ void item_remove(BVHHandle p_handle) { } // don't think refit_all is necessary? - //refit_all(_current_tree); + //refit_all(_tree_id); #ifdef BVH_VERBOSE_TREE - _debug_recursive_print_tree(_current_tree); + _debug_recursive_print_tree(tree_id); #endif } @@ -218,13 +218,13 @@ bool item_activate(BVHHandle p_handle, const Bounds &p_aabb) { BVHABB_CLASS abb; abb.from(p_aabb); - _current_tree = _handle_get_tree_id(p_handle); + uint32_t tree_id = _handle_get_tree_id(p_handle); // we must choose where to add to tree - ref.tnode_id = _logic_choose_item_add_node(_root_node_id[_current_tree], abb); + ref.tnode_id = _logic_choose_item_add_node(_root_node_id[tree_id], abb); _node_add_item(ref.tnode_id, ref_id, abb); - refit_upward_and_balance(ref.tnode_id); + refit_upward_and_balance(ref.tnode_id, tree_id); return true; } @@ -238,9 +238,11 @@ bool item_deactivate(BVHHandle p_handle) { return false; } + uint32_t tree_id = _handle_get_tree_id(p_handle); + // remove from tree BVHABB_CLASS abb; - node_remove_item(ref_id, &abb); + node_remove_item(ref_id, tree_id, &abb); // mark as inactive ref.set_inactive(); @@ -304,21 +306,21 @@ bool item_set_pairable(const BVHHandle &p_handle, bool p_pairable, uint32_t p_pa BVHABB_CLASS abb = leaf.get_aabb(ref.item_id); // make sure current tree is correct prior to changing - _current_tree = _handle_get_tree_id(p_handle); + uint32_t tree_id = _handle_get_tree_id(p_handle); // remove from old tree - node_remove_item(ref_id); + node_remove_item(ref_id, tree_id); // we must set the pairable AFTER getting the current tree // because the pairable status determines which tree ex.pairable = p_pairable; // add to new tree - _current_tree = _handle_get_tree_id(p_handle); - create_root_node(_current_tree); + tree_id = _handle_get_tree_id(p_handle); + create_root_node(tree_id); // we must choose where to add to tree - ref.tnode_id = _logic_choose_item_add_node(_root_node_id[_current_tree], abb); + ref.tnode_id = _logic_choose_item_add_node(_root_node_id[tree_id], abb); bool needs_refit = _node_add_item(ref.tnode_id, ref_id, abb); // only need to refit from the PARENT @@ -326,7 +328,7 @@ bool item_set_pairable(const BVHHandle &p_handle, bool p_pairable, uint32_t p_pa // only need to refit from the parent const TNode &add_node = _nodes[ref.tnode_id]; if (add_node.parent_id != BVHCommon::INVALID) { - refit_upward_and_balance(add_node.parent_id); + refit_upward_and_balance(add_node.parent_id, tree_id); } } } else { diff --git a/core/math/bvh_refit.inc b/core/math/bvh_refit.inc index 514c853ac54..717a3438c7c 100644 --- a/core/math/bvh_refit.inc +++ b/core/math/bvh_refit.inc @@ -66,10 +66,10 @@ void refit_upward(uint32_t p_node_id) { } } -void refit_upward_and_balance(uint32_t p_node_id) { +void refit_upward_and_balance(uint32_t p_node_id, uint32_t p_tree_id) { while (p_node_id != BVHCommon::INVALID) { uint32_t before = p_node_id; - p_node_id = _logic_balance(p_node_id); + p_node_id = _logic_balance(p_node_id, p_tree_id); if (before != p_node_id) { VERBOSE_PRINT("REBALANCED!"); diff --git a/core/math/bvh_structs.inc b/core/math/bvh_structs.inc index 4133ba6c101..1d1e0e64680 100644 --- a/core/math/bvh_structs.inc +++ b/core/math/bvh_structs.inc @@ -162,7 +162,6 @@ enum { NUM_TREES = 2, // Tree 1 - Pairable // This is more efficient because in physics we only need check non pairable against the pairable tree. uint32_t _root_node_id[NUM_TREES]; -int _current_tree = 0; // these values may need tweaking according to the project // the bound of the world, and the average velocities of the objects diff --git a/core/math/bvh_tree.h b/core/math/bvh_tree.h index 64c5f6e254c..3169d31ec77 100644 --- a/core/math/bvh_tree.h +++ b/core/math/bvh_tree.h @@ -196,7 +196,7 @@ private: new_child.parent_id = p_parent_id; } - void node_remove_child(uint32_t p_parent_id, uint32_t p_child_id, bool p_prevent_sibling = false) { + void node_remove_child(uint32_t p_parent_id, uint32_t p_child_id, uint32_t p_tree_id, bool p_prevent_sibling = false) { TNode &parent = _nodes[p_parent_id]; BVH_ASSERT(!parent.is_leaf()); @@ -231,7 +231,7 @@ private: if (grandparent_id == BVHCommon::INVALID) { if (sibling_present) { // change the root node - change_root_node(sibling_id); + change_root_node(sibling_id, p_tree_id); // delete the old root node as no longer needed _nodes.free(p_parent_id); @@ -243,16 +243,15 @@ private: if (sibling_present) { node_replace_child(grandparent_id, p_parent_id, sibling_id); } else { - node_remove_child(grandparent_id, p_parent_id, true); + node_remove_child(grandparent_id, p_parent_id, p_tree_id, true); } // put the node on the free list to recycle _nodes.free(p_parent_id); } - // this relies on _current_tree being accurate - void change_root_node(uint32_t p_new_root_id) { - _root_node_id[_current_tree] = p_new_root_id; + void change_root_node(uint32_t p_new_root_id, uint32_t p_tree_id) { + _root_node_id[p_tree_id] = p_new_root_id; TNode &root = _nodes[p_new_root_id]; // mark no parent @@ -272,7 +271,7 @@ private: node.neg_leaf_id = -(int)child_leaf_id; } - void node_remove_item(uint32_t p_ref_id, BVHABB_CLASS *r_old_aabb = nullptr) { + void node_remove_item(uint32_t p_ref_id, uint32_t p_tree_id, BVHABB_CLASS *r_old_aabb = nullptr) { // get the reference ItemRef &ref = _refs[p_ref_id]; uint32_t owner_node_id = ref.tnode_id; @@ -336,7 +335,7 @@ private: uint32_t parent_id = tnode.parent_id; - node_remove_child(parent_id, owner_node_id); + node_remove_child(parent_id, owner_node_id, p_tree_id); refit_upward(parent_id); // put the node on the free list to recycle