Fix ProductionTabsWidget not detecting ProductionQueue getting enabled/disabled during its lifetime

[michaeldgg2]

This is a fix for ProductionTabsWidget not picking up production queues' being enabled/disabled during their lifetime.

ProductionQueue is enabled in its Tick() method based on whether there's at least one Production trait enabled (if not, the queue is disabled). This value (ProductionQueue.Enabled) is used in several places, in this PRs case it's in ProductionTabsWidget, when actor with ProductionQueue is added/removed from World.

The issue (this PR fixes) is mainly visible, when ProductionQueue is disabled immediately, when the production actor is created. Example would be OpenE2140 mod, where buildings are always constructed from MCV-like vehicles (called MCUs). In case of OpenE2140, the building should not be usable while its being constructed. This is implemented by putting condition on Production trait. The issue is triggered, when one MCU is transformed to building while another one is already being constructed. In this case, the ProductionQueue for the second building has Enabled == false, so when the actor for the new building is created, ProductionTabsWidget wrongly assumes the ProductionQueue for the second building (the one being constructed) is not available and thus does not create tab for it (i.e. ProductionTab object).

This is how the bug looks like in OpenE2140:

(GIF is intentionally slowed down, to make it easier to follow changes in the ProductionTabsWidget)

[anvilvapre]

Could there be situations where this fix has the opposite effect? Production queue becoming visible - where it was never meant to have one?

[dnqbob]

This issue (Production UI upgrade) has some very nasty side effect on fixing.

You need a test on owner change before/after disable or enable. There can be some bugs that owner changed facility can still produce on old owner's production UI.

[dnqbob]

In case of OpenE2140, the building should not be usable while its being constructed.

I hope this PR can overcome the problem I met and mentioned above, but if it cannot, I will suggest this method used by @MustaphaTR :

1. Add a dummy/proxy actor with lines

 QueueUpdaterDummy:
	AlwaysVisible:
	ProvidesPrerequisite:
	KillsSelf:
		RemoveInstead: true

2. Use a FreeActor on production building, when it is enabled, enable the FreeActor that spawn a QueueUpdaterDummy. Like what you said, the remove of updater will force update the queue, and without owner change problem.

[dnqbob]

Add: This PR is also very worthy for the mods that can switch mutiqueue production or singlequeue production in gameplay, if you can make it.

[michaeldgg2]

@anvilvapre It shouldn't. The logic in the event of actor being added to world is basically the same as before. Plus it caches local player's ProductionQueues, and these are used to update queue tabs in ProductionTabsWidget.Tick(). Again, the logic pretty much the same in ProductionTabGroup.Update() (except the filter on ProductionQueue.Enabled, which is in the ProductionTabGroup).

@dnqbob Yes, I know about the bug, it affects us in OpenE2140 too. I actually fixed it in this PR #20705 (it's ready for the merge) - totally unintentionally.

1. Add a dummy/proxy actor with lines

2. Use a FreeActor on production building, when it is enabled, enable the FreeActor that spawn a QueueUpdaterDummy. Like what you said, the remove of updater will force update the queue, and without owner change problem.

No, I'm not going to do any hacks like this. I'd rather keep pushing devs to merge a PR that actually fixes a bug like this.

[dnqbob]

Then I think maybe we should add HasDependencies tag to this PR, because this PR depends on #20705 to work without owner change bugs.

[michaeldgg2]

No, this PR does not depend on #20705. They both affect widgets for production UI, but they change different parts of code base.

The bug with the owner change is separate to this bug.

[dnqbob]

But when we test if the owner change bug is truly fixed, it is better that put them together (more accurately, #20705 first and this PR the second), right?

Without #20705, this PR will have owner change bug (more precisely, expose the owner change bug that production UI has) which will affect the gameplay severely, and normally OpenRA won't directly merge a PR with a known bug.

Maybe from logic it is reluctantly to say this PR depends on #20705 as they fix unrelated things and #20705 happens to fix the bug in this PR. But at least "PR: Has dependencies" informs devs they should merge #20705 first to make the test here smoother.

[penev92]

#20705 was closed though.
The code seems fine, I left a bunch of superficial fix requests.
As for the owner change thing, can we cache that along with the enabled status and trigger an update if anything changes?

[michaeldgg2]

Like @penev92 said, #20705 is not really about the owner change issue of ProductionPaletteWidget, it's about new feature that should be tackled differently (possibly by changing how Buildable trait works). It was just a side effect of what that PR did. Owner change issue should be addressed at the root of the problem (i.e. the Production trait).

[dnqbob]

If the owner change problem is fixed in this PR it will be no problem on removing dependency tag, otherwise I would suggest fix the owner change issue first and then merge this PR.

[RoosterDragon]

Thanks for the detailed description in the ticket, it helped me with following through these changes.

This all looks good to me: we're detecting the tab state has gone out-of-sync and refreshing it. And by using the cache, the Tick method can cheaply check for this problem.

[abcdefg30]

Changelog