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

Added ability to customise what happens to an actor when its owner loses. #14982

Merged
merged 5 commits into from May 28, 2018

Conversation

Projects
None yet
4 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Mar 23, 2018

Wanted to hit two bird with one stone, this fixes the issues with the Aircraft Husks (#14858, #14849, #14902), also adds support for #4706, but not implements in this PR for now. I guess it may need furter discussion and to not block this PR as i think husk issues are a bit more important.

The fix for Aircraft Husks are in second commit. I wasn't sure what to do with vehicle husks, current implementation doesn't break anything from them, afaik.

Testcase commit is for testing other 2 things you can make actors to do, change their owner, or get disposed. Testcase makes Tech Structures turn back to neutral and Demo Truck get disposed/not explode.

Fixes #14858.
Fixes #14849.
Fixes #14902.

@@ -954,6 +952,8 @@
RevealOnDeath:
Duration: 60
Radius: 4c0
CustomOwnerLostAction:
Action: Nothing

This comment has been minimized.

@GraionDilach

GraionDilach Mar 23, 2018

Contributor

IMO this should be ChangeOwner to Neutral and then you should add a KeepEffectiveOwner boolean along.

This comment has been minimized.

@MustaphaTR

MustaphaTR Mar 24, 2018

Author Member

It would still get, mostly graphical after that point, issues like getting bounty text for neutral player and contrails being white.

Tho i see no problem with making effective owner an option for the trait.


namespace OpenRA.Mods.Common.Traits
{
public enum OwnerLostAction { ChangeOwner, Dispose, Nothing }

This comment has been minimized.

@GraionDilach

GraionDilach Mar 23, 2018

Contributor

I would drop Nothing altogether, that can be abused to post-defeat units and i don't think we should have an option for such.

This comment has been minimized.

@pchote

pchote May 20, 2018

Member

I kind of agree here, but as much as i'd like to restrict that I would like more to have streamlined code.

IMO it would be better to drop the Custom in the trait name here, change Nothing to Kill, and then remove the "if actor doesn't have this trait then kill it" logic from the *VictoryConditions traits. This trait can then be added to the defaults for all player-built actors. This trait then completely controls what happens on player loss, with the actor surviving (e.g. for plane husks) and remaining controllable (as an unwanted but IMO tolerable side-effect) if the trait isn't defined.

This comment has been minimized.

@pchote

pchote May 20, 2018

Member

Note that this will need a placeholder update rule that explains that OwnerLostAction now needs to be defined on all player-built actors.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:custom-owner-lost-action branch 5 times, most recently from 46c796a to e65922c Mar 31, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Mar 31, 2018

It appears that i have no idea how to properly implement EffectiveOwnerFromOwner. I'll need help with that.

@pchote pchote added this to the Next release milestone Apr 7, 2018

public object Create(ActorInitializer init) { return new CustomOwnerLostAction(init, this); }
}

public class CustomOwnerLostAction : IEffectiveOwner, INotifyOwnerChanged

This comment has been minimized.

@pchote

pchote May 20, 2018

Member

This needs to implement IDeathActorInitModifier and add an EffectiveOwnerInit to the TypeDictionary if EffectiveOwnerFromOwner is true. IEffectiveOwner and INotifyOwnerChanged should be removed.

This should hopefully fix your "Help wanted"!

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 20, 2018

Sorry for taking so long to get to this! IMO this is one of our highest priority PRs to get merged for the next release, so @MustaphaTR could you please try and address the comments ASAP?

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented May 20, 2018

I'll try to get this done tomorrow. I don't have time for it today.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:custom-owner-lost-action branch 2 times, most recently from e554df4 to 59dc559 May 21, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented May 21, 2018

Updated. Removed the EffectiveOwner hadling for now as discussed on IRC.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:custom-owner-lost-action branch from 59dc559 to 3be6bc7 May 21, 2018

@pchote
Copy link
Member

pchote left a comment

Looking good. A few minor requests, then this should be good to merge:


namespace OpenRA.Mods.Common.Traits
{
public enum LostAction { ChangeOwner, Dispose, Kill }

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

This is exported into the traits namespace, so can we please rename it to something a bit more descriptive like OwnerLostActionType?

{
public enum LostAction { ChangeOwner, Dispose, Kill }

[Desc("This actor does something when its owner loses.")]

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Can we please change this to "Perform an action when the actor's owner is defeated."?


public class OwnerLostAction : INotifyOwnerLost
{
public OwnerLostActionInfo Info;

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

readonly please.

public object Create(ActorInitializer init) { return new OwnerLostAction(init, this); }
}

public class OwnerLostAction : INotifyOwnerLost

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

It could be useful if we made this a ConditionalTrait?

@@ -109,8 +109,8 @@ void ITick.Tick(Actor self)

public void OnPlayerLost(Player player)
{
foreach (var a in player.World.Actors.Where(a => a.Owner == player))
a.Kill(a);
foreach (var a in player.World.ActorsWithTrait<INotifyOwnerLost>().Where(a => a.Actor.Owner == player))

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

then you can just add && !a.Trait.IsDisabled in these .Wheres

This comment has been minimized.

@GraionDilach

GraionDilach May 22, 2018

Contributor

This nit ended up unaddressed.

This comment has been minimized.

@MustaphaTR

MustaphaTR May 22, 2018

Author Member

I added the IsTraitDisabled check to trait itself because i used a trait interface (in case someone wants to add a custom trait about owner losing).

This comment has been minimized.

@GraionDilach

GraionDilach May 22, 2018

Contributor

Hmm, okay, fine by me.

{
public class DefineOwnerLostAction : UpdateRule
{
public override string Name { get { return "Define OwnerLostActor on player actors"; } }

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Suggested wording: "Add OwnerLostAction to player-controlled actors".

{
get
{
return "Actors now need OwnerLostActor trait to choose what will happen tho them when their owner loses.";

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Suggested wording: "A new OwnerLostAction trait has been introduced to control what happens to a\nplayer's actors when they are defeated. A warning is displayed notifying that\nthis trait must be added to actors."


public override IEnumerable<string> AfterUpdate(ModData modData)
{
yield return "Actors now need OwnerLostActor trait to choose what will happen tho them when their owner loses.\n" +

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Suggested wording + repeated warning (on all custom maps) fix:

bool reported;
public override IEnumerable<string> AfterUpdate(ModData modData)
{
	if (!reported)
		yield return "All player-controlled (or player-capturable) actors should define an OwnerLostAction trait\n" +
			"specifying an action (Kill, Dispose, ChangeOwner) to apply when the actor's owner is defeated.\n" +
			"You must manually define this trait on the appropriate default actor templates.\n" +
			"Actors missing this trait will remain controllable by their owner after they have been defeated.";

	reported = true;
}

@@ -767,6 +781,8 @@
Interactable:
CombatDebugOverlay:
AppearsOnRadar:
OwnerLostAction:
Action: ChangeOwner

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

I like the idea of keeping walls around after defeat, but this is currently mixing refactoring changes with gameplay changes. Can you please keep these as Kill in the first commit, and then introduce a new commit that changes them to ChangeOwner, like you did for the tech structures?

@@ -775,6 +781,10 @@
Inherits@2: ^ExistsInWorld
Inherits@3: ^Cloakable
Huntable:
OwnerLostAction:
Action: Kill
OwnerLostAction:

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Duplicated trait definition here.

MustaphaTR added some commits Mar 23, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:custom-owner-lost-action branch from 3be6bc7 to 5aa9547 May 21, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented May 21, 2018

Updated.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍 after the fixup though.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Confirming my 👍 again, just to dismiss confusion.

@pchote pchote dismissed their stale review May 28, 2018

Changes addressed.

@pchote pchote added the PR: Needs +2 label May 28, 2018

@pchote

pchote approved these changes May 28, 2018

Copy link
Member

pchote left a comment

LGTM 👍

@pchote pchote merged commit 4bf606b into OpenRA:bleed May 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.