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

Add the possibility to deny spawning actors for dead players #13276

Merged
merged 8 commits into from Dec 23, 2017

Conversation

Projects
None yet
7 participants
@abcdefg30
Member

abcdefg30 commented May 11, 2017

Some maps/mods spawn player controllable actors using SpawnActorOnDeath. This leads to some weird results when a player is defeated (e.g. surrenders), as the player is dead, but can still control units. It makes sense to still spawn non-controllable units like husks, imho, so I made this a setting.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 May 11, 2017

Member

Updated.

Member

abcdefg30 commented May 11, 2017

Updated.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach May 11, 2017

Contributor

This is clearly a bug. The fix should be to shift the ownership of these actors to the world's owner immediately or skip spawning. (Westwood even did this: mindcontrolled units ended up neutral when they got released after their original owner died, mutated infantry ended up neutral if the triggerer died between firing the projectile and mutation taking effect etc).

Noncontrollable player-owned units is mehy IMO as well (I believe husks should be ownerless in the currently shipped mods because it looks stupid when they blow up along with the player and we have the required traits and setups to change that, AS proves).

In any case, dead players should never have controllable actors.

Contributor

GraionDilach commented May 11, 2017

This is clearly a bug. The fix should be to shift the ownership of these actors to the world's owner immediately or skip spawning. (Westwood even did this: mindcontrolled units ended up neutral when they got released after their original owner died, mutated infantry ended up neutral if the triggerer died between firing the projectile and mutation taking effect etc).

Noncontrollable player-owned units is mehy IMO as well (I believe husks should be ownerless in the currently shipped mods because it looks stupid when they blow up along with the player and we have the required traits and setups to change that, AS proves).

In any case, dead players should never have controllable actors.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 13, 2017

Contributor

This isn't solution to issue described by Graion, though can be used as workaround to stop spawning actors of dead players. The bug could be fixed in another PR.
In case this new property SpawnAfterDefeat is useful anyway, then 👍 .
(if it didn't break current maps, it could be False by default)

Contributor

rob-v commented May 13, 2017

This isn't solution to issue described by Graion, though can be used as workaround to stop spawning actors of dead players. The bug could be fixed in another PR.
In case this new property SpawnAfterDefeat is useful anyway, then 👍 .
(if it didn't break current maps, it could be False by default)

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 13, 2017

Member

Noncontrollable player-owned units is mehy IMO as well (I believe husks should be ownerless in the currently shipped mods because it looks stupid when they blow up along with the player and we have the required traits and setups to change that, AS proves).

I suggest filing a specific issue for this for discussion. Just switching owner isn't good enough because that would change the player color, but that could be solved by implementing IEffectiveOwner on the husk trait.

Edit: on second thought, this is the best place to discuss this, because it offers an IMO better solution to the problem that this is about.

Member

pchote commented May 13, 2017

Noncontrollable player-owned units is mehy IMO as well (I believe husks should be ownerless in the currently shipped mods because it looks stupid when they blow up along with the player and we have the required traits and setups to change that, AS proves).

I suggest filing a specific issue for this for discussion. Just switching owner isn't good enough because that would change the player color, but that could be solved by implementing IEffectiveOwner on the husk trait.

Edit: on second thought, this is the best place to discuss this, because it offers an IMO better solution to the problem that this is about.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach May 13, 2017

Contributor

I can't imagine any usecase where this setting would be preferred and wouldn't considered as a bug.

The husk issue I mentioned is something which affects balance so I'm quite sure the playerbase would shoot that down. For the record, I am spawning all husks as neutral via

SpawnActorOnDeath:
		OwnerType: InternalName
		InternalOwner: Creeps
Contributor

GraionDilach commented May 13, 2017

I can't imagine any usecase where this setting would be preferred and wouldn't considered as a bug.

The husk issue I mentioned is something which affects balance so I'm quite sure the playerbase would shoot that down. For the record, I am spawning all husks as neutral via

SpawnActorOnDeath:
		OwnerType: InternalName
		InternalOwner: Creeps
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 14, 2017

Member

There are two limitations there, but easily fixable:

  • Spawning an actor that is supposed to be player controllable. This can be fixed by adding an OwnerType.InternalNameIfVictimDefeated and falling back to InternalOwner if the player is defeated.
  • The player color will change. This can be solved for Husk specifically by using IEffectiveOwner to spoof the ParentActorInit's owner. This would also fix any balance issues.
Member

pchote commented May 14, 2017

There are two limitations there, but easily fixable:

  • Spawning an actor that is supposed to be player controllable. This can be fixed by adding an OwnerType.InternalNameIfVictimDefeated and falling back to InternalOwner if the player is defeated.
  • The player color will change. This can be solved for Husk specifically by using IEffectiveOwner to spoof the ParentActorInit's owner. This would also fix any balance issues.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 21, 2017

Contributor

Needs rebase.

Contributor

reaperrr commented Jul 21, 2017

Needs rebase.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Aug 8, 2017

Member

Updated. (The defaults/yaml values might need some adjustments, but I want some comments on the changes first.)

Member

abcdefg30 commented Aug 8, 2017

Updated. (The defaults/yaml values might need some adjustments, but I want some comments on the changes first.)

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 25, 2017

Contributor

Needs another rebase, sorry.

Contributor

reaperrr commented Aug 25, 2017

Needs another rebase, sorry.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Sep 12, 2017

Member

Rebased.

Member

abcdefg30 commented Sep 12, 2017

Rebased.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Sep 14, 2017

Member

Updated.

Member

abcdefg30 commented Sep 14, 2017

Updated.

@pchote

This looks straightforward enough. A couple of comments:

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Husk.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/SpawnActorOnDeath.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/SpawnActorOnDeath.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/SpawnActorOnDeath.cs Outdated
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 21, 2017

Member

When a player surrenders we currently kill all husks that were already on the map. The easiest way to fix that is to always pass EffectiveOwnerInit (maybe with an *Info flag to enable it), and change the husk defaults to always spawn as Neutral (but pretending to be the original owner).

Member

pchote commented Oct 21, 2017

When a player surrenders we currently kill all husks that were already on the map. The easiest way to fix that is to always pass EffectiveOwnerInit (maybe with an *Info flag to enable it), and change the husk defaults to always spawn as Neutral (but pretending to be the original owner).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 15, 2017

Member

@abcdefg30: Any progress here, or should someone else take this over? IMO this is potentially (with my request above) too nice of a polish fix to close and forget about.

Member

pchote commented Nov 15, 2017

@abcdefg30: Any progress here, or should someone else take this over? IMO this is potentially (with my request above) too nice of a polish fix to close and forget about.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Dec 5, 2017

Member

Updated. Sorry for the delay.

Member

abcdefg30 commented Dec 5, 2017

Updated. Sorry for the delay.

@pchote pchote added this to the Next release milestone Dec 9, 2017

@pchote

Just a couple of nits, otherwise LGTM:

Show outdated Hide outdated OpenRA.Mods.Common/Traits/SpawnActorOnDeath.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/SpawnActorOnDeath.cs Outdated

@pchote pchote added the PR: Needs +2 label Dec 10, 2017

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Dec 11, 2017

Member

Updated.

Member

abcdefg30 commented Dec 11, 2017

Updated.

{
// Fall back to InternalOwner if the Victim was defeated,
// but only if InternalOwner is defined
if (!defeated || string.IsNullOrEmpty(Info.InternalOwner))

This comment has been minimized.

@GraionDilach

GraionDilach Dec 17, 2017

Contributor

This sounds like able to recreating the bug the ticket is about if I just empty out InternalOwner with SpawnAfterDefeat: true.

@GraionDilach

GraionDilach Dec 17, 2017

Contributor

This sounds like able to recreating the bug the ticket is about if I just empty out InternalOwner with SpawnAfterDefeat: true.

This comment has been minimized.

@abcdefg30

abcdefg30 Dec 19, 2017

Member

It is intentional then (if you put those values), so not really an issue? (Someone might want to do it after all.)

@abcdefg30

abcdefg30 Dec 19, 2017

Member

It is intentional then (if you put those values), so not really an issue? (Someone might want to do it after all.)

This comment has been minimized.

@GraionDilach

GraionDilach Dec 20, 2017

Contributor

Meh, alright - #9117 (comment) logic can be applied to this case as well.

@GraionDilach

GraionDilach Dec 20, 2017

Contributor

Meh, alright - #9117 (comment) logic can be applied to this case as well.

{
init.Add(new FacingInit(Facing));
}
// We return self.Owner if there's no effective owner
bool IEffectiveOwner.Disguised { get { return true; } }

This comment has been minimized.

@penev92

penev92 Dec 19, 2017

Member

I'm not sure I like the implications of "husks are always disguised".

@penev92

penev92 Dec 19, 2017

Member

I'm not sure I like the implications of "husks are always disguised".

This comment has been minimized.

@abcdefg30

abcdefg30 Dec 19, 2017

Member

See #13276 (comment) (and IRC).

@abcdefg30

abcdefg30 Dec 19, 2017

Member

See #13276 (comment) (and IRC).

@reaperrr reaperrr merged commit 38cb3de into OpenRA:bleed Dec 23, 2017

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.

Show comment
Hide comment
@reaperrr
Contributor

reaperrr commented Dec 23, 2017

changelog

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