Page MenuHomeWildfire Games

let technologies modify loot
ClosedPublic

Authored by Grugnas on May 3 2017, 11:11 AM.

Details

Summary

this patch allow loot modifications.
The modified value is rounded before return in order to assert an integer value.
AI correctly take care of the modified value.

Test Plan

make a test to check if this doesn't break things.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1404
Build 2211: Vulcan BuildJenkins
Build 2210: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Owners added a subscriber: Restricted Owners Package.May 3 2017, 11:11 AM
elexis added a subscriber: elexis.May 3 2017, 11:21 AM
elexis added inline comments.
binaries/data/mods/public/simulation/components/Loot.js
23

That sounds like a good idea (need to check if it breaks something)

binaries/data/mods/public/simulation/data/technologies/advanced_unit_bonus.json
6–26

(For this reason we better don''t align after the indentation to begin with)

25

sounds OP, maybe 1?

Grugnas planned changes to this revision.May 3 2017, 11:47 AM

unexpected bugs fix.

binaries/data/mods/public/simulation/data/technologies/advanced_unit_bonus.json
25

it isn't op because units stats at rank 3 are very similar to a Champion stats.

Vulcan added a subscriber: Vulcan.May 3 2017, 11:57 AM

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/967/ for more details.

Grugnas edited the test plan for this revision. (Show Details)May 3 2017, 12:30 PM
Grugnas updated this revision to Diff 1603.EditedMay 3 2017, 12:34 PM

properly patch upload.

Grugnas updated this revision to Diff 1604.May 3 2017, 12:39 PM

indentation align

Vulcan added a comment.May 3 2017, 1:20 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/968/ for more details.

fatherbushido requested changes to this revision.May 3 2017, 2:00 PM
fatherbushido added a subscriber: fatherbushido.

I would not be for that but why not. (Promotion starts only to be use in game, it means: taking care of its units, healing them, selecting them with triple click, sort things by health,... it's really nice that they are improved. Imo the enemy doesn't really did the choice to kill or not to kill a promoted unit and so has not to be rewarded for that...).

Else: excepting what you will see on a tooltip, the important thing is what you do when you change one line of a code. For example, having repeated many times to change something in a build restriction patch applies here too. For what remains, inlined comments.

binaries/data/mods/public/simulation/components/Loot.js
23

As elexis said.
Just quickly: why removing "||0", what happens if I multiply a res by 0.7?, does the AI use that or not?, what to do with the structure tree for an autopromoted units? What's the diff between ToEntity and ToTemplate? Isn't there too a Template.js file existing somewhere?

This revision now requires changes to proceed.May 3 2017, 2:00 PM
Vulcan added a comment.May 3 2017, 2:07 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/969/ for more details.

I would not be for that but why not. (Promotion starts only to be use in game, it means: taking care of its units, healing them, selecting them with triple click, sort things by health,... it's really nice that they are improved. Imo the enemy doesn't really did the choice to kill or not to kill a promoted unit and so has not to be rewarded for that...).

Else: excepting what you will see on a tooltip, the important thing is what you do when you change one line of a code. For example, having repeated many times to change something in a build restriction patch applies here too. For what remains, inlined comments.

binaries/data/mods/public/simulation/components/Loot.js
23

The Structure Tree display promoted units if they replace the considered templates (f.e. have a look at D407 where mercenaries have their basic unit template autopromoted and displayed as promoted in the structure tree).
ToEntity sends informations to the single entity rather than the template that could have many entities. In this case, i guess that using ToTemplate would have summed values whenever an unit get promoted.
I must admit that removing the || 0 was a forgetfulness, by the way a workaround could be to truncate the result in order to respect the integer value.

Grugnas updated this revision to Diff 1724.May 7 2017, 6:28 PM
Grugnas edited edge metadata.

added the check for the resources value argument of the function.

Vulcan added a comment.May 8 2017, 4:05 AM

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/1061/ for more details.

fatherbushido requested changes to this revision.May 14 2017, 6:01 PM
fatherbushido added inline comments.
binaries/data/mods/public/simulation/components/Loot.js
23

Moreover that part is itself problematic enough to be splitted.

This revision now requires changes to proceed.May 14 2017, 6:01 PM
Grugnas updated this revision to Diff 2015.May 18 2017, 7:56 PM
Grugnas edited edge metadata.

fixed bug that showed a warning whenever a gaia huntable animal is targetted.

Grugnas retitled this revision from [Proposal] Loot increased when units get promoted to Loot increased when units get promoted.May 18 2017, 7:57 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/1246/ for more details.

fatherbushido requested changes to this revision.May 25 2017, 9:23 AM

(Tag as request change as the modification code part is incomplete)

This revision now requires changes to proceed.May 25 2017, 9:23 AM
Grugnas planned changes to this revision.May 26 2017, 9:36 AM
fatherbushido resigned from this revision.Aug 2 2017, 9:54 PM

(Tag as request change as the modification code part is incomplete)

after some tests the patch seems to work in game (didn't try ai yet) but the structure tree doesn't receive the modified loot value

fatherbushido added a comment.EditedAug 21 2017, 4:40 PM
In D408#32303, @Grugnas wrote:

after some tests the patch seems to work in game (didn't try ai yet) but the structure tree doesn't receive the modified loot value

(Following pm request)
You had the infos in the previous inlined comment.
Most of the gui thing (let's say static gui things, so struct tree, construction or training panel) will grab template data with Template.js. By default the modification value is the same as the template one. And you choosed an exotic one.
See new inline comment for details.

binaries/data/mods/public/simulation/components/Loot.js
24

"Loot/Resource/"+res -> "Loot/"+res
(or add the optional second param in Template.js -> not the good solution.)
And that's the place to round things. (same in Template.js). (As those are integers, see schema).

Grugnas updated this revision to Diff 3275.Aug 22 2017, 12:12 AM

perhaps this isn't the cleaner solution.

Oh you should not have read (or I should not have writen) what was between parens.
The default modification key (the value name to modify) is "Loot/"+ type in the Templates function (most logical choice as it's the entries of the xml schema). (You could also use "SpecialGrugnas/" + type but it sounds a weird choice).
So use that one in the sim too.
See inlined comments.

And then you have to use that key in the modification array of you json technology files.

binaries/data/mods/public/simulation/components/Loot.js
24
- ret[res] = ApplyValueModificationsToEntity("Loot/Resource/"+res, +(this.template[res] || 0), this.entity);
+ ret[res] = ApplyValueModificationsToEntity("Loot/"+res, +(this.template[res] || 0), this.entity);
binaries/data/mods/public/simulation/data/technologies/elite_unit_bonus.json
22

"Loot/Resource/food" -> "Loot/food"
And so on

mimo added a subscriber: mimo.Aug 22 2017, 10:19 AM

So a Cavalry swordman for example would have +5 wood and +5 metal for each promotion! that's really a lot, compared to the price of such unit.
I would rather have put something like +10 xp + 2 metal.

As mimo said.

And you should take care of
spart_champion_infantry_sword (already a loot modification)
and of what you want for things like mace/champion_infantry_e.xml mace/champion_infantry_barracks_e.xml
etc...

In D408#32552, @mimo wrote:

So a Cavalry swordman for example would have +5 wood and +5 metal for each promotion! that's really a lot, compared to the price of such unit.
I would rather have put something like +10 xp + 2 metal.

thank you for the interest!
Perhaps the proposed loot values are a bit high but the logic followed is to compare rank 3 units and champions stats which are very similar:
Proposed Skiritai:
191 hp in phase 3


Champion Swordsman:
200 hp

I'd rather increase food, wood up to 20 and xp loot of champions in order to let player have an alternative resources income instead of the most popular trade which, in my opinion provides too many resources.
About xp: as far as i know those xp points are the total experience that an unit can obtain by killing another unit. This means that ranged units with 50 hp double thet xp gained per hit (100 xp for 50 hp) and champions and promoted units yield less xp than expected (100 xp for rank 3 units 191 hp and 150 xp for champions with 200 hp) thus, if my considerations are right, perhaps experience yield might always be equal to the unit max health points?

Grugnas updated this revision to Diff 3282.Aug 22 2017, 1:07 PM

this patch adds xp to the promote technologies with a multiplier of 1.2 on the previous xp amount.

(Gotta be careful with the balancing, the rich get richer, the poor get poorer. But +5 doesn't look that much, so might be ok.)

binaries/data/mods/public/simulation/data/technologies/elite_unit_bonus.json
28

(Can we use an array for affects to remove the duplication? Also Are Infantry Slingers affected twice this way? Perhaps it could be made agnostic of clases by multiplying)

Grugnas updated this revision to Diff 3283.Aug 22 2017, 1:16 PM

the xp is now correctly modified in the simulation

mimo added a comment.Aug 22 2017, 7:22 PM
In D408#32567, @Grugnas wrote:
In D408#32552, @mimo wrote:

So a Cavalry swordman for example would have +5 wood and +5 metal for each promotion! that's really a lot, compared to the price of such unit.
I would rather have put something like +10 xp + 2 metal.

thank you for the interest!
Perhaps the proposed loot values are a bit high but the logic followed is to compare rank 3 units and champions stats which are very similar:
Proposed Skiritai:
191 hp in phase 3


Champion Swordsman:
200 hp

I'd rather increase food, wood up to 20 and xp loot of champions in order to let player have an alternative resources income instead of the most popular trade which, in my opinion provides too many resources.
About xp: as far as i know those xp points are the total experience that an unit can obtain by killing another unit. This means that ranged units with 50 hp double thet xp gained per hit (100 xp for 50 hp) and champions and promoted units yield less xp than expected (100 xp for rank 3 units 191 hp and 150 xp for champions with 200 hp) thus, if my considerations are right, perhaps experience yield might always be equal to the unit max health points?

I do appreciate, when commenting that something is too big, that your first reaction is to still increase it!
Also I don't understand that patch and the arguments: wanting to replace trade by loot seems to be a complete nonsense.
Loot is something which should only reward the success in fighting, not become another source of resources. And champions are professionnal soldiers, with better equipment than simple citizen, so we may understand that they have higher loot value. For me, it is good that there are differences between an elite soldier and a champion, that leave more choice to the player.
For me, the changes on the loot are too big, and i would not agree to include it as is.

Grugnas added a comment.EditedAug 22 2017, 9:03 PM
In D408#32597, @mimo wrote:

I do appreciate, when commenting that something is too big, that your first reaction is to still increase it!

I could use better screenshots since they want to compare 2 different units and not an ulteriorly increase of the patch effect.
Skiritai is a swordsman and, with this patch, it would have a loot of 15 metal (any other unit has 10 metal loot at rank 3).
Any champion has 20 metal loot.

Also I don't understand that patch and the arguments: wanting to replace trade by loot seems to be a complete nonsense.

Maybe i could have given a more clear explanation. The intent isn't to replace the trade at all (i am sorry if you misunderstood, that would take a bit to explain). I won't discuss about the balance and how the features affect the game in the comments.

For me, it is good that there are differences between an elite soldier and a champion, that leave more choice to the player.

I agree on that one, that's why I raised the question if the champions food and wood loot could be increased by 5 ( a citizen soldier cavalry unit and an infantry champion unit give both +10 food as loot).

For me, the changes on the loot are too big, and i would not agree to include it as is.

I am sorry to have different opinions for 5 more resources per promotion. Having +2 from Basic -> Advanced and +3 from Advanced to Elite for a total of +5 resource could be ok aswell.

The last patch upload only adds more experience per promotion which raises the concern about the citizen soldiers giving as loot almost the same amount of experience as the one gained by a champion infantry unit.
I'd like to let you also notice that ranged units (50 hp in phase 1) give the same amount of experience given by a melee unit (with 100 hp in phase 1), thus I am waiting the reviewer @fatherbushido for opinions and eventually instructions.

Grugnas edited the test plan for this revision. (Show Details)Aug 22 2017, 9:07 PM
mimo added a comment.EditedAug 22 2017, 11:46 PM
In D408#32614, @Grugnas wrote:
In D408#32597, @mimo wrote:

I do appreciate, when commenting that something is too big, that your first reaction is to still increase it!

I could use better screenshots since they want to compare 2 different units and not an ulteriorly increase of the patch effect.
Skiritai is a swordsman and, with this patch, it would have a loot of 15 metal (any other unit has 10 metal loot at rank 3).
Any champion has 20 metal loot.

But skiritai are special, take a basic unit, let's see what an iber swordman would give:
cost = 50 f 40 w 0 s 10 m
loot = 5 0 0 5 100 xp

+5     +5              +5           +20        with your patch when advanced
+5     +5              +5           +20        with your patch when elite

that's too much compared to its price. And my counter-proposition was

+0     +0             +2            +10  for advanced
+0     +0             +2            +10  for elite

Of course the +2 can be +3, or adding a bit of food or wathever (i don't really care), but not this excessive loot.

Also I don't understand that patch and the arguments: wanting to replace trade by loot seems to be a complete nonsense.

Maybe i could have given a more clear explanation. The intent isn't to replace the trade at all (i am sorry if you misunderstood, that would take a bit to explain). I won't discuss about the balance and how the features affect the game in the comments.

of course i've not misunderstood that, but quoting yourself "in order to let player have an alternative resources income instead of the most popular trade" and this i can't agree. Loot won't be an alternative to gathering or trade.

For me, it is good that there are differences between an elite soldier and a champion, that leave more choice to the player.

I agree on that one, that's why I raised the question if the champions food and wood loot could be increased by 5 ( a citizen soldier cavalry unit and an infantry champion unit give both +10 food as loot).

For me, the changes on the loot are too big, and i would not agree to include it as is.

I am sorry to have different opinions for 5 more resources per promotion. Having +2 from Basic -> Advanced and +3 from Advanced to Elite for a total of +5 resource could be ok aswell.

The last patch upload only adds more experience per promotion which raises the concern about the citizen soldiers giving as loot almost the same amount of experience as the one gained by a champion infantry unit.
I'd like to let you also notice that ranged units (50 hp in phase 1) give the same amount of experience given by a melee unit (with 100 hp in phase 1), thus I am waiting the reviewer @fatherbushido for opinions and eventually instructions.

Is it true that the last patch only adds more experience per promotion? Looking at the patch, it also adds food for all and wood/stone/metal depending on the unit.

In D408#32616, @mimo wrote:

that's too much compared to its price.

the comparison I made was about their performance in battle if compared to champions and their loot. I didn't consider their cost at all.

Of course the +2 can be +3, or adding a bit of food or wathever (i don't really care), but not this excessive loot.

the intent wasn't to simply increase the already existent loot (why shouldn't also a swordsman give wood?)
i will try to rethink that if it is really too high.

of course i've not misunderstood that, but quoting yourself "in order to let player have an alternative resources income instead of the most popular trade" and this i can't agree. Loot won't be an alternative to gathering or trade.

What i meant is that by having a bit more cospicue loot won't make attacking players (expecially with infantry) not valuable in terms of economy lose while trading is the best way to gain resources which incentive players to have a really too defensive attitude.

Is it true that the last patch only adds more experience per promotion? Looking at the patch, it also adds food for all and wood/stone/metal depending on the unit.

maybe we misunderstood again. It does if compared to previously uploaded one.

In D408#32614, @Grugnas wrote:

The last patch upload only adds more experience per promotion which raises the concern about the citizen soldiers giving as loot almost the same amount of experience as the one gained by a champion infantry unit.
I'd like to let you also notice that ranged units (50 hp in phase 1) give the same amount of experience given by a melee unit (with 100 hp in phase 1), thus I am waiting the reviewer @fatherbushido for opinions and eventually instructions.

As I said, I am not really convinced by the thing, so you'd need to convince somebody else.
The only thing I can do is to help you for the modification part.

Nescio added a subscriber: Nescio.Oct 28 2017, 12:39 PM

This patch would be more useful if it would only support modifying loot via technologies (instead of actually using it to increase the loot of promoted units, which might be controversial).
Because this is something I was looking for, I've incorporated your code into my mod (0abc), and it seems to work:

  • 0+1=1
  • 5*2=10

However, less suitable is:

  • 1+0.5=1.5
  • 5*1.5=7.5

Cost and loot resources are supposed to be integers (I believe), therefore you might want to add a function to round off values (floor, ceiling, or whatever).

In D408#38502, @Nescio wrote:

This patch would be more useful if it would only support modifying loot via technologies (instead of actually using it to increase the loot of promoted units, which might be controversial).
Because this is something I was looking for, I've incorporated your code into my mod (0abc), and it seems to work:

  • 0+1=1
  • 5*2=10

However, less suitable is:

  • 1+0.5=1.5
  • 5*1.5=7.5

Cost and loot resources are supposed to be integers (I believe), therefore you might want to add a function to round off values (floor, ceiling, or whatever).

You are totally right. I disregarded the modify support only in order to achieve a feature improvement.
About rounding... there is a ticket somewhere about rounding code thus probably it shouldn't be done in this diff.

If you really want it included, split the templates part and the modifications part.
For roundings, you have to add the round just before calling the ApplyMod function (and also in Template.js).
You have to check that it doesn't break AI (that AI does the same thing) iirc.
At first glance, I would choose floor.

Grugnas updated this revision to Diff 4020.Oct 29 2017, 5:34 PM
Grugnas retitled this revision from Loot increased when units get promoted to let technologies modify loot.
Grugnas edited the summary of this revision. (Show Details)
Grugnas edited the test plan for this revision. (Show Details)
bb added a subscriber: bb.Nov 12 2017, 2:09 PM

Ai doesn't work, or at least warning the value at some different places after modification doesn't return the new value (by placing warns at some places in the entity.js ai component). Didn't test if this is the case for other values then loot too, or maybe I tested the wrong places: a "proof patch" for the ai would be nice (just something that warns the value in the AI code before and after a tech)

In D408#40402, @bb wrote:

Ai doesn't work, or at least warning the value at some different places after modification doesn't return the new value (by placing warns at some places in the entity.js ai component). Didn't test if this is the case for other values then loot too, or maybe I tested the wrong places: a "proof patch" for the ai would be nice (just something that warns the value in the AI code before and after a tech)

the original patch worked for AI, and this is one of the 2 parts in which the original patch was split into ( the other one is D995 ). I swear it properly works because i tested it with AI too ( tests with warnings were messy, so i simply ordered AI soldiers to attack an unit then check the loot ).

bb added a comment.EditedNov 12 2017, 6:37 PM

But that is not the same, the simstate can be correct (as the ai gets the resources), but the ai is not aware of the loot increase, which is bad. Currently not a problem since the ai doesn't use loot anywhere to determine its actions. But the AI should be aware of the change if in future the ai will care.
(The AI should have in the code the amount we can see in the tooltip)

is this screenshot ok?

bb accepted this revision.Nov 12 2017, 7:50 PM

False alarm on the ai, it actually works (my test code was getting the wrong value due to the ref above),

meh those style, can be done while comitting

binaries/data/mods/public/simulation/components/Loot.js
15

["xp"] => .xp

24

some whitespaces

This revision is now accepted and ready to land.Nov 12 2017, 7:50 PM
This revision was automatically updated to reflect the committed changes.