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

Correctly handle Production traits disabled by condition. #15045

Merged
merged 1 commit into from Nov 21, 2018

Conversation

Projects
None yet
7 participants
@IceReaper
Copy link
Contributor

IceReaper commented Apr 11, 2018

While Production trait is disabled via condition, ProductionQueue traits wont build. Additionally, the UI now ignores these actors.
While this is barely noticable in the core mods, it has a major impact on mods where construction takes longer.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 11, 2018

INotifyBuildComplete is deprecated and in the process of being untangled and removed from the codebase. You should instead have your custom code grant a condition that you then use to disable the production queue via the RequiresCondition yaml field.

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch 4 times, most recently from 4acd294 to 12a7093 Apr 12, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Apr 12, 2018

Also:

OpenRA.Mods.Common/Widgets/ProductionTabsWidget.cs:L176: [SP2000] Invalid spacing at the end of the line.

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch 3 times, most recently from 0f2e47f to db72709 Apr 12, 2018

var allQueues = a.World.ActorsWithTrait<ProductionQueue>()
.Where(p => p.Actor.Owner == p.Actor.World.LocalPlayer && p.Actor.IsInWorld && p.Trait.Enabled)
.Select(p => p.Trait).ToList();
var allQueues = world.ActorsWithTrait<ProductionQueue>()

This comment has been minimized.

@pchote

pchote Apr 12, 2018

Member

Running this every tick isn't great for performance. It should be possible to keep most of the original code to maintain the list of "all queues of my actors in the world" and then filter that down to "all enabled queues of my actors in the world" when needed each tick, avoiding most of the overhead.

@IceReaper IceReaper changed the title Buildings will only Produce if they are BuildCompleted. Correctly handle Production traits disabled by condition. Apr 12, 2018

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch 2 times, most recently from de49192 to 55dc994 Apr 12, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Apr 15, 2018

I guess that's related too, so while you are in it, can you fix that https://github.com/OpenRA/OpenRA/pull/15045/files#diff-a56831fc819020ea26578aec6535ba4bR127 looks for all the Production traits, and not the matching (p.Info.Produces.Contains(Info.Type)) ones?

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Apr 17, 2018

@MustaphaTR MustaphaTR referenced this pull request Apr 17, 2018

Merged

Fix the queue buttons. #10

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 7, 2018

Adding fixup tag for @MustaphaTR's request.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 21, 2018

This looks like it will be a prerequisite for removing the building lock and overhauling the capture code.

@pchote pchote added this to the Next + 1 milestone May 21, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Jun 18, 2018

@MustaphaTR comments:

https://github.com/OpenRA/OpenRA/pull/15045/files#diff-a56831fc819020ea26578aec6535ba4bR127
This does not need to be changed, it is used to store which production traits there are in general upon creation. The actual usage of this collection is always filtered for enabled ones upon usage because it may be changed every tick. If we filter this here, initialy disabled ones wont be available later if enabled.

https://github.com/OpenRA/OpenRA/pull/15045/files#diff-e87ba400b417079f410da8daa8f7f4edR212
There already is p.Trait.Enable at the end of the line?

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Jun 18, 2018

I feel like something is wrong with ProductionQueue>Enabled. But i think needs further investigation. I'm not sure if there is a better way to go there, but these changes were necessary for NG's upgrade system to work. There is an example on TD here, but needs my GrantConditionOnProduction too: https://gist.github.com/MustaphaTR/e8439632e8b23ea1d8d6002d99a6a361

Also checking for non-matching ProductionTraits later doesn't make much sense to me, imo it should be filtered at the beginning. Types aren't going to change ingame anyway. Currently Production queue numbers an a TD like UI appear for production queues without any matching Production trait enabled, if there are non-matching enabled Production Traits.

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Jun 18, 2018

But if we filter at the beginning, the ProductionQueue trait wont know about disabled Production traits at all. Even if they get enabled later on. If i am totaly misunderstanding your case, we should move this conversation over to Discord or IRC.

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch from 55dc994 to 5e3b988 Jun 18, 2018

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch from 7730023 to 9cfa69c Sep 30, 2018

@pchote
Copy link
Member

pchote left a comment

Looks sensible, just one perf comment.

Dependencies have been merged, so can you please rebase this on bleed to aid so we can properly test this?

@@ -161,6 +161,11 @@ public ProductionQueue CurrentQueue

public override void Draw()
{
var tabs = Groups[queueGroup].Tabs.Where(t => t.Queue.BuildableItems().Any()).ToArray();

This comment has been minimized.

@pchote

pchote Oct 8, 2018

Member

This gets called a lot, so we want to avoid the overhead (2x enumerations) and memory churn of the ToArray.

The .Any check only needs to peek at the first item, so keep it as an enumerable!

This comment has been minimized.

@IceReaper

IceReaper Oct 20, 2018

Contributor

Need more input here, not sure how to change it, besides just removing the "ToArray" results in my IDE (Rider) to inform me about multiple enumerations.

This comment has been minimized.

@pchote

pchote Oct 24, 2018

Member

Just remove the ToArray. Rider misses out the subtleties in this case which mean that a partial multiple enumeration is better than the alternative.

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch 2 times, most recently from dde9188 to b41528f Oct 20, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Oct 24, 2018

rebased

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch from b41528f to 5900daa Oct 26, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 28, 2018

When I add RequiresCondition: !build-incomplete to the cnc production traits the production tabs and buttons no longer work (but clicking on the structure directly to select does). Can you please fix this here? There is a second issue where the palette pops back briefly at the end of the conyard undeploy, but that is a bug in WithMakeAnimation that I have fixed in #15748.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 3, 2018

I see a new push here but the bug above still persists, so I guess this was just a rebase?

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch from a4e8be2 to 0b09270 Nov 6, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Nov 6, 2018

updated

@IceReaper IceReaper force-pushed the IceReaper:OnlyProduceWhenBuild branch from 0b09270 to 70fa364 Nov 12, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 17, 2018

There are two "issues" here that show up when disabling the only production trait on an actor in TD. I mention them here for completeness, but as this is not a configuration we are likely to use (preventing production during buildup will be done with pausing, which works fine) I don't think it is worth blocking this further over them.

Testcase: Add RequiresCondition: !build-incomplete to Production on the TD construction yard.

  • ProductionQueueFromSelection only acts when a unit is first selected, and so is not able to handle a queue becoming active on an actor that is already selected. This logic is what automatically populates the sidebar when the MCV is deployed, so if the trait starts disabled we get into the inconsistent state where the type buttons are available but the production palette is hidden.
  • The switch-to-available queue sidebar logic only runs when the actor is removed, not when the trait is disabled. Undeploying the conyard produces an inconsistent state where a tab is both highlighted and disabled, with the production palette being hidden while other queue buttons are available.

The second point is IMO a legitimate bug, and may become important for certain modding use cases. This can be addressed in its own PR in the future if there is a usecase for it.

@pchote

pchote approved these changes Nov 17, 2018

@@ -34,7 +35,7 @@ void SetupProductionGroupButton(ProductionTypeButtonWidget button)
tabs.PickUpCompletedBuilding();
};

button.IsDisabled = () => tabs.Groups[button.ProductionGroup].Tabs.Count == 0;
button.IsDisabled = () => !tabs.Groups[button.ProductionGroup].Tabs.Any(t => t.Queue.BuildableItems().Any());

This comment has been minimized.

@pchote

pchote Nov 17, 2018

Member

We could potentially drop this again and save some perf, as:

  • It puts the sidebar in a weird state where the tab is simultaneously active and disabled (see last comment)
  • It's not needed for our immediate usecases (which will now use pausing instead of disabling)

@pchote pchote added the PR: Needs +2 label Nov 17, 2018

@@ -244,6 +246,8 @@ public virtual IEnumerable<ActorInfo> AllItems()

public virtual IEnumerable<ActorInfo> BuildableItems()
{
if (productionTraits.Any() && productionTraits.All(p => p.IsTraitDisabled))

This comment has been minimized.

@reaperrr

reaperrr Nov 20, 2018

Contributor

Minor perf nit (no blocker, I think): The !Enabled check below is certainly cheaper than LINQ, so I'd switch order of these two.

@reaperrr
Copy link
Contributor

reaperrr left a comment

Looks and works well enough

@reaperrr reaperrr merged commit c3f4bc4 into OpenRA:bleed Nov 21, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 21, 2018

@IceReaper IceReaper deleted the IceReaper:OnlyProduceWhenBuild branch Nov 28, 2018

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