Value of variable set under ominous circumstances

Started by
6 comments, last by WitchLord 10 years ago

Hello,

I found a rather mysterious bug/issue in one of my scripts. I would eigther like to know if there is something that I am obviously missing here that could explain it; otherwise I hope I issued some helpful input for fixing a bug.

So thats what its about. I got this method as part of a script-class:


	void OnProcessInput(const MappedInput@ input)
	{
		// movement
		if(input.HasState(PLAYER_MOVE_FORWARD))
			moveStraight += MOVE_SPEED;
		if(input.HasState(PLAYER_MOVE_BACKWARD))
			moveStraight -= MOVE_SPEED;
		if(input.HasState(PLAYER_MOVE_LEFT))
			moveSide += MOVE_SPEED;
		if(input.HasState(PLAYER_MOVE_RIGHT))
			moveSide -= MOVE_SPEED;
			
		// zoom
		float zoom = 0.0f;
		if(input.HasRange(PLAYER_ZOOM, zoom))
			toZoom += zoom / ZOOM_FACTOR;
			
		// camera rotation
		input.HasRange(PLAYER_LOOK_X, toLookX);
		input.HasRange(PLAYER_LOOK_Y, toLookY);
	}

This method is called to process input. Now, PLAYER_MOVE_FOWARD is of an enum and equals to 0. However, when I press that very key (maps to W in my handler, but it doesn't matter, every key produces the same result), it seems that the toLookY-variable, as present in the last line of the function, is set to a certain value, and therefore my camera upwards movement is triggered.

Now I've unsured that there is no error in those functions on my application side. What the input.HasRange-method does is return true/false whether the specified range is present, and sets the second parameter in case. In case of the erroneus behavior, this method does not return true nor sets the variable specifically. There are three variants for removing the bug:

- Modifying the last two lines to:


		if(input.HasRange(PLAYER_LOOK_X, toLookX))
			log(2);
		if(input.HasRange(PLAYER_LOOK_Y, toLookY))
			log(1);

- Changing the order of the last two lines or

- Removing the last line that uses toLookY.

Both those variables are class members, just in case it was not clear.

So, do you see any possibility for a bug here that I am missing? I also hope the information is complete enough, in case it is not, please let me know.

Advertisement

It does indeed look like it might be a bug in the library. However, I'll need further information to be able to investigate this properly.

1. What version of AngelScript are you currently using?

2. What system are you using? Windows? Linux? 32bit? 64bit?

3. How is the MappedInput type registered. Most specifically the HasState and HasRange methods?

4. Where is toLookX and toLookY declared? Are they global script variables? members of the script class? application registered properties?

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

1. I'm using Version, 2.28.0 currently.

2. Windows, 64 bit, happens on both 7 and 8, I've got two PCs that produces the same results.

3. MappedInput is registered here:


			auto mappedInput = script.RegisterReferenceType("MappedInput", script::REF_NOCOUNT);
			mappedInput.RegisterMethod("bool HasAction(uint) const", asMETHOD(MappedInput, HasAction));
			mappedInput.RegisterMethod("bool HasState(uint) const", asMETHOD(MappedInput, HasState));
			mappedInput.RegisterMethod("bool HasRange(uint, float& out) const", asMETHOD(MappedInput, HasRange));
			mappedInput.RegisterMethod("bool HasValue(uint, float& out) const", asMETHOD(MappedInput, HasValue));

I'm using a custom light wrapper, I hope you still see how the registration is eventually happening.

4. They are part of the class, as I mentioned in one of the final paragraphs. Just in case, here is the full script:


enum InputState
{
	PLAYER_MOVE_FORWARD,
	PLAYER_MOVE_BACKWARD,
	PLAYER_MOVE_LEFT,
	PLAYER_MOVE_RIGHT
};

enum InputRanges
{
	PLAYER_ZOOM,
	PLAYER_LOOK_X,
	PLAYER_LOOK_Y,
};

const float ZOOM_SPEED = 2.5f;
const float MIN_ZOOM = 6.0f;
const float MAX_ZOOM = 30.0f;
// the larger, the less zoom with each wheel turn
const float ZOOM_FACTOR = 75.0f;

const float LOOK_SPEED = 0.1f;
const float CAP_Y_ANGLE = 66.0f;

const float MOVE_SPEED = 20.0f;

class PlayerController : IGameScript
{
	
	PlayerController(const Entity& in ent)
	{
		entity = ent;
		
		currentZoom = 5.0f;
		toZoom = 0.0f;
		angleX = 0.0f;
		angleY = 0.0f;
		toLookX = 0.0f;
		toLookY = 0.0f;
		moveStraight = 0.0f;
		moveSide = 0.0f;
	}
	
	void OnUpdate(double dt)
	{
		Body@ body = GetBody(entity);
		
		UpdateCameraValues(dt);
		
		// lock camera to entity
		Position@ position = GetPosition(entity);
		const Vector3 vLookAt = position.GetVec() + Vector3(0.0f, pivot, 0.0f);
		Cam@ camera = GetCamera(entity);
		camera.camera.SetLookAt(vLookAt);
		
		// calculate camera position offset
		Vector3 vLook = Vector3(1.0f, 0.0f, 0.0f);
		
		// horizontal rotation
		const Matrix mLookX = MatRotationY(degToRad(angleX));
		vLook = mLookX.TransformNormal(vLook);
		vLook.Normalize();
		
		// vertical rotation
		const Vector3 vUp = Vector3(0.0f, 1.0f, 0.0f);
		const Vector3 vRight = vUp.Cross(vLook).normal();
		const Matrix mLookY = MatRotationAxis(vRight, degToRad(angleY));
		vLook = mLookY.TransformNormal(vLook);
		vLook.Normalize();
		camera.camera.SetPosition(vLookAt +  vLook * currentZoom);	
		
		// TODO: optimize this, only set when camera really changed, includes caching of body position
		camera.dirty = true;
		
		// apply look to entity
		Direction@ dir = GetDirection(entity);
		const Vector3 vLookXZ = Vector3(vLook.x, 0.0f, vLook.z).normal();
		// TODO: this is a hackfix to fix this one model, undo at times
		dir.SetVec(Vector3(vLookXZ.z, 0.0f, -vLookXZ.x));

		body.body.StopMovement();
		if(moveStraight != 0.0f || moveSide != 0.0f)
		{
			const Vector3 vMoveRight = vUp.Cross(vLookXZ);
			body.body.SetLinearVelocity(-vLookXZ * moveStraight + vMoveRight * moveSide);
			moveStraight = 0.0f;
			moveSide = 0.0f;
		}

	}
	
	bool UpdateCameraValues(double dt)
	{
		bool hasChanged = false;
		
		// update zoom
		if(toZoom != 0.0f)
		{
			const float zoomNow = toZoom * dt * ZOOM_SPEED;
			toZoom -= zoomNow;
			
			if((zoomNow > 0.0f && toZoom < 0.0f) ||
			    (zoomNow < 0.0f && toZoom > 0.0f))
				toZoom = 0.0f;
				
			currentZoom += zoomNow;
			
			// cap zoom at min/max distance
			if(currentZoom >= MAX_ZOOM)
			{
				currentZoom = MAX_ZOOM;
				toZoom = 0.0f;
			}
			else if(currentZoom <= MIN_ZOOM)
			{
				currentZoom = MIN_ZOOM;
				toZoom = 0.0f;	
			}
			
			hasChanged = true;
		}
		
		// update look on x-axis
		if(toLookX != 0.0f)
		{
			const float lookNow = toLookX * LOOK_SPEED;
			toLookX = 0.0f;
				
			angleX -= lookNow;
			
			if(angleX >= 360.0f)
				angleX -= 360.0f;
			else if(angleX <= 0.0f)
				angleX += 360.0f;
						
			hasChanged = true;
		}
		
		// update look on x-axis
		if(toLookY != 0.0f)
		{
			const float lookNow = toLookY * LOOK_SPEED;
			toLookY = 0.0f;
				
			angleY -= lookNow;
			
			if(angleY >= CAP_Y_ANGLE)
				angleY = CAP_Y_ANGLE;
			else if(angleY <= -CAP_Y_ANGLE)
				angleY = -CAP_Y_ANGLE;

			hasChanged = true;
		}
		
		return hasChanged;
	}
	
	void OnProcessInput(const MappedInput@ input)
	{
		// movement
		if(input.HasState(PLAYER_MOVE_FORWARD))
			moveStraight += MOVE_SPEED;
		if(input.HasState(PLAYER_MOVE_BACKWARD))
			moveStraight -= MOVE_SPEED;
		if(input.HasState(PLAYER_MOVE_LEFT))
			moveSide += MOVE_SPEED;
		if(input.HasState(PLAYER_MOVE_RIGHT))
			moveSide -= MOVE_SPEED;
			
		// zoom
		float zoom = 0.0f;
		if(input.HasRange(PLAYER_ZOOM, zoom))
			toZoom += zoom / ZOOM_FACTOR;
			
		// camera rotation
		input.HasRange(PLAYER_LOOK_Y, toLookY);
		input.HasRange(PLAYER_LOOK_X, toLookX);
	}
	
	Entity entity;
	float pivot;
	private float moveStraight, moveSide;
	private float currentZoom, toZoom;
	private float angleX, angleY, toLookX, toLookY;
}

Hope this helps.

Thanks for the info.

I haven't been able to reproduce the problem yet. I'll continue the investigation tomorrow evening.

In the meantime I suggest you upgrade to the latest version of AngelScript, 2.28.2, or even to the 2.29.0 WIP version that you can get from the svn, and see if that solves the problem.

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

I tested now with 2.28.2, no difference, the bug still exists. One thing I found out is that it definately lies in the last line - if I switch both those lines like I mentioned, it does not fix the bug, but instead "writes" the bug result in the toLookX-variable instead. I also found out something else - the bug seems to relate to the number of lines present! If I insert any uneven number of non-empty lines, it will dissappear (see edit). If I add one more line, it is there again, the next line will make it dissappear again. Doesn't matter what it is - I tried both "log()" custom function, under "{}" in one of the ifs and also outside, as well as simple variable declaration:


		int i = 0; // fixes it...
		int j = 0; // ... now its there again
		int k = 0; // ... gone again

Does this help to possibly locate the bug better? Let me know if there is anything else I can do!

Edit: Those lines that I insert won't really make the bug dissappear, after further testing, instead it seems to reverse the value that goes into eigther of those toLook-variables, and reverse the condition for it to happen. So if I insert the "int i = 0"-line, camera will move in opposite direction, when I press the PLAYER_MOVE_BACKWARD-key (instead of the forward-key like before). Also, just for clarity, I inserted those line between the if-movement-conditions above and the zoom-condition, going to test if different positions affect the result and update it here.

EDIT2: OK, I appearently found the reason for the bugs (or at least it makes it finally disappear, if I find out after posting it does not I'm going to smack my head against my keyboard...). Its something I had in mind from the beginning but didn't really thing it was it... So thats the HasRange-function:


        bool MappedInput::HasRange(unsigned int range, float& out) const
        {
            auto itr = m_ranges.find(range);
            if(itr != m_ranges.end())
            {
                out = itr->second;
                return true;
            }
            else
                return false;
        }

As you can see, I'm not setting the out-value in case the desired range is not present. Now adding "out = 0.0f" at the top of the function seems to fix that, so it seems that angelscript is using some other memory in case I don't explicitely write to an "& out" declared paramter. Is this a bug and can it be easily fixed?

Ah, that definitely explains it.

And, no it is not a bug. AngelScript doesn't initialize primitives with 0 (same as C++), so if you do not assign a specific value to the output parameter it will get whatever uninitialized value was in slot on the stack before the call. AngelScript also do not pass the a reference to the actual variable that will receive the value (since it cannot guarantee that this reference will stay alive throughout the call), so the reference will always point to a temporary uninitialized variable. This temporary unitialized variable will then be assigned to the final variable after the call.

I have had an entry on my to-do list for quite some time to have AngelScript initialize all variables to 0 by default. It's been there for quite sometime because it is not a trivial task (contrary to what can be imagined). The complications come from the fact that I do not want to add unnecessary overhead that would impact the performance.

I'll add a note on this situation to my to-do list and bump up the priority, but I can't promise it will be changed anytime soon.

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

Alright, then I'll have to recheck those functions and make sure value is written all the times.

Initializing all values seems a little excessive, wouldn't it be possible/easier to just make sure the temporary uninitialized variable from the "& out" is initialized from the input value, for primitive types at least? Or add some special syntax to declare a reference out paramter as optional, if the first one is too excessive? Simply initializing to 0 would be sufficient for my case, but in reality, somebody might want to have a paramter that is written only under certain circumstances and otherwise be left unattended, something like if in my case I was like:


float toLookX = 5.0f;
if(input.HasRange(PLAYER_LOOK_X, toLookX)
      toLookX /= 10.0f;

// do something with toLookX regardless of the input-condition

Don't know, just a suggestion maybe for the future, I'm fine for now, thanks so far!

The best solution is simply to initialize the temporary variable to 0 before the call, even if it means that if the function doesn't explicitly initialize the value, it would return 0.

An &out variable cannot be optional, because there is no way for the called function to inform the caller function if it actually touched the variable or not. For that it would be necessary to have an additional argument, which would add additional overhead, not to mention make it incompatible with the ordinary C++ calling convention. It is also not possible to initialize the temporary variable with the original value of the argument, because that would mean that the argument expression has to be evaluated twice (before and after the call).

Note, if you don't care about sandboxed environments, you can turn on unsafe reference with the engine property asEP_ALLOW_UNSAFE_REFERENCES, and then use normal inout reference with primitives too, in which case it will work the same as in C++, i.e. the reference will point to the actual value and if the function doesn't explicitly initialize it, the value won't be modified.

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

This topic is closed to new replies.

Advertisement