[PATCH] Support notciv tech requirement in structure tree

[sanderd17]

As noted in this forum post (https://wildfiregames.com/forum/index.php?/topic/20760-bug-and-error/#comment-316628), apparently, notciv doesn't work great in the structure tree.

[Stan]

Fixed in r18219. For some reason it didn't update

[mimo]

No it was a typo, r18219 fixed #3992

[s0600204]

Proposed patch.

[s0600204]

I noticed while running tests to make sure notciv was being understood that more complex requirements were not being parsed properly. Admittedly 0AD doesn't use requirements as complex as those I was testing, but that's not to say that it never will, not to mention that modders might try them. I felt that as I was modifying the requirements calculation code anyhow, that I should attempt to fix it properly, rather than making modifications that I know will need to be rectified later.

Anyhow, in addition to support for the notciv requirement, with the attached patch the structree should now understand the following valid (but unused within 0AD core) tech requirement layouts:

{"all": [{"tech": "tech1"}, {"any": [{"civ": "civA"}, {"civ": "civB"}]}]}
{"all": [{"tech": "tech1"}, {"any": [{"civ": "civA"}, {"civ": "civB"}]}, {"notciv": "civC"}]}
{"any": [{"all": [{"civ": "civA"}, {"tech": "tech1"}]}, {"tech": "tech2"}]}
{"any": [{"all": [{"civ": "civA"}, {"tech": "tech1"}]}, {"all": [{"civ": "civB"}, {"tech": "tech2"}]}]}
{"any": [{"all": [{"civ": "civA"}, {"tech": "tech1"}]}, {"all": [{"any": [{"civ": "civB"}, {"civ": "civC"}]}, {"tech": "tech2"}]}]}

[s0600204]

With style changes as suggested by elexis

[s0600204]

Additional style change as suggested by elexis.

[s0600204]

Patch rebased.

[s0600204]

Suggestions from elexis from irc

[s0600204]

As requested in irc, moved to globalscripts.

[s0600204]

At the request of elexis, patch now also attempts to unify tech requirement calculation across gui, simulation and ai contexts to minimise duplicate code.

[fatherbushido]

(That patch will in the same move fix #4108 in the expected way.)

I will try to do a full review (not of the AI part).

[s0600204]

Clean up entity sorting

[fatherbushido]

Thanks for the patch and for your patience. I didn't make a complete review.

Some remarks :

• DeriveTechnologyRequirements

It would be nice to add JSDoc, but that's not a priority.

• InterpretTechRequirements

L126: it would be nice to describe the sorting or the object returned. L144: the "numberOfTypes" case is missing L149-L151: perhaps i miss something, but why you use "number" for both and add the "check" key to know if it's a number of buildings or a number of types of buildings. Perhaps, just use "number" and "numberOfTypes" as in the json object ? Perhaps is there any reason to do like you did. L156: i find that a bit ugly to check if the tech is a pair tech with the name (at least in the sim). Perhaps you can check if there is a top or bottom key instead ? L157: LoadTechnologyPair is a structure tree function (in gui/structree/load.js). Perhaps you will have to move it in globalscripts too.

• depath

It is trivial, but perhaps should be documented as it's in Utility (i have no opinion about that).

Results of different tests :

• i first look at several parsing in DeriveTechnologyRequirements

{ "all": [{ "entity": { "class": "Village", "number": 5 } }, { "civ": "athen" }] }
-> ({athen:{entities:[{class:"Village", number:5, check:"count"}]}})

{ "all": [{ "entity": { "class": "Village", "numberOfTypes": 5 } }, { "civ": "athen" }] }
->({athen:{entities:[{class:"Village", number:5, check:"variants"}]}})

{"all": [{"tech": "phase_town" }, {"all": [{"notciv": "maur"}, {"notciv": "imp"}]}]}
-> ({maur:false, imp:false, generic:{techs:["phase_town"]}})

{"all": [{"tech": "tech1"}, {"any": [{"civ": "civA"}, {"civ": "civB"}]}]}
-> ({civA:{techs:["tech1"]}, civB:{techs:["tech1"]}})

{"all": [{"tech": "tech1"}, {"any": [{"civ": "civA"}, {"civ": "civB"}]}, {"notciv": "civC"}]}
-> ({civC:false, civA:{techs:["tech1"]}, civB:{techs:["tech1"]}})

{"any": [{"all": [{"civ": "civA"}, {"tech": "tech1"}]}, {"tech": "tech2"}]}
-> InterpretTechRequirements failed (in the all case of the any case)

{"any": [{"all": [{"civ": "civA"}, {"tech": "tech1"}]}, {"all": [{"civ": "civB"}, {"tech": "tech2"}]}]}
-> same

{"any": [{"all": [{"civ": "civA"}, {"tech": "tech1"}]}, {"all": [{"any": [{"civ": "civB"}, {"civ": "civC"}]}, {"tech": "tech2"}]}]}
-> same

• The pair techs are broken in the sim (you don't have the pair tech in the wall towers for example) but not in the structure tree.

• A requirement like { "class": "Town", "numberOfTypes": 2 } will not be displayed in the selection panels but the patch fixes #4108.

[s0600204]

Fixes in response to comments by fatherbushido

[s0600204]

JSDoc comments added to/expanded upon:

• DeriveTechnologyRequirements
• InterpretTechRequirements
• depath

L144: the "numberOfTypes" case is missing

Fixed

L149-L151: perhaps i miss something, but why you use "number" for both and add the "check" key to know if it's a number of buildings or a number of types of buildings. Perhaps, just use "number" and "numberOfTypes" as in the json object ? Perhaps is there any reason to do like you did.

If I were to use number/numberOfTypes in the resultant object, then an if...else if... would be necessary to read the data out. Should the entity requirement be expanded upon later in develeopment, then an if...else if...else if... would then be required. By doing it the way I did, a switch...case can be used instead. Attempting for cleaner code, I guess.

L156: i find that a bit ugly to check if the tech is a pair tech with the name (at least in the sim). Perhaps you can check if there is a top or bottom key instead ? L157: LoadTechnologyPair is a structure tree function (in gui/structree/load.js). Perhaps you will have to move it in globalscripts too.

Removed pair-tech detection from that point in the code. Unnecessary to do it there as both structree and simulation expand tech pairs when necessary anyhow.

Results of different tests [...]

All tests should now provide the correct result.

The pair techs are broken in the sim (you don't have the pair tech in the wall towers for example) but not in the structure tree.

Fixed. Were also broken in the structree, but in a different way (wasn't disabling on notciv)

A requirement like { "class": "Town", "numberOfTypes": 2 } will not be displayed in the selection panels but the patch fixes #4108.

A message should now be displayed in such a case And thanks for the review!

[fatherbushido]

That's really nice, i will review it again as soon as possible.

Edit:

Just a first quick remark:

something like that

"requirements": { "all": [
{ "entity": { "class": "Village", "numberOfTypes": 5 } },
{ "entity": { "class": "Melee", "number": 6 } },
{ "civ": "athen" }] 
}

is not treated correctly.

[s0600204]

Updated to deal with problem with auto-generated requirements tooltip. For those looking for a diff of changes 'tween v5 and v5.1 -> ​https://github.com/s0600204/0ad/commit/5d0ac4e4689b9ca294146013edd542801a802f72

[fatherbushido]

Thx for your work!

Parsing in DeriveTechnologyRequirements the following reqs:

"requirements": { "all": [
{"any":[{ "entity": { "class": "Village", "numberOfTypes": 5 } },
{ "entity": { "class": "Melee", "number": 6 } }
]},
{ "civ": "athen" }] 
}

I get

({generic:{}, athen:{}})

And for

"requirements": { "all": [
{"all":[{ "entity": { "class": "Village", "numberOfTypes": 5 } },
{ "entity": { "class": "Melee", "number": 6 } }
]},
{ "civ": "athen" }] 
}

I get

({athen:{}})

But

{ "all": [
{ "entity": { "class": "Village", "numberOfTypes": 5 } },
{ "entity": { "class": "Melee", "number": 6 } },
{ "civ": "athen" }] 
}

is ok and gives me

({athen:{entities:[{class:"Village", number:5, check:"variants"}, 
{class:"Melee", number:6, check:"count"}]}})

And at least

template = { "requirements": { "all": [
{"any" :
[
{ "civ":  "brit"  },
{ "civ":  "spart"  }
]},
{ "civ" : "athen" }]
}}

returns

({brit:{}, spart:{}, athen:{}})

[s0600204]

Re-thought the layout of the resultant object. Now should support any properly.

[s0600204]

Fix small problem with techs with no requirements at all.

[s0600204]

Fixes to parsing of all/any and any/all cases to prevent clobbering.

[fatherbushido]

All the previous issues are fixed. So now, patch test is ok. The requirements tooltip can still be optimized but it's another issue (and it's not used in the game for the moment). pro: merge the different logics (contra: use complicated stuff for the sim checks) I am a bit lazy to check v6 code but v5 was ok. So i don't deliver here a complete review.

I put it in the review queue to grab a last review.

[elexis]

The good thing about the patch is that it merges simulation, AI and tech tree requirements checking logic. Thus further changes to the requirements logic wouldn't the tech tree not the AI anymore.

However 14 layers of nesting in that requirements function seem a bit excessive. Would be great if that could be reduced. Ideally there are no more than 4-6 if statements or loops stacked inside each other per function:

function InterpretTechRequirements
case "all":
for (let subvalue of value)
for (let newOper in subvalue)
case "any":
for (let civ in result)
if (civ === "generic")
for (let sr in subReqs)
for (let type of subReqTypes)
for (let option of result.generic)
if (option[type])
for (let t in option[type])
if (!subReqs[sr][type] || !subReqs[sr][type].length)
for (let k in subReqs[sr][type])

• Perhaps some logic that is specific to the tech tree can be moved to the tech tree. The goal would be that contributors can extend the simulation / template code without breaking the tech tree (and ideally without having to bother about the tech tree at all). That would be met if the tech tree can parse the globalscripts/ returned object, even if future contributors add some logic.

• Perhaps the code could become simpler if the returned array doesn't need to be ordered in a tech-tree friendly way. For example the need to have a global that is ordered by civs. Removing object transformations is prefered over moving transformations.

• Perhaps it could use {"prop1": value1, "prop2": value2} instead of {"value1": "value2"} to replace some Object.keys() calls with foo.bar calls.
• Ideally those switch statements would be merged (any / all / ... appearing only once.)

But since it has been weeks since I last looked at the patch and had trouble even than understanding the code and all its pitfalls, I have no clue on how feasible these wishes are.

[s0600204]

Perhaps some logic that is specific to the tech tree can be moved to the tech tree.

There is nothing left in InterpretTechRequirements that is specific to the structree. There hasn't been for several revisions.

Alternatively, you could argue that the entire function is specific to the structree, because the only thing the simulation and the ai really need is a boolean value.

Except that's not true, because the differentiation between any and all, the necessity to have to deal with entity/class/number, the reason why there's so many nested ifs and fors... is because of having to serve the simulation (and ai). All the structree needs is an array of techs that need to come before the one being processed.

InterpretTechRequirements is a compromise. Neither the structree nor the simulation/ai get what they want. Instead they get an object that is a heck of a lot easier for them to get what they need from than the short-form requirements object in the tech json files.

Perhaps the code could become simpler [...] the need to have a global that is ordered by civs.

I'm going to assume you meant 'top-level keys' rather than 'global'.

It should be possible to amend InterpretTechRequirements so that instead of calculating the requirements in a civ-agnostic manner, it calculates the tech requirements for a given civ.

This would mean that the structree would have to run the function {civ-count} times for each tech, but it would also mean that the function would be simpler. Just. And it would be better for the simulation.

[Itms]

Pushed to A22 as suggested by fatherbushido.

[fatherbushido]

I suggest to follow the work of the megapatch in #4263 as currently we are far of the simple notciv bug.

[elexis]

In 18847:

Add the least amount of code to fix the disappearance of the phase requirements tooltip after r18030 and mark the incomplete implementation from r15055 as TODO, to be solved by refs #3993. Patch by fatherbushido.

[elexis]

In r19120 by fatherbushido:

Merge different logics of technology requirements and fix the related bugs. Create a global script which derive the technology requirements objects in a form usable for structure tree, gui, simulation and AI. Reviewed by mimo (for AI) and fatherbushido. Fixes #3993, #1646, #4263, #4217. Refs #4108.

In r19121 by fatherbushido:

Add a simulation test to the previous commit of s0600204's patch. Refs #4263.