Page MenuHomeWildfire Games

Allow technologies to affect tokens.
Needs ReviewPublic

Authored by wraitii on Mar 28 2017, 4:47 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Splitting this from D266.

This adds a new mode to technology modifications called "tokens" which allows clever token parsing. Potential use cases are:

-Swapping trainable units without having to show all potential units from the start (which can clutter the GUI massively if you have enough upgrades). Could potentially be extended for triggers (not sure if we plan on having triggers allowed to change the sim templates?)
-Likewise with researches and build able entities
-Adding hard/soft counters to units through technology.
-Adding/Removing auras to units by technology.
-Changing Garrisonable entities by technology
-Changing Healable classes by technology.

As it stands now, only ProductionQueue handles it, but it would be quite easy to extend on a per-component basis.

While this has no immediate use in 0 A.D. proper, it would be a useful feature for mods, and I am working on a gameplay revamp which could really use it (as I'm upgrading a lot more units).
It would also allow us to get rid of the Promotion hack for upgrading, in combination with a way to upgrade existing entities (see d266 for that).

Changing garrison able tokens is on the list of desired technology effects, which this would help with (obviously there are other issues with changing garrisoned tokens that would need to be handled).

Test Plan

Test and review.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
D270_modifier_tokens
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7603
Build 12398: Vulcan BuildJenkins
Build 12397: arc lint + arc unit

Unit TestsFailed

TimeTest
0 msJenkins > cxxtest_debug.xml::[failed-to-read]
Failed to read test report file /mnt/data/jenkins-phabricator/workspace/differential/cxxtest_debug.xml org.dom4j.DocumentException: Error on line 1 of document : Content is not allowed in prolog. Nested exception: Content is not allowed in prolog. at org.dom4j.io.SAXReader.read(SAXReader.java:482)
0 msJenkins > TestAllocators::test_da
0 msJenkins > TestAtlasObjectXML::test_parse_attributes1
0 msJenkins > TestAtlasObjectXML::test_parse_attributes2
0 msJenkins > TestAtlasObjectXML::test_parse_basic
View Full Test Results (1 Failed · 318 Passed)

Event Timeline

wraitii created this revision.Mar 28 2017, 4:47 PM
Owners added a subscriber: Restricted Owners Package.Mar 28 2017, 4:47 PM
Vulcan added a subscriber: Vulcan.Mar 28 2017, 5:32 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/625/ for more details.

mimo added a subscriber: mimo.Mar 28 2017, 10:19 PM

I don't know if we should have that (seems to me like looking for troubles), but anyway this patch will currently break the ai which would need some changes.

Mh, is it because the AI only gets the templates once at the start?

Re the feature itself, this could also be used to implement the spartan team bonus, which grants its teammates access to a specific unit.

mimo added a comment.Mar 28 2017, 11:26 PM
In D270#10404, @wraitii wrote:

Mh, is it because the AI only gets the templates once at the start?

Yes, it would need at least to update the buildableEntities and trainableEntities functions in common-api/entity.js
or directly the get function in the same file.

I think there are better way to upgrade Hypaspists to Silvershields, for instance use the Upgrade feature, and a tech unlocking this.

But regardless of that detail, the feature wraitii propose would make an exacellent addition to the game and open many possibilities for mods to extend the gameplay. Specifically, I like the idea of adding hard and soft counters via techs. I have designed a pair of techs for the Mauryans: poison arrows, which I have a TODO to make it give archers additional bonus vs. infantry, paired with a steel arrows tech which would give archers a bonus vs. cavalry and elephants. Though, I can see using the Upgrade feature to swap between steel arrows and poison arrows using templates, which means you can swap back to the other arrow type -- see my Mauryan Maiden Archers in Delenda Est. Not sure what way would be preferred or if a whole new "secondary attack" feature would be more desirable. Using the Upgrade feature is admittedly hacky, but works.

I can see a tech that allows ships to transport cavalry, which was a big deal in ancient times. Done by adding the 'Cavalry' token to the garrisonable classes. There are many possibilities. Thanks wraitii for pushing the gameplay.

wraitii planned changes to this revision.Mar 29 2017, 11:39 AM

@mimo: ok, that sounds easy enough to change I guess.

Re this feature, I now share the opinion that techs are a little dangerous to change, because their implementation is poorer than I expected tbh. For example "replace" is completely buggy, since it returns on the first modification, which I had not realised was a problem (turns out we don't have tests for technology effects, only for technology triggers).

I think I'm just going to rewrite this all into a "ModifiersManager" class ideally called by both auras and technologies and anything else, that would handle temporaries as well as permanent modifiers in a centralised manner, supporting orders and a few other things correctly. Should not actually be too difficult, though obviously it might introduce new bugs. So stand-by.

wraitii updated this revision to Diff 8141.EditedMay 25 2019, 8:48 PM

I think this is worth having.

Still doesn't work with the AI, need to fix that.

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
|  48|  48| 
|  49|  49| /**
|  50|  50|  * Derives modifications (to be applied to entities) from a given technology.
|  51|    |- * 
|    |  51|+ *
|  52|  52|  * @param {Object} techTemplate - The technology template to derive the modifications from.
|  53|  53|  * @return {Object} containing the relevant modifications.
|  54|  54|  */
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
| 132| 132| 	let newTokens = modification.split(/\s+/);
| 133| 133| 
| 134| 134| 	for (let token of newTokens)
| 135|    |-	{
|    | 135|+	
| 136| 136| 		if (token.search(">") !== -1)
| 137| 137| 		{
| 138| 138| 			let [oldToken, newToken] = token.split(">");
| 148| 148| 		}
| 149| 149| 		else
| 150| 150| 			tokens.push(token);
| 151|    |-	}
|    | 151|+	
| 152| 152| 
| 153| 153| 	return tokens.join(" ");
| 154| 154| }
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
| 144| 144| 		{
| 145| 145| 			let index = tokens.indexOf(token.substr(1));
| 146| 146| 			if (index !== -1)
| 147|    |-				tokens.splice(index,1);
|    | 147|+				tokens.splice(index, 1);
| 148| 148| 		}
| 149| 149| 		else
| 150| 150| 			tokens.push(token);
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'civPermitted' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/Technologies.js
| 257| 257| 
| 258| 258| 	case "all":
| 259| 259| 	{
| 260|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 260|+		let civPermitted; // tri-state (undefined, false, or true)
| 261| 261| 		for (let subvalue of value)
| 262| 262| 		{
| 263| 263| 			let newOper = Object.keys(subvalue)[0];

binaries/data/mods/public/globalscripts/Technologies.js
| 267| »   »   »   switch·(newOper)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/globalscripts/Technologies.js
| 341| »   »   »   switch·(newOper)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/globalscripts/Technologies.js
| 260| »   »   let·civPermitted·=·undefined;·//·tri-state·(undefined,·false,·or·true)
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'civPermitted' to 'undefined'.

binaries/data/mods/public/globalscripts/Technologies.js
| 283| »   »   »   »   »   return·false;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/globalscripts/Technologies.js
| 293| »   »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/globalscripts/Technologies.js
| 362| »   »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/globalscripts/Technologies.js
| 368| »   »   »   »   civPermitted·=·true;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 474| 474| {
| 475| 475| 	var out = [];
| 476| 476| 	for (var item of this.queue)
| 477|    |-	{
|    | 477|+	
| 478| 478| 		out.push({
| 479| 479| 			"id": item.id,
| 480| 480| 			"unitTemplate": item.unitTemplate,
| 485| 485| 			"timeRemaining": item.timeRemaining,
| 486| 486| 			"metadata": item.metadata,
| 487| 487| 		});
| 488|    |-	}
|    | 488|+	
| 489| 489| 	return out;
| 490| 490| };
| 491| 491| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 567| 567| 	var spawnedEnts = [];
| 568| 568| 
| 569| 569| 	if (this.entityCache.length == 0)
| 570|    |-	{
|    | 570|+	
| 571| 571| 		// We need entities to test spawning, but we don't want to waste resources,
| 572| 572| 		//	so only create them once and use as needed
| 573| 573| 		for (var i = 0; i < count; ++i)
| 586| 586| 				cmpPlayerEntityLimits.ChangeCount(unitCategory, -1);
| 587| 587| 			}
| 588| 588| 		}
| 589|    |-	}
|    | 589|+	
| 590| 590| 
| 591| 591| 	let cmpAutoGarrison;
| 592| 592| 	if (cmpRallyPoint)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 647| 647| 	}
| 648| 648| 
| 649| 649| 	if (spawnedEnts.length > 0 && !cmpAutoGarrison)
| 650|    |-	{
|    | 650|+	
| 651| 651| 		// If a rally point is set, walk towards it (in formation) using a suitable command based on where the
| 652| 652| 		// rally point is placed.
| 653| 653| 		if (cmpRallyPoint)
| 660| 660| 					ProcessCommand(cmpOwnership.GetOwner(), com);
| 661| 661| 			}
| 662| 662| 		}
| 663|    |-	}
|    | 663|+	
| 664| 664| 
| 665| 665| 	if (createdEnts.length > 0)
| 666| 666| 		Engine.PostMessage(this.entity, MT_TrainingFinished, {

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 345| »   »   »   let·template·=·TechnologyTemplates.Get(templateName);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 347| »   »   »   let·time·=·techCostMultiplier.time·*·template.researchTime·*·cmpPlayer.GetTimeMultiplier();
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'time' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 599| »   for·(let·i·=·0;·i·<·count;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 601| »   »   let·ent·=·this.entityCache[0];
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 784| »   »   »   let·template·=·TechnologyTemplates.Get(item.technologyTemplate);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 192| »   for·(var·i·in·techList)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 194| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 204| »   for·(var·i·=·0;·i·<·techList.length;·i++)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 206| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 376| »   »   »   var·cmpTrigger·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Trigger);
|    | [NORMAL] JSHintBear:
|    | 'cmpTrigger' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 770| »   »   »   »   »   var·cmpPlayer·=·QueryOwnerInterface(this.entity);
|    | [NORMAL] JSHintBear:
|    | 'cmpPlayer' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 781| »   »   »   var·cmpTechnologyManager·=·QueryOwnerInterface(this.entity,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
| 473| 473| 		g_Selection.reset();
| 474| 474| 		g_Selection.addList(selection, false, cmd.type == "gather");
| 475| 475| 	},
| 476|    |-	"play-tracks": function (notification, player)
|    | 476|+	"play-tracks": function(notification, player)
| 477| 477| 	{
| 478| 478| 		if (notification.lock)
| 479| 479| 		{
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
| 580| 580| 	let notificationText =
| 581| 581| 		notification.instructions.reduce((instructions, item) =>
| 582| 582| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 583|    |-			"");
|    | 583|+		"");
| 584| 584| 
| 585| 585| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 586| 586| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/messages.js
|1088|1088| 
|1089|1089| 	let message = "";
|1090|1090| 	if (notifyPhase == "all")
|1091|    |-	{
|    |1091|+	
|1092|1092| 		if (msg.phaseState == "started")
|1093|1093| 			message = translate("%(player)s is advancing to the %(phaseName)s.");
|1094|1094| 		else if (msg.phaseState == "aborted")
|1095|1095| 			message = translate("The %(phaseName)s of %(player)s has been aborted.");
|1096|    |-	}
|    |1096|+	
|1097|1097| 	if (msg.phaseState == "completed")
|1098|1098| 		message = translate("%(player)s has reached the %(phaseName)s.");
|1099|1099| 
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1497/display/redirect

wraitii added a reviewer: Restricted Owners Package.Wed, Jun 12, 10:33 PM