Details

Reviewers

elexis

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP20203: Move damage types definition to a new helper similar to how resources are…

Summary

This should make adding new damage types a little easier.

I guess getting both of these (schema, and available types) from some helper would be a possible extension (similar to how we handle resources now (after @s0600204's resources work was merged).

Test Plan

Test that nothing broke. If someone wants to give adding a new damage type a go, I'm not going to stop anyone.

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

• leper created this revision.Sep 3 2017, 4:14 AM

Build is green

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

http://jenkins-master:8080/job/phabricator/1956/ for more details.

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

binaries/data/mods/public/simulation/components/Armour.js
|  28| »   »   »   "</interleave>"·+
|    | [NORMAL] ESLintBear (no-unused-expressions):
|    | Expected an assignment or function call and instead saw an expression.

binaries/data/mods/public/simulation/components/Armour.js
|  30| »   "</optional>";
|    | [NORMAL] JSHintBear:
|    | Expected an assignment or function call and instead saw an expression.

binaries/data/mods/public/simulation/components/Attack.js
| 197| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 237| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 532| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 584| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/473/ for more details.

Harbormaster completed remote builds in B2986: Diff 3469.Sep 3 2017, 5:08 AM

Completeness:
There's also DeathDamage.js having the same hardcoding.

	return {
		"hack": applyMods("Hack"),
		"pierce": applyMods("Pierce"),
		"crush": applyMods("Crush")
	};

GetTemplateDataHelper of globalscripts/Templates.js contains the following duplication when parsing the Attack and Armour template.

	"hack": getAttackStat("Hack"),
	"pierce": getAttackStat("Pierce"),
	"crush": getAttackStat("Crush"),

The AI also has to be considered in entity.js (and entityExtend.js):

	armourStrengths: function() {
		if (!this.get("Armour"))
			return undefined;

		return {
			hack: +this.get("Armour/Hack"),
			pierce: +this.get("Armour/Pierce"),
			crush: +this.get("Armour/Crush")
		};
	},
	attackStrengths: function(type) {
		if (!this.get("Attack/" + type +""))
			return undefined;

		return {
			hack: +(this.get("Attack/" + type + "/Hack") || 0),
			pierce: +(this.get("Attack/" + type + "/Pierce") || 0),
			crush: +(this.get("Attack/" + type + "/Crush") || 0)
		};
	},

The GUIInterface has this getter, which sounds like it could be moved to component itself to become agnostic:

	let cmpDeathDamage = Engine.QueryInterface(ent, IID_DeathDamage);
	if (cmpDeathDamage)
		ret.deathDeath = {
			"hack": cmpDeathDamage.GetDeathDamageStrengths("hack"),
			"pierce": cmpDeathDamage.GetDeathDamageStrengths("pierce"),
			"crush": cmpDeathDamage.GetDeathDamageStrengths("crush")
	};

common/tooltips.js is the only GUI part knowing about damage types:

var g_DamageTypes = {
	"hack": translate("Hack"),
	"pierce": translate("Pierce"),
	"crush": translate("Crush"),
};

Other than that, no occurrences of damage types afaics.

Correctness:
Those globals are fishy. They overwrite each other (so if they dont coincidentally have the same value, I expect the other place to not use the intended value). Also using globals sounds like breaking the idea of grouping everything in the prototype.
The toLowerCase() should be avoided somehow. Should have only one identifier globally.

(schema, and available types) from some helper would be a possible extension (similar to how we handle resources now (after @s0600204's resources work was merged).

That. The patch would be much more complete if it would unify the remaining duplication. (s0600204 committed 1649 lines on 21 days, other contributor 455 loc on 15 days that is)

Patch reads correct and the tests fatherbushido had written still pass.
So nuking the remaining duplication with the GUI and simulation could be done separately if one doesn't want to continue working on it here or squash things from github branch.

• leper mentioned this in D868: Cleanup of damage type related code in cmpDeathDamage.Sep 4 2017, 2:13 AM

Use a DamageTypes object, similar to how we handle resources.

This doesn't take care of the AI (yet?), the rest should be working.
Also does not load the damage types from files yet.
The use of .toLowerCase() is ugly, but unless we want to change things in the gui to use "Hack" as object property names, I see no way around that. One could use another function that returns pairs so one has ["hack", "Hack"], but that doesn't seem nicer.

Should be mostly independent of D865 and D868, however without those actually making things moddable is a bit harder.

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

binaries/data/mods/public/simulation/components/tests/test_Attack.js
| 167| »   attackComponentTest(className,·true,·(attacker,·cmpAttack,·defender)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/globalscripts/Templates.js
|  61| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/globalscripts/Templates.js
|  60| »   »   if·(sublist.every(c·=>·(c[0]·==·"!"·&&·classes.indexOf(c.substr(1))·==·-1)
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/globalscripts/Templates.js
| 149| »   »   »   let·getAttackStat·=·function(stat)·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/common/tooltips.js
| 602| »   »   '[/color][/font]'·+·"·"·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/common/tooltips.js
| 435| »   »   let·[rate,·count]·=·types.reduce((sum,·t)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/common/tooltips.js
| 614| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/common/tooltips.js
| 614| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 191| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 231| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 526| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 578| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/476/ for more details.

Build is green

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

http://jenkins-master:8080/job/phabricator/1961/ for more details.

Harbormaster completed remote builds in B2995: Diff 3476.Sep 4 2017, 4:43 AM

elexis awarded a token.Sep 4 2017, 12:11 PM

Nice addition. If you want to work more on this, there are some ideas below. Otherwise I can offer test it and colorize the state of the revision proposal. But according to my investigation, I shouldn't have a reason to complain if this ends up in the code as is.

lowercase:
The toLowerCase could be avoided by exposing the object keys separately as well, like GetCodes of the resources.
But having more than one identifier makes things more complex as needed.
It's not the GUI that's requiring the use of lowercase, but Templates.js (which always needs to be kept in sync with GuiInterface.js, as the tooltips.js functions are fed with the globalscripts template data in the structree and with an entity state ingame).

AI:
Code reads like the AI wouldn't fall apart with the proposed code.
Changing the GUIInterface to use the capitalized identifier implies having to fix the two AI files however.
So AI support and identifier merging could be done separately (depending on the size of the resulting patch and interest).

The two functions in entity.js do said GUIInterface entity state parsing and look like they can just use a loop (as the ones found in the proposed code already).

entityExtend.js
The two for-loops in entityExtend.js are duplication to be merged in a new helper function (that receives the strength object and a multiplier).
That number 3 seems to be the number of damage types, so should be replaced when possible.
Putting the three numbers (0.085, 0.075, 0.065) into the JSON files (similar to the aiAnalysisInfluenceGroup, see rP19012) seems wrong, because it's not a more abstract identifier used by the AI but an AI configuration value (which might depend on the map or AI character some day).
So ideally they should be moved to config.js afaics (someone who knows about it should confirm then).
The warning about an unknown attackStrength contradicts moddability and might have to be replaced with a fallback value.
So it looks like the next train stop is adapting config.js to be easily extensible and modifyable without introducing a copy of the file. (Would be an awesome feature)

binaries/data/mods/public/globalscripts/DamageTypes.js
1 ↗	(On Diff #3476)	(In case someone commits with svn, `svn propset svn:eol-style native filename`)
9 ↗	(On Diff #3476)	Code reads like it's correct internationalization as `"globalscripts/*.js",` is part of `messages.json`. Can be tested by setting the non-default language, as the strings didn't change. The string is located in `public-gui-other.pot` before and after the patch. If the strings are moved to JSON files, then the `translate(variable)` and the `markForTranslation("actual string")` are in different translation resources. But then the translation is still be correct, because it works for the `generic` and `specific` names (that are marked in `public-templates-.pot` but the translated in `public-gui-other.pot`. (So all translation resources must be combined somewhere down the road))
13 ↗	(On Diff #3476)	Could be `this.types = deepfreeze(...)` (this isn't done for the resources as we have to iterate beforehand over there)
binaries/data/mods/public/globalscripts/Templates.js
135 ↗	(On Diff #3476)	The resources object is created in each GUI page querying resources, the simulation uses the simulation helper prototype instead of a global, the AI gets a copy from the GUI Interface upon init (remotely ugly). If we want globals variables (other than functions) in `globalscripts/`, we could either create a new file that only contains variables (but then we would mix things of different logics that should stay grouped), or put that global into the new globalscripts file added here. Whatever we do here should be added later for the resources too.
binaries/data/mods/public/gui/common/tooltips.js
14 ↗	(On Diff #3476)	(`g_ResourceData` is defined in all pages that use it. Perhaps that could become merged too here later) (Parentheses indeed needed according to https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Operators/Operator_Precedence )
207 ↗	(On Diff #3476)	(Ack. Something like `resourceNameFirstWord` introduced by rP19771 is not needed here as we don't have context duplication)
binaries/data/mods/public/simulation/helpers/DamageTypes.js
7 ↗	(On Diff #3476)	(Could `return this.types.reduce((type, schema) => schema + "<element name='"+type+"' a:help='"+type+" "+helptext+"'><ref name='nonNegativeDecimal'/>", "");` if one is into that kind of thing.)

In D866#33862, @elexis wrote:

Nice addition. If you want to work more on this, there are some ideas below. Otherwise I can offer test it and colorize the state of the revision proposal. But according to my investigation, I shouldn't have a reason to complain if this ends up in the code as is.

I guess I'll have one more revision of this or something like that.

lowercase:
The toLowerCase could be avoided by exposing the object keys separately as well, like GetCodes of the resources.
But having more than one identifier makes things more complex as needed.
It's not the GUI that's requiring the use of lowercase, but Templates.js (which always needs to be kept in sync with GuiInterface.js, as the tooltips.js functions are fed with the globalscripts template data in the structree and with an entity state ingame).

Any objection to using .Hack and similar everywhere then? Since none of these seem to be used anywhere else (and we have no icons for these in the GUI).

AI:
Code reads like the AI wouldn't fall apart with the proposed code.

It would once someone adds things (or removes them) though.

But I'm not quite sure how that would be handled best for the AI, since as opposed to resources these properties aren't nicely grouped (though that might be worth exploring if it really makes things easier for that code). @mimo @Sandarac any hints or opinions?

binaries/data/mods/public/globalscripts/DamageTypes.js
1 ↗	(On Diff #3476)	(Not me.)
9 ↗	(On Diff #3476)	We load multiple pot files, but every string should only have a single translation (there is some config setting that warns if that isn't the case; we should probably also do such checks in the translation linting pass @Itms). So it doesn't really matter where they end up if things are handled properly on the translation side. (And last I checked translators will either have the strings (if unchanged) prefilled, or at least suggested by Transifex.)
13 ↗	(On Diff #3476)	This mimics the resources code, and if (when?) we move damage types to data files we might need that iteration too, so having the code like this seems nicer for future extensions.
binaries/data/mods/public/globalscripts/Templates.js
135 ↗	(On Diff #3476)	The simulation uses a global that is named the same as the helper, see the last line of that helper file (same for resources). It just seems wasteful and pointless to create that object each time we want information about a single template. I guess we should pass `damageTypes` just like we pass `resources` to this function and call it good enough.
binaries/data/mods/public/gui/common/tooltips.js
14 ↗	(On Diff #3476)	I thought about not doing `.GetNames()` here, but that would require more work for the few users of `g_DamageTypes` below. And this is used only in this file, so well.
binaries/data/mods/public/simulation/helpers/DamageTypes.js
7 ↗	(On Diff #3476)	hm, maybe.

In D866#34011, @leper wrote:

Any objection to using .Hack and similar everywhere then?

Would be a great final improvement.

Should be everything, except the AI (the changes there are only for consistency and nothing else).

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

binaries/data/mods/public/simulation/ai/petra/entityExtend.js
| 178| »   »   if·(bonus.Classes·&&·bonus.Classes.split(/\s+/).some(cls·=>·!target.hasClass(cls)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/globalscripts/Templates.js
|  61| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/globalscripts/Templates.js
|  60| »   »   if·(sublist.every(c·=>·(c[0]·==·"!"·&&·classes.indexOf(c.substr(1))·==·-1)
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/globalscripts/Templates.js
| 147| »   »   »   let·getAttackStat·=·function(stat)·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 167| »   »   »   let·w·=·+right["@x"]·+·+right["@width"]/2·-·+left["@x"]·+·+left["@width"]/2;
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 167| »   »   »   let·w·=·+right["@x"]·+·+right["@width"]/2·-·+left["@x"]·+·+left["@width"]/2;
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 168| »   »   »   let·h·=·Math.max(+right["@z"]·+·+right["@depth"]/2,·+left["@z"]·+·+left["@depth"]/2)
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 168| »   »   »   let·h·=·Math.max(+right["@z"]·+·+right["@depth"]/2,·+left["@z"]·+·+left["@depth"]/2)
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/ai/common-api/entity.js
| 169| »   »   »   ······-·Math.min(+right["@z"]·-·+right["@depth"]/2,·+left["@z"]·-·+left["@depth"]/2);
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '-'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Attack.js
| 191| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 231| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 525| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 577| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 209| 209| {
| 210| 210| 	if (this.miragedEntities[player])
| 211| 211| 		return this.renamedEntities.concat(this.miragedEntities[player]);
| 212|    |-	else
| 213|    |-		return this.renamedEntities;
|    | 212|+	return this.renamedEntities;
| 214| 213| };
| 215| 214| 
| 216| 215| GuiInterface.prototype.ClearRenamedEntities = function()
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|1236|1236|

http://jenkins-master:8080/job/phabricator_lint/484/ for more details.

Build is green

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

http://jenkins-master:8080/job/phabricator/1969/ for more details.

Harbormaster completed remote builds in B3012: Diff 3494.Sep 5 2017, 6:03 AM

Stan added a subscriber: Stan.Sep 12 2017, 3:17 PM

Stan added inline comments.

binaries/data/mods/public/simulation/ai/common-api/entity.js
222 ↗	(On Diff #3494)	Shouldn't those have quotes ? obj { "property": value, "property2": value };

O2: JS Simulation guys, is there an actual reviewer to (review) and accept it?
(just asking to not was review resources)

Code and translation still look correct and complete (All 10 occurrences of GetTemplateDataHelper calls modified).
Quickly tested in an AI match and it didn't complain.
Good that this was done before we have added many more magic dupes!

binaries/data/mods/public/globalscripts/DamageTypes.js
5 ↗	(On Diff #3494)	trailing whitespace according to my local linter
8 ↗	(On Diff #3494)	(Perhaps a context "damage type" in case someone considers "Hack" ambiguous)
9 ↗	(On Diff #3494)	Will be a fruitful follow-up: `pierce.json`: { "Title": "Pierce", "Description": "Hurts a lot" } let path = "simulation/data/damage_types/"; Engine.BuildDirEntList(path, "*.json", false) Engine.ReadJSONFile(path + "type.json"); translateObjectKeys(json.Data, keyContext);
binaries/data/mods/public/globalscripts/Templates.js
179 ↗	(On Diff #3494)	(wouldn't complain if there were spaces around operators everywhere)
binaries/data/mods/public/simulation/ai/common-api/entity.js
222 ↗	(On Diff #3494)	(would agree. mimo changes these when happening to change the surrounding code. perhaps we could give out a free quote fix commit some day)
binaries/data/mods/public/simulation/components/tests/test_Attack.js
151 ↗	(On Diff #3494)	(first paragraph has trailing zeroes, second doesn't, idc though)
164 ↗	(On Diff #3494)	(IMO nicer with each property on a separate line)

This revision is now accepted and ready to land.Sep 18 2017, 3:47 PM

You might have to retest this soon.

binaries/data/mods/public/globalscripts/DamageTypes.js
5 ↗	(On Diff #3494)	ack
8 ↗	(On Diff #3494)	Probably not a bad idea, will need a few different calls in other places though.
9 ↗	(On Diff #3494)	Most likely just name/title and order. No need for a description that is never displayed anywhere. Which would add a need for a loop, but then that is an extension, and that extension most likely requires making a few things in a few schemas optional. Also I'm quite certain that calling translateObjectKeys in the simulation (or possibly in there) will end in pain.
binaries/data/mods/public/simulation/ai/common-api/entity.js
222 ↗	(On Diff #3494)	Can add them here, but the whole file is inconsistent, and adding them makes the nearby code inconsistent.

Use a translation context, adjust whitespace in a few places, quote some things.

Harbormaster completed remote builds in B3213: Diff 3715.Sep 18 2017, 5:20 PM

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

binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js
| 100| »   "ApplyModificationsTemplate":·(valueName,·curValue,·template)·=>·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/globalscripts/Templates.js
|  62| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/globalscripts/Templates.js
|  61| »   »   if·(sublist.every(c·=>·(c[0]·==·"!"·&&·classes.indexOf(c.substr(1))·==·-1)
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/globalscripts/Templates.js
| 148| »   »   »   let·getAttackStat·=·function(stat)·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 209| 209| {
| 210| 210| 	if (this.miragedEntities[player])
| 211| 211| 		return this.renamedEntities.concat(this.miragedEntities[player]);
| 212|    |-	else
| 213|    |-		return this.renamedEntities;
|    | 212|+	return this.renamedEntities;
| 214| 213| };
| 215| 214| 
| 216| 215| GuiInterface.prototype.ClearRenamedEntities = function()
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|1236|1236| 
|1237|1237| 		return false;
|1238|1238| 	}
|1239|    |-	else
|1240|    |-	{
|    |1239|+	
|1241|1240| 		// Move all existing cached entities outside of the world and reset their use count
|1242|1241| 		for (let tpl in this.placementWallEntities)
|1243|1242| 		{
|1271|1270| 				}
|1272|1271| 			}
|1273|1272| 		}
|1274|    |-	}
|    |1273|+	
|1275|1274| 
|1276|1275| 	// prevent division by zero errors further on if the start and end positions are the same
|1277|1276| 	if (end.pos && (start.pos.x === end.pos.x && start.pos.z === end.pos.z))
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|2038|2038| {
|2039|2039| 	if (exposedFunctions[name])
|2040|2040| 		return this[name](player, args);
|2041|    |-	else
|2042|    |-		throw new Error("Invalid GuiInterface Call name \""+name+"\"");
|    |2041|+	throw new Error("Invalid GuiInterface Call name \""+name+"\"");
|2043|2042| };
|2044|2043| 
|2045|2044| Engine.RegisterSystemComponentType(IID_GuiInterface, "GuiInterface", GuiInterface);

binaries/data/mods/public/simulation/components/GuiInterface.js
| 664| »   for·(let·name·of·auraNames)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'name' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/GuiInterface.js
| 664| »   for·(let·name·of·auraNames)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'name' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/GuiInterface.js
| 764| »   if·(notification.players·==·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'undefined'.

binaries/data/mods/public/simulation/components/GuiInterface.js
|1032| »   »   »   »

http://jenkins-master:8080/job/phabricator_lint/536/ for more details.

elexis added inline comments.Sep 18 2017, 5:23 PM

binaries/data/mods/public/gui/common/tooltips.js
207 ↗	(On Diff #3476)	^

• leper added inline comments.Sep 18 2017, 5:24 PM

binaries/data/mods/public/gui/common/tooltips.js
207 ↗	(On Diff #3476)	?

elexis added inline comments.Sep 18 2017, 5:39 PM

binaries/data/mods/public/gui/common/tooltips.js
207 ↗	(On Diff #3476)	Now the premise of that statement (missing context duplication) is false, so the conclusion became relevant (at least for the long-term support of this feature. Right now there are only two calls, but its not unlikely that more places will show these strings.). Anyone (including modders) wanting to access the translated string is exposed to forgetting the context or adding typos. The getter is duplicated too, so it will become easier to the reader if the getter is abstracted too. Just as in that commit for resources. `unitFont(translateDamageType(dmgType))` function translateDamageType(type) { return translateWithContext("damage type", g_DamageTypes.GetNames()[dmgType]); } At the time wondered too if it shouldn't go to `globalscripts/` to be grouped, but indeed sim access to that is bad. Another option is `common/gui/l10n.js`, but since that is more an interface to `JSInterface_l10n.cpp` I wasn't too happy about that place either and just used the only place as of now refering to the strings. (Can be done by someone else in some other commit)

Build is green

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

http://jenkins-master:8080/job/phabricator/2052/ for more details.

Harbormaster completed remote builds in B3213: Diff 3715.Sep 18 2017, 6:11 PM

• leper added inline comments.Sep 18 2017, 6:18 PM

binaries/data/mods/public/gui/common/tooltips.js
207 ↗	(On Diff #3476)	Ah, and here I was thinking you wanted something like the FirstWord or WithinSentence thing. There are two uses, and modders that want to access them should use the tooltips, not change the code. If they change the code and don't take care a missing context isn't the only thing that's going to be missed. (Happy with leaving this for someone else who considers this an issue right now.)

Here the latest quick AI + nonAI match test

commands.txt35 KBDownload

so that we can see in hindsight how I missed the random hidden bug when testing.

Closed by commit rP20203: Move damage types definition to a new helper similar to how resources are… (authored by • leper). · Explain WhySep 18 2017, 6:34 PM

This revision was automatically updated to reflect the committed changes.

Factor out damage types in Attack.js and Armour.js
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 3716

ps/trunk/binaries/data/mods/public/globalscripts/DamageTypes.js

ps/trunk/binaries/data/mods/public/globalscripts/Templates.js

ps/trunk/binaries/data/mods/public/gui/common/tooltips.js

ps/trunk/binaries/data/mods/public/gui/reference/common/helper.js

ps/trunk/binaries/data/mods/public/gui/reference/common/load.js

ps/trunk/binaries/data/mods/public/simulation/ai/common-api/entity.js

ps/trunk/binaries/data/mods/public/simulation/ai/petra/entityExtend.js

ps/trunk/binaries/data/mods/public/simulation/components/Armour.js

ps/trunk/binaries/data/mods/public/simulation/components/Attack.js

ps/trunk/binaries/data/mods/public/simulation/components/DeathDamage.js

ps/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_Attack.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js

ps/trunk/binaries/data/mods/public/simulation/helpers/DamageTypes.js

Factor out damage types in Attack.js and Armour.jsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3716

ps/trunk/binaries/data/mods/public/globalscripts/DamageTypes.js

ps/trunk/binaries/data/mods/public/globalscripts/Templates.js

ps/trunk/binaries/data/mods/public/gui/common/tooltips.js

ps/trunk/binaries/data/mods/public/gui/reference/common/helper.js

ps/trunk/binaries/data/mods/public/gui/reference/common/load.js

ps/trunk/binaries/data/mods/public/simulation/ai/common-api/entity.js

ps/trunk/binaries/data/mods/public/simulation/ai/petra/entityExtend.js

ps/trunk/binaries/data/mods/public/simulation/components/Armour.js

ps/trunk/binaries/data/mods/public/simulation/components/Attack.js

ps/trunk/binaries/data/mods/public/simulation/components/DeathDamage.js

ps/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_Attack.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_Damage.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_DeathDamage.js

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_UpgradeModification.js

ps/trunk/binaries/data/mods/public/simulation/helpers/DamageTypes.js

Factor out damage types in Attack.js and Armour.js
ClosedPublic
Actions

Revision Contents
Changeset List