Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4263 closed defect (fixed)

[PATCH] Merge different logics of technology requirements and fix the related bugs.

Reported by: fatherbushido Owned by: s0600204
Priority: Must Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by fatherbushido)

There are several bugs with technology requirements :

#3993 (notciv args is bugged in structure tree) #4108 (unmet class number requirements tooltip is broken, same for number of types one wich is not handled) #1646 (same as #4108 about unmet requirements)

Moreover we have different logics : sim, gui, structure tree, ai.

The purpose of the ticket is to do a rewriting of all that and to fix the related bugs.

See the mega-patch http://trac.wildfiregames.com/attachment/ticket/3993/notciv_v6-2.patch

Attachments (8)

4263_techreqs_v1.patch (44.9 KB) - added by s0600204 3 years ago.
Rewrite of earlier work for #3993, this time deriving the specific tech reqs for a given civ (rather than trying to spit out an object that works for all civs). Also coincidentally (ie. unintentionally) provides an alternate solution to #4217.
4263_techreqs_v1-1.patch (50.7 KB) - added by s0600204 3 years ago.
Rename depath() -> basename() as suggested by vladislav on irc; look at and update ai for mimo
4263_techreqs_v1-2.patch (51.1 KB) - added by s0600204 3 years ago.
Update ai code in response to mimo's comments
4263_techreqs_v1-3.patch (51.2 KB) - added by s0600204 3 years ago.
Updated patch
test_technologies.js (3.8 KB) - added by fatherbushido 3 years ago.
4263_techreqs_v1-4.patch (73.3 KB) - added by s0600204 3 years ago.
Updated patch.
4263_techreqs_v1-5.patch (73.4 KB) - added by s0600204 3 years ago.
4263_techreqs_v1-5-1.patch (73.4 KB) - added by s0600204 3 years ago.
Updated

Download all attachments as: .zip

Change History (42)

comment:1 Changed 3 years ago by fatherbushido

Description: modified (diff)

comment:2 Changed 3 years ago by fatherbushido

Priority: Should HaveMust Have

comment:3 Changed 3 years ago by fatherbushido

@s0600204: perhaps we need to rethink some stuff. We really need your deep opinions on that.

It's also allowed to change the requirements object structure, (removing the notciv option too),

Moreover, I understand/agree that we can use that new reqs form for the sim check (he who can do more can do less) but about perfs I wonder if it's good to reinterpret each time the requirements creating that new object for doing the check (without caching anything) when we do the sim check.

About your comment:

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.

What do you prefer ?

comment:4 Changed 3 years ago by fatherbushido

Keywords: patch rfc added
Summary: Merge different logics of technology requirements and fix the related bugs.[PATCH] Merge different logics of technology requirements and fix the related bugs.

comment:5 Changed 3 years ago by elexis

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

Changed 3 years ago by s0600204

Attachment: 4263_techreqs_v1.patch added

Rewrite of earlier work for #3993, this time deriving the specific tech reqs for a given civ (rather than trying to spit out an object that works for all civs). Also coincidentally (ie. unintentionally) provides an alternate solution to #4217.

comment:6 Changed 3 years ago by Vladislav Belov

Lines like:

if (civ == value) 
    return []; 
return false; 

Could be written shorter as:

return civ == value ? [] : false; 

comment:7 Changed 3 years ago by fatherbushido

(@vladislavbelov: to be honest, the gain is little and some prefers 'ifs' instead of ternaries, but that's mainly a matter of taste.)

@s0600204: thanks for the patch! I have pm'd you in the forum about that and I thought you had given up. I will look at it as soon as possible. It seems really nicest to read.

comment:8 Changed 3 years ago by elexis

(return civ == value && []; but that seems less readable)

comment:9 Changed 3 years ago by mimo

I just had a very quick look at the patch concerning the ai, and it may miss something for common-api/gamestate.js lines 29 to 51. This part of code loops on the phase upgrades to check for civ-specific ones, and stores the needed requirements. This info is then used in headquarters.js, when gameState.getPhaseRequirements() is called. It's currently a bit hacky, so every improvment welcome :-)

Changed 3 years ago by s0600204

Attachment: 4263_techreqs_v1-1.patch added

Rename depath() -> basename() as suggested by vladislav on irc; look at and update ai for mimo

comment:10 Changed 3 years ago by fatherbushido

  • (I try to find how to avoid a simple way to get the value of object with only key but I didn't succeed. A solution would be to use { "op": op, "value": value } for reqs directly in json tech files but I don't think it's wanted. (it would simplify many stuff).)
  • I have a fail

template.requirements = { "all": [{ "tech": "phase_town" }, { "notciv": "maur" }, { "notciv": "spart" }] }; is parsed correctly but not template.requirements = { "all": [{ "tech": "phase_town" }, { "all": [{ "notciv": "maur" }, { "notciv": "spart" }] }] } Indeed the second way of write it is clumsy, but if we don't want to support such stuff, we should say it.

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

comment:11 Changed 3 years ago by mimo

Thanks s0600204 for the changes in the ai code. I've been through them and here are a few comments:

gamestate.js: In getPhaseEntityRequirements, i would have renamed option to requirement as option looks out of place here.

CanResearch?: why did you remove the check on supersedes technology?

checkTechRequirements: why returning false if no reqs? The function doesEntitySpecPass would not work if there is a requirement with number=0. Should not happen in principle, but who knows what modders could do.

headquarter.js seems that lines 1445-1446 should be removed? I think we should add in line 1437: let needed = 0; then add after 1452, needed = Math.max(needed, entityReq.count); and close the loop on entityReq. then replace entityReq.count by needed in line 1458.

and replace lines 1469-1471 by

let houseTemplate = gameState.getTemplate(gameState.applyCiv("structures/{civ}_house"));
if (gameState.getPhaseEntityRequirements(2).every(req =>
	!houseTemplate.hasClass(req.class) || gameState.getOwnStructures().filter(API3.Filters.byClass(req.class)).length >= req.count))
	this.requireHouses = undefined;

Changed 3 years ago by s0600204

Attachment: 4263_techreqs_v1-2.patch added

Update ai code in response to mimo's comments

comment:12 Changed 3 years ago by s0600204

CanResearch?: why did you remove the check on supersedes technology?

Because supersedes is taken into account by the new tech requirement globalscript code. (Rather than have the simulation, ai, and the session and structree GUIs all needing code to deal with it themselves.)

checkTechRequirements: why returning false if no reqs?

That returns false if reqs is false, which is a valid value, passed if the tech cannot be researched by the given civ. It there were indeed no requirements set in the json file, reqs would be [].

The function doesEntitySpecPass would not work if there is a requirement with number=0. Should not happen in principle, but who knows what modders could do.

That's more a problem with this.playerData.classCounts[entity.class] or this.playerData.typeCountsByClass[entity.class] not being logically true until an entity of the required class is created causing the class to be added as a key to those objects.

To fix, I've added a conditional to the tech requirement code to catch that, as both the simulation and the session gui also fail in this circumstance.

seems that lines 1445-1446 should be removed?

Uh, yes.

I think we should add [...]

That seems saner...

and replace [...]

...and although the codeblock is not very readable, okay.

comment:13 Changed 3 years ago by mimo

Thanks for the answers. There is a problem with the new patch in headquarters.js, count is not defined in the block at 1456, so we should change 1453 -> needed = Math.max(needed, entityReq.count - count); 1458 -> --needed; 1459 -> else if (needed > 0) 1462 -> --needed;

In addition, in technologies.js, wouldn't it be better to replace 139-141 by if (number > 0) and remove 137 and 149

comment:14 Changed 3 years ago by fatherbushido

@s0600204: DeriveTechnologyRequirements? fails on template.requirements = { "all": [{ "tech": "phase_town" }, { "all": [{ "notciv": "maur" }, { "notciv": "spart" }] }] } (it's a clumsy requirements but it should be handle).

comment:15 in reply to:  12 Changed 3 years ago by mimo

I did further checks, and noticed that in addition to my previous comments 13, line 1452 should be moved to 1455

Futhermore, Replying to s0600204:

checkTechRequirements: why returning false if no reqs?

That returns false if reqs is false, which is a valid value, passed if the tech cannot be researched by the given civ. It there were indeed no requirements set in the json file, reqs would be [].

Then shouldn't we have "if (!reqs.length) return true;" after this "if (!reqs) return false;",

Maybe it is supposed to never happen, but i encountered a case, unlock_champion_units, where DeriveTechnologyRequirements? return [] and thus the checkRequirement for it is always false in the current code (while i think it should have returned [{techs:phase_city?}]).

comment:16 Changed 3 years ago by fatherbushido

I confirm. In fact all-any and all-all case are broken.

edit: but simple stuff like {"all": [{"tech": "techC"}, {"all": [{"tech": "techA"}, {"tech": "techB"}]}]} are well parsed.

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

comment:17 Changed 3 years ago by fatherbushido

L162 L229 of Technologies.js, you can use the Object.keys(foo)[0] syntax you used elsewhere to keep consistency

(btw we should remove Technologies.js in technologies.js but let's do it after)

comment:18 Changed 3 years ago by fatherbushido

s0600204: I m perhaps wrong but we never fall in L271-274 of Technologies.js isn't it?

Changed 3 years ago by s0600204

Attachment: 4263_techreqs_v1-3.patch added

Updated patch

comment:19 Changed 3 years ago by s0600204

mimo:

Thanks for the answers.

No problem. Thank you for your patience as I modify the ai code, apparently not very well.

There is a problem [...] so we should change [...]

I think I understand what changes you're trying to direct me to make, but I may have got confused. If I've got it wrong in the next patch, sorry.

In addition, in technologies.js, wouldn't it be better to replace 139-141 by if (number > 0) and remove 137 and 149

Depends whether you count 139-40 as an early return/break or not. Changed it anyway. Also, the brackets on lines 137 and 149 (in next patch 137 and 147) are required by the code style guidelines as I'm declaring a new, temporary, variable.

Then shouldn't we have if (!reqs.length) return true; after this if (!reqs) return false;

The TechnologyManager component has this, so yes, thanks.

[...] unlock_champion_units [...]

Requirements in that file were not being parsed correctly due to the problem raised by fatherbushido. Should now be parsed correctly.


fatherbushido:

(it's a clumsy requirements but it should be handle).

Clumsy, yes, but similar to those used by seige_bolt_accuracy until I changed it to something saner within this patch.

I confirm. In fact all-any and all-all case are broken.

all/all and all/any should now work (he says hopefully)

I m perhaps wrong but we never fall in L271-274 of Technologies.js isn't it?

Only when running mods that haven't updated to use the changed syntax of the entity requirement spec. Which is currently all of them (except the main 'public' mod).

comment:20 Changed 3 years ago by fatherbushido

s0600204:

all/all and all/any should now work (he says hopefully)

Ok, I start to like that function ;-)

Clumsy, yes, but similar to those used by seige_bolt_accuracy until I changed it to something saner within this patch.

Sure, there is story about that...

Only when running mods that haven't updated to use the changed syntax of the entity requirement spec. Which is currently all of them (except the main 'public' mod).

We decide to change the syntax of that json object, so go in that.

Changed 3 years ago by fatherbushido

Attachment: test_technologies.js added

comment:21 Changed 3 years ago by fatherbushido

Looks good! I attached a file that you can add to the patch (in simulation/components/tests). It's not really the expected path but it's ok for the moment.

L203, it looks you can replace for (let sr in requirements) -> for (let foo of requirements) requirements[sr] -> foo

(Perhaps we could even simplify that concatenation/merging).

comment:22 Changed 3 years ago by mimo

Thanks for the latest changes. I did some further tests with ais and didn't noticed any problems (running on a replay with 16000 turns and 4 ais, the same techs were researched at the same elapsed time with and without the patch). Fatherbushido, you can consider the patch reviewed for the ai part.

comment:23 Changed 3 years ago by fatherbushido

mimo: nice! s0600204: would you agree to remove L276->L280 of Technologies.js and using element of the array instead of index/keys in L203 ? (I will then add a simple test for CheckTechnologyRequirements.) And it looks we are ok!

Changed 3 years ago by s0600204

Attachment: 4263_techreqs_v1-4.patch added

Updated patch.

comment:24 Changed 3 years ago by fatherbushido

nice! I will not hurry to commit it today, I will check again tommorow then I think we will go for it.

comment:25 Changed 3 years ago by elexis

Respect for constructing something reasonable to address this issue in a non-duplicated way! Only my code style remarks from my side as I can't get into voodoo right now:

  • I'd move the break into the scope of cases, not sure if others share my taste here
  • A tri-state boolean is an oxymoron
    • (civPermitted < true) better (civPermitted !== true), since it is an implementation detail whether true is -1 or 1.
    • undefined, true, false would be a more common tristate than -1, true, false
  • selection_panels.js: GetSimState().players[player] is used 4 times, so might be abbreviated with a heler variable. If that L849 isn't shorter than 120 characters, I'd split that ternary to multiple lines.

comment:26 Changed 3 years ago by fatherbushido

Keywords: review added; rfc removed

s0600204: can you adress elexis remarks (for the 1st one, I m more for letting the break out of the scope, imo the break should be outside the code block between the scope). Then I will commit it tommorow if now else complain / has remark anymore / has seen something we missed. Thanks for your job and your patience. (I put it in the review queue until tommorow).

comment:27 Changed 3 years ago by s0600204

Apologies for the wait - work got in the way.

With regards to whether the break should be inside the {} of the relevant case - some of the code that this patch replaces (from the structree) had the break outside. That was code I wrote, and was committed by leper (who told me to add the brackets where I'd missed them, but said nothing about the placement of the breaks).

A quick and crude search (passing the output of one ack through another ack), reveals that gamesetup_mp.js, lobby.js, and prelobby.js all have breaks inside a case's {} where that was applicable. I couldn't find any instances of break being outside (though I didn't look very hard).

I've moved them inside so as to be consistant with the above js files. Perhaps a decision should/could be made (not in this ticket though, obv.) and the coding conventions page updated for future reference.

Anyhow, updated patch. (And changes to the last patch.) Enjoy ;)

Changed 3 years ago by s0600204

Attachment: 4263_techreqs_v1-5.patch added

comment:28 Changed 3 years ago by fatherbushido

:/ I guess you have something to rebase with the previous AI commits. I get those errors at start of the game.

ERROR: JavaScript error: simulation/ai/common-api/gamestate.js line 39TypeError: this.civ is not a function m.GameState.prototype.init@simulation/ai/common-api/gamestate.js:39:101m.SharedScript.prototype.init@simulation/ai/common-api/shared.js:203:3InitGame@simulation/helpers/InitGame.js:78:2 ERROR: JavaScript error: simulation/ai/common-api/baseAI.js line 46TypeError: this.gameState is undefined m.BaseAI.prototype.Init@simulation/ai/common-api/baseAI.js:46:2InitGame@simulation/helpers/InitGame.js:78:2 GAME STARTED, ALL INIT COMPLETEERROR: JavaScript error: simulation/ai/petra/_petrabot.js line 112TypeError: this.gameState is undefined m.PetraBot.prototype.OnUpdate@simulation/ai/petra/_petrabot.js:112:2m.BaseAI.prototype.HandleMessage@simulation/ai/common-api/baseAI.js:71:2

comment:29 Changed 3 years ago by fatherbushido

Ok, see http://trac.wildfiregames.com/changeset/19104 Looking quickly, it seems you need to replace this.civ() by this.getPlayerCiv()

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

Changed 3 years ago by s0600204

Attachment: 4263_techreqs_v1-5-1.patch added

Updated

comment:30 Changed 3 years ago by fatherbushido

Well:

  • I read code again
  • I test it again (in game, some functions, with other techs)
  • I checked structure tree, tooltips,training panel, construction panel, upgrade panel.
  • That patch has received partial and complete reviews from logic and formal point of view
  • That patch has/had been desired by several devs and has been in rfc / review queue for months
  • That patch reaches the goal of merging logics and fixes several bugs in the same time.
  • It provides an extensive test of that globalscripts new function.

So I will commit it. If ever something is wrong then we will do our best to fix it or revert it. Thanks again for your work.

EDIT: I just edited the last whitespace issues and remove the poem ref in tests.

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

comment:31 Changed 3 years ago by elexis

Wasn't quick enough to post these:

for (let res of result) 
   requirements.push(res); 

Isn't that requirements = requirements.concat(result)?

L189 of gamestate.js missing semicolon, might want to paste the files on jshint.com

Also might want to check that there haven't been unwanted changes introduced in recent updates by checking git commits quickly (diff between the diffs https://github.com/s0600204/0ad/commits/4263_techreqs)

comment:32 Changed 3 years ago by elexis

Keywords: review removed
Milestone: Work In ProgressAlpha 22
Resolution: fixed
Status: newclosed

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.

Thanks for all the time you two have invested in fixing this, should receive some patch of the month award :) I hope the design decision to add the logic to the code instead of the JSON files and unifying all places is shared by the others as well.

comment:33 Changed 3 years ago by fatherbushido

Keywords: review added
Milestone: Alpha 22Work In Progress

s0600204: I forgot (How can I be fogiven for that :/) to display your name in r19120, so I did it in r19121.

comment:34 Changed 3 years ago by elexis

Keywords: review removed
Milestone: Work In ProgressAlpha 22
Note: See TracTickets for help on using tickets.