Page MenuHomeWildfire Games

clean up market and other structure classes
ClosedPublic

Authored by Nescio on Jul 19 2020, 9:37 PM.

Details

Reviewers
Angen
bb
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23865: Clean up market classes:
Summary

This patch is a follow-up to D2549/rP23853. Ideally each template_structure_*.xml file ought to have a corresponding unique class, as is already the case for practically all template_unit_*.xml files. This removes ambiguity for players and helps preventing mistakes when auras, civ bonuses, or technologies are added or altered. That is the aim for this patch.

The complicated thing is currently both template_structure_economic_market.xml and template_structure_military_dock.xml have the Market class. This patch attempts a more systematic approach:

  • template_structure_economic_market.xmlMarket
  • template_structure_military_dock.xmlDock
  • <Market/TradeType> node → Trade
  • requirement for bartering: BarterMarketBarter

The problem is the Petra AI currently uses the Market for trade and BarterMarket class for the {civ}_market.xml. Therefore the following changes are made:

  • Where it is used as a marker for trade routes, Market is replaced with Trade.
  • Where it is used as an identifier for the {civ}_market.xml, BarterMarket is replaced with Market.
  • Where it is used as a prerequisite for bartering resources, BarterMarket is replaced with Barter.

This should make the Petra AI more robust and flexible: if a mod would want to split the market or dock for only one function, that should not cause problems.

Further changes:

  • Renamed the Gates class to Gate, LongWall to WallLong (cf. WallTower), and StoneWall to Wall, to match their template file names. Adjusted AI code, technologies, and templates accordingly.
  • Entity build restrictions:
    • renamed Apadana to Palace;
    • replaced UniqueStructure with IshtarGate and TempleOfVesta.
    • added Council to the (unbuildable) spart_gerousia.xml, since it's functionally identical to athen_prytaneion.xml.
  • Introduced the following classes:
    • Council, IshtarGate, Library, TempleOfVesta, and Theater, to match their build restrictions.
    • WallMedium and WallShort, to complement the already existing WallLong and WallTower classes.
    • Stoa, TempleOfMars, and TriumphalArch, for completeness.
  • Made the ArmyCamp, Colony, Gate, Lighthouse, Naval, Palace, Palisade, SiegeWall, and StoneWall classes visible.
  • Deleted the now unused Apadana, Kennel, SpecialBuilding classes.
  • Removed the Village class from outposts and palisades, because those are not supposed to count towards the phase technology requirements.
    • Slightly rephrased the phase technology requirements tooltips.
  • Corrected the <GenericName> of the various palisade files.
  • Updated Structure tooltips from D2578 that have not already been committed elsewhere.
Test Plan

Check for correctness and completeness. Verify everything still works as before.

Event Timeline

Nescio created this revision.Jul 19 2020, 9:37 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jul 19 2020, 9:37 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["sell"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  95|  95| 	amountsToSubtract[resourceToSell] = amount;
|  96|  96| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  97|  97| 	{
|  98|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  98|+		var amountToAdd = Math.round(prices.sell[resourceToSell] / prices["buy"][resourceToBuy] * amount);
|  99|  99| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 100| 100| 
| 101| 101| 		// Display chat message to observers
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["buy"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  95|  95| 	amountsToSubtract[resourceToSell] = amount;
|  96|  96| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  97|  97| 	{
|  98|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  98|+		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices.buy[resourceToBuy] * amount);
|  99|  99| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
| 100| 100| 
| 101| 101| 		// Display chat message to observers

binaries/data/mods/public/simulation/components/Barter.js
|  98| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['sell'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Barter.js
|  98| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['buy'] is better written in dot notation.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2708/display/redirect

bb accepted this revision.Jul 21 2020, 9:42 PM
bb added a subscriber: bb.
bb added inline comments.
binaries/data/mods/public/simulation/ai/petra/headquarters.js
285

Not sure what this is meant to do, comes from rP21178

binaries/data/mods/public/simulation/components/Identity.js
68

IshtarGate missing

This revision is now accepted and ready to land.Jul 21 2020, 9:42 PM

sry for the typo in the commit message, please close the revision manually.

Nescio closed this revision.Jul 21 2020, 10:02 PM

Thanks a lot! This was a large patch, I didn't expect it to be reviewed and committed this quickly.
Since you're around now, I've several other, smaller clean up patches waiting for reviewers, including: D2888, D2887, D2855, D2850, D2834, D2744, D2721, D2595, D2579, D918. Hopefully they won't conflict too much.

binaries/data/mods/public/simulation/components/Identity.js
68

That's the only one I missed?

Angen added a comment.Jul 22 2020, 9:12 AM

I did not have time to check everything yet.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
285

this is here to disable limit for the fields once ai has build market to barter resources so reading summary, here should be "Barter" :)

1384

I feel this is likely to be wrong.
Generally ai needs to keep first market because of bartering or starting trading rout.
Since now it has two different classes,
we cannot just assume it will be one structure.
So having this correct, means ai needs at least one market with ability to barter and at least one market with ability to start trading route with allies.

1541

Barter

2673

canBarter -> Barter class

binaries/data/mods/public/simulation/ai/petra/queueplanBuilding.js
57

this is most likely wrong.
Result of this if should be following.
If it is not barter market check
If ai has at least one trade market check
With split of the clasess following should also apply
If it is not trade market do not check

260

should be Trade ? If market is not for trade we should not care here.

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
603

before here, it is asking for Trade class, so this needs also to ask for Trade class.

Thanks, @Angen! My understanding of javascript and the AI code is rather limited, I made guesses more than once, so I suspect there are other lines that could be improved. I'll hold off proposing a patch for now. Let me know when you're finished checking and double-checking.
Strictly speaking I believe the correct (and intended) AI class usage would be:

  • Naval refers to structures built upon shorelines, i.e. those reachable by ships (i.e. {civ}_dock.xml, brit_crannog.xml, cart_super_dock.xml, ptol_lighthouse.xml).
  • Dock refers to {civ}_dock.xml, i.e. the structure that constructs ships and researches ship technologies.
  • Market refers to {civ}_market.xml, i.e. the structure that trains traders and researches (sharing, barter, trade, and tribute) technologies; also for phase-based building lists (cf. Temple).
  • Barter refers to the entity that enables bartering resources.
  • Trade refers to trade route destinations and origins.
    • Trade+Naval refers to those reachable by ships.
    • Trade+!Naval refers to those not reachable by ships.

(I spelled this out for future reference, also for myself.)

binaries/data/mods/public/simulation/ai/petra/queueplanBuilding.js
260

Previously it had BarterMarket, i.e. excluding the dock, which is a proper destination for land traders though. So maybe Trade+!Naval should be used here?

bb added inline comments.Jul 22 2020, 4:08 PM
binaries/data/mods/public/simulation/ai/petra/headquarters.js
285

But is the reason "Barter" why not "Trade" ?

1384

There is a third aspect too: the ai wants to build town phase buildings to progress to city phase

1541

Disagree, ai just as well wants to start trading before bartering, so probably should check for both

binaries/data/mods/public/simulation/ai/petra/queueplanBuilding.js
260

Probably should be trade indeed

binaries/data/mods/public/simulation/components/Identity.js
68

Lets fix this once and foregood, with P217 we search all the templates and check if all classes are in this list. Currently some are missing, but I will commit that in due course.

Angen added a comment.Jul 26 2020, 9:24 AM

I think I am finished. I did not notice anything else.
@Nescio

binaries/data/mods/public/simulation/ai/petra/headquarters.js
285

because one market generally cant do trade without allies

1541

at least one means i do not care too much if i can trade since in solo player I cant with one market
Also notice L2732 it cares especially about bartering.
But since this is subject for discussion or maybe ai design wich type of market should be build first, I can agree to be checked for both. (this will not change current ai anyway)

Nescio added inline comments.Jul 27 2020, 9:59 AM
binaries/data/mods/public/simulation/ai/petra/headquarters.js
285

So MarketBarter?

1384

So what should be done here? Keep Market (i.e. structure for traders, technologies, and phasing up)?

1541

So insert the following line?
!gameState.getOwnEntitiesByClass("Barter", true).hasEntities() ||

2673

Thanks, this one is very clear.

binaries/data/mods/public/simulation/ai/petra/queueplanBuilding.js
57

So what exactly should be changed here?

260

So MarketTrade?

binaries/data/mods/public/simulation/ai/petra/tradeManager.js
603

So MarketBarter, understood.

Nescio added a comment.Dec 2 2020, 8:33 PM

@Angen, you pointed out some of the Barter/Market/Trade classes ought to be changed, however, it's not always clear to me into what exactly; see earlier posts.