Page MenuHomeWildfire Games

Attack Cursor
ClosedPublic

Authored by bb on Feb 3 2017, 10:37 PM.

Details

Summary

Adding restricted classes to an enemy's attack component does not result in not being able to attack. This is caused by the CanAttack function, not taking a type option.

refs #3484, it seems that ticket needs this one before, if it comes the patches are too related and the need to be done at ones, that is fine with me too, but I don't see a real reason.

Test Plan

Test with some restricted classes.

Run tests.
Let some units attack, maybe warn out the wantedType arrays and see the correct behaviour.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1303
Build 2051: Vulcan BuildJenkins
Build 2050: arc lint + arc unit

Event Timeline

bb created this revision.Feb 3 2017, 10:37 PM
Vulcan added a subscriber: Vulcan.Feb 3 2017, 11:21 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/307/ for more details.

elexis updated the Trac tickets for this revision.Feb 5 2017, 6:17 AM
Itms added a reviewer: Itms.Feb 19 2017, 6:22 PM
bb updated this revision to Diff 997.Mar 29 2017, 6:31 PM
bb edited the test plan for this revision. (Show Details)

While working on #252 (which depends on this patch), it became useful to have a function to get the wantedAttackTypes given some the wantedTypes array. Adding this function (and used it in CanAttack). Also adding tests for the new function and IIRC testing for 1 more edgy case in canAttack.

Owners added a subscriber: Restricted Owners Package.Mar 29 2017, 6:31 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/635/ for more details.

bb updated this revision to Diff 1504.Apr 27 2017, 8:37 PM

Merge GetAttackTypes and GetWantedAttackTypes, fix indentation in unit_actions

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/899/ for more details.

fatherbushido added inline comments.Apr 29 2017, 2:45 PM
binaries/data/mods/public/simulation/components/Attack.js
195

Perhaps let types = Object.keys(this.template)

bb added inline comments.Apr 29 2017, 10:51 PM
binaries/data/mods/public/simulation/components/Attack.js
195

Nope since slaughter needs to be excluded anyway, and having a global for the non types seems worse than for the pro types.

fatherbushido added inline comments.Apr 30 2017, 8:52 AM
binaries/data/mods/public/simulation/components/Attack.js
195

Ah yes I didn't consider that. Yes the second thing would be ugly (I didn't even have that idea).
Should we use var g_AttackTypes or const ATTACK_TYPES?
Then while at it, you can use it in the other function (GetBestAttackAgainst iirc).
Sounds good

bb added inline comments.May 1 2017, 12:53 PM
binaries/data/mods/public/simulation/components/Attack.js
195

It should be g_AttackTypes anyway following the coding conventions, whether we use var or const idc, I choose var here because a mod might want to extend it, and probably from a triggerscript.
Correct about the GetBestAttackAgainst

bb updated this revision to Diff 1582.May 1 2017, 1:13 PM

use g_AttackTypes in GetBestAttackAgainst

Vulcan added a comment.May 1 2017, 4:19 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/954/ for more details.

fatherbushido edited edge metadata.May 2 2017, 7:25 PM

@bb: Thanks. I hope I will find enough time tommorow to finish the review and hopefully commit it. It seems coherent with some other devs requests.

fatherbushido added inline comments.May 3 2017, 7:21 PM
binaries/data/mods/public/simulation/components/GuiInterface.js
1839

You choose on purpose to return undefined?

bb added inline comments.May 4 2017, 2:57 PM
binaries/data/mods/public/simulation/components/GuiInterface.js
1839

Don't see anything better: when data.types is not existing, empty or undefined, all those should return cmpAttack.CanAttack(data.target, undefined). Putting an empty array here seems less self-explaining than undefined. (Also we don't return undefined the CanAttack function is called with undefined)

Ok. Sorry for being so long. I should commit it this evening (I said that since a week :p)

binaries/data/mods/public/simulation/components/GuiInterface.js
1839

could you forget my question :) I missed the ")" and so I didn't understand :)

fatherbushido added inline comments.May 4 2017, 8:04 PM
binaries/data/mods/public/simulation/components/Attack.js
248

types.indexOf("Capture") == -1

wantedtypes undefined
Not a 'sheep', not an ennemy, capturable but the attacker don't have the capture attack, have another attack with no restricted classes.
We should get false.
But if Capture is the first (index 0) in the types array, it seems we will continue and some type will return true.

It's annoying to not have catched that with the test.

(I write quickly so be tolerant :p)

fatherbushido added inline comments.May 4 2017, 8:10 PM
binaries/data/mods/public/simulation/components/Attack.js
248

(which would be mostly impossible to trigger as Capture is the last in the array currently and then...)

bb added a comment.May 5 2017, 2:01 PM

When changing these things, I started wondering why we don't add the Slaughter attack into the GetAttackTypes function. This would need some rewritement in the UnitAI, and gui, so UnitAI won't choose to use this attack and the attack won't be displayed in gui. So its probably out of scope, but worth a consideration.

binaries/data/mods/public/simulation/components/Attack.js
248

The check isn't only wrong, but also badly placed, and even more wrong than this comment states. It should be merged better with the loop below, and the whole "domestic" part should be split from it.
Also adding tests for non enemy attacks

bb updated this revision to Diff 1661.May 5 2017, 2:01 PM

fixes as above

In D122#17519, @bb wrote:

When changing these things, I started wondering why we don't add the Slaughter attack into the GetAttackTypes function. This would need some rewritement in the UnitAI, and gui, so UnitAI won't choose to use this attack and the attack won't be displayed in gui. So its probably out of scope, but worth a consideration.

I had that feeling too (only a feeling). As you pointed out that needs to be do with caution. Keeping it for a next step would perhaps help for the review :p? It's your opinion too?

bb added a comment.May 5 2017, 2:30 PM
In D122#17519, @bb wrote:

When changing these things, I started wondering why we don't add the Slaughter attack into the GetAttackTypes function. This would need some rewritement in the UnitAI, and gui, so UnitAI won't choose to use this attack and the attack won't be displayed in gui. So its probably out of scope, but worth a consideration.

I had that feeling too (only a feeling). As you pointed out that needs to be do with caution. Keeping it for a next step would perhaps help for the review :p? It's your opinion too?

Yup

Vulcan added a comment.May 5 2017, 3:32 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1017/ for more details.

fatherbushido requested changes to this revision.May 7 2017, 6:06 PM

It looks there is a remaining typo else it should be ok :)
I would have test GetAttackTypes in a more isolated way as it doesn't need all that huge construction but it's ok like that.

binaries/data/mods/public/simulation/components/Attack.js
201

Perhaps it could be simplified but we'll let that for the future.

239

typo I guess

249

note for myself: Is it possible to have a null Player here?

This revision now requires changes to proceed.May 7 2017, 6:06 PM
bb added inline comments.May 7 2017, 8:48 PM
binaries/data/mods/public/simulation/components/Attack.js
239

I don't see him, please point me at it ;)

fatherbushido added inline comments.May 7 2017, 8:55 PM
binaries/data/mods/public/simulation/components/Attack.js
239

ah no it's ok, sorry for the noise :-)
You handle thing like !domes!tic in fact :p

fatherbushido accepted this revision.May 7 2017, 9:25 PM
This revision is now accepted and ready to land.May 7 2017, 9:25 PM
This revision was automatically updated to reflect the committed changes.
temple added a subscriber: temple.Dec 2 2017, 6:01 PM
temple added inline comments.
ps/trunk/binaries/data/mods/public/simulation/components/Attack.js
193–202 ↗(On Diff #1734)

This is on the verge of being comprehensible. :)