From IRC: CSceneCollisionManager::getPickedNodeBB() unconditionally checks...

From IRC: CSceneCollisionManager::getPickedNodeBB() unconditionally checks collision against children, so visible children of invisible nodes can be hit.  This seems nonsensical, so we now only check children of visible nodes.

Also clarified the comments on ISceneNode::setVisible() and isVisible(), and added a new method, isTrulyVisible() that recursively checks any Parent nodes as well.

Unit/regression tests updated, verifying that they tested (and failed) with the old behaviour and pass with the new.

git-svn-id: svn://svn.code.sf.net/p/irrlicht/code/trunk@1978 dfc29bdd-3216-0410-991c-e03cc46cb475

include/ISceneNode.h

@@ -201,20 +201,34 @@ namespace scene
 		}
 
 
-		//! Returns true if the node is visible.
+		//! Returns whether the node should be visible (if all of its parents are visible).
 		/** This is only an option set by the user, but has nothing to
 		do with geometry culling
-		\return The visibility of the node, true means visible. */
+		\return The requested visibility of the node, true means visible (if all parents are also visible). */
 		virtual bool isVisible() const
 		{
 			_IRR_IMPLEMENT_MANAGED_MARSHALLING_BUGFIX;
 			return IsVisible;
 		}
 
+		//! Returns whether the node is truly visible, taking into accounts its parents' visibility
+		/** \return true if the node and all its parents are visible, false if this or any parent node is invisible. */
+		virtual bool isTrulyVisible() const
+		{
+			_IRR_IMPLEMENT_MANAGED_MARSHALLING_BUGFIX;
+			if(!IsVisible)
+				return false;
+
+			if(!Parent)
+				return true;
+
+			return Parent->isTrulyVisible();
+		}
 
 		//! Sets if the node should be visible or not.
 		/** All children of this node won't be visible either, when set
-		to false.
+		to false.  Invisible nodes are not valid candidates for selection by
+		collision manager bounding box methods.
 		\param isVisible If the node shall be visible. */
 		virtual void setVisible(bool isVisible)
 		{

source/Irrlicht/CSceneCollisionManager.cpp

@@ -88,44 +88,47 @@ void CSceneCollisionManager::getPickedNodeBB(ISceneNode* root,
    {
       ISceneNode* current = *it;
 
-      if (current->isVisible() &&
-          (bNoDebugObjects ? !current->isDebugObject() : true) &&
-          (bits==0 || (bits != 0 && (current->getID() & bits))))
-      {
-         // get world to object space transform
-         core::matrix4 mat;
-         if (!current->getAbsoluteTransformation().getInverse(mat))
-            continue;
-
-         // transform vector from world space to object space
-         core::line3df line(ray);
-         mat.transformVect(line.start);
-         mat.transformVect(line.end);
-
-         const core::aabbox3df& box = current->getBoundingBox();
-
-         // do intersection test in object space
-         if (box.intersectsWithLine(line))
-         {
-            box.getEdges(edges);
-            f32 distance = 0.0f;
-
-            for (s32 e=0; e<8; ++e)
-            {
-               f32 t = edges[e].getDistanceFromSQ(line.start);
-               if (t > distance)
-                  distance = t;
-            }
-
-            if (distance < outbestdistance)
-            {
-               outbestnode = current;
-               outbestdistance = distance;
-            }
-         }
-      }
-
-      getPickedNodeBB(current, ray, bits, bNoDebugObjects, outbestdistance, outbestnode);
+      if (current->isVisible())
+	  {
+		  if((bNoDebugObjects ? !current->isDebugObject() : true) &&
+			(bits==0 || (bits != 0 && (current->getID() & bits))))
+		  {
+			 // get world to object space transform
+			 core::matrix4 mat;
+			 if (!current->getAbsoluteTransformation().getInverse(mat))
+				continue;
+
+			 // transform vector from world space to object space
+			 core::line3df line(ray);
+			 mat.transformVect(line.start);
+			 mat.transformVect(line.end);
+
+			 const core::aabbox3df& box = current->getBoundingBox();
+
+			 // do intersection test in object space
+			 if (box.intersectsWithLine(line))
+			 {
+				box.getEdges(edges);
+				f32 distance = 0.0f;
+
+				for (s32 e=0; e<8; ++e)
+				{
+				   f32 t = edges[e].getDistanceFromSQ(line.start);
+				   if (t > distance)
+					  distance = t;
+				}
+
+				if (distance < outbestdistance)
+				{
+				   outbestnode = current;
+				   outbestdistance = distance;
+				}
+			 }
+		  }
+
+		  // Only check the children if this node is visible.
+	      getPickedNodeBB(current, ray, bits, bNoDebugObjects, outbestdistance, outbestnode);
+	  }
    }
 } 
 

tests/sceneCollisionManager.cpp

@@ -10,18 +10,10 @@ using namespace core;
 using namespace scene;
 using namespace video;
 
-
-/** Test functionality of the sceneCollisionManager */
-bool sceneCollisionManager(void)
+static bool testGetCollisionResultPosition(IrrlichtDevice * device,
+										   ISceneManager * smgr,
+										   ISceneCollisionManager * collMgr)
 {
-	IrrlichtDevice * device = irr::createDevice(video::EDT_NULL);
-	assert(device);
-	if(!device)
-		return false;
-
-	ISceneManager * smgr = device->getSceneManager();
-	ISceneCollisionManager * collMgr = smgr->getSceneCollisionManager();
-
 	IMeshSceneNode * cubeNode = smgr->addCubeSceneNode(10.f);
 	ITriangleSelector * cubeSelector = smgr->createTriangleSelectorFromBoundingBox(cubeNode);
 
@@ -77,6 +69,118 @@ bool sceneCollisionManager(void)
 	}
 
 	cubeSelector->drop();
+	smgr->clear();
+	return result;
+}
+
+
+static bool testGetSceneNodeFromScreenCoordinatesBB(IrrlichtDevice * device,
+													ISceneManager * smgr,
+													ISceneCollisionManager * collMgr)
+{
+	IMeshSceneNode * cubeNode1 = smgr->addCubeSceneNode(10.f, 0, -1, vector3df(0, 0, 20));
+	IMeshSceneNode * cubeNode2 = smgr->addCubeSceneNode(10.f, 0, -1, vector3df(0, 0, 30));
+	IMeshSceneNode * cubeNode3 = smgr->addCubeSceneNode(10.f, 0, -1, vector3df(0, 0, 40));
+
+	ICameraSceneNode * camera = smgr->addCameraSceneNode();
+	device->run();
+	smgr->drawAll(); // Get the camera in a good state
+
+	ISceneNode * hitNode = collMgr->getSceneNodeFromScreenCoordinatesBB(position2d<s32>(80, 60));
+
+	// Expect the first node to be hit.
+	bool result = true;
+	if(hitNode != cubeNode1)
+	{
+		logTestString("Unexpected node hit. Expected cubeNode1.\n");
+		result = false;
+	}
+
+	// Now make cubeNode1 invisible and check that cubeNode2 is hit.
+	cubeNode1->setVisible(false);
+	hitNode = collMgr->getSceneNodeFromScreenCoordinatesBB(position2d<s32>(80, 60));
+	if(hitNode != cubeNode2)
+	{
+		logTestString("Unexpected node hit. Expected cubeNode2.\n");
+		result = false;
+	}
+
+	// Make cubeNode1 the parent of cubeNode2.  
+	cubeNode2->setParent(cubeNode1);
+
+	// Check visibility.
+	bool visible = cubeNode2->isVisible();
+	if(!visible)
+	{
+		logTestString("cubeNode2 should think that it (in isolation) is visible.\n");
+		result = false;
+	}
+
+	visible = cubeNode2->isTrulyVisible();
+	if(visible)
+	{
+		logTestString("cubeNode2 should know that it (recursively) is invisible.\n");
+		result = false;
+	}
+
+	// cubeNode2 should now be an invalid target as well, and so the final cube node should be hit.
+	hitNode = collMgr->getSceneNodeFromScreenCoordinatesBB(position2d<s32>(80, 60));
+	if(hitNode != cubeNode3)
+	{
+		logTestString("Unexpected node hit. Expected cubeNode3.\n");
+		result = false;
+	}
+
+
+	// Make cubeNode3 invisible and check that the camera node is hit (since it has a valid bounding box).
+	cubeNode3->setVisible(false);
+	hitNode = collMgr->getSceneNodeFromScreenCoordinatesBB(position2d<s32>(80, 60));
+	if(hitNode != camera)
+	{
+		logTestString("Unexpected node hit. Expected the camera node.\n");
+		result = false;
+	}
+
+	// Now verify bitmasking
+	camera->setID(0xAAAAAAAA); // == 101010101010101010101010101010
+	hitNode = collMgr->getSceneNodeFromScreenCoordinatesBB(position2d<s32>(80, 60), 0x02);
+	if(hitNode != camera)
+	{
+		logTestString("Unexpected node hit. Expected the camera node.\n");
+		result = false;
+	}
+
+	// Test the 01010101010101010101010101010101 bitmask (0x55555555)
+	hitNode = collMgr->getSceneNodeFromScreenCoordinatesBB(position2d<s32>(80, 60), 0x55555555);
+	if(hitNode != 0)
+	{
+		logTestString("A node was hit when none was expected.\n");
+		result = false;
+	}
+
+	smgr->clear();
+
+	if(!result)
+		assert(false);
+	return result;
+}
+
+
+/** Test functionality of the sceneCollisionManager */
+bool sceneCollisionManager(void)
+{
+	IrrlichtDevice * device = irr::createDevice(video::EDT_NULL, dimension2d<s32>(160, 120));
+	assert(device);
+	if(!device)
+		return false;
+
+	ISceneManager * smgr = device->getSceneManager();
+	ISceneCollisionManager * collMgr = smgr->getSceneCollisionManager();
+
+	bool result = testGetCollisionResultPosition(device, smgr, collMgr);
+	
+	result &= testGetSceneNodeFromScreenCoordinatesBB(device, smgr, collMgr);
+
 	device->drop();
 	return result;
 }

tests/tests-last-passed-at.txt

-Test suite pass at GMT Thu Dec 18 15:24:34 2008
+Test suite pass at GMT Fri Dec 19 11:49:34 2008