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

Private chat mode #1

Closed
wants to merge 6 commits into from
Closed

Private chat mode #1

wants to merge 6 commits into from

Conversation

AKMDM
Copy link

@AKMDM AKMDM commented Dec 28, 2020

This adds a new chat mode to Zandronum, allowing the player (or the server) to send chat messages privately to another player. Nobody else can see these messages and they won't be logged into the server's chat log, unless the server is sending or receiving a private message.

… other. The sender can either pass the player's name with "sayto" or a player's number with "sayto_idx". Alternatively, players can send private messages to the server by passing "server" as a name.
src/c_console.cpp Outdated Show resolved Hide resolved
src/cl_commands.cpp Outdated Show resolved Hide resolved
@AKMDM
Copy link
Author

AKMDM commented Jan 3, 2021

I'm finishing up on these adjustments, if that's everything. I should have a new commit ready very soon.

EDIT: Just committed the adjustments, I hope these are okay. One more thing, I added extra code to allow the client to cycle backwards through the player list if they're pressing TAB + SHIFT. Otherwise, it cycles forward with just TAB.

@TorrSamaho
Copy link
Owner

I'm finishing up on these adjustments, if that's everything. I should have a new commit ready very soon.

Yes, I'm done checking and commented on everything I noticed.

src/chat.cpp Outdated Show resolved Hide resolved
src/chat.cpp Outdated Show resolved Hide resolved
src/chat.cpp Outdated Show resolved Hide resolved
src/chat.cpp Outdated Show resolved Hide resolved
src/chat.cpp Outdated Show resolved Hide resolved
src/chat.cpp Outdated Show resolved Hide resolved
@TorrSamaho
Copy link
Owner

Sorry for overlooking the changes in chat.cpp last time. I didn't realize that github automatically hides changed files with too many changes...

@AKMDM
Copy link
Author

AKMDM commented Jan 10, 2021

No worries! I'm glad we took a good look in this file to see where things need to be improved for next time. I'll probably have these changes ready for next week, if that's okay. Thanks for the feedback!

@TorrSamaho
Copy link
Owner

Sure, there's no rush. Have a nice week!

@AKMDM
Copy link
Author

AKMDM commented Jan 10, 2021

Thank you, you as well. :)

@AKMDM
Copy link
Author

AKMDM commented Jan 16, 2021

I submitted a couple of new commits.

  • I fixed the private chat mode so that it now only works in online games and bots cannot be selected. Also, if the client tries to start typing a private message when there's no other clients, the server is selected instead.
  • chat_CanSendPrivateMessage( ULONG ulPlayer ) is replaced with chat_IsPlayerValidReceiver( ULONG ulPlayer ) and chat_FindValidReciever ( void ). The former checks if a player is eligible to become a receiver of a private message, and the latter checks for a valid player that can be a receiver. Otherwise, the server is chosen.
  • The possibility of any infinite loops occurring in the code should be resolved now.
  • The server can now be selected by passing -1 to sayto_idx.

I also added more commits, some of which aren't necessarily related to the private chat:

  • Players cannot name themselves as "server" in online games anymore. This should avert potential impersonation of the server, and the possibility that someone tries using sayto "server" when there's a player with the same name.
  • The chat cursor now blinks while the playing is creating a message. If they're typing or passing any input, the cursor remains visible. The additions are somewhat complicated, but I had to make sure displayString still looks convincing on the screen while the cursor is supposed to be invisible by taking account the width of the '_' character.
  • I touched up and re-added my commit which adds the new EVENT type: GAMEEVENT_CHAT and the ACS function GetChatMessage( int player, int index ). The biggest difference is that up to three chat messages from the player (or server) are stored in memory, so modders can also pass an index between 0 and 2 to get that corresponding chat message, with 0 being the oldest and 2 being newest. This is in case we receive more than one chat message from the client in a single tic, in which the last chat message replaces the older one.

EDIT: An index of 0 returns the newest chat message, and 2 returns the oldest chat message.

src/chat.cpp Outdated Show resolved Hide resolved
@TorrSamaho
Copy link
Owner

Thanks for the new patches! However, to keep things more manageable, can you do me a favor and remove the separate commits cd80940, d2fc101, f976178 from this merge request? Let's first get the private chat system merged, before extending it or adding other chat features.

@AKMDM
Copy link
Author

AKMDM commented Jan 17, 2021

Sure, will do.

EDIT: Is it okay if I open up a new pull request? I'm having trouble removing those few commits on my end and ended up making some mistakes.

@AKMDM
Copy link
Author

AKMDM commented Jan 17, 2021

I managed to fix it, Torr.

// [AK] Allows players (or the server) to send private messages to other players.
//
void chat_PrivateMessage( FCommandLine &argv, const ULONG ulReceiver, const bool bServerSelected )
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a separate argument bServerSelected to encode the server as target instead of just using ulReceiver == MAXPLAYERS?
BTW: Sorry that this comment only shows up now. I posted it last week but unfortunately using "Start a review" instead of "Add single comment". I just now realized that you couldn't see it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for asking. I added bServerSelected to ignore all the extra checks required to validate another player being the receiver, if the client explicitly chooses to the server. If the client wants indicates that they want to send a private message to the server using sayto server or sayto_idx -1, then it's not necessary to check if the receiver is invalid, a bot, ourselves, etc.

Additionally, if bServerSelected is true, then the function chat_FindValidReceiver won't be entered at all. If we did enter that function and there was only one client connected, then the message "There's no other clients to send private messages to. The server will be selected instead." would always appear in the console, even if the client intentionally chose to send a private message to the server.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also bypass all those checks by using ulReceiver == MAXPLAYERS instead of bServerSelected. For instance, you could add const bool bServerSelected = (ulReceiver == MAXPLAYERS); in the first line of the implementation of chat_PrivateMessage and keep the rest of chat_PrivateMessage untouched (except for removing the argument bServerSelected). Perhaps there is a reason why you don't want to encode the information bServerSelected in the argument ulReceiver, but right now I don't see it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I can change that quickly and resubmit it if you're still around.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm still around.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I gotta go now. I should be able to add this tomorrow though.

@TorrSamaho
Copy link
Owner

I managed to fix it, Torr.

Thanks! Looking all good now, except for the one comment I didn't properly post last week. Sorry for that.

src/chat.cpp Outdated
{
ULONG ulIdx;
FString ChatString;
const bool bServerSelected = ulReceiver;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be const bool bServerSelected = (ulReceiver == MAXPLAYERS);?

EDIT: Now I'm really gone for today ;).

Copy link
Author

@AKMDM AKMDM Jan 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I forgot to fix that (really stupid mistake). :(

EDIT: Uploaded the now fixed commit, so sorry about that!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@TorrSamaho
Copy link
Owner

I compressed, rebased and added this patch.

@TorrSamaho TorrSamaho closed this Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants