Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4220 closed defect (fixed)

[PATCH] Wrong cursor for capture attack on a structure wich is in RestrictedClasses

Reported by: fatherbushido Owned by: fatherbushido
Priority: Should Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

When adding a structure to the RestrictedClasses for the capture attack of a unit, the capture attack is indeed disabled (and the unit uses his melee/ranged attack) but it looks like the wrong cursor is displayed (the capture one).

Attachments (2)

4220.diff (6.8 KB ) - added by fatherbushido 7 years ago.
bb's patch as a diff
4220_testcase.diff (4.0 KB ) - added by fatherbushido 7 years ago.
an example of use wich was broken without the patch

Download all attachments as: .zip

Change History (18)

comment:1 by fatherbushido, 8 years ago

refs #3340, #252

comment:2 by bb, 8 years ago

The cause of this problem is that the CanAttack function in attack.js returns whether it is possible to attack the target with an undefined attackType (as in does not matter which). So unit_action GetActionInfo function from the Capture attack (this happens ofc via guiInterface) does not check if it can attack with a capture attack but with an attack. Now when we call the function from unit_action Capture it returns "true" since it is possible for another attackType.

The patch in #252 *should* solve this problem by setting a wantedType argument in the CanAttack function (maybe the checks in CanCapture in guiInteface need to move to CanAttack but that is a simple thing). But that does not solve the whole problem because the same can happen the other way around (e.g Capture is possible in CanAttack but we don't order a capture attack). We could solve this by explicitly setting the wantedType in the "attack" function in unit_actions and then calling the CanAttack() function twice with "Ranged" and "Melee" but this sounds hardcodish to me. Another solution would be allowing to set wantedType="noCapture" and handle that too in CanAttack but that means CanAttack will be a beast. I don't know what is the best solution here.

comment:3 by fatherbushido, 7 years ago

I guess we could/should add the optional types argument in Attack.CanAttack() but in a versatile form (types is an array, allowing even something like capture or !capture). That would fix the current issues and allowing (for mods or anything else) different cursors for different attacks for example.

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

comment:4 by bb, 7 years ago

Splitted patch from #252 with these array + white-listing changes here: https://github.com/bb-bb/0ad/tree/t4220_AttackCursor I hope this solves this bug (it should but didn't test by adding restricted classes and stuff)

comment:5 by fatherbushido, 7 years ago

Keywords: patch rfc added
Milestone: BacklogAlpha 22
Summary: Wrong cursor for capture attack on a structure wich is in RestrictedClasses[PATCH] Wrong cursor for capture attack on a structure wich is in RestrictedClasses

It sounds very good (very far overlook). About namming, types instead of wantedTypes is perhaps sufficient. Do you know if ai would need some changes ? Nothing to change in UnitAI.CanAttack() ? (Can you provide a diff, I m a bit lazy).

comment:6 by elexis, 7 years ago

Click on compare, then add .diff to the url.

comment:7 by fatherbushido, 7 years ago

I got the first step, thx for the second one.

in reply to:  5 comment:8 by bb, 7 years ago

Replying to fatherbushido:

Do you know if ai would need some changes ?

AFAIK ai orders the units via commands.js to unitAI and UnitAI will determine whether it is possible to use each attackType.

Nothing to change in UnitAI.CanAttack() ?

UnitAI.CanAttack() does not take "type" argument (yet) and the current calls have the function to check wheter it is possible to attack with any attack type. GetBestAttack() is used for determining the attackType, and that check for possibility on its own. So it is not necessary to change that unitAI function for solving this bug. But maybe it can be added i.o. to reduce the getBestAttack calls and make some CanAttack calls from that, but that is something for another patch as that would be rather cleanup then bugfix.

comment:9 by elexis, 7 years ago

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:10 by fatherbushido, 7 years ago

Keywords: review added; rfc removed

Code looks ok. It works like expected. (Adding Capturable to walls, and setting StoneWall in a the restricted class of a capture attack of an unit). I didn't notice anything broken. -> push in review queue.

by fatherbushido, 7 years ago

Attachment: 4220.diff added

bb's patch as a diff

by fatherbushido, 7 years ago

Attachment: 4220_testcase.diff added

an example of use wich was broken without the patch

comment:11 by fatherbushido, 7 years ago

Keywords: rfc added; review removed

I put it out of the review queue until closing #3484

comment:12 by fatherbushido, 7 years ago

Well some comments:

  • I would use ! instead of no
  • Take care that the GuiInterface split of the CanCapture and CanAttack stuff had some reason (so with that merging L234 of Attack is wrong). -> EDIT: It seems I read too fast
  • The fix of the remaining bug in #3484 is more or less linked to this one. I don't want to do stuff wich will conflict with what you have in mind. So I guess that you should provide the UnitAI modification with that patch too (you can use type or still do the stuff by passing the allowCapture bool).
  • about namming, perhaps types is enough clear
  • at last adding a quick test of CanAttack is something wich will help (testing some cases, empty wantedTypes, one type, two types, a ! one)

edit: I remove the now useless rfc word. You can upload the next one to phabricator.

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

comment:13 by fatherbushido, 7 years ago

Keywords: rfc removed

comment:14 by bb, 7 years ago

Updated patch at D122

comment:15 by fatherbushido, 7 years ago

Owner: set to fatherbushido
Resolution: fixed
Status: newclosed

In 19528:

Add a type argument for CanAttack method of Attack component. Patch by bb. Fixes #4220.
Differential Revision: https://code.wildfiregames.com/D122

comment:16 by fatherbushido, 7 years ago

Milestone: Work In ProgressAlpha 22
Note: See TracTickets for help on using tickets.