Page MenuHomeWildfire Games

Allow non Soldier Heroes
AbandonedPublic

Authored by Grugnas on Aug 20 2017, 1:23 AM.

Details

Reviewers
elexis
Stan
Summary

moved the entry VisibleClasses Soldier from template_unit_hero.xml to the inheriting templates which are actually Soldier in order to allow an eventual non Soldier Hero.

Test Plan

please play a game to test.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Grugnas created this revision.Aug 20 2017, 1:23 AM
elexis edited edge metadata.Aug 20 2017, 12:09 PM

Considering that we already had multiple issues with the hero healer requiring to copy things over and then forgetting half of them, this approach sounds like it should be welcomed.

I rather have some additional disabled="" for the hero healer than copies.

Stan edited edge metadata.Aug 20 2017, 12:35 PM

Can't we use something like

<VisibleClasses datatype="tokens">Infantry -Soldier</VisibleClasses>

I don't know if the code supports removing classes like it does for units.

fatherbushido added a subscriber: fatherbushido.EditedAug 20 2017, 12:49 PM
In D806#32013, @Stan wrote:

Can't we use something like

<VisibleClasses datatype="tokens">Infantry -Soldier</VisibleClasses>

I don't know if the code supports removing classes like it does for units.

datatype="tokens" is the answer to your question.
(And that's indeed the thing elexis wants)
(edit: and even if I am more for adding things than for removing things, it seems the proper solution)
(edit2: still don't know why that template is not fixed 6 months later :p)

Perhaps also template_unit_hero, template_unit_hero_infanty, template_unit_hero_cavalry, template_unit_hero_healer (Yes, cunobelin is a fun case).

Correct that it's better to add things than to remove it. The important thing to me is to avoid the duplication issues (being required to copy parts over means someone will and did forget to address the same change in all copies).

@fatherbushido, do you want to take care of this?

In D806#32024, @elexis wrote:

@fatherbushido, do you want to take care of this?

I would recommend Grugnas to print on paper (sorry for the trees)
https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/templates/template_unit.xml
https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/templates/units/maur_hero_chanakya.xml
https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_support_healer.xml
https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/templates/template_unit_hero.xml and inherited ones.
Then take a pen and do the stuff.

Retitling the diff too (as non heroes soldier are already allowed). (It can be merge with the other diff imo -> Make the Mauryan healer hero inherit from generic hero templates...)

template_unit_hero_healer is cleaner but perhaps useless while we only have 1 (or 2).

Then we could just commit that. (refs d225)

i didn't get joke?

Retitling the diff too (as non heroes soldier are already allowed). (It can be merge with the other diff imo -> Make the Mauryan healer hero inherit from generic hero templates...)

Since the template_unit_hero has the visible class Soldier, it made me think that Heroes are soldiers by default and if a modder wants to create a non hero soldier he should explicitly remove the Soldier class which is in my opinion inelegant, unnecessary (it is the same thing of adding the Soldier Class) and perhaps not immediate.

template_unit_hero_healer is cleaner but perhaps useless while we only have 1 (or 2).

i agree with this (cunobelin isn't a healer at all).

I could rename this diff "clean Heroes sounds inheritance" creating a dipendency with D808 for the elephant hero and simply modify the chanakya diff D747 in order to remove the Soldier class from its template. would that be ok?

elexis added a comment.EditedAug 20 2017, 2:51 PM

I dont see Soldier used anywhere in the code besides for the AI (and that part of the code seems irrelevant).

So the 0AD-brand Soldier can be considered equal to the common soldier and a soldier is a non-unique unit.
The current heroes don't have the Soldier tag, so why would we add one.

Edit: Didn't search JSON files, but Soldier is still what we casually understand as a soldier, so still don't see the point of adding the tag.

In D806#32054, @elexis wrote:

I dont see Soldier used anywhere in the code besides for the AI (and that part of the code seems irrelevant).

So the 0AD-brand Soldier can be considered equal to the common soldier and a soldier is a non-unique unit.
The current heroes don't have the Soldier tag, so why would we add one.

Edit: Didn't search JSON files, but Soldier is still what we casually understand as a soldier, so still don't see the point of adding the tag.

I don't understand. They have currently I think (Soldier, not CitizenSoldier) (btw it's more or less (Infantry || Cavalry) or something like that). It's indeed also often used in techs and auras iirc.

In D806#32044, @Grugnas wrote:

I would recommend Grugnas to print on paper (sorry for the trees)
[...]
Then take a pen and do the stuff.

i didn't get joke?

It's not a joke, an advice (but perhaps too much old school) -> refs d225

elexis added a comment.EditedAug 20 2017, 3:22 PM

The current heroes don't have the Soldier tag, so why would we add one.

They have currently

You are right. I ran a grep on template_unit_hero_* instead of template_unit_hero*.

(Edit: I just answered because Grugnas sent me a PM asking whether the Soldier tag should be added anywhere. I should either completely review it or shut up)

@Grugnas: perhaps close that one and finish the sound cleaning in D747

Grugnas abandoned this revision.Sep 16 2017, 8:30 PM

abandoned because of different design decision.