setTimeout causing problem in Loop

[kaasis]

Hey!

I got here little weird problem that i can't seem to figure out what's wrong. I'm making multiplayer game and i'm trying to create attacking system (kind of), i'm using loop for(var i in var). It all works, unless i add setTimeout to avoid spamming of attack. Problem is that, when i add timeout, for some reason for second player that connects variable attackdelay is not setting to false after timeout ends.

Here's code (which works but has no delay in attacking, and can spam hell out of it)

// player attacking
for (var i in players) {
	var attacker = players[i]
	if (attacker.attacking) {
		for (var i in players) {
			var victim = players[i]
			if (attacker != victim) {
				if (circleOverlap(getAnglePosition(attacker.x,attacker.y,attacker.angle,20),20,victim,playerCollisionRadius)) {
					victim.health--
				}
			}
		}
	}
}

Here's code where i used setTimeout to do delay between attacks (only works for first person who connects, haven't tested out for 3 players tho, only for 2)

// player attacking
for (var i in players) {
	var attacker = players[i]
	if (!attacker.attackdelay && attacker.attacking) {
		for (var i in players) {
			var victim = players[i]
			if (attacker != victim) {
				if (circleOverlap(getAnglePosition(attacker.x,attacker.y,attacker.angle,20),20,victim,playerCollisionRadius)) {
					victim.health -= 10
					attacker.attackdelay = true
					setTimeout(function(){attacker.attackdelay=false},1500)
				}
			}
		}
	}
}

 

[Antriel]

This is the standard loop and closure issue. Closures don't make a copy of the local variables, they just keep the scope alive. What that means is that your `attacker` won't have the value that it had at the time the closure was created, but whatever the value is when the callback is executed, i.e. the last player in the `players` array. See Creating closures in loops: A common mistake for more information.

I am not great at JS, but I thinks this should work. Basically it creates another scope that will remember the value at the time it was called. There might be more standard solution, not sure. If so, someone else should comment

function getEnableAttackCallback(player) {
	return function() {
		player.attackdelay = false;
	};
}

for (var i in players) {
	var attacker = players[i]
	if (!attacker.attackdelay && attacker.attacking) {
		for (var i in players) {
			var victim = players[i]
			if (attacker != victim) {
				if (circleOverlap(getAnglePosition(attacker.x,attacker.y,attacker.angle,20),20,victim,playerCollisionRadius)) {
					victim.health -= 10
					attacker.attackdelay = true
					setTimeout(getEnableAttackCallback(attacker),1500)
				}
			}
		}
	}
}

 

[kaasis]

24 minutes ago, Antriel said:

This is the standard loop and closure issue. Closures don't make a copy of the local variables, they just keep the scope alive. What that means is that your `attacker` won't have the value that it had at the time the closure was created, but whatever the value is when the callback is executed, i.e. the last player in the `players` array. See Creating closures in loops: A common mistake for more information.

I am not great at JS, but I thinks this should work. Basically it creates another scope that will remember the value at the time it was called. There might be more standard solution, not sure. If so, someone else should comment

function getEnableAttackCallback(player) {
	return function() {
		player.attackdelay = false;
	};
}

for (var i in players) {
	var attacker = players[i]
	if (!attacker.attackdelay && attacker.attacking) {
		for (var i in players) {
			var victim = players[i]
			if (attacker != victim) {
				if (circleOverlap(getAnglePosition(attacker.x,attacker.y,attacker.angle,20),20,victim,playerCollisionRadius)) {
					victim.health -= 10
					attacker.attackdelay = true
					setTimeout(getEnableAttackCallback(attacker),1500)
				}
			}
		}
	}
}

 

This actually sucks, 'cause i'm coming from Lua and in Lua loops work as well as they should and i always used loops in the way i did with this one. Gonna need to keep this in mind otherwise again gonna struggle my head what's wrong with it. Btw thanks mate, it works.  And yeah, if someone have even better solution, leave a comment. I'll definetly gonna check it out.

[Antriel]

It's not really a JS thing. C# has the same issue with loops and closures, it comes from the principle of how closures work. Although in C# the scope is different for the loop body, so there it would be enough to do `var _attacker = attacker` inside the inner loop. From what I read that wouldn't be enough for JS though.

[mattstyles]

36 minutes ago, Antriel said:

From what I read that wouldn't be enough for JS though.

You can do the same thing, just earlier in the chain.

You can be a a bit more functional and use a .forEach instead of the loop (ensuring you use the same `thunk` mechanism already described), although it would work out about the same it would just make things a little cleaner and easier for you here.

[kaasis]

1 hour ago, mattstyles said:

You can do the same thing, just earlier in the chain.

You can be a a bit more functional and use a .forEach instead of the loop (ensuring you use the same `thunk` mechanism already described), although it would work out about the same it would just make things a little cleaner and easier for you here.

I'm using objects not an array. Althrough i could make 'players' an array and then put objects in it and then use .forEach and it would make it cleaner, or just whole 'players' object make an array and those objects in it make to arrays aswell. Wouldn't be that hard, 'cause i don't have much yet code on server. 200+ lines for a moment.

[kaasis]

surprisingly this works aswell. Even tho @Antriel you said that it's not enough for JS.

// player attacking
for (var i in players) {
	var attacker = players[i]
	if (!attacker.attackdelay && attacker.attacking) {
		for (var i in players) {
			var _attacker = attacker
			var victim = players[i]
			if (_attacker != victim) {
				if (circleOverlap(getAnglePosition(_attacker.x,_attacker.y,_attacker.angle,20),20,victim,playerCollisionRadius)) {
					victim.health -= 10
					_attacker.attackdelay = true
					setTimeout(function(){_attacker.attackdelay=false},1500)
				}
			}
		}
	}
}

 

[Gio]

While that may appear to work, it doesn't... the value of _attacker, when the timeout funciton is executed, will be the last value that was assigned to _attacker in your foor loops.

I'd like to point out that you have a bigger problem there, in that you have 2 nested loops that are iterating using the same i variable (even though you are redefining it, it's the same i variable in both loops). Probably not what you want.

Anyway I wanted to suggest that you don't need to make players an array to use forEach. You can also do:

Object.keys(players).forEach(function(i)
{
    // do something with players[i]
});

 

[kaasis]

1 hour ago, Gio said:

While that may appear to work, it doesn't... the value of _attacker, when the timeout funciton is executed, will be the last value that was assigned to _attacker in your foor loops.

I'd like to point out that you have a bigger problem there, in that you have 2 nested loops that are iterating using the same i variable (even though you are redefining it, it's the same i variable in both loops). Probably not what you want.

Anyway I wanted to suggest that you don't need to make players an array to use forEach. You can also do:

Object.keys(players).forEach(function(i)
{
    // do something with players[i]
});

 

Thanks for letting me know, used your example. Works really well.

[mattstyles]

Probably worth noting that in most JS engines forEach is a fair amount slower than a for...loop, only optimise when you need to and when you can measure it, but something to keep in mind.