Paths

Table of Contentst

Differential D1181

Structure Tree - use phase techs to determine phase order.
ClosedPublic
Actions

Authored by s0600204 on Dec 28 2017, 4:49 AM.

Details

Reviewers

mimo

Commits

rP20749: Use phase techs to determine phase order in structure tree.

Summary

The Structure Tree (or structree) has no preconceptions on launch about what phases the game has or in which order they are. This is by design, and permits mods to add or remove phases as they wish, with no modification to the structree code necessary.

The way the structree currently determines phase order is that if makes use of the fact that the required phase of tech A usually comes before the required phase of tech B, where tech B requires tech A to be researched first and the two phases are different.

This works, so long as a civ has several non-phase techs that each require at least one other non-phase tech to be researched first.

However, what if a civ doesn't have any non-phase techs that require another non-phase tech? That's what appears to have happened with the Zora civilisation from The Undying Nephalim's Hyrule Conquest mod.

This revision rewrites the unravel_phases function to work out the phase order from the phase-techs themselves, as they should be expected to always require each other.

As to why the function was not written this way originally:

The phase-techs' actualPhase attribute (used below) is derived from the replaces attribute in the respective phase-tech's json template.
The unravel_phases function follows the same basic logic as the same function of the original php/web-version. (link)
The php version was written before the replaces attribute was added to json templates. (In rP16678.)

Test Plan

Apply to local install, then check each civ in the structree to make sure that all civs have the phases in the correct order, paying attention to the athen civ who has civ-specific phase techs.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

s0600204 created this revision.Dec 28 2017, 4:49 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B4216: Diff 4979.Dec 28 2017, 6:39 AM

The ai also has to determine this order, and it currently does it in a very rudimentary way (init function of common-api/gamestate.js) which would gain to be rewritten after the change to phase_{civ}. So it may be better to move this unravel into globalscripts to avoid dupplication.

binaries/data/mods/public/gui/reference/common/helper.js
122 ↗	(On Diff #4979)	techs -> phases in param and either phases or phase techs in the comment

(Only some linting)

work out the phase order from the phase-techs themselves, as they should be expected to always require each other

(They must? I guess so. Pair-phases probably not supported currently anyhow?)

binaries/data/mods/public/gui/reference/common/helper.js
122 ↗	(On Diff #4979)	^
129 ↗	(On Diff #4979)	(Not sure if phaseName would be more in line with the rest)
132 ↗	(On Diff #4979)	!length
135 ↗	(On Diff #4979)	(actual phase is a bit confusing, all code ought be actually be correct, but out of scope of this diff)
141 ↗	(On Diff #4979)	removing assumptions is good
146 ↗	(On Diff #4979)	shortening code too

Linting from elexis and mimo, and support for phase-pairs.

In D1181#47726, @mimo wrote:

The ai also has to determine this order, and it currently does it in a very rudimentary way (init function of common-api/gamestate.js) which would gain to be rewritten after the change to phase_{civ}. So it may be better to move this unravel into globalscripts to avoid dupplication.

I'm looking into it, although it'll probably be a follow-up revision to this one. Trying to work out how this part of the ai code handles phase-pairs currently.

Couple of questions:

How did you envision phase-pairs to work, post D1024?
- Prior, phase-pairs template names followed the pattern phase_{level}_pair or phase_{level}_pair_{civ}. Clearly these wouldn't be picked up by D1024's changes, so I'm taking that we're dropping that. (/me makes a note to check the rest of the structree)
- It is possible to simply have the contents of, say phase_city_gaul, to be that of a pair tech, (handled by common-api/technology.js setting _definesPair to true) but the current code in common-api/gamestate.js appears to have no provision for that, falling back to { name: "phase_city", requirements: [] } in this case.
How (in abstract terms) does the ai handle phase-pairs, in particular decide which one to research?

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B4222: Diff 4984.Dec 28 2017, 6:30 PM

In D1181#47775, @s0600204 wrote:

In D1181#47726, @mimo wrote:

The ai also has to determine this order, and it currently does it in a very rudimentary way (init function of common-api/gamestate.js) which would gain to be rewritten after the change to phase_{civ}. So it may be better to move this unravel into globalscripts to avoid dupplication.

I'm looking into it, although it'll probably be a follow-up revision to this one. Trying to work out how this part of the ai code handles phase-pairs currently.

fine

Couple of questions:

How did you envision phase-pairs to work, post D1024?

Prior, phase-pairs template names followed the pattern phase_{level}_pair or phase_{level}_pair_{civ}. Clearly these wouldn't be picked up by D1024's changes, so I'm taking that we're dropping that. (/me makes a note to check the rest of the structree)

It is possible to simply have the contents of, say phase_city_gaul, to be that of a pair tech, (handled by common-api/technology.js setting _definesPair to true) but the current code in common-api/gamestate.js appears to have no provision for that, falling back to { name: "phase_city", requirements: [] } in this case.

How (in abstract terms) does the ai handle phase-pairs, in particular decide which one to research?

In fact, the ai does not support phase-pairs currently. It had in the past (when celt were still one civ) where it would choose randomly between gaul and brit.
But as you said, i assumed that with the individualization of each civ as we have now, we won't support it anymore. I don't know if we need to reallow if for mods as we can still have a pair_phase_city_{civ} which would contains phase_city_a_{civ} and phase_city_b_{civ}, each giving different modifications.

(while writing these lines, i'm wondering if the {civ} replacement would work for pairs, i'd need to check).

In D1181#47733, @elexis wrote:

work out the phase order from the phase-techs themselves, as they should be expected to always require each other

(They must? I guess so. Pair-phases probably not supported currently anyhow?)

Was just looking for ways to falsify the assumption - if there aren't any, the approach must be correct.

The only other exception I could think of would be phases that can be researched without any requirements.
But my understanding is that phases can be modeled as a directed acyclic graph,
so the approach should be correct.

Intuitively I would also say that it's much more reasonable to deduce the phase order from the phases than from non-phase technologies.

So I guess the rest is only handcraft and if there is a stupid bug, you're here to fix it and if you aren't we others can fix it too before the release.
I would agree that you can commit it, possibly after some more handcrafting, but I didn't formaly prove the entire thing and I didn't test it.

binaries/data/mods/public/gui/reference/common/helper.js
136 ↗	(On Diff #4984)	I had a look into `Technologies.js` and this is indeed the `supersedes` technology which is the required phase of the current phase.
137 ↗	(On Diff #4984)	If the condition is not met, then the mod is broken? Hence having an error would make the code not only shorter but also help revealing the error sooner than later?
143 ↗	(On Diff #4984)	(out of scope, but using the `sort` function would not reinvent sorting. But likely only nice if that could decrease the complexity.)
177 ↗	(On Diff #4984)	Shouldn't that just test for `top`? Should this actually return true if the filename starts with `pair_` but doesn't have top?
185 ↗	(On Diff #4984)	Oh yeah having multiple filename patterns for pair techs seems ugly. Even simpler (thus less error prone, easier to comprehend, maintain and extend) would be making the filename irrelevant globally.

s0600204 mentioned this in D1183: Move unravelPhases function from structree to globalscripts..Dec 28 2017, 9:38 PM

Tweak pair tech detection.

In D1181#47781, @elexis wrote:

But my understanding is that phases can be modeled as a directed acyclic graph, so the approach should be correct.

Uh, yes. From what I can ascertain from a skim-read of the page, yes. Phases come into effect one after another with no loops or cycles (or, presently, backtracking).

(Love that they use a family tree of the Ptolemaic dynasty as an example.)

I would agree that you can commit it, [...], but I didn't formaly prove the entire thing and I didn't test it.

Heh, noted.

binaries/data/mods/public/gui/reference/common/helper.js
137 ↗	(On Diff #4984)	If the condition is not met, then the mod is broken? Not necessarily, as all incarnations of the town phase technology require `phase_village`, which isn't in the `phases` object passed to the function (as it isn't researchable in any structure or by any unit).
177 ↗	(On Diff #4984)	Yeah, good point. I think that my original thinking was trying to minimise how many times we load from disk, but as the template will needed to get loaded anyway at some point, and we have caching... sure, that'd make sense. It's also how the ai determines a pair tech.

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B4225: Diff 4986.Dec 28 2017, 11:42 PM

mimo accepted this revision.Dec 31 2017, 11:00 AM

mimo added inline comments.

binaries/data/mods/public/gui/reference/common/helper.js
146 ↗	(On Diff #4986)	as you wish, but i'd rather keep the symmetry between cuts, so either reqPhasePos == -1 && myPhasePos != -1 or reqPhasePos < 0 && myPhasePos >= 0
148 ↗	(On Diff #4986)	same