[PATCH] Building attacks should be able to use restricted and preferred classes

[michael]

Make cmpBuildingAI adhere to cmpAttack restrictions and preferences.

In the template <Attack> element:

<RestrictedClasses datatype="tokens">Organic</RestrictedClasses>
<PreferredClasses datatype="tokens">Gates Structure</PreferredClasses>

Forum Discussion: http://www.wildfiregames.com/forum/index.php?showtopic=17394

[James Scott]

This is my stab at implementing preferred classes for Building AI. It includes a new WeightedList JavaScript helper class for the random selection of targets with a preference score.

The weights used for WeightedList are the values returned by GetPreference. This means that a preferred target is 2x as likely to be selected as an regular target. If the attacking building has two preferences the 1st will 3x likely, 2nd 2x and so on. We may want to apply some down-weighting here - I'd be interested in your thoughts.

I wanted to have an instance of WeightedClass replace BuildingAI#targetUnits so that preference weighting would only need to be fetched as targets moved in to range rather than in every call to FireArrows. However this meant that saved games broke. I can attach the extended proposed patch if you're interested.

Restricted classes was added by another dev on a separate ticket but seems to mostly be a reimplementation of Attack#CanAttack so I've substituted that here.

Patch can be viewed on Github at ​https://github.com/jammus/0ad/compare/0ad:430af4a008...2dde082

[sanderd17]

Looks like a very clean solution. I'd replace the randomIndex function with a getRandomElement function. So that saves a line, and I can't think how you'd use it otherwise.

Also, it's not usual to have underscores in front of object related variables in our code, but a dev more acquainted with the code should decide.

Why don't you use the splice function for the removeAt. It gets you the weight to remove, and deletes the right index from the array in one go.

Btw, thanks for fixing that CanAttack (I didn't remember I committed that, and I didn't remember there was an CanAttack function either).

But overall, a very good piece of code.

[James Scott]

Full diff including splice code review feedback

[James Scott]

Full diff including splice and underscore property names code review feedback

[James Scott]

Thanks sanderd17. Good call with the splice suggestion, it's much cleaner that way. I've also attached a version with the underscore property names removed.

I chose to use randomIndex rather than randomItem so that targets can be easily removed by index if they fail the visibility test (line 306). An alternative would be to add a removeItem(target) method. My concern here was that this would work fine for this case where elements are integers or strings as the filter callback, or findItem internal method, would do a simple equality check. However if more complicated objects were stored in WeightedList this would need some rethink. I'm happy to implement this if preferred.

Github for latest version: ​https://github.com/jammus/0ad/compare/0ad:430af4a008...0796168

[sanderd17]

You're right about the randomIndex. Best would be to leave it that way.

I think it's good enough to commit.

[sanderd17]

In 13823:

Add preferred classes to BuildingAI. Patch by jammus. Fixes #1985

[mimo]

there are some flaws in the BuildingAI code which prevent it to work as expected :

• line 292, when we have one PreferredClass, GetPreference returns 0, so the preferred class will have the same weight as the non-preferred ones. Futhermore, if we have several PreferredClasses, GetPreference will return a higher value for the less preferred one, and thus the less preferred one will have higer weight. These two problems would be solved by something like (a value of N=2 would look reasonnable to me)

preference = cmpAttack.GetPreference(target)

if (preference !== null)

weight = 1 + N/(1+preference)

else

weight = 1

• in templates/template_structure_defense_defense_tower.xml, template_structure_defense_wall_tower.xml and template_structure_military_fortress.xml, the Preferred class should be Organic, and not organic, to be taken into account.

• Then I think we need a semi-colon at the end of line 292, and it would be better to have variable declarations in separate statements (lines 291-292 and 297-298)

[James Scott]

Thanks for the spots. Patch incoming.

[James Scott]

Sorry for the delay. Additional fixes attached and available to view at ​https://github.com/jammus/0ad/compare/0ad:ec2c47c197fb8c1d35fe13dc54c246f79797351b...398eaf8595a8019b1dfe919a32da353be12f6e5f

[mimo]

Thanks, the patch looks fine now. But while we are at it, it's maybe the opportunity to discuss what should be the preferred targets ? I would change it to "Siege Organic" instead of "Organic".

[sanderd17]

In 14011:

fix some flaws in the preferred classes. Patch by Jammus, Fixes #1985