Opened 3 years ago

Closed 2 years ago

#3993 closed defect (fixed)

[PATCH] Support notciv tech requirement in structure tree

Reported by: sanderd17 Owned by:
Priority: Should Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by 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.

Attachments (13)

notciv.patch (8.8 KB) - added by s0600204 3 years ago.
Proposed patch.
notciv_rebased.patch (8.8 KB) - added by s0600204 3 years ago.
notciv_v3.patch (9.1 KB) - added by s0600204 3 years ago.
With style changes as suggested by elexis
notciv_v3-1.patch (9.1 KB) - added by s0600204 3 years ago.
Additional style change as suggested by elexis.
notciv_v3-2.patch (9.2 KB) - added by s0600204 3 years ago.
Patch rebased.
notciv_v3-3.patch (9.3 KB) - added by s0600204 3 years ago.
Suggestions from elexis from irc
notciv_v4.patch (30.1 KB) - added by s0600204 3 years ago.
As requested in irc, moved to globalscripts.
notciv_v4-1.patch (34.4 KB) - added by s0600204 3 years ago.
Clean up entity sorting
notciv_v5.patch (37.0 KB) - added by s0600204 3 years ago.
Fixes in response to comments by fatherbushido
notciv_v5-1.patch (37.2 KB) - added by s0600204 3 years ago.
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
notciv_v6.patch (39.2 KB) - added by s0600204 3 years ago.
Re-thought the layout of the resultant object. Now should support any properly.
notciv_v6-1.patch (39.2 KB) - added by s0600204 3 years ago.
Fix small problem with techs with no requirements at all.
notciv_v6-2.patch (39.3 KB) - added by s0600204 3 years ago.
Fixes to parsing of all/any and any/all cases to prevent clobbering.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 3 years ago by sanderd17

Description: modified (diff)
Keywords: simple removed
Summary: Mixing notciv and other requirements should be supportedSupport notciv tech requirement in structure tree

comment:2 Changed 3 years ago by stanislas69

Milestone: BacklogAlpha 21
Resolution: fixed
Status: newclosed

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

Last edited 3 years ago by stanislas69 (previous) (diff)

comment:3 Changed 3 years ago by mimo

Resolution: fixed
Status: closedreopened

No it was a typo, r18219 fixed #3992

Changed 3 years ago by s0600204

Attachment: notciv.patch added

Proposed patch.

comment:4 Changed 3 years ago by 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"}]}]}

comment:5 Changed 3 years ago by s0600204

Keywords: patch review added
Summary: Support notciv tech requirement in structure tree[PATCH] Support notciv tech requirement in structure tree

Changed 3 years ago by s0600204

Attachment: notciv_rebased.patch added

Changed 3 years ago by s0600204

Attachment: notciv_v3.patch added

With style changes as suggested by elexis

Changed 3 years ago by s0600204

Attachment: notciv_v3-1.patch added

Additional style change as suggested by elexis.

Changed 3 years ago by s0600204

Attachment: notciv_v3-2.patch added

Patch rebased.

Changed 3 years ago by s0600204

Attachment: notciv_v3-3.patch added

Suggestions from elexis from irc

Changed 3 years ago by s0600204

Attachment: notciv_v4.patch added

As requested in irc, moved to globalscripts.

comment:6 Changed 3 years ago by s0600204

Keywords: rfc added; review removed

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

comment:7 Changed 3 years ago by 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).

Last edited 3 years ago by fatherbushido (previous) (diff)

Changed 3 years ago by s0600204

Attachment: notciv_v4-1.patch added

Clean up entity sorting

comment:8 Changed 3 years ago by fatherbushido

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

Some remarks :

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

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 :

{ "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.
Last edited 3 years ago by fatherbushido (previous) (diff)

Changed 3 years ago by s0600204

Attachment: notciv_v5.patch added

Fixes in response to comments by fatherbushido

comment:9 Changed 3 years ago by 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!

comment:10 Changed 3 years ago by 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.

Last edited 3 years ago by fatherbushido (previous) (diff)

Changed 3 years ago by s0600204

Attachment: notciv_v5-1.patch added

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

comment:11 Changed 3 years ago by 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:{}})
Last edited 3 years ago by fatherbushido (previous) (diff)

Changed 3 years ago by s0600204

Attachment: notciv_v6.patch added

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

Changed 3 years ago by s0600204

Attachment: notciv_v6-1.patch added

Fix small problem with techs with no requirements at all.

Changed 3 years ago by s0600204

Attachment: notciv_v6-2.patch added

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

comment:12 Changed 3 years ago by fatherbushido

Keywords: review added; rfc removed

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.

comment:13 Changed 3 years ago by 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.

comment:14 Changed 3 years ago by 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.

comment:15 Changed 3 years ago by Itms

Milestone: Alpha 21Alpha 22

Pushed to A22 as suggested by fatherbushido.

comment:16 Changed 3 years ago by fatherbushido

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

comment:17 Changed 3 years ago by 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.

comment:18 Changed 3 years ago by fatherbushido

Keywords: review removed

comment:19 Changed 2 years ago by elexis

Resolution: fixed
Status: reopenedclosed

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.

Note: See TracTickets for help on using tickets.