Skip to content
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

Fix bots using player actor powers after defeat #17463

Merged
merged 1 commit into from Dec 30, 2019

Conversation

@reaperrr
Copy link
Contributor

reaperrr commented Dec 14, 2019

Fixes #17456.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 14, 2019

We should really (also?) be blocking this in the SupportPower itself! Otherwise human players with a modified client may be able to exploit this.

@reaperrr reaperrr force-pushed the reaperrr:fix-deadbot-powers branch 2 times, most recently from 10c9cdc to 1cfaada Dec 14, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Dec 14, 2019

Updated, moved the check to SupportPowerManager instead.

If its Powers list is empty, the module will automatically consider any remaining waitingPowers stale and clear them, too.

@pchote pchote added this to the Next Release milestone Dec 15, 2019
@reaperrr reaperrr force-pushed the reaperrr:fix-deadbot-powers branch from 1cfaada to 1d20b61 Dec 17, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Dec 17, 2019

Updated.

Copy link
Member

abcdefg30 left a comment

Fix appears to work.
However, wouldn't it be better to use INotifyWinStateChanged.OnPlayerLost? (We should use OnPlayerWon as well for completeness, although that shouldn't really be needed.)

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 18, 2019

Never mind, a crash I found during testing I thought was a different issue is actually caused by this PR: We can't clear the powers of players that still have buildings, at least not without changing a few other locations like SupportPowerChargeBar#L46. It might be enough to fix GetPowersForActor though.

Example crash log:

OpenRA engine version {DEV_VERSION}
Red Alert mod version {DEV_VERSION}
on map c03e7fe6cf205de831d90175512663ca4a03042f (East vs West by Chris Forbes).
Date: 2019-12-18 20:31:37Z
Operating System: Windows (Microsoft Windows NT 6.2.9200.0)
Runtime Version: .NET CLR 4.0.30319.42000
Exception of type `System.Collections.Generic.KeyNotFoundException`: Der angegebene Schlüssel war nicht im Wörterbuch angegeben.
   at System.ThrowHelper.ThrowKeyNotFoundException()
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   at System.Linq.Enumerable.WhereEnumerableIterator`1.MoveNext()
   at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
   at OpenRA.Mods.Common.Traits.Render.SupportPowerChargeBar.OpenRA.Traits.ISelectionBar.GetValue()
   at OpenRA.Mods.Common.Graphics.SelectionBarsAnnotationRenderable.DrawExtraBars(WorldRenderer wr, float2 start, float2 end)
   at OpenRA.Mods.Common.Graphics.SelectionBarsAnnotationRenderable.Render(WorldRenderer wr)
   at OpenRA.Graphics.WorldRenderer.DrawAnnotations()
   at OpenRA.Game.RenderTick()
   at OpenRA.Game.Loop()
   at OpenRA.Game.Run()
   at OpenRA.Game.InitializeAndRun(String[] args)
   at OpenRA.Program.Main(String[] args)

The crash happens here: https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Mods.Common/Traits/SupportPowers/SupportPowerManager.cs#L116

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 19, 2019

Btw, thinking about it, I don't think we should always clear the powers, that will break the spectator overlay. Imho the best way to go forward would be to only clear if the game is not over and a player lost (i.e. check if the game is not over in INotifyWinStateChanged.OnPlayerLost and only then clear) since all other cases are irrelevant here.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 19, 2019

Can you elaborate on the spectator overlay issue?

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 19, 2019

I suppose you will not see any support powers although the buildings are still present, which is imho a bug. (It is not unusual to check the tab after a game ended.)

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 19, 2019

Why would the buildings still be present if they've lost?

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 19, 2019

Usually not, but that can be the case if we're looking at a scripted mission. The overlay issue is mostly about players that won.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 19, 2019

Ah, I see. I thought you meant "every tick" by always rather than "on win or loss".

I wonder whether it would be better to instead treat loss in the same place/way as the prerequisites not being satisfied?

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 19, 2019

Sounds like an idea. Wouldn't that still hide the powers in the spectator overlay, though?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 19, 2019

For players who have lost, yes. I don't see why that is a problem?

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 19, 2019

It would look a bit unpolished for scripted maps I suppose, but no big problems.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 28, 2019

The following works and should be pretty safe against regressions. Do you want to fix it here @reaperrr, or should I file a new PR?

diff --git a/OpenRA.Mods.Common/Traits/SupportPowers/SupportPowerManager.cs b/OpenRA.Mods.Common/Traits/SupportPowers/SupportPowerManager.cs
index 30e0909ae1..9704de9518 100644
--- a/OpenRA.Mods.Common/Traits/SupportPowers/SupportPowerManager.cs
+++ b/OpenRA.Mods.Common/Traits/SupportPowers/SupportPowerManager.cs
@@ -151,7 +151,7 @@ public class SupportPowerInstance
                protected int remainingSubTicks;
                public int RemainingTicks { get { return remainingSubTicks / 100; } }
                public bool Active { get; private set; }
-               public bool Disabled { get { return (!prereqsAvailable && !Manager.DevMode.AllTech) || !instancesEnabled || oneShotFired; } }
+               public bool Disabled { get { return Manager.Self.Owner.WinState != WinState.Undefined || (!prereqsAvailable && !Manager.DevMode.AllTech) || !instancesEnabled || oneShotFired; } }
 
                public SupportPowerInfo Info { get { return Instances.Select(i => i.Info).FirstOrDefault(); } }
                public bool Ready { get { return Active && RemainingTicks == 0; } }
@reaperrr reaperrr force-pushed the reaperrr:fix-deadbot-powers branch 2 times, most recently from 1600f1e to d847b2f Dec 29, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Dec 29, 2019

Updated.

Do you want to fix it here @reaperrr, or should I file a new PR?

Fixed it here.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Dec 29, 2019

However, wouldn't it be better to use INotifyWinStateChanged.OnPlayerLost? (We should use OnPlayerWon as well for completeness, although that shouldn't really be needed.)

Hm, only saw this now. Need to check.

Edit: Updated.

@reaperrr reaperrr force-pushed the reaperrr:fix-deadbot-powers branch from d847b2f to 331f7b4 Dec 29, 2019
Fixes bots using player actor powers after defeat.
@reaperrr reaperrr force-pushed the reaperrr:fix-deadbot-powers branch from 331f7b4 to 8068ef2 Dec 29, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Dec 29, 2019

Updated.

@pchote
pchote approved these changes Dec 30, 2019
Copy link
Member

pchote left a comment

LGTM, and fix confirmed.

@@ -109,7 +109,7 @@ public void ResolveOrder(Actor self, Order order)

public IEnumerable<SupportPowerInstance> GetPowersForActor(Actor a)
{
if (a.Owner != Self.Owner || !a.Info.HasTraitInfo<SupportPowerInfo>())
if (Powers.Count == 0 || a.Owner != Self.Owner || !a.Info.HasTraitInfo<SupportPowerInfo>())

This comment has been minimized.

Copy link
@pchote

pchote Dec 30, 2019

Member

Is this still needed with the Disabled fix below? I guess it can't hurt...

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Dec 30, 2019

Member

No it's not. It makes this even a bit more secure against crashes. (#17463 (comment) might have happened a different way.)

@pchote pchote added the PR: Needs +2 label Dec 30, 2019
@abcdefg30 abcdefg30 merged commit 9d7ecdb into OpenRA:bleed Dec 30, 2019
2 checks passed
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 Dec 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.