Merge pull request #45311 from lawnjelly/bvh_fixed_updates

BVH render tree new updating scheme

core/math/bvh.h

@@ -175,6 +175,11 @@ public:
 #endif
 	}
 
+	// this can be called more frequently than per frame if necessary
+	void update_collisions() {
+		_check_for_collisions();
+	}
+
 	// prefer calling this directly as type safe
 	void set_pairable(const BVHHandle &p_handle, bool p_pairable, uint32_t p_pairable_type, uint32_t p_pairable_mask) {
 		tree.item_set_pairable(p_handle, p_pairable, p_pairable_type, p_pairable_mask);
@@ -191,7 +196,10 @@ public:
 			_add_changed_item(p_handle, aabb, false);
 
 			// force an immediate collision check (probably just for this one item)
-			_check_for_collisions();
+			// but it must be a FULL collision check, also checking pairable state and masks.
+			// This is because AABB intersecting objects may have changed pairable state / mask
+			// such that they should no longer be paired. E.g. lights.
+			_check_for_collisions(true);
 		}
 	}
 
@@ -275,7 +283,12 @@ public:
 
 private:
 	// do this after moving etc.
-	void _check_for_collisions() {
+	void _check_for_collisions(bool p_full_check = false) {
+		if (!changed_items.size()) {
+			// noop
+			return;
+		}
+
 		AABB bb;
 
 		typename BVHTREE_CLASS::CullParams params;
@@ -297,7 +310,7 @@ private:
 
 			// find all the existing paired aabbs that are no longer
 			// paired, and send callbacks
-			_find_leavers(h, abb);
+			_find_leavers(h, abb, p_full_check);
 
 			uint32_t changed_item_ref_id = h.id();
 
@@ -369,13 +382,31 @@ private:
 	}
 
 	// returns true if unpair
-	bool _find_leavers_process_pair(typename BVHTREE_CLASS::ItemPairs &p_pairs_from, const BVH_ABB &p_abb_from, BVHHandle p_from, BVHHandle p_to) {
+	bool _find_leavers_process_pair(typename BVHTREE_CLASS::ItemPairs &p_pairs_from, const BVH_ABB &p_abb_from, BVHHandle p_from, BVHHandle p_to, bool p_full_check) {
 		BVH_ABB abb_to;
 		tree.item_get_ABB(p_to, abb_to);
 
 		// do they overlap?
-		if (p_abb_from.intersects(abb_to))
-			return false;
+		if (p_abb_from.intersects(abb_to)) {
+			// the full check for pairable / non pairable and mask changes is extra expense
+			// this need not be done in most cases (for speed) except in the case where set_pairable is called
+			// where the masks etc of the objects in question may have changed
+			if (!p_full_check) {
+				return false;
+			}
+			const typename BVHTREE_CLASS::ItemExtra &exa = _get_extra(p_from);
+			const typename BVHTREE_CLASS::ItemExtra &exb = _get_extra(p_to);
+
+			// one of the two must be pairable to still pair
+			// if neither are pairable, we always unpair
+			if (exa.pairable || exb.pairable) {
+				// the masks must still be compatible to pair
+				// i.e. if there is a hit between the two, then they should stay paired
+				if (tree._cull_pairing_mask_test_hit(exa.pairable_mask, exa.pairable_type, exb.pairable_mask, exb.pairable_type)) {
+					return false;
+				}
+			}
+		}
 
 		_unpair(p_from, p_to);
 		return true;
@@ -383,18 +414,15 @@ private:
 
 	// find all the existing paired aabbs that are no longer
 	// paired, and send callbacks
-	void _find_leavers(BVHHandle p_handle, const BVH_ABB &expanded_abb_from) {
+	void _find_leavers(BVHHandle p_handle, const BVH_ABB &expanded_abb_from, bool p_full_check) {
 		typename BVHTREE_CLASS::ItemPairs &p_from = tree._pairs[p_handle.id()];
 
-		// opportunity to de-extend pairs, before removing leavers
-		p_from.update();
-
 		BVH_ABB abb_from = expanded_abb_from;
 
 		// remove from pairing list for every partner
 		for (unsigned int n = 0; n < p_from.extended_pairs.size(); n++) {
 			BVHHandle h_to = p_from.extended_pairs[n].handle;
-			if (_find_leavers_process_pair(p_from, abb_from, p_handle, h_to)) {
+			if (_find_leavers_process_pair(p_from, abb_from, p_handle, h_to, p_full_check)) {
 				// we need to keep the counter n up to date if we deleted a pair
 				// as the number of items in p_from.extended_pairs will have decreased by 1
 				// and we don't want to miss an item
@@ -465,11 +493,12 @@ private:
 
 	void _add_changed_item(BVHHandle p_handle, const AABB &aabb, bool p_check_aabb = true) {
 
-		// only if uses pairing
-		// no .. non pairable items seem to be able to pair with pairable
+		// Note that non pairable items can pair with pairable,
+		// so all types must be added to the list
 
 		// aabb check with expanded aabb. This greatly decreases processing
 		// at the cost of slightly less accurate pairing checks
+		// Note this pairing AABB is separate from the AABB in the actual tree
 		AABB &expanded_aabb = tree._pairs[p_handle.id()].expanded_aabb;
 
 		// passing p_check_aabb false disables the optimization which prevents collision checks if
@@ -478,6 +507,13 @@ private:
 		if (p_check_aabb && expanded_aabb.encloses(aabb))
 			return;
 
+		// ALWAYS update the new expanded aabb, even if already changed once
+		// this tick, because it is vital that the AABB is kept up to date
+		expanded_aabb = aabb;
+		expanded_aabb.grow_by(tree._pairing_expansion);
+
+		// this code is to ensure that changed items only appear once on the updated list
+		// collision checking them multiple times is not needed, and repeats the same thing
 		uint32_t &last_updated_tick = tree._extra[p_handle.id()].last_updated_tick;
 
 		if (last_updated_tick == _tick)
@@ -486,12 +522,7 @@ private:
 		// mark as on list
 		last_updated_tick = _tick;
 
-		// opportunity to de-extend pairs (before collision detection, which will delete then recreate pairs)
-
-		// new expanded aabb
-		expanded_aabb = aabb;
-		expanded_aabb.grow_by(tree._pairing_expansion);
-
+		// add to the list
 		changed_items.push_back(p_handle);
 	}
 

core/math/bvh_cull.inc

@@ -133,6 +133,22 @@ bool _cull_hits_full(const CullParams &p) {
 	return (int)_cull_hits.size() >= p.result_max;
 }
 
+// write this logic once for use in all routines
+// double check this as a possible source of bugs in future.
+bool _cull_pairing_mask_test_hit(uint32_t p_maskA, uint32_t p_typeA, uint32_t p_maskB, uint32_t p_typeB) const {
+	// double check this as a possible source of bugs in future.
+	bool A_match_B = p_maskA & p_typeB;
+
+	if (!A_match_B) {
+		bool B_match_A = p_maskB & p_typeA;
+		if (!B_match_A) {
+			return false;
+		}
+	}
+
+	return true;
+}
+
 void _cull_hit(uint32_t p_ref_id, CullParams &p) {
 
 	// take into account masks etc
@@ -141,13 +157,8 @@ void _cull_hit(uint32_t p_ref_id, CullParams &p) {
 	if (USE_PAIRS) {
 		const ItemExtra &ex = _extra[p_ref_id];
 
-		// double check this as a possible source of bugs in future.
-		bool from_match_to = p.mask & ex.pairable_type;
-
-		if (!from_match_to) {
-			bool to_match_from = ex.pairable_mask & p.pairable_type;
-			if (!to_match_from)
-				return;
+		if (!_cull_pairing_mask_test_hit(p.mask, p.pairable_type, ex.pairable_mask, ex.pairable_type)) {
+			return;
 		}
 	}
 

core/math/bvh_pair.inc

@@ -58,7 +58,4 @@ struct ItemPairs {
 
 		return userdata;
 	}
-
-	void update() {
-	}
 };

servers/visual/visual_server_raster.cpp

@@ -104,6 +104,7 @@ void VisualServerRaster::draw(bool p_swap_buffers, double frame_step) {
 	VSG::rasterizer->begin_frame(frame_step);
 
 	VSG::scene->update_dirty_instances(); //update scene stuff
+	VSG::scene->update_scenarios(); // render tree collision detection (pairing)
 
 	VSG::viewport->draw_viewports();
 	VSG::scene->render_probes();

servers/visual/visual_server_scene.cpp

@@ -120,6 +120,10 @@ void VisualServerScene::SpatialPartitioningScene_BVH::update() {
 	_bvh.update();
 }
 
+void VisualServerScene::SpatialPartitioningScene_BVH::update_collisions() {
+	_bvh.update_collisions();
+}
+
 void VisualServerScene::SpatialPartitioningScene_BVH::set_pairable(SpatialPartitionID p_handle, bool p_pairable, uint32_t p_pairable_type, uint32_t p_pairable_mask) {
 	_bvh.set_pairable(p_handle - 1, p_pairable, p_pairable_type, p_pairable_mask);
 }
@@ -191,7 +195,7 @@ void VisualServerScene::SpatialPartitioningScene_Octree::set_balance(float p_bal
 VisualServerScene::Scenario::Scenario() {
 	debug = VS::SCENARIO_DEBUG_DISABLED;
 
-	bool use_bvh_or_octree = GLOBAL_DEF("rendering/quality/spatial_partitioning/use_bvh", true);
+	bool use_bvh_or_octree = GLOBAL_GET("rendering/quality/spatial_partitioning/use_bvh");
 
 	if (use_bvh_or_octree) {
 		sps = memnew(SpatialPartitioningScene_BVH);
@@ -3574,24 +3578,47 @@ void VisualServerScene::_update_dirty_instance(Instance *p_instance) {
 	p_instance->update_materials = false;
 }
 
+void VisualServerScene::update_scenarios() {
+	// go through all scenarios and update BVH in each
+	if (!_use_bvh) {
+		return;
+	}
+
+	List<RID> owned;
+	scenario_owner.get_owned_list(&owned);
+
+	for (List<RID>::Element *E = owned.front(); E; E = E->next()) {
+		RID rid = E->get();
+		Scenario *scenario = scenario_owner.get(rid);
+
+		scenario->sps->update();
+	}
+}
+
 void VisualServerScene::update_dirty_instances() {
 
 	VSG::storage->update_dirty_resources();
 
+	// only define this if you run into problems with missed pairing collisions for debugging
+//#define GODOT_RENDER_SPS_EXTRA_COLLISION_CHECKS
+#ifdef GODOT_RENDER_SPS_EXTRA_COLLISION_CHECKS
 	// this is just to get access to scenario so we can update the spatial partitioning scheme
 	Scenario *scenario = nullptr;
 	if (_instance_update_list.first()) {
 		scenario = _instance_update_list.first()->self()->scenario;
 	}
+#endif
 
 	while (_instance_update_list.first()) {
 
 		_update_dirty_instance(_instance_update_list.first()->self());
 	}
 
+#ifdef GODOT_RENDER_SPS_EXTRA_COLLISION_CHECKS
 	if (scenario) {
-		scenario->sps->update();
+		scenario->sps->update_collisions();
 	}
+#endif
 }
 
 bool VisualServerScene::free(RID p_rid) {
@@ -3652,7 +3679,7 @@ VisualServerScene::VisualServerScene() {
 
 	render_pass = 1;
 	singleton = this;
-	_use_bvh = false;
+	_use_bvh = GLOBAL_DEF("rendering/quality/spatial_partitioning/use_bvh", true);
 }
 
 VisualServerScene::~VisualServerScene() {

servers/visual/visual_server_scene.h

@@ -118,6 +118,7 @@ public:
 		virtual void erase(SpatialPartitionID p_handle) = 0;
 		virtual void move(SpatialPartitionID p_handle, const AABB &p_aabb) = 0;
 		virtual void update() {}
+		virtual void update_collisions() {}
 		virtual void set_pairable(SpatialPartitionID p_handle, bool p_pairable, uint32_t p_pairable_type, uint32_t p_pairable_mask) = 0;
 		virtual int cull_convex(const Vector<Plane> &p_convex, Instance **p_result_array, int p_result_max, uint32_t p_mask = 0xFFFFFFFF) = 0;
 		virtual int cull_aabb(const AABB &p_aabb, Instance **p_result_array, int p_result_max, int *p_subindex_array = nullptr, uint32_t p_mask = 0xFFFFFFFF) = 0;
@@ -164,6 +165,7 @@ public:
 		void erase(SpatialPartitionID p_handle);
 		void move(SpatialPartitionID p_handle, const AABB &p_aabb);
 		void update();
+		void update_collisions();
 		void set_pairable(SpatialPartitionID p_handle, bool p_pairable, uint32_t p_pairable_type, uint32_t p_pairable_mask);
 		int cull_convex(const Vector<Plane> &p_convex, Instance **p_result_array, int p_result_max, uint32_t p_mask = 0xFFFFFFFF);
 		int cull_aabb(const AABB &p_aabb, Instance **p_result_array, int p_result_max, int *p_subindex_array = nullptr, uint32_t p_mask = 0xFFFFFFFF);
@@ -552,6 +554,7 @@ public:
 	void render_camera(RID p_camera, RID p_scenario, Size2 p_viewport_size, RID p_shadow_atlas);
 	void render_camera(Ref<ARVRInterface> &p_interface, ARVRInterface::Eyes p_eye, RID p_camera, RID p_scenario, Size2 p_viewport_size, RID p_shadow_atlas);
 	void update_dirty_instances();
+	void update_scenarios();
 
 	//probes
 	struct GIProbeDataHeader {