Page MenuHomeWildfire Games

Cleanup Resource Supply
ClosedPublic

Authored by Stan on Jan 31 2019, 3:57 PM.

Details

Reviewers
Imarok
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22103: Clean ResourceSupply up.
Summary
  • Early returns
  • Doxygen
  • If/else removal
  • This.isInfinite is no longer serialized
Test Plan

Make sure all the tests pass ask for other tests.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6947
Build 11370: Vulcan BuildJenkins

Event Timeline

Stan created this revision.Jan 31 2019, 3:57 PM
Vulcan added a subscriber: Vulcan.Jan 31 2019, 3:58 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|  34|  34| 
|  35|  35| 	// List of IDs for each player
|  36|  36| 	this.gatherers = [];
|  37|    |-	let numPlayers = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers()
|    |  37|+	let numPlayers = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers();
|  38|  38| 	for (let i = 0; i < numPlayers; ++i)
|  39|  39| 		this.gatherers.push([]);
|  40|  40| 
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|  99|  99| {
| 100| 100| 	if (!this.template.DiminishingReturns)
| 101| 101| 		return null;
| 102|    |-	
|    | 102|+
| 103| 103| 	let diminishingReturns = ApplyValueModificationsToEntity("ResourceSupply/DiminishingReturns", +this.template.DiminishingReturns, this.entity);
| 104| 104| 	if (!diminishingReturns)	
| 105| 105| 		return null;
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 101| 101| 		return null;
| 102| 102| 	
| 103| 103| 	let diminishingReturns = ApplyValueModificationsToEntity("ResourceSupply/DiminishingReturns", +this.template.DiminishingReturns, this.entity);
| 104|    |-	if (!diminishingReturns)	
|    | 104|+	if (!diminishingReturns)
| 105| 105| 		return null;
| 106| 106| 
| 107| 107| 	let numGatherers = this.GetNumGatherers();
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 164| 164| ResourceSupply.prototype.RemoveGatherer = function(gathererID, player)
| 165| 165| {
| 166| 166| 	if (player != undefined && player != INVALID_PLAYER)
| 167|    |-	{		
|    | 167|+	{
| 168| 168| 		let index = this.gatherers[player].indexOf(gathererID);
| 169| 169| 		if (index == -1)
| 170| 170| 			return;

binaries/data/mods/public/simulation/components/ResourceSupply.js
|  37| »   let·numPlayers·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_PlayerManager).GetNumPlayers()
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/ResourceSupply.js
| 109| »   »   return·diminishingReturns·==·1·?·1·:·(1.·-·Math.pow(diminishingReturns,·numGatherers))·/·(1.·-·diminishingReturns)·/·numGatherers;
|    | [NORMAL] JSHintBear:
|    | A trailing decimal point can be confused with a dot: '1.'.

binaries/data/mods/public/simulation/components/ResourceSupply.js
| 109| »   »   return·diminishingReturns·==·1·?·1·:·(1.·-·Math.pow(diminishingReturns,·numGatherers))·/·(1.·-·diminishingReturns)·/·numGatherers;
|    | [NORMAL] JSHintBear:
|    | A trailing decimal point can be confused with a dot: '1.'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1022/

Stan updated this revision to Diff 7432.Feb 1 2019, 1:19 AM

Try to fix Vulkan's comments.

Vulcan added a comment.Feb 1 2019, 1:21 AM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1023/

Imarok requested changes to this revision.Mar 3 2019, 4:04 PM
Imarok added a subscriber: Imarok.
Imarok added inline comments.
binaries/data/mods/public/simulation/components/ResourceSupply.js
76

"An object containing both the subtype, and the generic type. All resources must have both."→"An object containing the subtype and the generic type. All resources must have both."?

93

additionnal→additional
twice "rate" sounds wrong.

114

resource→resources?
Maybe "The amount[...]"?

124–125

Why have you added that?

131

I think this comment should be one line lower.

140–141

Aren't those in the wrong order?

141

"The gatherer's id."→"The gatherer's player id."

162

"The gatherer's id."→"The gatherer's player id."

167–181

I would stay with the if else, but remove the return.

This revision now requires changes to proceed.Mar 3 2019, 4:04 PM
Stan updated this revision to Diff 7515.Mar 5 2019, 4:37 PM
Stan marked 9 inline comments as done.

Fix comments.

binaries/data/mods/public/simulation/components/ResourceSupply.js
76

Yes it's better your way !

114

Agreed.

124–125

Typo ^^

Vulcan added a comment.Mar 5 2019, 5:26 PM

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'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js
| 176| 176| 		Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() });
| 177| 177| 		return;
| 178| 178| 	}
| 179|    |-	else
| 180|    |-		for (let i = 0; i < this.gatherers.length; ++i)
|    | 179|+	for (let i = 0; i < this.gatherers.length; ++i)
| 181| 180| 			this.RemoveGatherer(gathererID, i);
| 182| 181| };
| 183| 182| 
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1078/display/redirect

Stan updated this revision to Diff 7516.Mar 5 2019, 5:46 PM
  • Fix vulkan warning
Vulcan added a comment.Mar 5 2019, 5:49 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1079/display/redirect

Rest looks good.

binaries/data/mods/public/simulation/components/ResourceSupply.js
167–180

Imho the way how the original code is structured is more readable.

Stan updated this revision to Diff 7517.Mar 6 2019, 2:44 PM
  • Revert the Remove gatherer to its original state, with early returns.
Stan marked an inline comment as done.Mar 6 2019, 2:45 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/ResourceSupply.js
167–180

Fair enough.

The recursive call could be replaced by this, but not sure its more readable

let playerID = this.gatherers.findIndex(player => player.findIndex(gatherer => gatherer == gathererID) != -1)
this.RemoveGatherer(gathererID, playerID);
Vulcan added a comment.Mar 6 2019, 2:49 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1080/display/redirect

Imarok accepted this revision.Mar 6 2019, 3:55 PM

Nice patch

This revision is now accepted and ready to land.Mar 6 2019, 3:55 PM
Stan marked an inline comment as done.Mar 6 2019, 4:26 PM

Thanks :)

This revision was automatically updated to reflect the committed changes.