[Fixed]setKeyMap doesn't update member KeyMapArray

[MasterGod]

Sorry for not reporting it as I should (with test code etc.) but I hope it would be clear why this is a bug.

The issue is about CSceneNodeAnimatorCameraFPS not updating its SKeyMap* KeyMapArray member when setting a new key map using the setKeyMap() method.

Let me explain:
When you create a new animator it sets in the initialization list of the ctor the key map argument to the correct member:

Code: Select all

KeyMapArray(keyMapArray), KeyMapSize(keyMapSize)

then calls setKeyMap(KeyMapArray, KeyMapSize); which is fine.
Now if I want a copy of this animator I'll just call createClone() method, right? because it copies the members to a new instance:

Code: Select all

ISceneNodeAnimator* CSceneNodeAnimatorCameraFPS::createClone(ISceneNode* node, ISceneManager* newManager)
{
	CSceneNodeAnimatorCameraFPS * newAnimator = 
		new CSceneNodeAnimatorCameraFPS(CursorControl,	RotateSpeed, (MoveSpeed * 1000.0f), JumpSpeed,
											KeyMapArray, KeyMapSize, NoVerticalMovement);
	return newAnimator;
}

Now lets say I've changed the key map using setKeyMap() and called createClone() again, it won't copy the new key map because the setKetMap() call didn't copy the new key map to the right member but only updated the array (which is why this bug probably wasn't noticed before) and createClone() copies the SKeyMap* KeyMapArray member and not the core::array<SCamKeyMap> KeyMap member.

A quick fix would be I guess to add the following line to setKeyMap:

Code: Select all

//! Sets the keyboard mapping for this animator
void CSceneNodeAnimatorCameraFPS::setKeyMap(SKeyMap *map, u32 count)
{
        // those lines
	memcpy(KeyMapArray, map, count);
	KeyMapSize = count;

	// clear the keymap
	KeyMap.clear();

edit: Rethinking, memcpy() might not be the right solution but you understand the problem/solution.
This way the key map member would always have the latest values and the clone method would clone the right values.

[MasterGod]

A better fix could be:
SKeyMap.h
Adding this:

Code: Select all

//! Struct storing which key belongs to which action.
	struct SKeyMap
	{
		EKEY_ACTION Action;
		EKEY_CODE KeyCode;

		static SKeyMap * copyArray(SKeyMap *pMapArray, u32 ElementsCount)
		{
			SKeyMap *pNewMapArray = 0;

			if (pMapArray)
			{
				pNewMapArray = new SKeyMap[ElementsCount];
				
				memcpy(pNewMapArray, pMapArray, ElementsCount);
			}

			return pNewMapArray;
		}
	};

And in CSceneNodeAnimatorCameraFPS.cpp:

Code: Select all

//! Sets the keyboard mapping for this animator
void CSceneNodeAnimatorCameraFPS::setKeyMap(SKeyMap *map, u32 count)
{
	KeyMapArray = SKeyMap::copyArray(map, count);
	KeyMapSize = count;

	// clear the keymap
	KeyMap.clear();

and in the constructor:

Code: Select all

KeyMapArray(SKeyMap::copyArray(keyMapArray, keyMapSize)), KeyMapSize(keyMapSize)

destructor:

Code: Select all

//! destructor
CSceneNodeAnimatorCameraFPS::~CSceneNodeAnimatorCameraFPS()
{
	if (CursorControl)
		CursorControl->drop();

	if (KeyMapArray)
		delete[] KeyMapArray;
}

That should be a safer fix then the one I suggested in the previous post.

[hybrid]

I don't see any problem with the current code. The setKeyMap properly copies the parameter array into the member, hence everythign is properly updated. So please show full compilable code which reproduces this problem (if there is one).

[rogerborg]

This looks like a job for Test Case Man, with the power of reproducibility.

[MasterGod]

Here's a test case which does the following:
Creates an empty camera.
Creates a FPS animator.
Sets a key map for the animator.
Sets a new key map for the animator.
Creates a clone of the animator.
Sets the new clone as the camera animator.

Expected behavior is that the camera would use the new key map.
Actual behavior is that the KeyMapArray pointer is NULL in the clone hence no key map is active for the camera with the cloned animator.

Code: Select all

#include <irrlicht.h>
#include <ISceneNodeAnimatorCameraFPS.h> 

using namespace irr;
using namespace core;
using namespace scene;
using namespace video;
using namespace io;
using namespace gui;

#ifdef _IRR_WINDOWS_
#pragma comment(lib, "Irrlicht.lib")
#pragma comment(linker, "/subsystem:windows /ENTRY:mainCRTStartup")
#endif

int main()
{
	IrrlichtDevice *device =
#ifdef _IRR_OSX_PLATFORM_
		createDevice( video::EDT_OPENGL, dimension2d<s32>(640, 480), 16,
		false, false, false, 0);
#else
		createDevice( video::EDT_OPENGL, dimension2d<s32>(640, 480), 32,
		false, false, false, 0);
#endif
	if (!device)
		return 1;

	device->setWindowCaption(L"Hello World! - Irrlicht Engine Demo");

	IVideoDriver* driver = device->getVideoDriver();
	ISceneManager* smgr = device->getSceneManager();
	IGUIEnvironment* guienv = device->getGUIEnvironment();

	guienv->addStaticText(L"Hello World! This is the Irrlicht Software renderer!",
		rect<s32>(10,10,260,22), true);

	IAnimatedMesh* mesh = smgr->getMesh("../../media/sydney.md2");
	if (!mesh)
		return 1;
	IAnimatedMeshSceneNode* node = smgr->addAnimatedMeshSceneNode( mesh );

	if (node)
	{
		node->setMaterialFlag(EMF_LIGHTING, false);
		node->setMD2Animation(scene::EMAT_STAND);
		node->setMaterialTexture( 0, driver->getTexture("../../media/sydney.bmp") );
	}

	// First key map
	SKeyMap keyMap[8];
	keyMap[0].Action = EKA_MOVE_FORWARD;
	keyMap[0].KeyCode = KEY_UP;
	keyMap[1].Action = EKA_MOVE_FORWARD;
	keyMap[1].KeyCode = KEY_KEY_W;

	keyMap[2].Action = EKA_MOVE_BACKWARD;
	keyMap[2].KeyCode = KEY_DOWN;
	keyMap[3].Action = EKA_MOVE_BACKWARD;
	keyMap[3].KeyCode = KEY_KEY_S;

	keyMap[4].Action = EKA_STRAFE_LEFT;
	keyMap[4].KeyCode = KEY_LEFT;
	keyMap[5].Action = EKA_STRAFE_LEFT;
	keyMap[5].KeyCode = KEY_KEY_A;

	keyMap[6].Action = EKA_STRAFE_RIGHT;
	keyMap[6].KeyCode = KEY_RIGHT;
	keyMap[7].Action = EKA_STRAFE_RIGHT;
	keyMap[7].KeyCode = KEY_KEY_D;

	// New key map
	SKeyMap NewKeyMap[8];
	NewKeyMap[0].Action = EKA_MOVE_FORWARD;
	NewKeyMap[0].KeyCode = KEY_UP;
	NewKeyMap[1].Action = EKA_MOVE_FORWARD;
	NewKeyMap[1].KeyCode = KEY_KEY_I;

	NewKeyMap[2].Action = EKA_MOVE_BACKWARD;
	NewKeyMap[2].KeyCode = KEY_DOWN;
	NewKeyMap[3].Action = EKA_MOVE_BACKWARD;
	NewKeyMap[3].KeyCode = KEY_KEY_K;

	NewKeyMap[4].Action = EKA_STRAFE_LEFT;
	NewKeyMap[4].KeyCode = KEY_LEFT;
	NewKeyMap[5].Action = EKA_STRAFE_LEFT;
	NewKeyMap[5].KeyCode = KEY_KEY_J;

	NewKeyMap[6].Action = EKA_STRAFE_RIGHT;
	NewKeyMap[6].KeyCode = KEY_RIGHT;
	NewKeyMap[7].Action = EKA_STRAFE_RIGHT;
	NewKeyMap[7].KeyCode = KEY_KEY_L;

	// Adding a dumb camera
	ICameraSceneNode *cam = smgr->addCameraSceneNode(0, vector3df(0,30,-40), vector3df(0,5,0));

	// Creating a fps scene node animator and setting it to the camera
	ISceneNodeAnimatorCameraFPS *pFPSAnimator = (ISceneNodeAnimatorCameraFPS *)smgr->getSceneNodeAnimatorFactory(0)->createSceneNodeAnimator(ESNAT_CAMERA_FPS, cam);

	// Setting cam the first key map
	pFPSAnimator->setKeyMap(keyMap, 8);
	
	// Setting cam the new key map
	pFPSAnimator->setKeyMap(NewKeyMap, 8);

	// Creating a clone of the animator
	ISceneNodeAnimatorCameraFPS *pFPSAnimatorCLONE = (ISceneNodeAnimatorCameraFPS *)pFPSAnimator->createClone(cam);

	// Setting the cloned animator to the camera.
	cam->removeAnimators();
	cam->addAnimator(pFPSAnimatorCLONE);
	
	// But hey, there is no keymap??
	// If you'll check closely you'll see that the KeyMapArray pointer is NULL.
	

	while(device->run())
	{
		driver->beginScene(true, true, SColor(255,100,101,140));

		smgr->drawAll();
		guienv->drawAll();

		driver->endScene();
	}

	device->drop();

	return 0;
}

I took the hello world example, stripped the documentation and added the test code.

Using latest trunk revision.

P.S
I had to #include <ISceneNodeAnimatorCameraFPS.h>. should it be like that or the irrlicht.h should include it already?

[hybrid]

Yes, the includes are now all added. However, I still cannot see any problems when using the clone methods properly. I cannot test your code ATM, bt it seems to me that you're messing with the types. You're creating a clone of the cam, but use it as the camera animator...

[MasterGod]

You're creating a clone of the cam, but use it as the camera animator...

Code: Select all

// Creating a clone of the animator
   ISceneNodeAnimatorCameraFPS *pFPSAnimatorCLONE = (ISceneNodeAnimatorCameraFPS *)pFPSAnimator->createClone(cam); 

I clone the animator. It actually doesn't matter what I pass on to the createClone() argument here because it does nothing with it - see createClone() implementation:

Code: Select all

ISceneNodeAnimator* CSceneNodeAnimatorCameraFPS::createClone(ISceneNode* node, ISceneManager* newManager)
{
   CSceneNodeAnimatorCameraFPS * newAnimator =
      new CSceneNodeAnimatorCameraFPS(CursorControl,   RotateSpeed, (MoveSpeed * 1000.0f), JumpSpeed,
                                 KeyMapArray, KeyMapSize, NoVerticalMovement);
   return newAnimator;
}

When I debugged my code the KeyMapArray pointer was NULL when the clone was created.

Why? Because when creating a dumb camera and using setKeyMap() to set a key map the KeyMapArray pointer is unchanged due to the reasons I explained in the first post (in short it is unchanged in the setKeyMap() method) and so when cloning the animator the key map is still NULL (as it was initialized).
When creating a FPS camera scene node and setting it a key map (like in the documentation of the addCameraSceneNodeFPS() the constructor assigns the KeyMapArray pointer to the received key map array and so when using the following way of setting a key map the KeyMapArray pointer is set to the right values:

Code: Select all

                 SKeyMap keyMap[8];
                 keyMap[0].Action = EKA_MOVE_FORWARD;
                 keyMap[0].KeyCode = KEY_UP;
                 keyMap[1].Action = EKA_MOVE_FORWARD;
                 keyMap[1].KeyCode = KEY_KEY_W;

                 keyMap[2].Action = EKA_MOVE_BACKWARD;
                 keyMap[2].KeyCode = KEY_DOWN;
                 keyMap[3].Action = EKA_MOVE_BACKWARD;
                 keyMap[3].KeyCode = KEY_KEY_S;

                 keyMap[4].Action = EKA_STRAFE_LEFT;
                 keyMap[4].KeyCode = KEY_LEFT;
                 keyMap[5].Action = EKA_STRAFE_LEFT;
                 keyMap[5].KeyCode = KEY_KEY_A;

                 keyMap[6].Action = EKA_STRAFE_RIGHT;
                 keyMap[6].KeyCode = KEY_RIGHT;
                 keyMap[7].Action = EKA_STRAFE_RIGHT;
                 keyMap[7].KeyCode = KEY_KEY_D;

                 camera = sceneManager->addCameraSceneNodeFPS(0, 100, 500, -1, keyMap, 8);

The catch is this: Because KeyMapArray is given any values only in the constructor, when changing the key map only the core::array object is changed (and so the actual behavior DOES change) but when cloning, because we're using the KeyMapArray it is not guaranteed that we would get the same values the core::array object has.

In my test case the first value KeyMapArray had was NULL and so when I cloned it I got that NULL value regardless of the core::array having other values. If I would change the KeyMapArray (using the constructor) then changing it again using setKeyMap() the clone version would have the first values which I inserted in the constructor - because it was never changed again (which it should have by setKeyMap()).

I hope I'm clear enough.

[hybrid]

Ok, it took a little longer, but I've added a quick-fix which will at least copy the pointer to make the clone method work while the original array exists. A proper fix would require to copy the whole array as you suggested, but I think this is pretty much redundant.

[MasterGod]

3 things:
1 - You assigned the array pointer only, you need to copy the size too. (in setKeyMap()).
2 - Its a bit unsafe because there might be a copy to that array outside the method which means it can be freed without the object knowing which might cause it to access a freed memory.
3 - I believe my fix few posts above should be just fine and takes care of what I just mentioned as it would reallocate a new memory for the array without too much overhead.

P.S
If you're busy I can assemble a patch that would make it shorter for you to apply, if you agree to the fix I suggested.

[MasterGod]

Cool, I see you fixed it.

Better fix then mine .

P.S
I'll try harder next time