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

Rewrite a bit of the team chat logic #15615

Merged
merged 12 commits into from Apr 22, 2019

Conversation

@abcdefg30
Copy link
Member

commented Sep 14, 2018

Fixes #12226.

Team chat is...

  • now disabled if you are the only spectator.
  • re-enabled if the count of spectators is greater than one due to dead players.
  • permanently disabled when the game ended.

Note that team chat currently does not get disabled if you are the only player left on your team. However, that shouldn't be too hard to do if we want that fixed, too.

@kyrreso

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

This is so nice, will change a lot in the means as watching games... It could be that team-chat after game will be requested later, if we feel we miss it. But lets try this out..

@abcdefg30 abcdefg30 force-pushed the abcdefg30:specTeam branch from c590c80 to 3f0f391 Dec 18, 2018

@kyrreso

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Will be hard to chat with yourself then... In your team of one :-)

@abcdefg30 abcdefg30 force-pushed the abcdefg30:specTeam branch 2 times, most recently from 01953d2 to 53f1ba9 Mar 11, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

Rebased.

@pchote
Copy link
Member

left a comment

Every time I try to review this I run into the ridiculously twisty chat logic, 🤦‍♂️ , 🤷‍♂️ , and then 🏃. I know this mess isn't your fault and it looks like this PR does improve things, but I have zero faith (and tbh a similar level of interest) that I can work through this code and understand the intended and actual behaviour well enough to give a meaningful review.

I suspect the real problem is that the desired behaviour is not consistent, or maybe is just being modeled wrong. Would implementing a separate "SpectatorChat" order help here?

We also need to be careful about player status changing while orders are in flight. IMO its not right for a message that was valid team chat when it was sent to be rendered in the global chat (possibly only for some players! chat orders are immediate, so there may be a way for different clients to validate e.g. before / after simulating a player losing) because of unfortunate timing.

OpenRA.Game/Network/UnitOrders.cs Outdated Show resolved Hide resolved
OpenRA.Game/Network/UnitOrders.cs Show resolved Hide resolved
OpenRA.Game/Network/UnitOrders.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

If we want to keep the current behaviour then we could make the implementation signficantly more understandable by pulling out a ChatMessageType CalculateMessageType(Client sender, bool wantsTeam) method that implements all the down/side-grade logic in one place, separate from the other concerns. ChatMessageType would be an enum with values for Regular, Team, and Spectator.

This can then be used from IngameChatLogic to determine the chat mode: call with wantsTeam: true - if it returns Regular then disable the toggle, otherwise set it to Team/Spec based on the result. It also implements the first set of checks needed in UnitOrders, which can be followed by a separate set of checks to determine whether the viewer is allowed to see spec or the sender's team chats (and breaking if not), and then finally formatting the message to show in the chat.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

Would implementing a separate "SpectatorChat" order help here?

Spectator chat is basically team chat (for Team -1 if you wish), I wouldn't special case it.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

My thought was that the spectator "team" is a special case, because players can move into it after the game has started and it ignores the client/slot team assignment.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

I thought it might make sense to send the actual team number in the order. Not to calculate it client side. This means a player moved to spectators will just send -1 (for example) instead of his team number.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

We'll need to validate that on the receiving end to avoid exploits, but otherwise sounds reasonable to me.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:specTeam branch from 53f1ba9 to feb84d5 Apr 14, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Updated.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:specTeam branch from feb84d5 to f205947 Apr 14, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Rebased and updated as discussed on IRC.

@pchote
Copy link
Member

left a comment

Code changes LGTM, just two minor comments.

I will hold off on a 👍 until I have a chance to test this properly ingame, but anybody can do this testing, not just me.

OpenRA.Game/Server/ServerOrder.cs Outdated Show resolved Hide resolved

@abcdefg30 abcdefg30 force-pushed the abcdefg30:specTeam branch from f205947 to eafb3f0 Apr 21, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Rebased and updated.

@matjaeck
Copy link
Contributor

left a comment

I accidentally tested on bleed first and all the "issues" that I found there (and noted below if you're interested) are fixed here (I repeated these testcases). Additionally tested switching teams including joining and leaving spectator chat in the lobby and everything seems to work here as intended. Dead players now speak in the spectator team chat if available.

I have tested this in the constellation of 2 clients in team A vs 2 clients in team B with 2 clients as spectators and noticed the following: - dead player of team A could not see the spectator chat, his team chat messages have been visible for all - team chat was still available after the other player of team A lost and the game ended - when players from team A used the team chat now, it was visible to everybody - when spectators used the team chat, only other spectators saw it - when players from team B used the spec chat, only players of team B saw it
@pchote
pchote approved these changes Apr 22, 2019
Copy link
Member

left a comment

I'll trust @abcdefg30 and @matjaeck's testing here. Code LGTM, so any remaining bugs can shake out on bleed.

@pchote pchote merged commit 6163523 into OpenRA:bleed Apr 22, 2019

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
Projects
None yet
4 participants
You can’t perform that action at this time.