Page MenuHomeWildfire Games

Explicitly check for undefined when applying Auras and Technologies.
Needs RevisionPublic

Authored by Freagarach on Mar 1 2020, 8:18 AM.

Details

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

As reported by @UndyingNephalim on the forum a replace by zero does not work for Auras due to zero being falsy in JS.
This fixes that and multiplication by 0, also for Status Effects.

Notice there is no explicit check for addition of 0, because it would not change the actual value.

Test Plan

Verify that using a replace by 0 in a technology throws no warnings nor does multiplying by 0 but rather the expected effect is achieved. Expected in this case would be that the modified value == 0.

Event Timeline

Freagarach created this revision.Mar 1 2020, 8:18 AM
Vulcan added a comment.Mar 1 2020, 8:33 AM

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
| 151| 151| 
| 152| 152| 	case "all":
| 153| 153| 	{
| 154|    |-		let civPermitted = undefined; // tri-state (undefined, false, or true)
|    | 154|+		let civPermitted; // tri-state (undefined, false, or true)
| 155| 155| 		for (let subvalue of value)
| 156| 156| 		{
| 157| 157| 			let newOper = Object.keys(subvalue)[0];

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

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

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

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

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

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

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

Silier added a subscriber: Silier.Mar 1 2020, 9:18 AM
Silier added inline comments.
binaries/data/mods/public/globalscripts/ModificationTemplates.js
167
binaries/data/mods/public/globalscripts/Technologies.js
28
Silier requested changes to this revision.Mar 1 2020, 11:52 AM
Silier added subscribers: bb, elexis.

desired effect by "multiply": 0 sounds like "replace": 0

Also multiply = 0 does not grand 0 as there is still add so i do not think multiply !== undefined is so important and possibly not even the correct way, because one eliminates only multiplication part and author would expect to eliminate whole effect by that. It may be confusing what multiply = 0 exactly does because that I stop this to be changed.

@elexis , @bb what do you think about this ?

This revision now requires changes to proceed.Mar 1 2020, 11:52 AM

Does seem worth having for consistency, even if it's kind of a slower version of "replace" anyways.
Note that multiply: 0 and add: X are not the same as replace: X as replace early-exits and so further techs are still applied.
That in itself could be seen as a bug, as our ordering is only "kinda" consistent.