Page MenuHomeWildfire Games

[gameplay] Remove <PreferredClasses> from axeman/swordman/maceman.
ClosedPublic

Authored by borg- on Jul 2 2020, 5:19 AM.

Details

Summary

Like D2786.

If your units are in combat and any siege unit appears in your vision range, then all of your units will attack siege units avoiding all other units, losing a lot of effectiveness (units Will try capture no kill) and can be killed on the way.
These units simply have "<PreferredClasses>" for being supposedly good against siege units.
Following this logic, Spear cav should have <PreferredClasses>Cavalry</PreferredClasses>, spearman should have "cavalry", sword cavalry should have "archer".
Well, that doesn't seem to make sense to me. Let the player make his choices.

Test Plan

Check for mistakes.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

borg- created this revision.Jul 2 2020, 5:19 AM
borg- edited the summary of this revision. (Show Details)Jul 2 2020, 12:49 PM
Nescio requested changes to this revision.EditedJul 2 2020, 1:46 PM

The idea is nice, the implementation is wrong.
template_unit_cavalry_melee.xml and template_unit_infantry_melee.xml set a first preference to Human, which is inherited by all their children, i.e. this patch removes the second preference (Siege), but keeps the first.
What I would recommend is:

  • remove <PreferredClasses> from *_melee.xml;
  • set <PreferredClasses> to Human or Organic in *_pikeman.xml and *_spearman.xml;
  • set <PreferredClasses> to Unit or Unit+!Ship in *_axeman.xml and *_swordsman.xml;
  • remove <PreferredClasses> from *_maceman.xml.

See also D1354.

[EDIT] While at it:

  • remove <PreferredClasses> from *_ranged.xml;
  • set <PreferredClasses> to Human in *_archer.xml, *_javelinist.xml, *_slinger.xml.
This revision now requires changes to proceed.Jul 2 2020, 1:46 PM
borg- added a comment.EditedJul 3 2020, 3:56 AM

The idea is nice, the implementation is wrong.
template_unit_cavalry_melee.xml and template_unit_infantry_melee.xml set a first preference to Human, which is inherited by all their children, i.e. this patch removes the second preference (Siege), but keeps the first.
What I would recommend is:

  • remove <PreferredClasses> from *_melee.xml;
  • set <PreferredClasses> to Human or Organic in *_pikeman.xml and *_spearman.xml;
  • set <PreferredClasses> to Unit or Unit+!Ship in *_axeman.xml and *_swordsman.xml;
  • remove <PreferredClasses> from *_maceman.xml.

See also D1354.

[EDIT] While at it:

  • remove <PreferredClasses> from *_ranged.xml;
  • set <PreferredClasses> to Human in *_archer.xml, *_javelinist.xml, *_slinger.xml.

Better set human for everyone?
Champion_cavalry_xml
Champion_Infantry.xml
Cavalry_melee.xml
Cavalry_ranged.xml
Infantry_ranged.xml
Infantry_melee.xml
Same to hero.

Better set human for everyone?

If that's what you want, fine, but it has to be set in the individual templates, for consistency, so the above still applies.

Nescio updated this revision to Diff 12568.Jul 5 2020, 10:39 PM
Nescio edited the summary of this revision. (Show Details)
  • correct and complete changes
Nescio accepted this revision.Jul 5 2020, 10:41 PM
This revision is now accepted and ready to land.Jul 5 2020, 10:41 PM
borg- added a comment.EditedJul 5 2020, 11:18 PM

@Nescio tnx update!!!

Set "unit" can be a problem chasing ships, no?

Set "unit" can be a problem chasing ships, no?

Given that entities look for the nearest entity, and ships have a much larger footprint, i.e. their centre is farther away, I don't expect it'll be much of a problem. But you can replace Unit with Unit+!Ship, if you like.

borg- updated this revision to Diff 12575.Jul 7 2020, 2:07 AM

Up patch

Unit --> Unit+!Ship

Nescio added inline comments.Jul 7 2020, 10:59 AM
template_unit_cavalry_melee.xml
17 ↗(On Diff #12575)

Too much indentation.

template_unit_cavalry_ranged_archer.xml
14 ↗(On Diff #12575)

What changed here?

borg- updated this revision to Diff 12581.Jul 7 2020, 11:53 PM

Correct idendation.

Nescio accepted this revision.Jul 8 2020, 9:43 PM

The patch is correct and complete, the changes make sense, the reason behind it is valid, and it improves consistency and maintainability.

Nescio edited the summary of this revision. (Show Details)Jul 8 2020, 9:44 PM

I don't really understand the reason for the split between Human and Unit+!Ship ?

I'm not sure this improves maintainability a lot if all base templates now have custom preferredClasses.

I'm not sure this improves maintainability a lot if all base templates now have custom preferredClasses.

<PreferredClasses> is an ordered list. If a template wants to have a different preference than its parent, it first has to remove the classes it inherited, which is more work and easy to overlook (see first iteration of this patch). Since different unit types have different attacks, it makes sense to define the preferences in the same file where its damage and range are defined, not in the shared template.

I don't really understand the reason for the split between Human and Unit+!Ship ?

In practice all units have one of the following classes: Animal, Human, Ship, Siege. The first two also are Organic, the latter two are not. Unit+!Ship effectively means human or siege, whichever is closest, whereas Human Siege means first all humans within sight and only afterwards siege engines. Hack damage is quite effective vs siege engines, but pierce damage is not, hence the need for different soldier types to have different preferred classes.

borg- added a comment.Jul 13 2020, 5:12 AM

I don't really understand the reason for the split between Human and Unit+!Ship ?

Complementing what @Nescio said, "Ship" are part of "Unit". We want the melee cavalry give the same preference to soldiers and siege, but we don't want them to chase ships and possibly die without causing any damage.

wraitii accepted this revision.EditedJul 14 2020, 9:59 AM

If a template wants to have a different preference than its parent, it first has to remove the classes it inherited.

Thanks for the explanation, I missed that. You could use the "replace" attribute I believe though. Still, doesn't make too much sense so long as we don't have mixins...

I don't really understand the reason for the split between Human and Unit+!Ship ?

...

Alright, makes sense.


Accepting this on the grounds that preferring Siege is a weird default and it makes sense to leave some of this to macro.

This revision was automatically updated to reflect the committed changes.