Page MenuHomeWildfire Games

Allow Modifiers to affect tokens.
ClosedPublic

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

Details

Reviewers
Freagarach
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23843: Allow Modifiers to affect tokens.
Summary

This adds a new mode to modifications called "tokens" which allows clever token parsing.

Technologies, auras and modifiers in general can use this to switch out, add or delete tokens dynamically.


Implemented use cases:

  • Changing production queue and builder entities at runtime.
    • Could be used for phase upgrades / unit upgrades (fixes #4332).

Future use cases (will be implemented later, as they are trickier):

  • Changing garrisonable entities.
  • Changing healable classes.
  • Changing attackable classes.
  • Changing auras dynamically.
  • Changing Identity stuff
    • Should be extendable to do fancy-pants props switching on tech upgrades, ultimately.
    • That might make it possible to simulate "promotion" without actually promoting, too.
Test Plan

Test and review.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

Had the rather good idea of testing this further, and I noticed that I didn't handle the case where a template being produced was changed.

...

This proved to be impossible to fix cleanly. At the moment, tokens are processed and {civ}/{native} strings replaced very early on. I don't have a way to recover the original token.
I should look into refactoring that, but in the meantime this works.

Also added a test case for this, which ended up being rather useful in debugging this annoying issue.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'civPermitted' to undefined.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
| 181| 181| 
| 182| 182| 	case "all":
| 183| 183| 	{
| 184|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 184|+		let civPermitted; // tri-state (undefined, false, or true)
| 185| 185| 		for (let subvalue of value)
| 186| 186| 		{
| 187| 187| 			let newOper = Object.keys(subvalue)[0];

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 184| »   »   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
| 207| »   »   »   »   »   return·false;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 292| »   »   »   »   civPermitted·=·true;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/gui/session/session.js
| 698| »   »   button.onPress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 699| »   »   button.onDoublePress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 700| »   »   button.onPressRight·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 339| 339| 	let notificationText =
| 340| 340| 		notification.instructions.reduce((instructions, item) =>
| 341| 341| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 342|    |-			"");
|    | 342|+		"");
| 343| 343| 
| 344| 344| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 345| 345| 	g_TutorialMessages.push(notificationText);

binaries/data/mods/public/simulation/components/Player.js
| 346| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 800| »   »   if·(this.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 817| »   »   if·(this.position()·!==·undefined·&&·unitToFleeFrom.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 947| }(API3);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'API3' was used before it was defined.
Executing section cli...

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

wraitii requested review of this revision.Jun 1 2020, 7:15 AM

For the record, given the hack I'm now introducing in ProductionQueue.js, I won't commit this until I either:

  • Get some other dev to look at this and agree that it's painful but we should go along with it
  • Refactor so I can remove the hack.

This might or might not entail removing the "auto-Promotion" business, I am so far unclear on that.

binaries/data/mods/public/globalscripts/Technologies.js
57 ↗(On Diff #12027)

There is a bunch of other code using /\s+/ and some using ' '. I'm leaving this as is, for consistency one should do an actual check someday.

binaries/data/mods/public/simulation/components/AIInterface.js
254 ↗(On Diff #8647)

wouldn't work, since if it was a number we wouldn't need to '+' it.

binaries/data/mods/public/simulation/components/ProductionQueue.js
91 ↗(On Diff #12027)

?

wraitii updated this revision to Diff 12116.Jun 3 2020, 7:03 PM
wraitii edited the summary of this revision. (Show Details)

Turns out removing the hack wasn't so hard.

i'll leave this up for a few more days and commit it, likely over the WE.

Vulcan added a comment.Jun 3 2020, 7:04 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1772/display/redirect

Vulcan added a comment.Jun 3 2020, 7:04 PM

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

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

Freagarach resigned from this revision.Jun 3 2020, 7:18 PM
Freagarach removed a reviewer: Freagarach.
Freagarach added a subscriber: bb.

I don't see myself finding time to review/test this soon ;( Sorry.

binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
23 ↗(On Diff #12116)

One can seperate logical parts in the test using comments instead of creating a function and calling it once (agreeing with @bb here).

wraitii added inline comments.Jun 3 2020, 7:22 PM
binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
23 ↗(On Diff #12116)

Mh, I would disagree on the grounds that this creates some weird global state. I don't really care what "testEntitiesList" does in my own test below.

wraitii updated this revision to Diff 12119.Jun 3 2020, 7:30 PM

Fix tests, I stopped halfway through.

binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
23 ↗(On Diff #12116)

er -> I say that but I keep the global state since things don't get magically cleaned up -__-

Still...

Vulcan added a comment.Jun 3 2020, 7:35 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/session.js
| 698| »   »   button.onPress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 699| »   »   button.onDoublePress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 700| »   »   button.onPressRight·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 800| »   »   if·(this.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 817| »   »   if·(this.position()·!==·undefined·&&·unitToFleeFrom.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 947| }(API3);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'API3' was used before it was defined.
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'civPermitted' to undefined.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
| 181| 181| 
| 182| 182| 	case "all":
| 183| 183| 	{
| 184|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 184|+		let civPermitted; // tri-state (undefined, false, or true)
| 185| 185| 		for (let subvalue of value)
| 186| 186| 		{
| 187| 187| 			let newOper = Object.keys(subvalue)[0];

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 184| »   »   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
| 207| »   »   »   »   »   return·false;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 292| »   »   »   »   civPermitted·=·true;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/simulation/components/Player.js
| 346| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 339| 339| 	let notificationText =
| 340| 340| 		notification.instructions.reduce((instructions, item) =>
| 341| 341| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 342|    |-			"");
|    | 342|+		"");
| 343| 343| 
| 344| 344| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 345| 345| 	g_TutorialMessages.push(notificationText);
Executing section cli...

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

@Freagarach Do you intend to test this again? If so I'll wait, but otherwise I plan to merge this around this WE.

I do intend to, but I'll need to find some time.

Freagarach added a comment.EditedJun 4 2020, 8:05 PM

Upgrade.js also handles tokens, that can be updated as well, or left to a separate patch, since there are more components that can use this.

[EDIT]: As said in the summary,,,

binaries/data/mods/public/globalscripts/Technologies.js
15 ↗(On Diff #11978)

^

binaries/data/mods/public/simulation/components/Builder.js
37 ↗(On Diff #12119)

Why /_string? And not just "Builder/Entities"? (Same for ProductionQueue.)

binaries/data/mods/public/simulation/components/ProductionQueue.js
93–95 ↗(On Diff #12119)

Should be gone, we could want to start with the possibility to produce entities, but only after researching a tech. (So starting with an empty entity list.)

Stan added a subscriber: Krinkle.Jun 4 2020, 8:11 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/ProductionQueue.js
130 ↗(On Diff #12119)

Don't use concat, it's very bad. At least two times slower than a foreach with push... cc @bb

There seem to be a lot of split and join here, maybe it can be rewritten to be faster.

149 ↗(On Diff #12119)

I guess you can't use the one in Identity?

152 ↗(On Diff #12119)

Not sure you need !! unless the value of the array can be falsy?

931 ↗(On Diff #12119)

We should really have a convention for === Maybe @Krinkle has an opinion :)

wraitii added inline comments.Jun 5 2020, 4:39 PM
binaries/data/mods/public/simulation/components/Builder.js
37 ↗(On Diff #12119)

I have a hunch it was useful pre D274, but I can't think of one _now_

binaries/data/mods/public/simulation/components/ProductionQueue.js
93–95 ↗(On Diff #12119)

ah, good point, thanks.

130 ↗(On Diff #12119)

Thanks for the note, I'll rewrite

149 ↗(On Diff #12119)

not sure what you mean? We explicitly want the civilisation of the player here, otherwise it's {native}.

wraitii added inline comments.Jun 5 2020, 5:34 PM
binaries/data/mods/public/simulation/components/Builder.js
37 ↗(On Diff #12119)

Er, it's actually useful for the AI which uses the XML path. I think I'm keeping it as is tbh.

wraitii updated this revision to Diff 12146.Jun 5 2020, 6:28 PM

Fix remarks.

Vulcan added a comment.Jun 5 2020, 6:29 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1793/display/redirect

Vulcan added a comment.Jun 5 2020, 6:29 PM

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

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

wraitii updated this revision to Diff 12148.Jun 5 2020, 6:50 PM

Accidentally broke the patch -__-

Vulcan added a comment.Jun 5 2020, 6:56 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 339| 339| 	let notificationText =
| 340| 340| 		notification.instructions.reduce((instructions, item) =>
| 341| 341| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 342|    |-			"");
|    | 342|+		"");
| 343| 343| 
| 344| 344| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 345| 345| 	g_TutorialMessages.push(notificationText);

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 800| »   »   if·(this.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 817| »   »   if·(this.position()·!==·undefined·&&·unitToFleeFrom.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 947| }(API3);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'API3' was used before it was defined.

binaries/data/mods/public/gui/session/session.js
| 698| »   »   button.onPress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 699| »   »   button.onDoublePress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 700| »   »   button.onPressRight·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'civPermitted' to undefined.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
| 181| 181| 
| 182| 182| 	case "all":
| 183| 183| 	{
| 184|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 184|+		let civPermitted; // tri-state (undefined, false, or true)
| 185| 185| 		for (let subvalue of value)
| 186| 186| 		{
| 187| 187| 			let newOper = Object.keys(subvalue)[0];

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 184| »   »   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
| 207| »   »   »   »   »   return·false;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 292| »   »   »   »   civPermitted·=·true;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/simulation/components/Player.js
| 346| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
Executing section cli...

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

wraitii added inline comments.Jun 6 2020, 8:43 AM
binaries/data/mods/public/simulation/components/ProductionQueue.js
130 ↗(On Diff #12119)

Did a few changes but it seems the tech call is by far the slowest bit.
Seems to take around 30-40µs, which is consequent but this is supposed to be a rare occurrence.

After a while when researching the tech:
WARNING: PlayerID 1 | Petra error in incrementBuilderCounters for structures/stonehenge not yet set

binaries/data/mods/public/simulation/components/ProductionQueue.js
144 ↗(On Diff #12027)

^

134 ↗(On Diff #12148)

(Multiline?)

After a while when researching the tech:
WARNING: PlayerID 1 | Petra error in incrementBuilderCounters for structures/stonehenge not yet set

Would you happen to have a replay of this? I usually get the AI errors that it can't find builders for "house", but that seems just a redundant hardcoding that's out of scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
134 ↗(On Diff #12148)

seems readable like that to me.

Freagarach edited the summary of this revision. (Show Details)Jun 10 2020, 12:21 PM
wraitii updated this revision to Diff 12246.Jun 10 2020, 5:50 PM

Reset the AI builder cache on value modification. This isn't very fast (scales with # of units, but it'll be in the milliseconds range), but I don't think there is really any easier solution.
It affects builder/entities which are less interesting to change than production queue so it should be fine.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'civPermitted' to undefined.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
| 181| 181| 
| 182| 182| 	case "all":
| 183| 183| 	{
| 184|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 184|+		let civPermitted; // tri-state (undefined, false, or true)
| 185| 185| 		for (let subvalue of value)
| 186| 186| 		{
| 187| 187| 			let newOper = Object.keys(subvalue)[0];

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 184| »   »   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
| 207| »   »   »   »   »   return·false;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 292| »   »   »   »   civPermitted·=·true;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/simulation/components/Player.js
| 346| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.

binaries/data/mods/public/gui/session/session.js
| 698| »   »   button.onPress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 699| »   »   button.onDoublePress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 700| »   »   button.onPressRight·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 339| 339| 	let notificationText =
| 340| 340| 		notification.instructions.reduce((instructions, item) =>
| 341| 341| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 342|    |-			"");
|    | 342|+		"");
| 343| 343| 
| 344| 344| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 345| 345| 	g_TutorialMessages.push(notificationText);

binaries/data/mods/public/simulation/ai/petra/buildManager.js
|  93| ····»   let·civ·=·gameState.getPlayerCiv();
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/ai/petra/buildManager.js
|  93| ····»   let·civ·=·gameState.getPlayerCiv();
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'civ' is already declared in the upper scope.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 800| »   »   if·(this.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 817| »   »   if·(this.position()·!==·undefined·&&·unitToFleeFrom.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 947| }(API3);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'API3' was used before it was defined.
Executing section cli...

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

Stan added a comment.Jun 24 2020, 2:05 PM

Another use case of that patch was suggested today on https://linuxfr.org/news/sortie-de-0-a-d-alpha-23-ken-wood-annonce-tardive#comment-1815788 Basically the idea is to create a tech that would remove the field token on polar maps, forcing the users to use corrals.

I'm leaving this one up for further comments for now.

Angen resigned from this revision.Jun 27 2020, 12:31 PM

I think this feature can in some cases mess up ai planning but it is similar if entity would upgrade or get deleted. as far as it will not be used to remove crutial things it ought to be ok (like removing or adding Siege class to units in the battle not sure how that would play out)

binaries/data/mods/public/simulation/components/ProductionQueue.js
943 ↗(On Diff #12246)

what does this mean?

wraitii added inline comments.Jun 27 2020, 12:33 PM
binaries/data/mods/public/simulation/components/ProductionQueue.js
943 ↗(On Diff #12246)

The whole selection is dirtied anytime something may change, we don't actually check if something has changed.

I'll clarify the comment.

One situation where this is useful, even with its drawbacks, is that it lets triggers change production tokens, which would be good for scenarios to enable/disable units.

If only for that reason, I'm now itching towards merging the patch.

Stan added a comment.Jul 14 2020, 12:19 PM

Do you have 100% coverage ? This patch is useful, but we should be on the safer side if we can add more tests :)

My tests cover the different cases I've thought of. I've also tested this rather extensively now, I'm fairly confident there aren't obvious issues.

AI should be able to cope but that's less easy to guarantee at the moment, given the possibly large changes this can lead to.

Freagarach accepted this revision.Jul 16 2020, 5:44 PM
  • Nice feature.
  • Techs apply correctly.
  • PetraAI vs PetraAI goes correctly, even after researching the test-tech.
  • Code looks good.
binaries/data/mods/public/simulation/ai/petra/buildManager.js
86 ↗(On Diff #12246)

+\t

93–95 ↗(On Diff #12246)

+\t

binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
23 ↗(On Diff #12116)

Still...

Yes? :P

This revision is now accepted and ready to land.Jul 16 2020, 5:44 PM
wraitii updated this revision to Diff 12732.Jul 17 2020, 10:03 AM

Final tweaks to comments, letting the CI run before merging.

wraitii added inline comments.Jul 17 2020, 10:07 AM
binaries/data/mods/public/simulation/components/tests/test_ProductionQueue.js
23 ↗(On Diff #12116)

Point is I don't think this is much worse or much better :p
I'm leaving as is

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/session.js
| 698| »   »   button.onPress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 699| »   »   button.onDoublePress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 700| »   »   button.onPressRight·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 339| 339| 	let notificationText =
| 340| 340| 		notification.instructions.reduce((instructions, item) =>
| 341| 341| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 342|    |-			"");
|    | 342|+		"");
| 343| 343| 
| 344| 344| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 345| 345| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/common-api/entity.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/common-api/entity.js
| 766| 766| 			let restrictedClasses = this.get("Attack/" + type + "/RestrictedClasses/_string");
| 767| 767| 			if (!restrictedClasses || !MatchesClassList(target.classes(), restrictedClasses))
| 768| 768| 				return true;
| 769|    |-		};
|    | 769|+		}
| 770| 770| 
| 771| 771| 		return false;
| 772| 772| 	},

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 828| »   »   if·(this.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 845| »   »   if·(this.position()·!==·undefined·&&·unitToFleeFrom.position()·!==·undefined)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 975| }(API3);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'API3' was used before it was defined.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 769| »   »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'civPermitted' to undefined.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
| 181| 181| 
| 182| 182| 	case "all":
| 183| 183| 	{
| 184|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 184|+		let civPermitted; // tri-state (undefined, false, or true)
| 185| 185| 		for (let subvalue of value)
| 186| 186| 		{
| 187| 187| 			let newOper = Object.keys(subvalue)[0];

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 184| »   »   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
| 207| »   »   »   »   »   return·false;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

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

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

binaries/data/mods/public/globalscripts/Technologies.js
| 292| »   »   »   »   civPermitted·=·true;
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'case'.

binaries/data/mods/public/simulation/ai/petra/buildManager.js
|  93| »   »   let·civ·=·gameState.getPlayerCiv();
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'civ' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/Player.js
| 346| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
Executing section cli...

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

This revision was automatically updated to reflect the committed changes.