Page MenuHomeWildfire Games

Fix GetTechModifiedProperty for string values when the modifications do not apply.
ClosedPublic

Authored by wraitii on Jul 20 2020, 10:46 AM.

Details

Summary

As reported by @wowgetoffyourcellphone, and raised by @Angen following D270 / rP23843.

String-values in modifications fail if the modifications do not apply, as they end up in the "multiply/add" path.

I fix this by splitting in two sub-functions, which lets us report errors more accurately.
I've profiled this to be rather equivalent to our current implementation in terms of speed.


Further, as noted by @Freagarach , I did not allow multiple token replacement (to avoid ordering issues). However, tokens-modifiers are somewhat likely to conflict, and general usage should be sane enough with multiple techs, so I think these can be allowed.

Test Plan

Test that things still work.

I'm somewhat worried that there might still be numeric techs that pass strings, but they should now be easily detected, and running a quick AI-AI game revealed nothing.

Event Timeline

wraitii created this revision.Jul 20 2020, 10:46 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Technologies.js
|  58|  58| 			return modification.replace;
|  59|  59| 		else if (modification.tokens !== undefined)
|  60|  60| 			return HandleTokens(originalValue, modification.tokens);
|  61|    |-		else
|  62|    |-			warn("GetTechModifiedProperty: string modification format not recognised : " + uneval(modification));
|    |  61|+		warn("GetTechModifiedProperty: string modification format not recognised : " + uneval(modification));
|  63|  62| 	}
|  64|  63| 	return originalValue;
|  65|  64| }
|    | [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
| 206| 206| 
| 207| 207| 	case "all":
| 208| 208| 	{
| 209|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 209|+		let civPermitted; // tri-state (undefined, false, or true)
| 210| 210| 		for (let subvalue of value)
| 211| 211| 		{
| 212| 212| 			let newOper = Object.keys(subvalue)[0];

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

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

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

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

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

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

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

Freagarach added inline comments.
binaries/data/mods/public/globalscripts/Technologies.js
59

else can be dropped.

60

Multiple techs adding some tokens is not supported?

wraitii added inline comments.Jul 20 2020, 10:56 AM
binaries/data/mods/public/globalscripts/Technologies.js
60

Hm, good point. It does seem like it should.

wraitii added inline comments.Jul 20 2020, 11:20 AM
binaries/data/mods/public/globalscripts/Technologies.js
60

The trouble with that is that it runs into ordering issue -> the order in which you research tech matters.

This is incidentally why the "multiply/add" stuff was added in the first place for numeric values, but I don't really see an easy way to fix it for tokens unless I were to implement a rather complex tree-system.

As of now, it'll keep the order of research, I believe. This is sane enough I think, so I'll still allow it, but it's a thing to keep in mind.

Freagarach added inline comments.Jul 20 2020, 11:23 AM
binaries/data/mods/public/globalscripts/Technologies.js
60

Yep, I figured that :) A word of caution -- in the code as well as in any documentation -- may be beneficial.

Stan added a subscriber: Stan.Jul 20 2020, 11:27 AM
Stan added inline comments.
binaries/data/mods/public/globalscripts/Technologies.js
60

Will need a test too.

wraitii updated this revision to Diff 12801.Jul 20 2020, 11:28 AM

Allow multiple token replacement (in particular, this lets multiple modifiers add different tokens, which is rather nice).

See above inlines for ordering concerns, but tests & documentation make it somewhat explicit.

wraitii marked 4 inline comments as done.Jul 20 2020, 11:28 AM
wraitii edited the summary of this revision. (Show Details)

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
| 211| 211| 
| 212| 212| 	case "all":
| 213| 213| 	{
| 214|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 214|+		let civPermitted; // tri-state (undefined, false, or true)
| 215| 215| 		for (let subvalue of value)
| 216| 216| 		{
| 217| 217| 			let newOper = Object.keys(subvalue)[0];

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

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

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

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

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

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

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

Silier accepted this revision.Jul 20 2020, 6:53 PM

This is fixing the issue and numeric modifications apply normally.

This revision is now accepted and ready to land.Jul 20 2020, 6:53 PM