#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)
Change History (18)
comment:1 by , 8 years ago
comment:2 by , 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 , 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.
comment:4 by , 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)
follow-up: 8 comment:5 by , 7 years ago
Keywords: | patch rfc added |
---|---|
Milestone: | Backlog → Alpha 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:8 by , 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:10 by , 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 , 7 years ago
Attachment: | 4220_testcase.diff added |
---|
an example of use wich was broken without the patch
comment:11 by , 7 years ago
Keywords: | rfc added; review removed |
---|
I put it out of the review queue until closing #3484
comment:12 by , 7 years ago
Well some comments:
- I would use
!
instead ofno
- Take care that the
GuiInterface
split of theCanCapture
andCanAttack
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.
comment:13 by , 7 years ago
Keywords: | rfc removed |
---|
comment:16 by , 7 years ago
Milestone: | Work In Progress → Alpha 22 |
---|
refs #3340, #252