[PATCH] Show a different notification when livestock is attacked

[fcxSanya]

Currently there is a generic "You have been attacked by %(attacker)s!" notification. It's a bit confusing to see it when an ally decides to gather player's livestock.

[fcxSanya]

In this patch a different "Your livestock have been attacked by %(attacker)s!" notification is displayed if the attacked entity has the ResourceSupply component

[fcxSanya]

Alternative patch; displays the attacked entity's specific name in the notification, for example "Your Sheep have been attacked by %(attacker)s!", "Your Agorā́ have been attacked by %(attacker)s!"; this might be harder to translate to other languages

[fcxSanya]

In this patch a different "Your livestock have been attacked by %(attacker)s!" notification is displayed if the attacked entity has the ResourceSupply component; this file differs from the previous version of the same patch by adding a code comment

[fcxSanya]

Alternative patch; displays the attacked entity's generic name in the notification, for example "Your Fauna have been attacked by %(attacker)s!", "Your Civic Center have been attacked by %(attacker)s!"; this might be harder to translate to other languages

[fcxSanya]

Another alternative patch; suspends the attack notification if the attacked entity has classes "Animal" and "Domestic"

[fcxSanya]

Hm, on second thought, in the third patch (no_attack_notification_for_domestic_animals_r16549.diff​) it's better to not send the notification from the AttackDetection component in the first place.

I still prefer the first patch though.

[fcxSanya]

Another alternative patch; suspends the attack notification if the attacked entity has classes "Animal" and "Domestic"; fixed version: the notification for domestic animals isn't sent in the first place

[leper]

I prefer livestock_attack_notification_r16549.diff​, though an enemy attacking your livestock will have the benefit of being able to attack your other units/buildings in the same area without triggering another message.

[fcxSanya]

In this patch a different "Your livestock have been attacked by %(attacker)s!" notification is displayed if the attacked entity has classes "Animal" and "Domestic"; if a non-livestock entity is attacked in the same area during the livestock notification suppression, the regular notification is still displayed

[fcxSanya]

Replying to leper:

I prefer livestock_attack_notification_r16549.diff​, though an enemy attacking your livestock will have the benefit of being able to attack your other units/buildings in the same area without triggering another message.

Should be fixed in livestock_attack_notification_improved_suppression_r16572.diff​, but at the cost of complicating the code a bit.

[leper]

var targetIsDomesticAnimal = cmpTargetIdentity && ..., else an entity without an Identity component (mods might do that, unlikely but possible) will throw errors.

event should use "" around property names.

Couldn't the first

for (var i = 0; i < this.suppressedList.length; i++) 
{ 
    var element = this.suppressedList[i];

be replaced with for (var element of this.suppressedList)? (or at least change i++ to ++i (also in the other loop below))

The assignment to element in the if seems broken, or at least not doing what it should (not an issue in your code, but since you're at it :-) ).

Is isPriorityIncreased actually needed? It seems just doing the break would be sufficient.

Also CCing Itms since he wrote that code IIRC.

[Itms]

Why not removing the notification if it's livestock when the "attacker" is an ally? Enemies gathering from it would be considered as attackers, which sounds logical to me.

[fcxSanya]

New version of livestock_attack_notification_improved_suppression_r16572.diff​, updated according to leper's review in #comment:5

[fcxSanya]

Replying to leper:

var targetIsDomesticAnimal = cmpTargetIdentity && ..., else an entity without an Identity component (mods might do that, unlikely but possible) will throw errors.

Fixed

event should use "" around property names.

Fixed

The assignment to element in the if seems broken, or at least not doing what it should

I replaced the assigment with UpdateSuppressionEvent(index, event) method, which updates suppressedList's element by index and creates a timer (like the existing AddSuppression(event) method)

Couldn't the first

for (var i = 0; i < this.suppressedList.length; i++) 
{ 
    var element = this.suppressedList[i];

be replaced with for (var element of this.suppressedList)?

With for...of loop it wouldn't be possible to replace element by index in the first loop (see above) and to use splice to remove element in the second one, right?

(or at least change i++ to ++i (also in the other loop below))

Done.

Is isPriorityIncreased actually needed? It seems just doing the break would be sufficient.

It's telling whether the event should be added into the suppressedList after the loop. Am I missing a simpler way to do this?

---

Replying to Itms:

Why not removing the notification if it's livestock when the "attacker" is an ally?

We can just remove it indeed, but since it provides somewhat useful information I prefer to preserve it if possible (but I won't be too disappointed if the consensus will be to get rid of it :))

[leper]

In 16663:

Show a different notification if livestock is attacked. Patch by fcxSanya. Fixes #3176.

[leper]

Thanks for the patch.

I inlined a few system component calls, removed a useless event parameter for ActivateTimer, and made the loop use an early continue.

The code works quite nicely :-). I'll open another ticket about attack notifications when someone starts capturing your buildings.