Opened 15 years ago

Last modified 3 years ago

#252 assigned enhancement

[PATCH] Gameplay Scripting: Entity and Actor coding for Secondary Attacks

Reported by: Michael D. Hafer Owned by: bb
Priority: Should Have Milestone: Work In Progress
Component: Core engine Keywords: design, patch, rfc
Cc: mimo, Lionkanzen Patch: Phab:D368

Description (last modified by temple)

This could (should?) be separated into multiple tasks.

Essentially some units, usually Super Units and Heroes, have the ability to use a "secondary" attack. For instance, the Persian Super Unit 1 "The Immortal" has a melee (spear) primary attack and a ranged (bow) secondary attack. What I suggest is that we use the 'Alt' key to toggle between attack types, similar to Rome:Total War. What we will need (that I can foresee) are these changes/features:

  • Actor support for this. A way to dictate in the actor when to switch between animations and props for each attack type (I suspect very similar to the way we now tell the actor how to switch animations and props for various resource gathering methods).
  • Entity code support. A way to dictate in the unit's entity XML file the difference between the primary and secondary attacks (the stats, as well as which attack is which).

Attachments (27)

t252_cleanupattack.diff (3.2 KB ) - added by bb 8 years ago.
Some quick trivial pre- cleanup
t252_secondattack_1.diff (27.0 KB ) - added by bb 8 years ago.
Patch
t252_secondattack_2.diff (27.6 KB ) - added by bb 8 years ago.
Tweaked some little things, mainly need to be decided which units get secondary attacks
attack-switch.png (9.4 KB ) - added by Lionkanzen 8 years ago.
t252_secondattack_3.2.diff (36.9 KB ) - added by bb 8 years ago.
rebased and further tweaking, EDIT added new files
t252_secondattack_3.diff (35.2 KB ) - added by bb 8 years ago.
rebased and further tweaking, EDIT added new files
attackswitch.png (9.1 KB ) - added by Lionkanzen 8 years ago.
t252_secondattack_4.diff (38.1 KB ) - added by bb 8 years ago.
Fixing some bugs found by fatherbushido, (it still crashes with any other spear champ than the persian immortal)
t252_secondattack_5.diff (38.9 KB ) - added by bb 8 years ago.
make prefType some more robust
action-attack2.png (5.7 KB ) - added by Lionkanzen 8 years ago.
a cursor for the attack
t252_secondattack_6.diff (105.0 KB ) - added by bb 8 years ago.
t252_secondattack_7.diff (112.9 KB ) - added by bb 8 years ago.
t252_secondattack_8.diff (54.1 KB ) - added by bb 8 years ago.
removed most secondary attacks, to have a easier review. In need a design decision for wether they should get back or not (and which other ones should be added).
t252_secondattack_9.diff (53.0 KB ) - added by bb 8 years ago.
fixing most remarks as above
t252_secondattack_10.diff (53.0 KB ) - added by bb 8 years ago.
t252_secondattack_11.diff (54.8 KB ) - added by bb 8 years ago.
previous patch broke test, now solved
t252_secondattack_12.diff (55.4 KB ) - added by bb 8 years ago.
fixed as above
t252_secondattack_13.diff (56.8 KB ) - added by bb 8 years ago.
cleanup as above
t252_secondattack_14.diff (55.9 KB ) - added by bb 8 years ago.
t252_secondattack_15.diff (62.1 KB ) - added by bb 8 years ago.
dunno if that new function in template.js is in the right place
t252_secondattack_16.diff (63.6 KB ) - added by bb 8 years ago.
Also check in simulation, just that check in templates.js L80 looks odd
t252_secondattack_17.diff (56.5 KB ) - added by bb 8 years ago.
rebased
t252_secondattack_18.diff (56.0 KB ) - added by bb 8 years ago.
rebased again
t252_secondattack_19.diff (66.8 KB ) - added by elexis 8 years ago.
all your rebase are belong to us. also removed unneeded else
t252_secondattack_20.diff (53.5 KB ) - added by bb 8 years ago.
t252_secondattack_21.diff (55.1 KB ) - added by bb 8 years ago.
rebased, fixed broken test and updated intro.txt
t252_secondattack_21.2.diff (54.9 KB ) - added by bb 8 years ago.
nuke charge

Download all attachments as: .zip

Change History (96)

comment:1 by Michael D. Hafer, 15 years ago

Description: modified (diff)

comment:2 by Michael D. Hafer, 15 years ago

Priority: minormajor

comment:3 by Michael D. Hafer, 15 years ago

Summary: Gameplay Scripting: Secondary AttacksGameplay Scripting: Entity and Actor coding for Secondary Attacks

comment:4 by (none), 13 years ago

Milestone: Alpha

Milestone Alpha deleted

comment:5 by Andrew, 13 years ago

Milestone: Backlog

comment:6 by Darth_Malloc, 11 years ago

Owner: set to Darth_Malloc
Status: newassigned

comment:7 by Stan, 9 years ago

Owner: Darth_Malloc removed
Status: assignednew

comment:8 by Stan, 8 years ago

Keywords: design added

comment:9 by wraitii, 8 years ago

Milestone: BacklogAlpha 20

Bumping for interest. I still believe this is something we should support, for example recently it has been suggested to create a "murder holes" technology for tower, which could use this.

Can be extended to actually choosing between both attacks or not.

by bb, 8 years ago

Attachment: t252_cleanupattack.diff added

Some quick trivial pre- cleanup

comment:10 by bb, 8 years ago

Keywords: patch review added
Owner: set to bb
Status: newassigned
Summary: Gameplay Scripting: Entity and Actor coding for Secondary Attacks[PATCH] Gameplay Scripting: Entity and Actor coding for Secondary Attacks

Just some cleanup to start, more is upcomming.

by bb, 8 years ago

Attachment: t252_secondattack_1.diff added

Patch

comment:11 by bb, 8 years ago

Previous patch implants secondary attacks with the alt hotkey, AI needs to be done. Champion spears got it in template. The Actors need to be designed properly.

Probably some clean up is necessary before committing (especialy input.js is a bit messy).

Last edited 8 years ago by bb (previous) (diff)

by bb, 8 years ago

Attachment: t252_secondattack_2.diff added

Tweaked some little things, mainly need to be decided which units get secondary attacks

comment:12 by Itms, 8 years ago

Milestone: Alpha 20Alpha 21

comment:13 by sanderd17, 8 years ago

Some remarks:

  • Even if you have no 2D art skills, it's better to include the new icon directly, so artists only have to improve the image and not search in the code where to change it (it's even good enough if you copy an existing image to be replaced)
  • The XML schema should offer a "choice" instead of just random text for primary and secondary attack
  • I don't know why you moved some split lines into long ones. It's nice IMO to have lines split at some point (I like the split them at 80 chars, counting a tab as 4).
  • It would be nice to let it work better with capturing, where capturing could be a primary, secondary or even tertiary attack. Allowing the templates to determine the order.

comment:14 by fatherbushido, 8 years ago

This ticket needs more interest and the patch needs some feedback. Here are my remarks.

  • the ticket description is quite old. On wikitrac, i found something close : If the unit has dual weapons (a melee and a ranged weapon), the player can force a unit to use its melee attack by double-right-clicking his target opponent. A ship will therefore close to melee range and then use its ram.
  • perhaps it needs some design decision. Perhaps it can be done more automatically. I see 4 behaviours:
    • the one described on the ticket and implement in the patch
    • an automatic one, the unit do the attack it can done. I don't precisely how it can works.
    • a toggle between a global (melee or ranged) prefered type attack (with an hotkey as in the patch or with a stance).
    • something between 3 & 4 described in the wikitrac: always prefer the range and toggle to go to melee if we can.
  • the main current problem is that this "alt selecting second attack" needs some micro and moreover, if we select many units where some have melee attack for first attack and others have ranged attack for first attack, when using alt we will have some weird behaviour.
  • perhaps the capture attack, (and then charge) can be included in this system.
  • about the patch himself: it looks good but i didn't look all details (i m not aware of the art part) and was also lazy to test it (some line of the code have changed since upload of the patch).

So having some feedbacks about design/patch might be nice to avoid wasting time in basing, re-basing, writing, re-writing code, and editing, re-editing templates (many infos in the design docs).

Last edited 8 years ago by fatherbushido (previous) (diff)

comment:15 by Lionkanzen, 8 years ago

If you request a icon, I can work in one, I have the concept, but I need to know the size.

comment:16 by bb, 8 years ago

If you want to I would be very pleased (my artistic talent won't be of much use). There would be two icons needed, "action-second-attack-move.png" and "action-second-attack.png". For size: see the icons already in game "action-attack-move.png" and "action-attack.png", somewhere in art/textures/cursors, it should be the same size.

by Lionkanzen, 8 years ago

Attachment: attack-switch.png added

comment:17 by Lionkanzen, 8 years ago

oops sorry I attached wrong file first. attack-switch.png is the correct

by bb, 8 years ago

Attachment: t252_secondattack_3.2.diff added

rebased and further tweaking, EDIT added new files

comment:18 by fatherbushido, 8 years ago

Lionkanzen: most of the time, it's a range and a melee attack, if you had something like a bow or javelin vs a sword, it would be better. Thx for the icon anyway.

comment:19 by bb, 8 years ago

Patch including most remarks above, not sure what how to solve fatherbusido's third remark. For this patch two new cursor icons are needed ("action-attack-move.png" and "action-attack.png") just grab some random picture for that :P.

NOTE: the "_3" file is the newest so not "_3.2"

@Lionkanzen: nice icon but I think I was not clear in what icon was needed. I was looking for a cursor icon not a tech/unit one. Though this one can be used for something related ;). (Maybe add a tech somewhere to unlock second attacks?)

by bb, 8 years ago

Attachment: t252_secondattack_3.diff added

rebased and further tweaking, EDIT added new files

in reply to:  18 comment:20 by Lionkanzen, 8 years ago

Replying to fatherbushido:

Lionkanzen: most of the time, it's a range and a melee attack, if you had something like a bow or javelin vs a sword, it would be better. Thx for the icon anyway.

Yes indeed, I will try with another, but the range weapons are some light/thin than the sword,except by the bow. But I can work in another version. Use this for now.

by Lionkanzen, 8 years ago

Attachment: attackswitch.png added

comment:21 by Lionkanzen, 8 years ago

fixed... need test.

by bb, 8 years ago

Attachment: t252_secondattack_4.diff added

Fixing some bugs found by fatherbushido, (it still crashes with any other spear champ than the persian immortal)

by bb, 8 years ago

Attachment: t252_secondattack_5.diff added

make prefType some more robust

in reply to:  19 comment:22 by Lionkanzen, 8 years ago

@Lionkanzen: nice icon but I think I was not clear in what icon was needed. I was looking for a cursor icon not a tech/unit one. Though this one can be used for something related ;). (Maybe add a tech somewhere to unlock second attacks?)

Ok, I thought you need a button to give the command in the interface not a cursor, I will make one. Later. I didn't a cursor in my mind. I will make one but I need think in redesign, similar, but I don't test this before, my exp with this is different, only total war have secondary attack with cursor, so in my opinion the sword should be changed by a bow or by all ranged weapons.

comment:23 by Lionkanzen, 8 years ago

here i put some icons and others. i attached the cursor now.

by Lionkanzen, 8 years ago

Attachment: action-attack2.png added

a cursor for the attack

by bb, 8 years ago

Attachment: t252_secondattack_6.diff added

comment:24 by bb, 8 years ago

New patch includes:

  • secondary attacks for ranged and sword citizen infantry
  • better AI handling (still a hack)
  • new hotkey schema:
    • ctrl: primary attack
    • alt: melee attack
    • space: ranged attack
  • new cursors for these actions (action-melee-attack.png and action-ranged-attack.png)

the melee cursor can be the already in game sword and for ranged we can use the bow.

by bb, 8 years ago

Attachment: t252_secondattack_7.diff added

comment:25 by bb, 8 years ago

  • slightly better UnitAi handling
  • fixing #3492 (fix is inlined and splitting is not really possible as we need to mess with the old allowCapture flag and we now can just use the prefType.)

comment:26 by fatherbushido, 8 years ago

That sounds pretty good ! Now you need to convince some devs !

I would suggest to split the templates stuff from the patch as it needs some design decisions. Btw, the ship ramming is a very good idea (iberians, britons, gauls, mauryans "trireme" should not have it). Moreover only few units were designed with a second attack :

  • brit

slingers (sling + long knife - sword)

  • cart

hero maharbal (javelin + spear)

  • gaul

slingers (sling + long knife - sword)

  • iber

champ Devotio Cavalry (sword + flaming javelins)

  • maur

Maiden Guard. (currently two separate units)

  • pers

Persian Immortal. (spear + bow)

  • ptol

Mercenary Thureos Spearman. (see sele)

  • rome

Hastatus Roman Swordsman. Which can throw heavy Roman javelins (pilla) at advanced and elite ranks Italic Heavy Infantry. (javelin and sword) Consular Cavalry. (same)

  • sele

Militia Thureos Spearman. Uses javelins at range, spear for melee Romanized Heavy Swordsman. (javelin and sword)

  • spart

hero brasidas. (sword + javelin)

(and elephants in an automatic way, the turret man use his spear and the elephant do his attack).

by bb, 8 years ago

Attachment: t252_secondattack_8.diff added

removed most secondary attacks, to have a easier review. In need a design decision for wether they should get back or not (and which other ones should be added).

comment:27 by sanderd17, 8 years ago

Keywords: review removed

Some questions/remarks:

  • Why do you have target checks on the melee-attack-move and ranged-attack-move in unit_actions.js? If they're similar to attack-move, they should be targetted at a point. If they're similar to attack, they shouldn't have move in their name
  • lines 120, 158, 183, 210, ... in unit_actions.js is there a reason why you don't use the getActionInfo of input.js? It has some extra code you shouldn't be doing anymore (like looping over the selection, or getting the entity state).
  • In Attack.js, it's better to query the 2D position if you only need that much. And you should also use the squared distance when possible (to avoid roots as much as possible).
  • In Attack.js, you shouldn't use this.template.ChangeDistance directly, as that's a string. Better make a correct getter for it, or store it under this.
  • Perhaps it's worth to store the attack types in a Set, so you can easier query if it exists (instead of using indexOf). And perhaps these should be cached (together with the preferred and restricted classes list), but that may be for a different ticket.
  • I can't find where you moved the player check of GuiInterface.js
  • For WalkAndFight, it would be better to leave queued as last argument, as it's for every function
  • By getting rid of the GetFormationUnitAIs in commands.js, it will disable formations completely for attack-walk and attack. This isn't something to be implemented in this patch. It also omits the check whether the unitAI component even exists, and there's also no check whether the attack component exists. It would be better to keep preftype undefined in that case, and let UnitAI decide what the preferred type is.

comment:28 by sanderd17, 8 years ago

Cc: mimo added

I also didn't review the AI code, so I'm CC-ing mimo now.

in reply to:  27 comment:29 by bb, 8 years ago

Replying to sanderd17:

  • Why do you have target checks on the melee-attack-move and ranged-attack-move in unit_actions.js? If they're similar to attack-move, they should be targetted at a point. If they're similar to attack, they shouldn't have move in their name
  • In Attack.js, it's better to query the 2D position if you only need that much. And you should also use the squared distance when possible (to avoid roots as much as possible).
  • In Attack.js, you shouldn't use this.template.ChangeDistance directly, as that's a string. Better make a correct getter for it, or store it under this.
  • By getting rid of the GetFormationUnitAIs in commands.js, it will disable formations completely for attack-walk and attack. This isn't something to be implemented in this patch. It also omits the check whether the unitAI component even exists, and there's also no check whether the attack component exists. It would be better to keep preftype undefined in that case, and let UnitAI decide what the preferred type is.

DONE

  • Perhaps it's worth to store the attack types in a Set, so you can easier query if it exists (instead of using indexOf). And perhaps these should be cached (together with the preferred and restricted classes list), but that may be for a different ticket.

in another ticket (as patch is already big enough)

  • I can't find where you moved the player check of GuiInterface.js

attack.js line 281-284, the check at 286 got some refinement in new patch.

  • For WalkAndFight, it would be better to leave queued as last argument, as it's for every function

won't be done for now.

  • lines 120, 158, 183, 210, ... in unit_actions.js is there a reason why you don't use the getActionInfo of input.js? It has some extra code you shouldn't be doing anymore (like looping over the selection, or getting the entity state).

in patch another fix for same issue (#3492) this one might be easier to split, its only the last change in input.js.

by bb, 8 years ago

Attachment: t252_secondattack_9.diff added

fixing most remarks as above

comment:30 by sanderd17, 8 years ago

  • input.js, if (r) was changed to if (r.possible). That should be if (r && r.possible)
  • GetChangeDistance should return a number instead of a string.

Apart from that, the simulation code looks good (and I'll let mimo review the AI code). I didn't test it yet, but heard fatherbushido tested it, and found no issues with it.

by bb, 8 years ago

Attachment: t252_secondattack_10.diff added

by bb, 8 years ago

Attachment: t252_secondattack_11.diff added

previous patch broke test, now solved

comment:31 by mimo, 8 years ago

Thanks for working on this patch.

  • prefType is not explicit enough -> preferredAttack would be better imo.
  • would be better to move Attack.prototype.GetNormalDistribution in globalscripts/Math.js
  • why do you need this "primary" prefType (in the ai or in unit_actions.js)? I thought an undefined prefType would default to the primary attack.
  • also in UnitAI.js, you sometimes define prefType to undefined ? would be simpler to not have this property in the object. And you also sometimes use this.GetBestAttackAgainst(target, undefined) ? better to have only this.GetBestAttackAgainst(target) which looks clearer to me.
  • and line 4582, this.GetBestAttackAgainst(target, true, undefined) ?? 3 args ?

ai/common-api/entity.js:

  • move up the isCapturable before the isGarrisonHolder (to keep the 3 garrison functions together)

ai/petra/attackPlan.js:

  • line 1323 the 3rd argument false could be removed (was not there before)

ai/petra/entityExtend.js

  • the new function prefType (better call it getPreferredAttack or something like that) must have a 3rd argument which would be a Set of entities not to be captured so that each attackPlan which has such a Set (this.noCapture) could use it to define its preferred attack.

in reply to:  31 comment:32 by bb, 8 years ago

Replying to mimo:

  • prefType is not explicit enough -> preferredAttack would be better imo.

agree, made it prefAttackType

Stands apart from this ticket, though it might be good. Should be a separate commit.

  • why do you need this "primary" prefType (in the ai or in unit_actions.js)? I thought an undefined prefType would default to the primary attack.

Undefined can also result in "Capture", and as these function need to specify that the archer will shoot and swordman will do his thing, it needs a "primary" tag. This way we can have a group (ranged+melee) and all will do there best attack and not capture.

  • also in UnitAI.js, you sometimes define prefType to undefined ? would be simpler to not have this property in the object.

It will give some warnings if we do not declare it (as we don't check if it exists later and checking is a bit of a pain). What can be done is calling "GetBestAttackAgainst" from these places but that gives us some extra code (and it duplicates code in the execution).

And you also sometimes use this.GetBestAttackAgainst(target, undefined) ? better to have only this.GetBestAttackAgainst(target) which looks clearer to me.

ai/common-api/entity.js:

  • move up the isCapturable before the isGarrisonHolder (to keep the 3 garrison functions together)

ai/petra/attackPlan.js:

  • line 1323 the 3rd argument false could be removed (was not there before)

ai/petra/entityExtend.js

  • the new function prefType (better call it getPreferredAttack or something like that) must have a 3rd argument which would be a Set of entities not to be captured so that each attackPlan which has such a Set (this.noCapture) could use it to define its preferred attack.

Solved

by bb, 8 years ago

Attachment: t252_secondattack_12.diff added

fixed as above

comment:33 by bb, 8 years ago

Keywords: review added

comment:34 by mimo, 8 years ago

It is not really clear to me how this patch is supposed to work. Can you give more details ? When looking at the code, sometimes i'm not sure if there is a problem or if i have not understood what the code is supposed to do. Then, i think some comments in the code + some description here on this ticket would help.

otherwise, here are some defects i've noticed on the patch:

entity.js keep a consistent order of arguments (attackMove has prefAttackType after queued while attack has it before). Better put queued at the end as most of other functions there.

attackPlan.js

line 1656 m.getPrefAttackType should have the attacker ent and not the attacker id as argument

defenseArmy.js

I guess lines 120-121 sould be

if (target) {

let prefAttackType = m.getPrefAttackType(ent, target); if (prefAttackType !== "Capture")

ent.attack(orderData[0].target, prefAttackType);

}

entityExtend.js

line 87 noCapture.has(target) should be noCapture.has(target.id()) line 89 i guess it should be return ent.hasClass("Melee") ? "Melee" : "Range";

Attack.js

line 246 and 254 imo the previous indentation was better line 284 could be moved just after line 289, same thing for lines 275-278 to move after 294 line 298 should be "&& !cmpCapturable" I don't think CanAttackWithType does what is expected. We can have the right type in the template, but not being able to attack with this type for example due to the heightdiff cut while CanAttack returns true for another type. I think it would be better to remove this CanAttackWithType and add a second argument to CanAttack(target, wantedType), and add in the loop if (wantedType && type != wantedType) continue;

GetBestAttackAgainst: why do you now choose according to distance instead of the previous way based on preferredClasses ? that was far from perfect, but imo better than a simple distance cut. Against a ram for example, an archer should better come near and use its melee attack rather than its range one even if farther than the cut.

line 447 missing semicolon

GuiInterface.js

What is this GetAttackGetAttackTypeFromOrder ? should be GetAttackTypeFromOrder. I suppose the tests fail with this patch.

in reply to:  34 comment:35 by bb, 8 years ago

Replying to mimo:

It is not really clear to me how this patch is supposed to work. Can you give more details ? When looking at the code, sometimes i'm not sure if there is a problem or if i have not understood what the code is supposed to do. Then, i think some comments in the code + some description here on this ticket would help.

For AI you are absolutely right, what I putted in was a wild hack to make it work (if anyone has a better opinion, just tell me).

About more detail: what you need to know? User interface see comment 24 and further all seems to be along the already implemented actions

otherwise, here are some defects i've noticed on the patch:

entity.js keep a consistent order of arguments (attackMove has prefAttackType after queued while attack has it before). Better put queued at the end as most of other functions there.

Better keep preAttackType last as it is in the rest of the code.

line 89 i guess it should be return ent.hasClass("Melee") ? "Melee" : "Range";

(not necessary but cleaner indeed, the attack component will figure it out by itself)

Attack.js

GetBestAttackAgainst: why do you now choose according to distance instead of the previous way based on preferredClasses ? that was far from perfect, but imo better than a simple distance cut. Against a ram for example, an archer should better come near and use its melee attack rather than its range one even if farther than the cut.

The preferredClasses was working even worse than the distance cut because most units were not listed in the preferredClasses list so it could easily result in archers trying to shoot a horse from 1 meter, what is even worse in my eyes. What could be done is combining the two ways (like if there are preferredClasses and the target is one of them we should follow that, if not range cut). Did this in new patch. Maybe we now need to make isTargetClass a prototype function, not sure.

GuiInterface.js

What is this GetAttackGetAttackTypeFromOrder ? should be GetAttackTypeFromOrder. I suppose the tests fail with this patch.

(test didn't fail, but its wrong indeed)

by bb, 8 years ago

Attachment: t252_secondattack_13.diff added

cleanup as above

comment:36 by mimo, 8 years ago

What i'd like is a description of what the patch is supposed to do, for the moment i am only guessing, and it does not help for a review. A first basic question is: when will this "primary" tag be used ? you've added it only for pers_champion_infantry. Does it mean that it will be given only for a few units and others will have a default behaviour ? or will all units have a primary attack ? Depending on that, i'm unable to judge if this approach will be easily scalable when we (or a mod) want to add a new attack. Btw, you never test on the existence of ChangeDistance, so this patch should fail for most units for which you don't put it in their template ?

Concerning the order of arguments in entity.js, the important is consistency inside the ai code. What the order is in the simulation is not relevant.

in reply to:  36 comment:37 by bb, 8 years ago

Replying to mimo:

What i'd like is a description of what the patch is supposed to do, for the moment i am only guessing, and it does not help for a review. A first basic question is: when will this "primary" tag be used ?

When I order an attack with ctrl all selected units go to there primary attack (archer ranged and swordsmen melee) and we need somehow define which attack is primary (the secondary tag is unused now indeed, but as any mod wants a command for secondary attack it might be relevant to put it in). The GetAttackTypeFromOrder function will figure out which type it really (e.g. Melee, Ranged) is.

you've added it only for pers_champion_infantry. Does it mean that it will be given only for a few units and others will have a default behaviour ? or will all units have a primary attack ?

The pers champ is just a test case, for which units need a secondary attack is mainly a design decision.

Depending on that, i'm unable to judge if this approach will be easily scalable when we (or a mod) want to add a new attack.

Also see version 7 where I putted in a little Melee attack for all citizen ranged infantry.

Btw, you never test on the existence of ChangeDistance, so this patch should fail for most units for which you don't put it in their template ?

As there is no ChangeDistance in the template there may not be a ranged + melee attack in template as then the template is broken, in this occasion indeed it will crash but it shouldn't happen. See the check and return in L432-435 of Attack.js.

Concerning the order of arguments in entity.js, the important is consistency inside the ai code. What the order is in the simulation is not relevant.

Will change this if there are some more flaws (though I think consistency throughout the code is nice too)

comment:38 by Lionkanzen, 8 years ago

Cc: Lionkanzen added

comment:39 by mimo, 8 years ago

OK so here are a few additional comments on the changes in Attack.js, comments on the ai will come later as they depend on the implementation in the simulation.

GetChangeDistance, you should test if it exists and otherwise return a default value as it is an optional element. Even if in the patch, you assume that it will be necessarily defined for units with both a Melee and a Ranged attack, that may not be the case in the future or in mods.

You have moved the check that attacker and target are enemies from UnitAI+GuiInterface to Attack: any good reason to do that ? not that i'm against it, but to understand the reason as I'm still not sure if it should be a gameplay choice (so better stay in UnitAI) or something intrinsic to the Attack component.

CanAttackWithType could be simplified to

return this.template[type] && this.CanAttack(target, type);

or even better be removed and add the test on this.template[type] at the beginning of CanAttack when type is defined.

GetBestAttackAgainst,

lines 447 to 453 are already checked for in the CanAttack function, so can be omitted here. lines 432 to 435 makes some assumptions on the order of the attack types which are far from obvious (and which a mod could invalidate). So I would

move 437 to 443 just after 431 as there is no asumptions there replace 432 to 435 + 445 to 457 by

   meleeIndex = types.indexOf("Melee");
   rangedIndex = types.indexOf("Ranged");
   if (meleeIndex != -1 && rangedIndex != -1)
      doTestOnDistance
   else if (meleeIndex != -1)
      return "Melee";
   else if (rangedIndex != -1)
      return "Ranged";
   else
      return types[0];

and after line 408, add "if (types.length == 0) return undefined"

what is the use of GetAttackTypeFromOrder ? shouldn't we just put prefAttackType in lines 4989 and 5025 of UnitAI and let GetBestAttackAgainst deals with it (by moving lines 557 to 560 somewhere inside GetBestAttackAgainst).

And while at it, there are places in UnitAI where GetBestAttackAgainst may not protected against returning undefined.

in reply to:  39 comment:40 by bb, 8 years ago

Replying to mimo:

You have moved the check that attacker and target are enemies from UnitAI+GuiInterface to Attack: any good reason to do that ? not that i'm against it, but to understand the reason as I'm still not sure if it should be a gameplay choice (so better stay in UnitAI) or something intrinsic to the Attack component.

To avoid duplicate code, we need the cmpEntity and cmpTarget components in attack in order to solve the problem we cannot attack sheeps (not enemy units we do can attack) with champs. So why call the engine in two places as 1 is enough? ofc the same check can be done in guiinterface and unitAI but as we need "targetClasses" for this check and we already need that in Attack, better place the check here.

lines 447 to 453 are already checked for in the CanAttack? function, so can be omitted here.

It cannot since canAttack is not always called when GetBestAttackAgainst is called (e.g. L419 UnitAI.js). Yes, GetBestAttackAgainst is here used as CanAttack what probably should change in a seperate commit.

   meleeIndex = types.indexOf("Melee");
   rangedIndex = types.indexOf("Ranged");
   if (meleeIndex != -1 && rangedIndex != -1)
      doTestOnDistance
   else if (meleeIndex != -1)
      return "Melee";
   else if (rangedIndex != -1)
      return "Ranged";
   else
      return types[0];

The idea will work, but we need to check for (types.length !== 0) and remove the last else but just return types[0] (as we need to a plain return to not get warning spammed)

what is the use of GetAttackTypeFromOrder ?

Get the attackType "primary" really is.

shouldn't we just put prefAttackType in lines 4989 and 5025 of UnitAI and let GetBestAttackAgainst deals with it (by moving lines 557 to 560 somewhere inside GetBestAttackAgainst).

Better call this function from GetBestAttackAgainst as that looks way cleaner than inlining.

And while at it, there are places in UnitAI where GetBestAttackAgainst may not protected against returning undefined.

fixed the one place where is not checked for GetBestAttackAgainst or canAttack.

by bb, 8 years ago

Attachment: t252_secondattack_14.diff added

comment:41 by Stan, 8 years ago

line 94,96,127,129,635,637,646,649 of unit_action.js why not let ? line 437 of Attack.js Might be better to actually check it than to add a Todo What are the txt files containing 1 1 ?

comment:42 by elexis, 8 years ago

  • Those arguments for PostNetworkCommand and PostCommand should be split across several lines, maybe some others like PushOrderFront too
  • entityExtend.js L86 L90 unneeded parenthesis

in reply to:  41 comment:43 by bb, 8 years ago

Replying to stanislas69:

line 94,96,127,129,635,637,646,649 of unit_action.js why not let ?

Because we cannot declare a let within a code block and use it outside. Yes we can declare the variable outside ofcourse, but that will add some more changed lines (to keep consistence) and we need to keep the patch reasonable.

line 437 of Attack.js Might be better to actually check it than to add a Todo

Nah, we might sometime want to add two melee/ranged attacks to the same unit and that is not supported yet, so I placed a TODO (and we do check see the if + elses comming after that comment).

What are the txt files containing 1 1 ?

Files for the new cursors.

comment:44 by mimo, 8 years ago

GetChangeDistance can be simplified to return +(this.templateChangeDistance || 0);

in entityCollection.js, better put "prefAttackType": null or undefined in both PostCommand. I understand that it would thus switch to Capture if possible, or otherwise take the best one from getBestAttackAgainst which is the current behaviour, while with "prefAttackType": "primary" it will give priority to the primary attack against capture.

in entity.js, same comment, put prefAttackType = null in the function definition of attackMove and attack.

attackPlan.js

line 1657, attacker is not defined, you would need to (but not needed if following the next comment) after line 1642, add "let attacker;" after line 1651, add "if (!gameState.getEntityById(unit.unitAIOrderData()[0].target)) return;" and on line 1654 add "attacker = gameState.getEntityById(attackerId);"

But a more general comment concerning the ai, i think it would be best to reproduce first the current state in this patch, and then try to improve the ai later (I'm not sure all the changes using m.getPrefAttackType will have the expected effect, in particular the test on the garrisonHolder which was not supposed to be used when an army is attacking, but only when called from defenseArmy).

So as the previous argument of the different attacks was only the boolean allowCapture, we could use two possible values for prefAttackType for the ai: either null or undefined when allowCapture was true, or "noCapture" when allowCapture was false, and modify accordingly Attack.js. That is basically to modify the filter isAllowed in GetBestAttackAgainst to not select Capture when prefAttackType="noCapture", and also removing Capture from the array best in line 384 when prefAttackType="noCapture".

comment:45 by fatherbushido, 8 years ago

I wonder what will happen with archers (visible-)garrisoned on a wall.

in reply to:  44 comment:46 by bb, 8 years ago

Replying to mimo:

But a more general comment concerning the ai, i think it would be best to reproduce first the current state in this patch, and then try to improve the ai later (I'm not sure all the changes using m.getPrefAttackType will have the expected effect, in particular the test on the garrisonHolder which was not supposed to be used when an army is attacking, but only when called from defenseArmy).

So as the previous argument of the different attacks was only the boolean allowCapture, we could use two possible values for prefAttackType for the ai: either null or undefined when allowCapture was true, or "noCapture" when allowCapture was false, and modify accordingly Attack.js. That is basically to modify the filter isAllowed in GetBestAttackAgainst to not select Capture when prefAttackType="noCapture", and also removing Capture from the array best in line 384 when prefAttackType="noCapture".

Did some AI tests: putted some different unit combo's against each other and looked what the AI did. Did not look to worse, I think is about ok for now, though can be improved (ofc).

Replying to fatherbushido:

I wonder what will happen with archers (visible-)garrisoned on a wall.

Whith last patch: nothing, as only ranged units could go into walls, and we have some hight diff. Changed that logic so it uses the attack stats (yes added the classes would work too, but thats an ugly hack and the classes have no meaning at all than). On walls its only allowed to ranged attack for now.

by bb, 8 years ago

Attachment: t252_secondattack_15.diff added

dunno if that new function in template.js is in the right place

by bb, 8 years ago

Attachment: t252_secondattack_16.diff added

Also check in simulation, just that check in templates.js L80 looks odd

comment:47 by elexis, 8 years ago

In 18214:

Remove trailing whitespace and simplify some length checks. Refs #252.

comment:48 by elexis, 8 years ago

In 18224:

Session / unit-actions cleanup.

Eliminate deprecated for-each.
Merge if-conditions.
Add linebreaks to objects, so that each property is declared on a separate line, refs #252.
For consistency, use returns in negative instead of positive cases.
Make three functions global, one of which had been defined thrice.
Remove comments stating the same as the code.

by bb, 8 years ago

Attachment: t252_secondattack_17.diff added

rebased

by bb, 8 years ago

Attachment: t252_secondattack_18.diff added

rebased again

comment:49 by wraitii, 8 years ago

I'm not entirely sure this is the right direction to go in. First of all your patch seems to only support 2 different attack types, but it's also rather unintuitive in the way it'd ultimately work, since it's relying on orders rather than unit states.

I think it would be much better to just allow choosing "auto", "melee", "ranged" - or anything else - in the GUI for a group of given units. Auto would switch based on some sort of DPS heuristic and max/min range, otherwise units would follow your orders.

by elexis, 8 years ago

Attachment: t252_secondattack_19.diff added

all your rebase are belong to us. also removed unneeded else

comment:50 by wraitii, 8 years ago

I slightly changed my mind about the GUI state thing. I think I've got a better idea: the player should be able to enable/disable attack types individually on an entity basis, in the GUI.

ie If you have an archer with a melee attack, by default you'd have both attacks enabled and the unit auto chooses. But if you disable the melee attack, it'll stay ranged, and vice versa. This is imo the simplest way to implement something like that clearly and extensively.

Furthermore I would not rely too much on primary/secondary attacks as a hardcoded thing, we may want to support more and we probably want that for mods too. The best attack should be determined by the characteristics: range, minimal range, DPS, ...

With that in mind, I believe most of your patch could be kept.

comment:51 by elexis, 8 years ago

Notice that it should ideally be possible to instantaneously chose between attacks, you shouldn't have to wait one turn to change the attack and then send the attack order. This is a problem with unit stances right now. If you move your units somewhere, then change the stance from aggressive to defensive, they will stop walking and you have to send the walk command again.

comment:52 by Lionkanzen, 8 years ago

I recorded this Is different to Total War mechanic, it's more switch in the interface.

https://youtu.be/A1z90Rhv8J8 The first 7 seconds.

Last edited 8 years ago by Lionkanzen (previous) (diff)

in reply to:  50 comment:53 by bb, 8 years ago

Replying to wraitii:

I slightly changed my mind about the GUI state thing. I think I've got a better idea: the player should be able to enable/disable attack types individually on an entity basis, in the GUI.

ie If you have an archer with a melee attack, by default you'd have both attacks enabled and the unit auto chooses. But if you disable the melee attack, it'll stay ranged, and vice versa. This is imo the simplest way to implement something like that clearly and extensively.

The idea seems nice but there is a problem when using groups of units (not necessarily formations), what should this button show then and how should the units respond to an attack order? I guess some chaos will arise... Just resetting the setting on change of group would lead to more click-work so is not a good option. Any other ideas in order to solve/avoid this problem?

Furthermore I would not rely too much on primary/secondary attacks as a hardcoded thing, we may want to support more and we probably want that for mods too. The best attack should be determined by the characteristics: range, minimal range, DPS, ...

We could propably do this on basis of unit class (so a ranged class unit will get automatic the ranged attack as primary, we can skipp the primary/secondary things attack component then.

comment:54 by wraitii, 8 years ago

If there are conflicting states, the usual behavior is to show a "3rd state" and reset everything when you click. I don't think an attack order should change anything, though we might want attack order that trigger some attack states.

comment:55 by bb, 8 years ago

See what happens in a group of archers (with melee attack) set half of them ranged disabled and other half melee disabled, select all. The thing will now show a 3rd state (were there is no difference between 1 unit with melee and and 1 unit with ranged disabled), half of the group will do ranged other half melee, ok. But player cannot know how many of his archers do which attack (3rd state doesn't show, and if u show in tooltip it will use too much mouse-work) as the state can be set earlier. So most players will just skipp these buttons as they just produce chaos and new players won't understand what happens.

by bb, 8 years ago

Attachment: t252_secondattack_20.diff added

comment:56 by bb, 8 years ago

The new patch contains a DPS/Range system too choose between attacks when there is no direct order.

Just for testing the persian archer got a melee attack, only that actor is updated but the template of many more units is changed (all ranged infantry), so I would recommend to only use that ranged infantry for testing...

comment:57 by Itms, 8 years ago

Keywords: rfc added; review removed

Move tickets from the review queue to the rfc one.

by bb, 8 years ago

Attachment: t252_secondattack_21.diff added

rebased, fixed broken test and updated intro.txt

comment:58 by Stan, 8 years ago

Hey i think you should remove the charge todo has fagherbushido removed all mentions of charge in the code. Also maybe rename "possible" to "isCapturePossible,' or "isCapturable"

comment:59 by bb, 8 years ago

Will remove the charge in next patch. "possible" is needed to stay in see input.js L231 and is not only used for Capture but for all actions so it needs to be something universal.

by bb, 8 years ago

Attachment: t252_secondattack_21.2.diff added

nuke charge

comment:60 by fatherbushido, 8 years ago

refs #4220

comment:61 by elexis, 8 years ago

Milestone: Alpha 21Alpha 22

Feature freeze in 2 days.

comment:62 by elexis, 7 years ago

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:63 by temple, 6 years ago

Description: modified (diff)

I was thinking rather than primary and secondary attacks, we could have a list of them in the unit's template in order of preference. We'd include matching classes because e.g. spearman might want the order: melee against units, then capture anything, then melee anything (structures). This seems more general than DPS, since how else would we decide between melee and capture? Ranged attacks could first match Unit+!Siege, for example. It seems like we could include heal too rather than treat that as a separate state. We could modify (usually through stances, but maybe sometimes with hotkeys) an attack by matching types of attack (e.g. don't capture) or classes of targets (e.g. ignore structures). When figuring out if we can attack or the best attack against a unit, we could just go through the attacks one by one and see if it has the right properties depending on the type (damage receiver, capturable, healable) and matches the right classes. When searching for new targets we could go through the attacks one by one using the relevant range queries.

comment:64 by FeXoR, 5 years ago

In short: 0 A.D. is a realtime strategy game with 100s of units. That's not a place for such a feature IMO.

AFAICS the ticket description stated nothing about automated choices of the attack type (that where proposed in the discussion later) and the impact on the needs for the unitIA to handle that.

I want to raise my concerns about both ways but would be glad to be wrong here ;)

1) Attacks switched by the player This would be a lot more practical. I still see some issues here:

  • In the case of multiple units selected it is not clear what to do
  • How does the player know which attack state the units are in?

2) Attack switched automated by unitAI This approach is potentially game breaking in several ways:

  • The outcome of the unit behavior is unclear to begin with and IMO the unitAI is far from optimal in choosing it in the current state already IMO. Adding such decisions would greatly increase the complexity.
  • Even in a case of a flawless implementation I expect an enormous performance impact IMO not worth the gain.

comment:65 by wraitii, 5 years ago

I like supporting multiple attacks if only because:

  • "Melee" and "Ranged" might be renamed.
  • Player-chosen attacks would be useful for artillery-like units (or say archer, we could switch between long-range volleys and short-range precision shots).

I don't think UnitAI choosing correctly is such a concern. So long as the default choice is sensible _enough_, we are imo OK.

comment:66 by FeXoR, 5 years ago

Sure, just that I don't think the unitAI's decision making is "sensible enough" ATM (e.g. stop gathering at some point the player can't foretell, move when it has an attack-move command though a unit is in attack range) and until I have gained the confidence that the unitAI is working well I'll advise against adding complexity to it.

Automated choices for dual attacks would needs to take into account both unit speed and range to make sane decisions (e.g. to determine if a unit should use a more damaging melee attack or a ranged attack for it could get killed before it even reaches the target and thus still deal less damage with the melee attack in the end or the target could just run away - or all the other possibilities on the battlefield).

comment:67 by wraitii, 4 years ago

Patch: Phab:D368

comment:68 by bb, 3 years ago

In 24095:

Do not hardcode attacktypes in the engine/Atlas

fixes rP23592
refs #252, D368
Reviewed By: vladislavbelov
Comments By: Stan, wraitii
Differential Revision: D2998

comment:69 by bb, 3 years ago

In 24209:

Generalize to arbitrary translatable attacknames in the GUI

Reviewed By: Nescio, Freagarach
Comments By: wraitii, elexis

Differential Revision: D2995
refs: #252

Note: See TracTickets for help on using tickets.