Page MenuHomeWildfire Games

Rome teambonus affects citizen soldiers only
ClosedPublic

Authored by Grugnas on Aug 31 2017, 9:16 PM.

Details

Summary

Rome team bonus in the current version affects also ally champion infantry for a -20% training time which is quite relevant in a team game, thus this patch limits the effect to ally citizen soldiers only.

Test Plan

Test checking infantry champions training time in the barracks before and after applying the patch.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Grugnas created this revision.Aug 31 2017, 9:16 PM
Grugnas updated this revision to Diff 3441.Aug 31 2017, 9:19 PM

updated also the aura description in order to avoid ambiguities.

bb added a subscriber: bb.Sep 22 2017, 9:32 PM

Besides the fact we need to agree on that we really want such a patch (seems ok to me but my ack is not enough). The patch should be correct also: aura.affects is a classlist and matched vs another with MatchesClassList, there it says where spaces are handled as OR and '+'-signs as AND thus the patch is now giving the bonus to all units with either CitizenSoldier or Infantry, instead units having both.

Is that a recent update? I recall thst different classes are defined by different strings thus space is interpreted as AND

In D859#36176, @Grugnas wrote:

Is that a recent update? I recall thst different classes are defined by different strings thus space is interpreted as AND

I could be sarcastic. I won't.

bb added a comment.Sep 22 2017, 10:36 PM

nope has been like that for a long time (MatchesClassList works like that from before rP18000), it is probably just how you define "AND" since now the aura is added to CitizenSoldier and Infantry, which means that a unit needs to be CitizenSoldier OR Infantry (or both), not that the unit need to have CitizenSoldier AND Infantry as class.

Grugnas added a comment.EditedSep 22 2017, 10:55 PM
In D859#36176, @Grugnas wrote:

Is that a recent update? I recall thst different classes are defined by different strings thus space is interpreted as AND

I could be sarcastic. I won't.

https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/data/auras/teambonuses/iber_player_teambonus.json

standing at this aura then the iberian bonus should affect CitizenSoldiers or Javelin thus it should be applied also to Champion Javelin ( like briton javelin skirmisher champion cavalry)? actually it doesn't.

or this?
https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/data/auras/teambonuses/spart_player_teambonus.json

bb added a comment.Sep 22 2017, 11:09 PM

hmmpf you are right, however that is a bug (maybe the behaviour is wanted be still) caused by he array around the string. Also related to a comment from leper at D869. There either shouldn't be that array or we should use the proper array structure defined in the comment from MatchesClassList in Templates.js

elexis added a subscriber: elexis.Dec 5 2017, 11:10 PM

Agree to nerf in the proposed approach.

So you're saying ["CitizenSoldier Infantry"] means Citizen and Infantry, but "CitizenSoldier Infantry" means Citizen or Infantry`?
Who would we be able to yell at?

binaries/data/mods/public/simulation/data/auras/teambonuses/rome_player_teambonus.json
9 ↗(On Diff #3441)

dash ok

bb added a comment.Dec 5 2017, 11:47 PM

No the code in this manner is not consistent with the rest of the codebase. I have been working on a patch fixing the stuff, but ran into the limitations of the MatchingClasses function (it doesn't allow braces). So whatever is done here doesn't matter as about everything is AND here.

In D859#44412, @elexis wrote:

So you're saying ["CitizenSoldier Infantry"] means Citizen and Infantry, but "CitizenSoldier Infantry" means Citizen or Infantry`?

according to: https://trac.wildfiregames.com/wiki/Aura_Templates

affects determines the classes it affects, listed as ["Class1 Class2", "Class3"], which means the target needs to have (Class1 AND Class2) OR Class3 to benefit from the aura.

the code syntax is right.

temple accepted this revision.Feb 19 2018, 9:05 PM
This revision is now accepted and ready to land.Feb 19 2018, 9:05 PM
This revision was automatically updated to reflect the committed changes.