[PATCH] Battle Detection

[historic_bruno]

Need a way for the simulation to detect "battles", for UI, sound, and music system notifications, possibly for stats. A battle obviously shouldn't occur every time a unit is attacked, that's merely a skirmish, but rather when the attack is "significant". Maybe we can have different types of notification for skirmishes after periods of peace vs. a battle.

[historic_bruno]

Discussion here.

[zoot]

I have a prototype of this here: ​https://github.com/zootzoot/0ad/compare/battle-detection

The diff is here: ​https://github.com/zootzoot/0ad/compare/battle-detection.diff

It is functionally complete, but it may be necessary to tweak the various thresholds, cross-fade periods etc. (And, of course, we need some battle music.)

[Jonathan Waller]

Thanks for your patch. A few comments.

You should use a getter to find the state from the BattleDetection component. This is the convention that we use and it makes it more flexible if someone wants to do some calculation when the state is asked for.

Why did you change the Armour component instead of the Health component when adding the state.damage = total; property?

BattleDetection.prototype.OnGlobalAttacked is defined twice. The second version should be kept.

In BattleDetection.prototype.setState = function(state) move the cmpPlayer definition inside the if statement. (Otherwise I expect cmpPlayer to be used as part of the if statement).

It would be nice to have a brief description of how alertness is calculated, perhaps in the Init function.

BattleDetection.prototype.StartTimer doesn't mention that the repeat is optional in the comment. The fact that you can only use this function when a timer is not active should be mentioned. Perhaps it would be best to stop the timer when the error message is shown (though of course this should never happen).

I'm also concerned about efficiency. Your damage rate algorithm is n² with n being the number of attacks in a 5 second period. With units attacking once per second, in a large battle we could have a lot of attacks happening. It should be fairly easy to store damage per turn instead.

[zoot]

Replying to quantumstate:

You should use a getter to find the state from the BattleDetection component. This is the convention that we use and it makes it more flexible if someone wants to do some calculation when the state is asked for.

What is meant by getter here? Can you point to an example of how this is usually done?

Why did you change the Armour component instead of the Health component when adding the state.damage = total; property?

I figured the Reduce function in the Health component is not just for damage dealing - it could be used for other types of health reduction too. The TakeDamage function, on the other hand, is specifically for applying damage.

BattleDetection .prototype.OnGlobalAttacked is defined twice. The second version should be kept.

​Done.

(I'll respond to the rest of the comments later.)

[Jonathan Waller]

Replying to zoot:

What is meant by getter here? Can you point to an example of how this is usually done?

Just having a function GetState() instead of accessing the property directly. The Health component has some examples (GetHitpoints, GetMaxHitpoints).

I figured the Reduce function in the Health component is not just for damage dealing - it could be used for other types of health reduction too. The TakeDamage function, on the other hand, is specifically for applying damage.

I think it would be nicer to put it into the Reduce function, you could give it a more generic name than damage (e.g. change). Then it is available to anything else that uses the function, also if we ever made the health component use the input for that function less directly (e.g. some tech which has health change multiplier) then there is a risk of inconsistency.

[zoot]

Replying to quantumstate:

Just having a function GetState() instead of accessing the property directly. The Health component has some examples (GetHitpoints, GetMaxHitpoints).

​Done.

Replying to quantumstate:

In BattleDetection.prototype.setState = function(state) move the cmpPlayer definition inside the if statement. (Otherwise I expect cmpPlayer to be used as part of the if statement).

​Done.

It would be nice to have a brief description of how alertness is calculated, perhaps in the Init function.

​Done.

BattleDetection.prototype.StartTimer doesn't mention that the repeat is optional in the comment. The fact that you can only use this function when a timer is not active should be mentioned. Perhaps it would be best to stop the timer when the error message is shown (though of course this should never happen).

​Done.

(Will deal with the remaining points soon.)

[zoot]

Replying to quantumstate:

I'm also concerned about efficiency. Your damage rate algorithm is n² with n being the number of attacks in a 5 second period. With units attacking once per second, in a large battle we could have a lot of attacks happening. It should be fairly easy to store damage per turn instead.

I've ​done this. Instead of recording damage per attack, it now records damage per timer period. (Edit: I subsequently did ​this logic fix to the damage rate calculation.)

[zoot]

Replying to quantumstate:

I think it would be nicer to put it into the Reduce function, you could give it a more generic name than damage (e.g. change). Then it is available to anything else that uses the function, also if we ever made the health component use the input for that function less directly (e.g. some tech which has health change multiplier) then there is a risk of inconsistency.

I can do it if you insist, but I have to say, it does not feel quite right, though I suppose it is to some degree a question of semantics. For instance, what if the enemy have a large group of units with just one HP each - if you strike them all down, the total loss of HP is relatively small, even though you have dealt a massive amount of damage. Should that not count as a battle? Conversely, if someone has made a mod to the Health component which cause units to take 1000 times the damage dealt against them, merely a female citizen hitting a unit with her rolling pin may trigger the battle state. Is that desirable? To me at least, engaging in a battle is about the force you apply, not about how frail your opponent is.

What do you prefer?

[Jonathan Waller]

Replying to zoot:

Replying to quantumstate:

I think it would be nicer to put it into the Reduce function, you could give it a more generic name than damage (e.g. change). Then it is available to anything else that uses the function, also if we ever made the health component use the input for that function less directly (e.g. some tech which has health change multiplier) then there is a risk of inconsistency.

I can do it if you insist, but I have to say, it does not feel quite right, though I suppose it is to some degree a question of semantics. For instance, what if the enemy have a large group of units with just one HP each - if you strike them all down, the total loss of HP is relatively small, even though you have dealt a massive amount of damage. Should that not count as a battle? Conversely, if someone has made a mod to the Health component which cause units to take 1000 times the damage dealt against them, merely a female citizen hitting a unit with her rolling pin may trigger the battle state. Is that desirable? To me at least, engaging in a battle is about the force you apply, not about how frail your opponent is.

What do you prefer?

Sorry for the late reply.

I would still like to see it reflecting the actual damage dealt. I think whether this is the best thing for battle detection is open for debate but I would find the idea that the value returned isn't the actual damage dealt surprising and surprising return values are a bad thing.

If you want to factor unit deaths into the battle code or something else fancy that can be done, but that is an easy tweak to do later.

Apart from that the patch looks good to me. Is there anything else you want to do before you consider the code ready to commit?

[zoot]

I've done this:

​https://github.com/zootzoot/0ad/commit/8b38128ae875059f31c55f3c1c9e3564fc465004

If that is what you wanted, the final patch is here:

​https://github.com/zootzoot/0ad/compare/battle-detection-quantumstate.diff

If not, let me know. Otherwise, I consider the above ready for commit.

[Jonathan Waller]

In 13091:

Add battle detection support to the simulation. Refs #1425 (Patch by Zoot)

The audio code does not yet support actually playing battle music.

[Jonathan Waller]

Thanks for the patch. I changed the sign for the health change since a change of 5 for a loss of 5 health and a change of -5 for a gain of 5 health seemed weird.

[zoot]

r13091 seems to be missing the two new files, BattleDetection.js and interfaces/BattleDetection.js, though. Was the diff bad?

[Jonathan Waller]

In 13093:

Actually add the battle detection code. Refs #1425.

[Jonathan Waller]

Replying to zoot:

r13091 seems to be missing the two new files, BattleDetection.js and interfaces/BattleDetection.js, though. Was the diff bad?

Nope I just forgot to add the two new files to the commit.

[zoot]

I've improved the damage rate tracking slightly, if anyone would care to review it:

​https://github.com/zootzoot/0ad/compare/battle-detection-moving (​diff)

Instead of being based on a regular, periodic average updated every 5 seconds, it now is based on a moving average updated every 0.1 seconds. Also, the damage rate timer now starts at the first blow dealt against an enemy instead of starting at the arbitrary point in time at which the component was initialized. Both of these changes should help the battle detection feel a bit more responsive.

[zoot]

I've moved the hardcoded thresholds out into the player template where non-programmers can more easily tweak them:

​https://github.com/zootzoot/0ad/compare/battle-detection-template (​diff)

All of the template elements are optional and will default to reasonable values if not set.

(The patch also includes the damage rate tracking improvements from the comment above.)

[Jonathan Waller]

We prefer to have non optional xml generally. This is so that configuration options stay in the xml rather than being mixed between the source and xml. One place to find things keeps things tidier and makes it obvious what options can be set in the xml (since the must be there already).

Also your interval is now longer than a game turn, I would recommend 0.2 instead.

Other than that the change looks good to me.

[zoot]

Replying to quantumstate:

We prefer to have non optional xml generally. This is so that configuration options stay in the xml rather than being mixed between the source and xml. One place to find things keeps things tidier and makes it obvious what options can be set in the xml (since the must be there already).

​Done.

Also your interval is now longer than a game turn, I would recommend 0.2 instead.

​Done.

I consider this version ready for commit:

​https://github.com/zootzoot/0ad/compare/battle-detection-final (​diff)

[Jonathan Waller]

In 13163:

Improvements to battle detection. Patch from zoot. Fixes #1425.

[Jonathan Waller]

Thanks for the improvements.