Page MenuHomeWildfire Games

ProductionQueue - Cleanup timer
Needs ReviewPublic

Authored by Polakrity on May 18 2019, 4:03 PM.

Details

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

In the current state, the progress of the queue is updated with "Timeout" timers.
Whenever we deduce time to the currently processed item, we declare a new timer.
It is better to use a "Interval" timer to avoid declare/delete timer each time.

Also integrate lateness parameter.
And a little cleanup not directly related.

Test Plan

Test if it works as before.

Diff Detail

Repository
rDD 0 A.D. Design Document
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Polakrity created this revision.May 18 2019, 4:03 PM

The timers can have lateness, should not we include this?

binaries/data/mods/public/simulation/components/ProductionQueue.js
545

Already Timer.js clean timers when entity is no longer present.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 470| 470| {
| 471| 471| 	var out = [];
| 472| 472| 	for (var item of this.queue)
| 473|    |-	{
|    | 473|+	
| 474| 474| 		out.push({
| 475| 475| 			"id": item.id,
| 476| 476| 			"unitTemplate": item.unitTemplate,
| 481| 481| 			"timeRemaining": item.timeRemaining,
| 482| 482| 			"metadata": item.metadata,
| 483| 483| 		});
| 484|    |-	}
|    | 484|+	
| 485| 485| 	return out;
| 486| 486| };
| 487| 487| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 559| 559| 	var spawnedEnts = [];
| 560| 560| 
| 561| 561| 	if (this.entityCache.length == 0)
| 562|    |-	{
|    | 562|+	
| 563| 563| 		// We need entities to test spawning, but we don't want to waste resources,
| 564| 564| 		//	so only create them once and use as needed
| 565| 565| 		for (var i = 0; i < count; ++i)
| 578| 578| 				cmpPlayerEntityLimits.ChangeCount(unitCategory, -1);
| 579| 579| 			}
| 580| 580| 		}
| 581|    |-	}
|    | 581|+	
| 582| 582| 
| 583| 583| 	let cmpAutoGarrison;
| 584| 584| 	if (cmpRallyPoint)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 639| 639| 	}
| 640| 640| 
| 641| 641| 	if (spawnedEnts.length > 0 && !cmpAutoGarrison)
| 642|    |-	{
|    | 642|+	
| 643| 643| 		// If a rally point is set, walk towards it (in formation) using a suitable command based on where the
| 644| 644| 		// rally point is placed.
| 645| 645| 		if (cmpRallyPoint)
| 652| 652| 					ProcessCommand(cmpOwnership.GetOwner(), com);
| 653| 653| 			}
| 654| 654| 		}
| 655|    |-	}
|    | 655|+	
| 656| 656| 
| 657| 657| 	if (createdEnts.length > 0)
| 658| 658| 		Engine.PostMessage(this.entity, MT_TrainingFinished, {

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 347| »   »   »   let·template·=·TechnologyTemplates.Get(templateName);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 349| »   »   »   let·time·=·techCostMultiplier.time·*·template.researchTime·*·cmpPlayer.GetTimeMultiplier();
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'time' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 591| »   for·(let·i·=·0;·i·<·count;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 593| »   »   let·ent·=·this.entityCache[0];
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 677| ·»   //·Allocate·the·1000msecs·to·as·many·queue·items·as·it·takes
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 777| »   »   »   let·template·=·TechnologyTemplates.Get(item.technologyTemplate);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 194| »   for·(var·i·in·techList)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 196| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 206| »   for·(var·i·=·0;·i·<·techList.length;·i++)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 208| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 376| »   »   »   var·cmpTrigger·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Trigger);
|    | [NORMAL] JSHintBear:
|    | 'cmpTrigger' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 763| »   »   »   »   »   var·cmpPlayer·=·QueryOwnerInterface(this.entity);
|    | [NORMAL] JSHintBear:
|    | 'cmpPlayer' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 774| »   »   »   var·cmpTechnologyManager·=·QueryOwnerInterface(this.entity,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.
Executing section cli...

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

Stan added a subscriber: Stan.May 18 2019, 11:15 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/ProductionQueue.js
0–1

Maybe it could be part of the prototype as well.

37

Does it make sense to make it configurable in the template ?
If so it could be a local non all caps variable set in the init function.

676

Yeah maybe it should have the unused lateness parameter.

681

Whitespace .

Polakrity added inline comments.May 18 2019, 11:39 PM
binaries/data/mods/public/simulation/components/ProductionQueue.js
0–1

That's what I thought too but I don't know if I should include it in this diff.

37

Same here too. It was my first idea.
Is there a need to process with times smaller than 1 second?

Stan added inline comments.May 19 2019, 9:17 AM
binaries/data/mods/public/simulation/components/ProductionQueue.js
0–1

I guess you can :)

Angen added a subscriber: Angen.May 19 2019, 11:28 AM
Angen added inline comments.
binaries/data/mods/public/simulation/components/ProductionQueue.js
690–691

let item; before while loop if needs to exists out of while else var -> let

820

ProductionQueue. prototype. StartTimer takes no parameters

825

PauseProduction - > stoptimer - > timer = undefined
unpauseproduction -> paused=false - > startrimer
so this.paused is not needed check?
or did I miss something?

Polakrity updated this revision to Diff 8085.May 19 2019, 7:13 PM
Polakrity edited the summary of this revision. (Show Details)
  • Moves MAX_QUEUE_SIZE into the prototype.
  • Integrates the lateness parameter from Timers.
  • Also fix errors forgotten according to the comments.

This change influences the rounding of time in the GUI because the values of lateness are mostly inferior/superior to 1000msec.
Currently, the rounding seems to be a floored rounding.
I think it is better to round to the nearest integer.

Polakrity marked 2 inline comments as done.May 19 2019, 7:22 PM
Polakrity added inline comments.
binaries/data/mods/public/simulation/components/ProductionQueue.js
543

Idk if this comment is really needed.

690–691

Don't need to declare this outside of loop. But var -> let.

768

Already declared outside the loop.

820

Old code. Thx

825

Same as above.

I called this function in AddBatch() before and I wanted to avoid declaring a new timer in case of queue paused.
But i changed by calling directly ProgressTimeout().
So this check it's useless now. Ty for noticing

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 472| 472| {
| 473| 473| 	var out = [];
| 474| 474| 	for (var item of this.queue)
| 475|    |-	{
|    | 475|+	
| 476| 476| 		out.push({
| 477| 477| 			"id": item.id,
| 478| 478| 			"unitTemplate": item.unitTemplate,
| 483| 483| 			"timeRemaining": item.timeRemaining,
| 484| 484| 			"metadata": item.metadata,
| 485| 485| 		});
| 486|    |-	}
|    | 486|+	
| 487| 487| 	return out;
| 488| 488| };
| 489| 489| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 562| 562| 	var spawnedEnts = [];
| 563| 563| 
| 564| 564| 	if (this.entityCache.length == 0)
| 565|    |-	{
|    | 565|+	
| 566| 566| 		// We need entities to test spawning, but we don't want to waste resources,
| 567| 567| 		//	so only create them once and use as needed
| 568| 568| 		for (var i = 0; i < count; ++i)
| 581| 581| 				cmpPlayerEntityLimits.ChangeCount(unitCategory, -1);
| 582| 582| 			}
| 583| 583| 		}
| 584|    |-	}
|    | 584|+	
| 585| 585| 
| 586| 586| 	let cmpAutoGarrison;
| 587| 587| 	if (cmpRallyPoint)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ProductionQueue.js
| 642| 642| 	}
| 643| 643| 
| 644| 644| 	if (spawnedEnts.length > 0 && !cmpAutoGarrison)
| 645|    |-	{
|    | 645|+	
| 646| 646| 		// If a rally point is set, walk towards it (in formation) using a suitable command based on where the
| 647| 647| 		// rally point is placed.
| 648| 648| 		if (cmpRallyPoint)
| 655| 655| 					ProcessCommand(cmpOwnership.GetOwner(), com);
| 656| 656| 			}
| 657| 657| 		}
| 658|    |-	}
|    | 658|+	
| 659| 659| 
| 660| 660| 	if (createdEnts.length > 0)
| 661| 661| 		Engine.PostMessage(this.entity, MT_TrainingFinished, {

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 349| »   »   »   let·template·=·TechnologyTemplates.Get(templateName);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 351| »   »   »   let·time·=·techCostMultiplier.time·*·template.researchTime·*·cmpPlayer.GetTimeMultiplier();
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'time' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 594| »   for·(let·i·=·0;·i·<·count;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 596| »   »   let·ent·=·this.entityCache[0];
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 781| »   »   »   let·template·=·TechnologyTemplates.Get(item.technologyTemplate);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'template' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 196| »   for·(var·i·in·techList)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 198| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 208| »   for·(var·i·=·0;·i·<·techList.length;·i++)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 210| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 378| »   »   »   var·cmpTrigger·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Trigger);
|    | [NORMAL] JSHintBear:
|    | 'cmpTrigger' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 778| »   »   »   var·cmpTechnologyManager·=·QueryOwnerInterface(this.entity,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.
Executing section cli...

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

Angen added a comment.May 19 2019, 9:09 PM

please change var -> let in progresstimeout :)

binaries/data/mods/public/simulation/components/ProductionQueue.js
798

not needed check but maybe it is easier to understand

Polakrity updated this revision to Diff 8089.May 20 2019, 2:29 AM
Polakrity marked 3 inline comments as done.

var -> let

Stan added inline comments.May 20 2019, 8:48 AM
binaries/data/mods/public/simulation/components/ProductionQueue.js
673

for → to
I guess you can set the type to {Object}

Polakrity marked an inline comment as done.May 20 2019, 9:51 AM

For the rounding I thought about ceil rounding timeRemaining in GetQueue().
But I don't know if it's the best thing to do or the best place.

binaries/data/mods/public/simulation/components/ProductionQueue.js
673

I didn't know if it was really necessary because unused but i can.

wraitii added a reviewer: Restricted Owners Package.Wed, Jun 12, 10:34 PM