Rewrite and simplify GPS dots #14117

pchote · 2017-10-05T20:39:20Z

The GPS code is a giant hack that doesn't fit with the current engine design, and can't reasonably be changed without changing its behaviour. #14115 shows that the legacy behaviour is controversial, and provides good motivation to rip out the problematic code.

The GPS dots are now purely visual, rendering for enemy actors who are (a) under fog (not shroud), (b) otherwise visible if the fog wasn't there (i.e. not cloaked), and (c) not already showing a visible frozen actor. Disguised actors (i.e. spies) now show indicators in their effective owner color instead of being hidden.

Smittytron

Looks good in testing. Works just as I imagined it.

I do wonder about making spies the color of their disguise. If you see a dot that's your color, there is only one thing in the game that can be. It'll tell you exactly what's going on instead of, 'He has a guy there'.

reaperrr · 2017-10-07T10:02:42Z

Needs rebase.

pchote · 2017-10-08T12:36:46Z

Rebased.

pchote · 2017-10-11T08:19:20Z

I do wonder about making spies the color of their disguise.

The motivation here is mainly technical. Having special case code in the GPS to check for “is this a disguised spy? Then remove the dot” is horrible for code generality. It is much nicer to have a blanket rule for “show dot for any hidden unit in the colour that the unit appears to the viewer”. I figured that this also matches the way that the satellite logically works: looking at the battlefield and reporting the location and owner of everything it sees.

If you think that change will unduely hurt gameplay then I don’t have a strong objection against adding it back, but it would be a bit of a shame.

dragunoff · 2017-10-11T10:24:27Z

Disguised actors (i.e. spies) now show indicators in their effective owner color instead of being hidden.

@pchote I understand that as "dots for spies will be shown in the color of the player who controls them, regardless of their current disguise (or lack of)". Is that correct?

I do wonder about making spies the color of their disguise.

And @Smittytron is worried that the spy dot will show up as the color of their disguise. Is there a misunderstanding here?

pchote · 2017-10-11T10:34:15Z

The spy dot will show up as the color of the disguise.

dragunoff · 2017-10-11T10:48:48Z

The spy dot will show up as the color of the disguise.

Then I also wonder if that is a good idea. As Smitty put it "there is only one thing in the game that can be"

On second thought that would make GPS a great counter to spies so maybe not such a bad idea. And yeah - that's how a satellite logically works.

Smittytron · 2017-10-11T13:36:24Z

What about leaving spy dots as the color of their owner? The player with GPS will know something is there, they just won't know it's a spy until they see it.

If this is also technically unfeasible, I have no objections to shipping this PR as is. Fixing GPS is much more important to gameplay.

pchote · 2017-10-11T19:04:46Z

Updated. I kept my first approach, but added a check to hide the indicator if it would otherwise be drawn in the color of the viewer or one of their allies.

This is an improvement over the original (release/playtest) behavior, which would hide the indicator for all disguised spies, even if they were disguised as an enemy player. Now, in a FFA game with two enemies then enemy A's spy disguised as you will be hidden, and enemy A's spy disguised as enemy B will show as enemy B's.

RoosterDragon · 2017-10-11T19:49:44Z

OpenRA.Mods.Cnc/Effects/GpsDotEffect.cs

+					toPlayer.IsAlliedWith(actor.EffectiveOwner.Owner))
+				return false;
+
+			return !visibility.IsVisible(actor, toPlayer) && toPlayer.Shroud.IsExplored(actor.CenterPosition);


You specify visibility = actor.TraitOrDefault<IDefaultVisibility>(); but don't appear to have handling for when it's null.

RoosterDragon · 2017-10-11T19:50:02Z

OpenRA.Mods.Cnc/Effects/GpsDotEffect.cs

+			visibilityModifiers = actor.TraitsImplementing<IVisibilityModifier>().ToArray();
+
+			dotStates = new PlayerDictionary<DotState>(actor.World,
+			player => new DotState(actor, player.PlayerActor.Trait<GpsWatcher>(), player.PlayerActor.TraitOrDefault<FrozenActorLayer>()));


Nit: Should be indented.

RoosterDragon · 2017-10-11T19:50:57Z

OpenRA.Mods.Common/Activities/Attack.cs

@@ -90,7 +90,8 @@ protected virtual Activity InnerTick(Actor self, AttackBase attack)
 			// HACK: This would otherwise break targeting frozen actors
 			// The problem is that Shroud.IsTargetable returns false (as it should) for
 			// frozen actors, but we do want to explicitly target the underlying actor here.
-			if (!attack.Info.IgnoresVisibility && type == TargetType.Actor && !Target.Actor.Info.HasTraitInfo<FrozenUnderFogInfo>() && !self.Owner.CanTargetActor(Target.Actor))
+			if (!attack.Info.IgnoresVisibility && type == TargetType.Actor && !Target.Actor.Info.HasTraitInfo<FrozenUnderFogInfo>()
+					&& !Target.Actor.CanBeViewedByPlayer(self.Owner))


Feel free to throw some more newlines in to aid readability since you're here.

penev92

Some small things to clean up, otherwise works as advertised.
Also needs a rebase.

penev92 · 2017-10-16T23:01:40Z

OpenRA.Mods.Cnc/Traits/GpsWatcher.cs

@@ -92,20 +90,11 @@ void RefreshGranted()
 					tp.Trait.OnGpsRefresh(tp.Actor, owner);
 		}

-		public bool HasFogVisibility()
+		bool IPreventsShroudReset.PreventShroudReset(Actor self, Player byPlayer)


Is there a point to having these arguments if the only implementation of the interface is not going to be using them?
Flag was raised when I noticed the only two calls to this method passed logically different things as arguments, so something was bogus...

penev92 · 2017-10-17T00:07:56Z

OpenRA.Mods.Common/Activities/Move/MoveAdjacentTo.cs

@@ -85,7 +85,7 @@ public override Activity Tick(Actor self)

 			// Target moved under the fog. Move to its last known position.
 			if (Target.Type == TargetType.Actor && canHideUnderFog
-				&& !self.Owner.CanTargetActor(Target.Actor))
+				&& !target.Actor.CanBeViewedByPlayer(self.Owner))


Small style thing, but the Target property wrapping the target variable and then we using both throughout the class is messy, and you switched one of the usages from one to the other.

Player.CanTargetActor(Actor) has been removed. This is just switching it for the otherwise equivalent Actor.CanBeViewedByPlayer(Player), with no other changes to this class.

I mean Target.Actor vs target.Actor. That's the "change" here.

penev92 · 2017-10-17T02:19:21Z

OpenRA.Mods.Cnc/Effects/GpsDotEffect.cs

+			player => new DotState(actor, player.PlayerActor.Trait<GpsWatcher>(), player.PlayerActor.TraitOrDefault<FrozenActorLayer>()));
+		}
+
+		bool ShouldRender(DotState state, Player toPlayer)


I may be wrong here after all this time, but I have a nagging feeling the checks in this method aren't exactly ordered by how expensive they are to make.
On the offchance that I'm right, it's something small but worth the hassle.

penev92 · 2017-10-17T10:28:55Z

OpenRA.Mods.Cnc/Effects/GpsDotEffect.cs

+			// Hide the indicator if the unit appears to be owned by an allied player
+			if (actor.EffectiveOwner != null && actor.EffectiveOwner.Owner != null &&
+					toPlayer.IsAlliedWith(actor.EffectiveOwner.Owner))
+				return false;


This check means that you woudn't see dots for spies that are disguised as your/allied soldiers.
Simply removing it should be more than fine, as if it really was an allied unit it wouldn't be under fog (barring some really sick edge case in scripted missions?!?), so you wouldn't be seeing a dot anyway, and without it you see a dot in the proper color.

See my comment about this in the original PR description and the discussion that followed. This was an intentional change in response to that PR feedback.

[13:45] <+penev> fine, I guess it's too obvious if you see a dot in your color...
[13:45] <+penev> damn gameplay implications...
[13:45] <+penev> always messing with good code
[13:45] <+penev> :D

pchote · 2017-10-21T12:16:43Z

TODO: include a fix for #14229.

pchote · 2017-10-22T16:47:30Z

Updated. #14229 requires cleaning up a different set of ugly GPS code than what is here, so i'm going to leave that for a later PR.

penev92 · 2017-10-26T15:17:22Z

👍

reaperrr · 2017-11-03T08:57:51Z

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/dfe582fbc0bb192dda61c10b9f68c54df6a9a842...0e5517643aedb4c21a59a382232cf0285a71f6ed

pchote force-pushed the remove-fog-visibility-modifier branch from 7c52238 to 93ed24b Compare October 5, 2017 20:45

Smittytron reviewed Oct 7, 2017

View reviewed changes

reaperrr added the PR: Rebase me! label Oct 7, 2017

pchote force-pushed the remove-fog-visibility-modifier branch from 93ed24b to b778e55 Compare October 8, 2017 12:36

pchote removed the PR: Rebase me! label Oct 8, 2017

pchote force-pushed the remove-fog-visibility-modifier branch from b778e55 to 9ca225b Compare October 8, 2017 17:42

pchote force-pushed the remove-fog-visibility-modifier branch from 9ca225b to a622b48 Compare October 11, 2017 19:03

pchote force-pushed the remove-fog-visibility-modifier branch 2 times, most recently from 436b742 to 709a270 Compare October 11, 2017 19:08

pchote mentioned this pull request Oct 11, 2017

Implement local sensor trait #12912

Open

RoosterDragon reviewed Oct 11, 2017

View reviewed changes

penev92 requested changes Oct 17, 2017

View reviewed changes

penev92 added PR: Fixup requested PR: Rebase me! and removed PR: Rebase me! labels Oct 17, 2017

Remove IFogVisibilityModifier.

6e282b6

pchote force-pushed the remove-fog-visibility-modifier branch from 709a270 to 2722ea5 Compare October 22, 2017 16:46

pchote removed the PR: Fixup requested label Oct 22, 2017

pchote added 2 commits October 22, 2017 17:49

Remove Player.CanViewActor and .CanTargetActor.

125c948

Simplify and fix code quality in GpsDot.

afacac5

pchote force-pushed the remove-fog-visibility-modifier branch from 2722ea5 to afacac5 Compare October 22, 2017 16:49

penev92 approved these changes Oct 26, 2017

View reviewed changes

penev92 added the PR: Needs +2 label Oct 26, 2017

reaperrr approved these changes Nov 3, 2017

View reviewed changes

reaperrr merged commit 4fe7ffe into OpenRA:bleed Nov 3, 2017

pchote deleted the remove-fog-visibility-modifier branch April 28, 2018 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite and simplify GPS dots #14117

Rewrite and simplify GPS dots #14117

pchote commented Oct 5, 2017

Smittytron left a comment

reaperrr commented Oct 7, 2017

pchote commented Oct 8, 2017

pchote commented Oct 11, 2017

dragunoff commented Oct 11, 2017

pchote commented Oct 11, 2017

dragunoff commented Oct 11, 2017 •

edited

Smittytron commented Oct 11, 2017

pchote commented Oct 11, 2017

RoosterDragon Oct 11, 2017

RoosterDragon Oct 11, 2017

RoosterDragon Oct 11, 2017

penev92 left a comment

penev92 Oct 16, 2017

penev92 Oct 17, 2017

pchote Oct 17, 2017 •

edited

penev92 Oct 17, 2017

penev92 Oct 17, 2017

penev92 Oct 17, 2017

pchote Oct 17, 2017

penev92 Oct 17, 2017

pchote commented Oct 21, 2017

pchote commented Oct 22, 2017

penev92 commented Oct 26, 2017

reaperrr commented Nov 3, 2017

Rewrite and simplify GPS dots #14117

Rewrite and simplify GPS dots #14117

Conversation

pchote commented Oct 5, 2017

Smittytron left a comment

Choose a reason for hiding this comment

reaperrr commented Oct 7, 2017

pchote commented Oct 8, 2017

pchote commented Oct 11, 2017

dragunoff commented Oct 11, 2017

pchote commented Oct 11, 2017

dragunoff commented Oct 11, 2017 • edited

Smittytron commented Oct 11, 2017

pchote commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchote commented Oct 21, 2017

pchote commented Oct 22, 2017

penev92 commented Oct 26, 2017

reaperrr commented Nov 3, 2017

dragunoff commented Oct 11, 2017 •

edited

pchote Oct 17, 2017 •

edited