Page MenuHomeWildfire Games

Progress more quickly next item in queue & remove duplicate call
AbandonedPublic

Authored by Polakrity on Apr 23 2017, 3:33 AM.

Details

Reviewers
fatherbushido
Summary

The current working is:

  • Add a new item in an empty queue, there is a delay of 1 second for treat it.
  • Same when the first item of queue is finished and the queue need to progress with next item.

There is a duplicate call when we adds a technology in an empty queue.
We call cmpTechnologyManager.StartedResearch twice in two steps.
Once when we add technology and again during the queue progress when check if the item is started in queue.
Just need to call it during the queue progress in the startedProd check.

Test Plan

Try to remove item from the queue, add new item and plan a large queue and check if all works.

Diff Detail

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

Event Timeline

Polakrity created this revision.Apr 23 2017, 3:33 AM
Polakrity updated this revision to Diff 1443.Apr 23 2017, 3:40 AM
Vulcan added a subscriber: Vulcan.Apr 23 2017, 8: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!

http://jw:8080/job/phabricator/870/ 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!

http://jw:8080/job/phabricator/871/ for more details.

elexis added a subscriber: elexis.

(Adding fatherbushido as a reviewer since he said he understood the change in IRC recently)

fatherbushido edited edge metadata.Apr 26 2017, 4:46 PM

I don't succeed to reproduce your issue.
The function is called twice because it's called in two steps. But in the previous related commit, you took care to call it with false and true. So I really don't see the problem.

If we started the tech. when the queue is empty, we don't need to recall the function through the second step.
It's not really a problem just logic thing.

elexis added a comment.EditedApr 26 2017, 5:35 PM

(Also if the function is called at most once, it would be possible to remove the notification argument of it, but it's probably nice to keep it in case we would ever call that function from another place, also consistent with that stopped function)

fatherbushido added a comment.EditedApr 26 2017, 5:42 PM

If we started the tech. when the queue is empty, we don't need to recall the function through the second step.
It's not really a problem just logic thing.

The first one is just to mark the tech as queued (so we don't try to search / queue it somewhere else).
The second one is when we actually search it.

Imo if we look at function in a unit way, it's perhaps better to keep it as in.
(I need to read the code again)

Polakrity added a comment.EditedApr 26 2017, 5:56 PM

So the 2 lines can be removed in AddBatch. (condition+call)

So the 2 lines can be removed in AddBatch.

I will read the code again this evening.

researchStarted allows to mark the techs currently researched (not in queue).

Refs rP12243

First we need to not mismatch what search mean in ProductionQueue and TechnologyManager.
When adding that element in the queue, productionStarted must be false. So that change is not wanted.
Indeed we could wonder if the block

if (this.queue.length == 0)
cmpTechnologyManager.StartedResearch(templateName, false);

is needed as it will be done for that one in ProgressTimeout.
I expect that it was added on purpose. Anyway, it's just setting a boolean.

If you want to get rid of the booleans introduced in rP19445,
you could send the notification in ProductionQueue.ProgressTimeout and not in cmpTechnologyManager.StartedResearch (and same for the other for consistency). (As another possibility).
Or this.researchStarted[tech] = true; after sending the gui notification (and checking for !this.researchStarted[tech] in that one).

fatherbushido requested changes to this revision.Apr 27 2017, 9:38 AM
This revision now requires changes to proceed.Apr 27 2017, 9:38 AM

The condition block:

if (this.queue.length == 0)
cmpTechnologyManager.StartedResearch(templateName, false);

allows to display faster the in-progress status than to pass by the second call in ProductionQueue.ProgressTimeout (1 to 2 seconds if removed).
I don't think it should be removed.

Add the booleen in patch allows to display the notification as fast than the in-progress status.

fatherbushido added a comment.EditedMay 9 2017, 10:04 PM

The condition block:

if (this.queue.length == 0)
cmpTechnologyManager.StartedResearch(templateName, false);

allows to display faster the in-progress status than to pass by the second call in ProductionQueue.ProgressTimeout (1 to 2 seconds if removed).
I don't think it should be removed.

Add the booleen in patch allows to display the notification as fast than the in-progress status.

Anyway the suggested change is wrong.

Polakrity updated this revision to Diff 2092.May 21 2017, 8:04 PM
Polakrity retitled this revision from StartedResearch function called twice when a research is added in an empty queue to ProductionQueue cleanup and process more quickly next item in queue.
Polakrity edited the summary of this revision. (Show Details)
Polakrity edited the test plan for this revision. (Show Details)
Polakrity edited edge metadata.
Polakrity updated this revision to Diff 2093.May 21 2017, 8:15 PM
Polakrity updated this revision to Diff 2100.May 22 2017, 1:16 AM
Polakrity retitled this revision from ProductionQueue cleanup and process more quickly next item in queue to Progress more quickly next item in queue & remove duplicate call.
Polakrity edited the summary of this revision. (Show Details)
Polakrity edited the test plan for this revision. (Show Details)

Remove the cleanup file part.

Polakrity updated this revision to Diff 2104.May 22 2017, 1:17 AM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1291/
See console output for more information: http://jw:8080/job/phabricator/1291/console

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1292/
See console output for more information: http://jw:8080/job/phabricator/1292/console

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1297/
See console output for more information: http://jw:8080/job/phabricator/1297/console

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!

http://jw:8080/job/phabricator/1298/ for more details.

Polakrity updated this revision to Diff 2116.May 22 2017, 11:15 AM
Polakrity edited the summary of this revision. (Show Details)
Polakrity updated this revision to Diff 2122.May 22 2017, 1:29 PM

reupload with context

Polakrity updated this revision to Diff 2124.May 22 2017, 2:36 PM
Polakrity updated this revision to Diff 2129.May 22 2017, 4:33 PM
Polakrity updated this revision to Diff 2131.May 22 2017, 5:20 PM

Full context this time.

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1306/
See console output for more information: http://jw:8080/job/phabricator/1306/console

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!

http://jw:8080/job/phabricator/1307/ 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!

http://jw:8080/job/phabricator/1308/ 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!

http://jw:8080/job/phabricator/1309/ for more details.

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1311/
See console output for more information: http://jw:8080/job/phabricator/1311/console

fatherbushido requested changes to this revision.Jul 31 2017, 4:47 PM
This revision now requires changes to proceed.Jul 31 2017, 4:47 PM
fatherbushido resigned from this revision.Aug 2 2017, 9:54 PM
bb abandoned this revision.Dec 23 2020, 10:57 PM
bb added a subscriber: bb.

I don't reproduce the issue anymore. I don't see any time gap between two consecutive batches, nor on the first batch. Maybe it got fixed in the mean time. For now I will abandon the patch, please reopen if the issue persists.

binaries/data/mods/public/simulation/components/ProductionQueue.js
729 ↗(On Diff #2131)

Adding this break is wrong. We could be finishing the batch immediately

735–736 ↗(On Diff #2131)

Don't understand what this is good for.

796 ↗(On Diff #2131)

This should stay

815 ↗(On Diff #2131)

This messes up the animation I reckon

This proposal is old, I now have a little easier to describe the "issue" (which is not really one) .

In AddBatch(): we push the processing of the queue in the interval of 1000ms, so in the next simulations not in current one.
I think it would be better to do all the checks to pass the item in productionStarted state in the same simulation.
If I had to take a real case, I don't believe that if your boss asks you to do a job now you will tell him that it will be done immediately but in reality you will only start it in 2 weeks.

Indeed the proposal was not the best fix nor the best explained (and I believe the rendering on Phab between the diff is bugged, some lines are not there).

The if (item.timeRemaining > time) has disappeared.