Page MenuHomeWildfire Games

Correct a couple of incorrect assumptions in the structree
ClosedPublic

Authored by s0600204 on Jan 30 2017, 7:41 PM.

Details

Summary

wgoyc's problems with the recently rewritten technology requirements code highlighted a couple of assumptions within the code of the structree, namely

  • Assuming that a derived list of required techs doesn't contain a pair tech
  • Assuming that just because a unit has a production component, that it actually possesses lists of units/technologies to train/research

This revision resolves these assumptions, and also adds an error to display in the case that a tech/unit/structure requires a technology that is not included in any of that civ's units' or structures' production lists.

Originally #4455
Also, see https://wildfiregames.com/forum/index.php?/topic/21573-ugh-okay-help/

Test Plan

Assuming that a derived list of required techs doesn't contain a pair tech
To test this, add pair tech to a technology's requirements object. Without the below changes, the structree will throw an error, complaining that it cannot find the tech. With the changes, the the techs should display properly..

Assuming that just because a unit has a production component, that it actually possesses lists of units/technologies to train/research
To test this, add an empty production component to a unit. Without the below changes, that unit will appear under "Unit Trainers" on the right of the structree, but show no production items. With the below changes, the unit should not appear there.

Adds an error to display in the case that a tech/unit/structure requires a technology that is not included in any of that civ's units' or structures' production lists.
To test this, make a tech, unit, or structure require a technology that the civ cannot ordinarily research. Without the changes, the structree will throw an error. With the changes, you get a nice warning.


Alternatively, just run Delenda Est. Without the below changes, errors galore. With the below changes, there should be no problems. (p.s. you will need to erase Delenda Est's copy of globalscripts/templates.js)

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

s0600204 created this revision.Jan 30 2017, 7:41 PM
Owners added a subscriber: Restricted Owners Package.Jan 30 2017, 7:41 PM
Vulcan added a subscriber: Vulcan.Jan 30 2017, 8:37 PM

Build is green

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

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

elexis added a subscriber: elexis.Feb 7 2017, 1:57 PM

Seems okay

binaries/data/mods/public/globalscripts/Templates.js
370 ↗(On Diff #402)

Why was it changed? x ? y : z seems nicer to read than x && y || z (and why wouldnt the other instance above not be changed as well)

binaries/data/mods/public/gui/structree/helper.js
84 ↗(On Diff #402)

Remove asterisk
Could we move this warning below the following statement and display the name of the requirement, so it's easier to debug?

98 ↗(On Diff #402)

startsWith, also underscore. Changed many occurances in the other structree diff, doesnt matter where they are changed

binaries/data/mods/public/gui/structree/structree.js
250 ↗(On Diff #402)

Splice out -> Replace * with?

s0600204 marked 3 inline comments as done.Feb 10 2017, 12:12 AM
s0600204 added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
370 ↗(On Diff #402)

Expanding on my comment in the trac ticket linked at the top of the page; any of the following

  • template.cost ? +template.cost[type] : 0
  • template.cost && +template.cost[type] || 0
  • +(template.cost ? template.cost[type] : 0)

results in warnings about template.cost[type] being undefined if the value of type does not exist as a key in template.cost. This happens if a technology template has not got a cost set for a given resource, such as when a technology file from "vanilla" 0ad is used in a mod which has added a new resource.

If we don't support this, we would instead require any mod that adds a new resource type to have to modify every technology template that 0ad possesses so as to add that new resource type to the cost of the technology.

Alternatively, template.cost && template.cost[type] ? +template.cost[type] : 0; would work, but this was apparently not acceptable to you in the original patch as posted on trac.

And before you ask, +(template.cost[type] || 0) results in an error complaining that template.cost sometimes doesn't exist.

And assuming you meant the line with researchTime - that was not changed because it is unnecessary to do so - we check that it exists before we try casting it to a number.

s0600204 updated this revision to Diff 496.Feb 10 2017, 12:13 AM

Small changes in response to elexis' inline comments.

Build is green

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

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

elexis accepted this revision.Feb 10 2017, 3:00 PM

Now you got me to install delenda est, indeed that fix is totally needed!

binaries/data/mods/public/gui/structree/helper.js
84 ↗(On Diff #402)

That change is good too, but I meant replacing that "something" in that sentence with the thing that requires the tech. I guess that's not easily possible and we'd have to change the function arguments.

Adding a newline though (http://trac.wildfiregames.com/wiki/Coding_Conventions recommend 80 characters per line, my tolerance ceases at 120 characters and 230 characters are way too much ;-) )

98 ↗(On Diff #402)

Still no underscore, so it might fail for something for techs whose names happen to start with phase or pair but are not phase nor pair technologies. Without the underscore it's nicer to read though and I agree that it's not really something to bother about.

This revision is now accepted and ready to land.Feb 10 2017, 3:00 PM
This revision was automatically updated to reflect the committed changes.