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 tooltip descriptions for spectators. #15203

Merged
merged 1 commit into from Jul 28, 2018

Conversation

Projects
None yet
6 participants
@matjaeck
Copy link
Contributor

matjaeck commented Jun 3, 2018

To fix #15200, regression of #15144.

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Jun 3, 2018

Well this definitely fixes the crash, but i'm not sure if this is a good idea. I would prefer specs to also see the tooltip descs in my mod. In any case i guess #15160 can fix that properly.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 3, 2018

It would be better if this can follow the same pattern we do in most other cases: use the RenderPlayer unless that is null, falling back to Localplayer, falling back to assuming that the stance is valid.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jun 3, 2018

Other than I expected the tooltip description can be shown to spectators if set to true. Please pardon my lack of understanding and knowledge, it's a learning process for me and I'm willing to improve.

It will be required to control which tooltip descriptions in case of multiple instances of the trait should be shown to spectators or add a description that is only shown to spectators. My current idea is to add a "generic description" that is not aimed at players of a certain stance, integrate ITooltipInfo and use TooltipForPlayerStance to return the generic description for stance == Stance.None. If I understood it correctly then this is similar to how Tooltip filters out the generic prefix for spectators.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 3, 2018

var stance = o == null || world.RenderPlayer == null ? Stance.None : o.Stances[world.RenderPlayer];
is how we do this for the generic stance prefixes. Using the same thing here will ensure consistent behaviour.

In this case we will want to use forPlayer instead of world.RenderPlayer, and then change

if (info.IsTooltipVisible(world.LocalPlayer))
to pass RenderPlayer instead of LocalPlayer.

@pchote pchote added this to the Next release milestone Jun 3, 2018

@matjaeck matjaeck changed the title Disable tooltip description for spectators. Filter tooltip descriptions for spectators. Jun 5, 2018

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jun 5, 2018

Still WIP, haven't found out yet how to filter the descriptions for spectators. Currently it shows all description instances to spectators regardless of valid stances.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jun 5, 2018

That's expected, see #10892.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jun 6, 2018

It looks like a proper implementation requires the spectator stances from #15160 which are assigned to next +1 while this is assigned to next release. IMO showing all descriptions to spectators looks broken so I suggest we either disable them for spectators until we have the spectator stances (preferably) or we revert #15144 entirely.

@@ -41,7 +49,7 @@ public bool IsTooltipVisible(Player forPlayer)
if (IsTraitDisabled)
return false;

var stance = forPlayer.Stances[self.Owner];
var stance = Owner == null || forPlayer == null ? Stance.None : Owner.Stances[forPlayer];

This comment has been minimized.

@GraionDilach

GraionDilach Jun 11, 2018

Contributor

You coulda try hardcoding this to Stance.Allies instead of Stance.None for now and adding a comment with a HACK prefix above it to mention it being a workaround. It might hide the neutral/enemy tooltips - I haven't tested if that would work.

This comment has been minimized.

@GraionDilach

GraionDilach Jun 11, 2018

Contributor

If that doesn't work, then just hide them all for spectators IMO.

This comment has been minimized.

@matjaeck

matjaeck Jun 27, 2018

Author Contributor

See comment below.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jun 27, 2018

I would appreciate it if somebody could take over this PR. It's not that I lost interest in it but I doubt that I'll be able to work on it in the next days as I want to focus on OpenRA/OpenRAWeb/381 for the remaining time until next release.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jun 27, 2018

I'll do it on Sunday at worst.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 1, 2018

@reaperrr: Would you be able to take this one over?

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jul 10, 2018

The other project is in a state where I have to wait for a review so I'll give this another try in the meantime.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jul 12, 2018

Updated, but still working on it.

Edit: This will now either show the description defined for stances Ally or Neutral to spectators but it can not filter for the stance "None".

@matjaeck matjaeck changed the title Filter tooltip descriptions for spectators. Fix tooltip descriptions for spectators. Jul 12, 2018

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jul 12, 2018

Will leave it like this and wait for a review.

101

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 25, 2018

The default spectator view now shows all the tooltip options, in a rather broken way:
screen shot 2018-07-25 at 22 29 45

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 25, 2018

I suspect the way to fix this will be to remove Stance.None from the defaults, and then list that explicitly in the yaml definition for the "provides additional funds" label.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 26, 2018

Or alternately remove Stance.None from the defaults, and leave them disabled for spectators in the default view. This could then be revisited after @GraionDilach's player stance overhaul.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 26, 2018

I will reopen that PR after we get through the playtest.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jul 26, 2018

Updated. Removed Stance.None from ValidStances again, however I can not reproduce the issue shown in the picture above.

101

Spectators will see the descriptions for either the neutral stance or the allied stance. The reason that I removed Stance.None from the valid stances is that if you write a yaml definition for TooltipDescription : ValidStances: None , this description will not be shown to spectators.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Jul 26, 2018

OpenRA.Mods.Common/Traits/TooltipDescription.cs:L51: [SP2000] Invalid spacing at the end of the line.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jul 26, 2018

Fixed the style error.

@Smittytron
Copy link
Contributor

Smittytron left a comment

Tested, LGTM

@@ -21,7 +21,7 @@ public class TooltipDescriptionInfo : ConditionalTraitInfo
public readonly string Description = "";

[Desc("Player stances who can view the description.")]
public readonly Stance ValidStances = Stance.Ally | Stance.Neutral | Stance.Enemy;
public readonly Stance ValidStances = Stance.Enemy | Stance.Neutral | Stance.Ally;

This comment has been minimized.

@GraionDilach

GraionDilach Jul 28, 2018

Contributor

This ordering is bad. :S

This comment has been minimized.

@matjaeck

matjaeck Jul 28, 2018

Author Contributor

The previous order caused my IDE to show an error so I changed it to the order used in the Enum, see https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Game/Traits/TraitsInterfaces.cs#L52.

This comment has been minimized.

@matjaeck

matjaeck Jul 28, 2018

Author Contributor

Fixed the stance order.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 28, 2018

👍

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

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 28, 2018

The issue I posted above still happens for the "Disable Shroud" view. This happens because Stance.None has a value of 0, and N & 0 == 0 is true for any value of N.

@@ -41,7 +49,7 @@ public bool IsTooltipVisible(Player forPlayer)
if (IsTraitDisabled)
return false;

var stance = forPlayer.Stances[self.Owner];
var stance = Owner == null || forPlayer == null ? Stance.None : Owner.Stances[forPlayer];

This comment has been minimized.

@pchote

pchote Jul 28, 2018

Member

Please change this to something like

// Tooltip states can't be determined for null owners or viewers, so just disable them
if (Owner == null || forPlayer == null)
	return false;

if (!Info.ValidStances.HasStance(Owner.Stances[forPlayer]))
	return false;

to disable this problematic case so we can move on with everything else.

@pchote
Copy link
Member

pchote left a comment

I have force pushed my suggested workaround. 👍 with that now in place.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 28, 2018

👍 after Travis confirms the build works.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Jul 28, 2018

OpenRA.Mods.Common/Traits/TooltipDescription.cs:L55: [SP2000] Invalid spacing at the end of the line.

@pchote

pchote approved these changes Jul 28, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 28, 2018

👍 after Travis confirms the build works. 😄

@abcdefg30 abcdefg30 merged commit d33ac0c into OpenRA:bleed Jul 28, 2018

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 Jul 28, 2018

@matjaeck matjaeck deleted the matjaeck:ttdescfix branch Jul 28, 2018

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.