Paths

Table of Contentst

clean up market and other structure classes
ClosedPublic
Actions

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

Details

Reviewers

Silier
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.xml → Market
template_structure_military_dock.xml → Dock
<Market/TradeType> node → Trade
requirement for bartering: BarterMarket → Barter

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.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 12503
Build 23995: Vulcan Build	Jenkins
Build 23994: Vulcan Build (macOS)	Jenkins
Build 23993: Vulcan Build (Windows)	Jenkins
Build 23992: arc lint + arc unit

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

Herald added a reviewer: Silier. · View Herald TranscriptJul 19 2020, 9:37 PM

Nescio mentioned this in D2549: clean up tower classes.Jul 19 2020, 9:44 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

Harbormaster completed remote builds in B12503: Diff 12787.Jul 19 2020, 9:56 PM

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

bb mentioned this in rP23865: Clean up market classes:.Jul 21 2020, 9:45 PM

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

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?

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.

Nescio mentioned this in D2742: [gameplay] disable pers hall and ishtar gate.Jul 25 2020, 11:10 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 `Market` → `Barter`?
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 `Market` → `Trade`?
binaries/data/mods/public/simulation/ai/petra/tradeManager.js
603	So `Market` → `Barter`, understood.

Nescio mentioned this in D2919: Make PlayerHasMarket event-based.Jul 28 2020, 8:08 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.

Nescio mentioned this in D2578: update structure template tooltips.Jan 12 2021, 7:08 PM