New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented ParallelProductionQueue. #15018

Merged
merged 2 commits into from Sep 30, 2018

Conversation

Projects
None yet
10 participants
@IceReaper
Copy link
Contributor

IceReaper commented Apr 5, 2018

preview

Yaml changes will be removed upon approvement, and before merge.

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch 2 times, most recently from 654bc58 to 7e0f837 Apr 5, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 5, 2018

We probably want to get #15006 merged first, then this may need some tweaks.

@ABrandau

This comment has been minimized.

Copy link
Contributor

ABrandau commented Apr 5, 2018

Gonna insist with my request, just to have a reminder of it at the very least.

It would be nice to have the option to slowdown production for each unit that is paused (meaning that it has a little amount of progress on it)

This would discourage queueing a lot of units and deploying it on the same instant (like capping an enemy factory and deploy all that stuff)

@IceReaper IceReaper referenced this pull request Apr 6, 2018

Merged

Infinite build after #15021

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch from 7e0f837 to fe049f9 Apr 7, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 21, 2018

#15006 has been merged, so this needs a rebase.

SourceFiles="$(TargetPath)"
DestinationFolder="$(SolutionDir)mods/common/"/>
<MakeDir Directories="$(SolutionDir)mods/common/" />
<Copy SourceFiles="$(TargetPath)" DestinationFolder="$(SolutionDir)mods/common/" />

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Please unstage these project file formatting changes


public class ParallelProductionQueue : ProductionQueue
{
public ParallelProductionQueue(ActorInitializer init, Actor playerActor, ParallelProductionQueueInfo info) : base(init, playerActor, info)

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Style nit: : base(...) { } should go on its own line.

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Player/ParallelProductionQueue.cs
if (added.Contains(i.Item))
continue;

added.Add(i.Item);

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

It should be possible to simplify this by using a HashSet instead of a list and then doing if (!added.Add(i.Item)) (which returns false if the item already existed)


if (queue == null)
return;

// Find the ProductionItem associated with the building that we are trying to place
ProductionItem item = null;

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

It should be possible to simplify this to something like

var item = targetActor.TraitsImplementing<ProductionQueue>()
    .SelectMany(q => q.AllQueued())
    .FirstOrDefault(i => i.Item == order.TargetString && i.Done);

if (item == null)
    return;

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch 7 times, most recently from 557702b to 03db08c Jun 11, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Jun 16, 2018

Thanks IceReaper! This is important for Dark Reign, too.

{
public ParallelProductionQueue(ActorInitializer init, Actor playerActor, ParallelProductionQueueInfo info)
: base(init, playerActor, info)
{

This comment has been minimized.

@GraionDilach

GraionDilach Jun 16, 2018

Contributor

Style nit: Empty braces tend to go into the line of the function header.

This comment has been minimized.

@MustaphaTR

MustaphaTR Jul 12, 2018

Member

This seems still unadressed.

This comment has been minimized.

@IceReaper

IceReaper Jul 12, 2018

Contributor

So in this case... : base in the next line, afer the ) comes " { }" ? I just wasnt able to find an example with a base block and empty body... It would be done weeks ago, if someone had hinted me an example how you want this to be formatted :)

This comment has been minimized.

@MustaphaTR

MustaphaTR Jul 12, 2018

Member

It should be : base(init, playerActor, info) { } Here is an example

: base(widget, modData, "TakeScreenshotKey", "GLOBAL_KEYHANDLER", logicArgs) { }

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch from 03db08c to 95aa162 Jun 16, 2018

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Jul 1, 2018

Works as promised. ☑️

@@ -146,7 +146,7 @@ IEnumerable<Order> InnerOrder(World world, CPos cell, MouseInput mi)

public void Tick(World world)
{
if (queue.CurrentItem() == null || queue.CurrentItem().Item != actorInfo.Name)
if (queue.AllQueued().All(i => i.Item != actorInfo.Name))

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

This probably also needs to check that the item has completed construction? Otherwise this will change behaviour if the building being placed is cancelled, but another item of the same type exists later in the queue (but hasn't started construction yet)

public override int RemainingTimeActual(ProductionItem item)
{
var parallelBuilds = 0;
var added = new HashSet<string>();

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

RemainingTimeActual is called a lot, so you ideally want to avoid allocating objects here. I won't block review over this as this trait isn't used in the default mods, but you should still try and fix this.

@@ -126,7 +126,7 @@ protected override bool BuildUnit(ActorInfo unit)

if (p.Trait.Produce(p.Actor, unit, type, inits))
{
FinishProduction();
FinishProduction(Queue.FirstOrDefault(i => i.Item == unit.Name));

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

Doesn't this need a check for whether the item has finished production?

@@ -494,7 +504,7 @@ protected virtual bool BuildUnit(ActorInfo unit)

if (!mostLikelyProducerTrait.IsTraitPaused && mostLikelyProducerTrait.Produce(self, unit, type, inits))
{
FinishProduction();
FinishProduction(Queue.FirstOrDefault(i => i.Item == unit.Name));

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

Doesn't this need a check for whether the item has finished production?

@@ -334,7 +334,7 @@ bool HandleMiddleClick(ProductionItem item, ProductionIcon icon, int handleCount
bool HandleEvent(ProductionIcon icon, MouseButton btn, Modifiers modifiers)
{
var startCount = modifiers.HasModifier(Modifiers.Shift) ? 5 : 1;
var cancelCount = modifiers.HasModifier(Modifiers.Ctrl) ? CurrentQueue.QueueLength : startCount;
var cancelCount = modifiers.HasModifier(Modifiers.Ctrl) ? CurrentQueue.AllQueued().Count() : startCount;

This comment has been minimized.

@pchote

pchote Jul 1, 2018

Member

.Count() is slow. Can we instead try casting this to a collection and using .Count?

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch from 95aa162 to da863a3 Jul 2, 2018

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch from da863a3 to 0a689b8 Jul 13, 2018

@pchote
Copy link
Member

pchote left a comment

Still a few bugs to squash, i'm afraid. Hopefully just about ready to merge once these are fixed.


if (queue == null)
return;

// Find the ProductionItem associated with the building that we are trying to place
var item = targetActor.TraitsImplementing<ProductionQueue>()

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

We already know the queue, so this simplifies to var item = queue.FirstOrDefault(i => i.Item == order.TargetString && i.Done);

@@ -290,20 +282,24 @@ protected virtual void Tick(Actor self)

Enabled = IsValidFaction && anyEnabledProduction;

for (var i = 0; i < Queue.Count; i++)
{
if (BuildableItems().All(b => b.Name != Queue[i].Item))

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

BuildableItems() is backed by a LINQ query, which isn't free - we can reduce enumerations by pulling a var buildableNames = BuildableItems.Select(b => b.Name).ToList(); and then use if (!buildableNames.Contains(Queue[i].Item)) here.

@@ -290,20 +282,24 @@ protected virtual void Tick(Actor self)

Enabled = IsValidFaction && anyEnabledProduction;

for (var i = 0; i < Queue.Count; i++)

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

Style/readability nit: prefer enumerating backwards from Count - 1 to 0 instead of enumerating forward and then stepping back when removing something.

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs
Show resolved Hide resolved OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs
@@ -67,7 +67,7 @@ void INotifyCreated.Created(Actor self)

void ITick.Tick(Actor self)
{
var current = queue.CurrentItem();
var current = queue.AllQueued().OrderBy(i => i.RemainingTime).FirstOrDefault();

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

This needs to ignore units that haven't started yet!

It breaks the production bar if you are are producing a long build time unit (e.g. a mammoth tank in RA) and then queue a unit that has a total build time shorter than what is left on the current item (e.g. a flak truck).

@@ -49,7 +49,7 @@ public class WithProductionOverlay : INotifyDamageStateChanged, INotifyCreated,

bool IsProducing
{
get { return queues != null && queues.Any(q => q.Enabled && q.CurrentItem() != null && !q.CurrentPaused); }
get { return queues != null && queues.Any(q => q.Enabled && q.AllQueued().Any(i => !i.Paused)); }

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

This is another case that needs to filter out units that haven't started yet.

It breaks the welding animations in d2k if you start producing a vehicle (animation shows), pause it (animation correctly pauses), then queue a different unit behind it (animation incorrectly restarts).

Show resolved Hide resolved OpenRA.Mods.Common/Widgets/ProductionPaletteWidget.cs

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch 5 times, most recently from bacc34d to 2d60d46 Sep 28, 2018

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch 2 times, most recently from c49d592 to bbbe61b Sep 28, 2018

@pchote
Copy link
Member

pchote left a comment

Code LGTM now, just a couple of minor style/perf comments.
Please also split this into two commits as discussed in IRC.

Will give my 👍 once I have a chance to test this ingame

@@ -149,7 +149,7 @@ IEnumerable<Order> InnerOrder(World world, CPos cell, MouseInput mi)

public void Tick(World world)
{
if (queue.CurrentItem() == null || queue.CurrentItem().Item != actorInfo.Name)
if (queue.AllQueued().All(i => i.Item != actorInfo.Name || !i.Done))

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Minor perf nit: check !i.Done first, it is most likely to be false and significantly cheaper than the string comparison

@@ -126,7 +126,7 @@ protected override bool BuildUnit(ActorInfo unit)

if (p.Trait.Produce(p.Actor, unit, type, inits))
{
FinishProduction();
EndProduction(Queue.FirstOrDefault(i => i.Item == unit.Name && i.Done));

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Likewise here - check i.Done first.


public override int RemainingTimeActual(ProductionItem item)
{
var parallelBuilds = Queue.FindAll(i => !i.Paused && !i.Done).GroupBy(i => i.Item).ToList().Count;

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Style nit: split this over multiple lines to improve readability

var parallelBuilds = Queue.FindAll(i => !i.Paused && !i.Done)
	.GroupBy(i => i.Item)
	.ToList()
	.Count;

if (queue == null)
return;

// Find the ProductionItem associated with the building that we are trying to place
var item = queue.AllQueued().FirstOrDefault(i => i.Item == order.TargetString && i.Done);

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

i.Done first here and above too.

Queue[0].Tick(playerResources);
}

protected void RefundUnbuildableItems()

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

IMO CancelUnbuildableItems would be a better name - refunding is just a consequence of the cancellation.

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Player/ProductionQueue.cs
@@ -504,7 +510,7 @@ protected virtual bool BuildUnit(ActorInfo unit)

if (!mostLikelyProducerTrait.IsTraitPaused && mostLikelyProducerTrait.Produce(self, unit, type, inits))
{
FinishProduction();
EndProduction(Queue.FirstOrDefault(i => i.Item == unit.Name && i.Done));

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

i.Done first again here

@pchote pchote dismissed their stale review Sep 28, 2018

done

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch 3 times, most recently from 1a4a11d to 2c41550 Sep 28, 2018

@pchote pchote referenced this pull request Sep 28, 2018

Closed

Bulk production queue #7056

@pchote
Copy link
Member

pchote left a comment

Code LGTM and I didn't notice any issues in with our original production types in TS and TD.

I didn't test the behaviour of the parallel queue as I expect you have already verified that this works correctly.

@pchote pchote added the PR: Needs +2 label Sep 29, 2018

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch from 2c41550 to 81b49d8 Sep 29, 2018


var before = item.RemainingTime;

if (!item.Paused)

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 29, 2018

Member

Does this make sense? You selected item as the first one not being paused.

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Player/ParallelProductionQueue.cs

@IceReaper IceReaper force-pushed the IceReaper:ParallelProductionQueue branch from 81b49d8 to 7cf1d2e Sep 29, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Works in TD.

@abcdefg30 abcdefg30 merged commit 52a7d39 into OpenRA:bleed Sep 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 30, 2018

@IceReaper IceReaper deleted the IceReaper:ParallelProductionQueue branch Sep 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment