Page MenuHomeWildfire Games

Show entity upgrades in the Structure Tree
ClosedPublic

Authored by s0600204 on Jan 24 2017, 2:46 PM.

Details

Summary

r18467 added an Update component. This should be taken into consideration when displaying the Structure Tree.

Originally reported by @fatherbushido on trac, ticket #4079. Please see original ticket for previous discussion.

Test Plan
  1. Start 0ad without patch applied
  2. Go to the Structure Tree
    • Note the incorrect cost of gates in walls (Palisade, Stone, Rome's Siege Walls, etc)
    • Also note the absence of an upgrade displayed under the Village Phase Sentry Tower
  3. Close 0ad, apply patch and restart
  4. Go to the Structure Tree
    • Note the now correct cost of gates in walls
    • Also note that the upgrade from Sentry Tower to Defence Tower is now displayed

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
4079_upgradeEntity
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 538
Build 858: Vulcan BuildJenkins
Build 857: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jan 24 2017, 2:46 PM
Vulcan added a subscriber: Vulcan.Jan 24 2017, 3:30 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/240/ for more details.

fatherbushido added inline comments.Jan 24 2017, 5:52 PM
binaries/data/mods/public/globalscripts/Templates.js
321

perhaps you can use 'option' instead of upKey as in Upgrade code

328

then use something like choice = template.Upgrade[upKey] wich is used many times

331

are you sure that's right?
With a quick overlook,
I think the key (tech_type) don't use the upKey (I guess getEntityValue function doesn't fit here).

334

same

344

It seems constistent with GuiInterface stuff.

binaries/data/mods/public/gui/structree/load.js
52

you can use a for of as you only use upgrades[idx] and never idx itself
(it seems)

88

same here?

binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
5

Not sure if we should nuke that.
The patch needs that to work?

fatherbushido added inline comments.Jan 24 2017, 6:01 PM
binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
5

I mean, imo we could keep the own resources cost (even if it's not use).
If the patch needs it to be nuked, imo it's bad (we could add another way to check that).

elexis requested changes to this revision.Jan 24 2017, 7:40 PM
elexis added a subscriber: elexis.

Requesting duplication removal and some code style updates.

(Also does the gate upgrade cost 60 stone when a long wall piece costs 28 stone?)

binaries/data/mods/public/globalscripts/Templates.js
336

Slight difference to the GUIInterface for the Icon:
"icon": choice.Icon || undefined, the same can be used here making the code a bit shorter as well and more consistent (Icon being defined as undefined).
Same for requiredTechnology - that property always exists.

344

Good call! Considering that we already had a number of inconsistency cases, one might start thinking about writing a unit test that checks for such inconsistencies.

binaries/data/mods/public/gui/structree/draw.js
93

For those wondering why it's a break and not a continue: falseis only returned if more production icons than can be supported by the current GUI layout.

182–183

Unneeded pair of braces

binaries/data/mods/public/gui/structree/helper.js
55

Changes required: The two uses of this function look duplicate, can't you just pass a third argument structure/ unit and absorb the code?

61

As to is an object, it is passed by reference, the code modifies the properties of to. Hence it appears that the return is unneeded/misleading.

binaries/data/mods/public/gui/structree/load.js
43

Remove duplication here

79

Remove duplication here

This revision now requires changes to proceed.Jan 24 2017, 7:40 PM
bb requested changes to this revision.Jan 25 2017, 12:02 PM
bb added a subscriber: bb.
bb added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
326

Trailing comma

binaries/data/mods/public/gui/structree/load.js
49

I guess this first data can be inlined. (Second one can be too, but no strong opinion on it, regarding readablilty)

85

Inline the first data too.

binaries/data/mods/public/gui/structree/structree.js
244

Could become ternary

binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
8

This is ugly, better remove the Cost component from gates completely.

fatherbushido added inline comments.Jan 25 2017, 12:54 PM
binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
8

The entity itself can/must have his own Cost.
Moreover, the time at least is needed for the repair rate.

elexis added inline comments.Jan 25 2017, 1:20 PM
binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
8

Since that question was asked by me in the ticket as well, adding a XML comment might be appropriate.

fatherbushido added inline comments.Jan 25 2017, 1:45 PM
binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
8

I could live with such an xml comment, and I understand why you want to add it but again it seems imo pointless (perhaps also a matter of taste).
We must have in mind that the intrinsic cost of an entity is something different (can even be not correlated) to the upgrade costs.
An entity can also have a cost component even if in common games (rms map) that entity is not in any Builder / ProductionQueue list (see other/celt_longhouse.xml...)
Moreover here, we just have to not touch at it, so how would look like such a comment? "Here you want perhaps remove the cost entry but don't do it?".
;-)

elexis added inline comments.Jan 25 2017, 1:55 PM
binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
8

<!-- Inherit Cost, as the BuildTime determines the repair rate -> just to avoid further repetitions of the question. I could also live with not adding it though ;)

fatherbushido added inline comments.Jan 25 2017, 3:24 PM
binaries/data/mods/public/simulation/templates/template_structure_defense_wall_gate.xml
8

sure, I would emphasize that it's also bad to remove (or to have to remove) the not time ressource cost.

s0600204 added inline comments.Jan 25 2017, 11:15 PM
binaries/data/mods/public/globalscripts/Templates.js
331

Yes, I'm sure that's right. If it wasn't, then the cost value wouldn't appear correctly in the structure tree.

binaries/data/mods/public/gui/structree/structree.js
244

How?

let phase = g_ParsedData.phaseList.indexOf(GetPhaseOfTemplate(upgrade.data)) < structPhaseIdx) ? structInfo.phase : GetPhaseOfTemplate(upgrade.data);`

or

let phase = GetPhaseOfTemplate(upgrade.data);
phase = g_ParsedData.phaseList.indexOf(phase) < structPhaseIdx ? structInfo.phase : phase;
elexis added inline comments.Jan 25 2017, 11:18 PM
binaries/data/mods/public/gui/structree/structree.js
244

Second one, as we don't want to repeat executing the function

s0600204 updated this revision to Diff 346.Jan 25 2017, 11:27 PM
s0600204 edited edge metadata.

Modifications based on comments provided.

  • Follow example set by GUIInterface in new code added to globalscripts (returning a definite undefined where applicable)
  • Reduce replication of code in structree/load.js
  • Add explanatory xml comment
s0600204 marked 7 inline comments as done.Jan 25 2017, 11:35 PM
s0600204 added inline comments.
binaries/data/mods/public/gui/structree/structree.js
244

It was more, how would a ternary make it better in this case, as imho neither option is more readable than what exists.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/248/ for more details.

fatherbushido added inline comments.Jan 26 2017, 9:02 AM
binaries/data/mods/public/globalscripts/Templates.js
331

That's not a proof.
I mean if someone wants to use those ones in simulation.
Are you sure the modifications will be applied?
Could you check again (else I will :p)?

binaries/data/mods/public/gui/structree/structree.js
244

Indeed seems not really worth here (bb said "could" and not "must" btw)
(I guess it's more a matter of own style here).

elexis added inline comments.Jan 26 2017, 12:02 PM
binaries/data/mods/public/gui/structree/structree.js
244

Ah yes, sorry, things like a = b ? c : a should be avoided.

s0600204 added inline comments.Jan 26 2017, 2:01 PM
binaries/data/mods/public/globalscripts/Templates.js
331

Applying P6 temporarily on top of the changes listed thus far, causes GUIInterface (the only simulation component currently using globalscripts/Templates::GetTemplateDataHelper()) to print out the entity name and upgrade cost of the first upgrade in any template it comes across (And no template has more than one upgrade, yet.) It yields the following output (repeated over and over):

WARNING: other/palisades_rocks_gate : ({stone:0, wood:20, time:5})
WARNING: structures/{civ}_defense_tower : ({stone:100, wood:50, time:100})
WARNING: structures/{civ}_wall_gate : ({stone:60, time:10})
fatherbushido added inline comments.Jan 26 2017, 3:01 PM
binaries/data/mods/public/globalscripts/Templates.js
331

You don't get what I meant.
I meant getEntityValue doesn't seems to fit here (relating to modification key).
I will describe it after.

bb added inline comments.Feb 10 2017, 1:35 PM
binaries/data/mods/public/globalscripts/Templates.js
331

What fatherbushido is referring to (I guess) is the fact that the ingame structree does not take any researched technologies into account when drawing structree. But this is done with all values see f.e. the walk/run speeds. This might needs to be fixed or all values, but that lies outside the scope of this patch imo.

335

Just wondering: can't those || undefined be removed?

binaries/data/mods/public/gui/structree/draw.js
89–90

Looping over an array with 1 element? nuke?

binaries/data/mods/public/gui/structree/helper.js
63

Is this new upgrade.entity not used only once, so can be inlined?

bb requested changes to this revision.Feb 10 2017, 1:35 PM
This revision now requires changes to proceed.Feb 10 2017, 1:35 PM
s0600204 requested review of this revision.Feb 16 2017, 7:45 PM
s0600204 edited edge metadata.

Any further comments? (Before I upload a new patch that's only one line different to the last...)

binaries/data/mods/public/globalscripts/Templates.js
331

...still not following, sorry.

I'll wait for fatherbushido to elaborate/explain further...

335

Please read elexis' comment directly below.

binaries/data/mods/public/gui/structree/draw.js
89–90

Hmm... I'll get that in the next patch, honest.

binaries/data/mods/public/gui/structree/helper.js
63

So as to avoid making the line overly long, I'd rather not.

fatherbushido added inline comments.Feb 16 2017, 7:56 PM
binaries/data/mods/public/globalscripts/Templates.js
331

Just write a test file with TechnologyManager (just create your modification directly) and load your GetTemplateDataHelper with the player arg (don't forget to also load the ValueModification) helper.

s0600204 added inline comments.Feb 17 2017, 12:38 AM
binaries/data/mods/public/globalscripts/Templates.js
331

Huh. Weird. There is indeed a problem (or two), but it's not here - its within the code of the Upgrade component... (patch for that incoming)

Test code: P18

s0600204 planned changes to this revision.Feb 17 2017, 4:42 PM
s0600204 added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
331

Okay, I think I finally understand what you mean.

Essentially...

[...] the value/property name used for modifications is just a key in the modifications cache objects stored in manager, it doesn't need to match with any template entry

...this is what I was missing

The subfunction getEntityValue() walks the modification path/key passed to it to get the current value directly from the template.

ApplyValueModificationsToTemplate(), on the other hand, does not: instead it is passed a value by the caller that it takes as the "current" value, then looks for tech modifications that match the modification path/key that it can apply to this value. (It only gets handed the template object so it can compare Identity classes)

So, from the perspective of getEntityValue(), Upgrade/Tower/Cost/stone is correct (because it is a reachable location within the template), but Upgrade/Cost/stone is not. However, http://trac.wildfiregames.com/wiki/TechModifications says the latter, not the former, is the appropriate valid modification key.

*mutters darkly*

s0600204 updated this revision to Diff 543.Feb 17 2017, 5:01 PM
  • Nuke array with only one entry
  • Hopefully resolve problem with modification key(s)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/367/ for more details.

bb accepted this revision.Feb 22 2017, 6:00 PM

No more comments form my side, do notice that f.e. mace mace walls in structree have 3000 hp, and ingame 3001 (rounding issue), and when upgrading the new gate will have 3000/3001 hp (right after upgrade). Also structree shows 3000 hp. But I don't think these rounding issues, are in the scope of this diff.

This revision is now accepted and ready to land.Apr 3 2017, 9:24 PM
s0600204 planned changes to this revision.Apr 11 2017, 10:58 PM

With the committing of the changes from D154 (in rP19410) this revision no longer cleanly applies. I'll update with a rebased state soon.

s0600204 updated this revision to Diff 1223.Apr 12 2017, 9:51 PM
s0600204 added a subscriber: fatherbushido.

Rebased to take into account recent commits.

This revision is now accepted and ready to land.Apr 12 2017, 9:51 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/752/ for more details.

This revision was automatically updated to reflect the committed changes.