Page MenuHomeWildfire Games

Fix issue with D274/rP22767 - modifications are a list of effects, not a single effect
ClosedPublic

Authored by wraitii on Sep 22 2019, 11:19 AM.

Details

Summary

/me somehow failed to realise that at any point during development.

D274 introduced a silent change that modifications would onlyy have one effect per value-path, which was broken.

This changes that back.

Detected following the comments from SonarQube reported here: https://code.wildfiregames.com/rP22767#inline-4214

Test Plan

Test a tech/aura with multiple effects (not sure if there actually are any?)

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

wraitii created this revision.Sep 22 2019, 11:19 AM

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

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

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

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

elexis added a subscriber: elexis.Sep 22 2019, 11:36 AM
elexis added inline comments.
binaries/data/mods/public/simulation/components/Auras.js
460 ↗(On Diff #9904)

Might be more performant when passing the array of entities, (at least it would make it 1 function call instead of N function calls)

binaries/data/mods/public/simulation/components/ModifiersManager.js
109 ↗(On Diff #9904)

Only use x => y when you intend to return the return value of y, but not when you want to execute y as a function regardless of return value.
Use x => { y; } for the ones where y should be a statement. (I got a concern for that once and it seems correct )

The inner loop can be avoided with a concat call it seems.

binaries/data/mods/public/simulation/components/TechnologyManager.js
224 ↗(On Diff #9904)

(derivedModifiers can be inlined)

wraitii updated this revision to Diff 9909.Sep 22 2019, 12:17 PM
wraitii edited the summary of this revision. (Show Details)

Elexis comments, and hopefully jenkins now works since the latest autobuild contains prereqs changes

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

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

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

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

binaries/data/mods/public/simulation/helpers/Cheat.js
| 110| »   »   var·cmpTechnologyManager·=·Engine.QueryInterface(playerEnt,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.

binaries/data/mods/public/simulation/helpers/Cheat.js
| 117| »   »   »   var·cmpProductionQueue·=·Engine.QueryInterface(input.selected[0],·IID_ProductionQueue);
|    | [NORMAL] JSHintBear:
|    | 'cmpProductionQueue' is already defined.
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|  39|  39| 	for (let key of this.unresearchedAutoResearchTechs)
|  40|  40| 	{
|  41|  41| 		let tech = TechnologyTemplates.Get(key);
|  42|    |-		if ((tech.autoResearch && this.CanResearch(key))
|  43|    |-			|| (tech.top && (this.IsTechnologyResearched(tech.top) || this.IsTechnologyResearched(tech.bottom))))
|    |  42|+		if ((tech.autoResearch && this.CanResearch(key)) ||
|    |  43|+			(tech.top && (this.IsTechnologyResearched(tech.top) || this.IsTechnologyResearched(tech.bottom))))
|  44|  44| 		{
|  45|  45| 			this.unresearchedAutoResearchTechs.delete(key);
|  46|  46| 			this.ResearchTechnology(key);
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|  50|  50| };
|  51|  51| 
|  52|  52| // Checks an entity template to see if its technology requirements have been met
|  53|    |-TechnologyManager.prototype.CanProduce = function (templateName)
|    |  53|+TechnologyManager.prototype.CanProduce = function(templateName)
|  54|  54| {
|  55|  55| 	var cmpTempManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager);
|  56|  56| 	var template = cmpTempManager.GetTemplate(templateName);
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 260| 260| 		cmpPlayerEntityLimits.UpdateLimitsFromTech(tech);
| 261| 261| 
| 262| 262| 	// always send research finished message
| 263|    |-	Engine.PostMessage(this.entity, MT_ResearchFinished, {"player": playerID, "tech": tech});
|    | 263|+	Engine.PostMessage(this.entity, MT_ResearchFinished, { "player": playerID, "tech": tech});
| 264| 264| };
| 265| 265| 
| 266| 266| /**
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 260| 260| 		cmpPlayerEntityLimits.UpdateLimitsFromTech(tech);
| 261| 261| 
| 262| 262| 	// always send research finished message
| 263|    |-	Engine.PostMessage(this.entity, MT_ResearchFinished, {"player": playerID, "tech": tech});
|    | 263|+	Engine.PostMessage(this.entity, MT_ResearchFinished, {"player": playerID, "tech": tech });
| 264| 264| };
| 265| 265| 
| 266| 266| /**

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 125| »   »   »   switch·(type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 140| »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  43| »   »   »   ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 185| »   »   var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTemplateManager' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 186| »   »   var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 191| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 194| »   »   »   »   var·classes·=·cmpIdentity.GetClassesList();
|    | [NORMAL] JSHintBear:
|    | 'classes' is already defined.
Executing section cli...

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

wraitii added inline comments.Sep 22 2019, 12:48 PM
binaries/data/mods/public/simulation/components/Auras.js
460 ↗(On Diff #9904)

It's still N function calls underneath, and the interface of returning bool on change would be changed, so I'll do this in a later diff.

wraitii updated this revision to Diff 9910.Sep 22 2019, 12:51 PM

One more (y) >> { y; } change. I'm keeping the one that calls ForEach because it's not really more readable and the linter is fine with it.

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

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

elexis added inline comments.Sep 22 2019, 12:57 PM
binaries/data/mods/public/simulation/components/Auras.js
460 ↗(On Diff #9904)

It's still N function calls underneath

Not sure how this changes anything. its 2 * entCount function calls, and one can make it 1*entCount calls by passing the entity array and looping inside the called function instead of outside of it.

the interface of returning bool

(Wondering if it wouldnt be faster too if it wouldnt return the boolean if that boolean is never used, but I suppose it depends on the most recent JIT.)

I'll do this in a later diff

Your choice, but "will" sounds like it won't be forgotton :p

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|  39|  39| 	for (let key of this.unresearchedAutoResearchTechs)
|  40|  40| 	{
|  41|  41| 		let tech = TechnologyTemplates.Get(key);
|  42|    |-		if ((tech.autoResearch && this.CanResearch(key))
|  43|    |-			|| (tech.top && (this.IsTechnologyResearched(tech.top) || this.IsTechnologyResearched(tech.bottom))))
|    |  42|+		if ((tech.autoResearch && this.CanResearch(key)) ||
|    |  43|+			(tech.top && (this.IsTechnologyResearched(tech.top) || this.IsTechnologyResearched(tech.bottom))))
|  44|  44| 		{
|  45|  45| 			this.unresearchedAutoResearchTechs.delete(key);
|  46|  46| 			this.ResearchTechnology(key);
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|  50|  50| };
|  51|  51| 
|  52|  52| // Checks an entity template to see if its technology requirements have been met
|  53|    |-TechnologyManager.prototype.CanProduce = function (templateName)
|    |  53|+TechnologyManager.prototype.CanProduce = function(templateName)
|  54|  54| {
|  55|  55| 	var cmpTempManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager);
|  56|  56| 	var template = cmpTempManager.GetTemplate(templateName);
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 260| 260| 		cmpPlayerEntityLimits.UpdateLimitsFromTech(tech);
| 261| 261| 
| 262| 262| 	// always send research finished message
| 263|    |-	Engine.PostMessage(this.entity, MT_ResearchFinished, {"player": playerID, "tech": tech});
|    | 263|+	Engine.PostMessage(this.entity, MT_ResearchFinished, { "player": playerID, "tech": tech});
| 264| 264| };
| 265| 265| 
| 266| 266| /**
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 260| 260| 		cmpPlayerEntityLimits.UpdateLimitsFromTech(tech);
| 261| 261| 
| 262| 262| 	// always send research finished message
| 263|    |-	Engine.PostMessage(this.entity, MT_ResearchFinished, {"player": playerID, "tech": tech});
|    | 263|+	Engine.PostMessage(this.entity, MT_ResearchFinished, {"player": playerID, "tech": tech });
| 264| 264| };
| 265| 265| 
| 266| 266| /**

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 125| »   »   »   switch·(type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 140| »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  43| »   »   »   ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 185| »   »   var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTemplateManager' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 186| »   »   var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 191| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 194| »   »   »   »   var·classes·=·cmpIdentity.GetClassesList();
|    | [NORMAL] JSHintBear:
|    | 'classes' is already defined.

binaries/data/mods/public/simulation/helpers/Cheat.js
| 110| »   »   var·cmpTechnologyManager·=·Engine.QueryInterface(playerEnt,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.

binaries/data/mods/public/simulation/helpers/Cheat.js
| 117| »   »   »   var·cmpProductionQueue·=·Engine.QueryInterface(input.selected[0],·IID_ProductionQueue);
|    | [NORMAL] JSHintBear:
|    | 'cmpProductionQueue' is already defined.
Executing section cli...

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

wraitii added inline comments.Sep 22 2019, 1:08 PM
binaries/data/mods/public/simulation/components/Auras.js
460 ↗(On Diff #9904)

From some testing I did, inlining is done fairly well in the JIT so I'm not too concerned.

I have another idea for an optimisation, so I'll do it then.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 22 2019, 2:05 PM
This revision was automatically updated to reflect the committed changes.