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

show_activity() functions overhaul #505

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

OciXCrom
Copy link
Contributor

@OciXCrom OciXCrom commented Jul 24, 2018

Since all of the show_activity functions have always been a huge problem for users who want to add colored chat messages to their plugins, especially the default AMXX plugins, I decided to make a complete overhaul of the functions and make changes that will alow users to actually use color chat with them.

The changes include:

  • complete rewrite of the show_activity, show_activity_id and show_activity_key functions - I decided to do this because of the very poor design of the code inside them;
  • added global static variables that will control whether the show_activity functions will used colored or default chat messages and the message color itself
  • added a new stock called send_activity_message that will send a message depending on the global bool - if it's true, it will send a message using client_print_color, otherwise it will use client_print;
  • added a new stock called set_activity_rules that can be used to set whether or not the show_activity functions will use colored chat messages, as well as the actual color/sender of them - this functions similarly to set_hudmessage, as the parameters from the function stay unchanged until the next call;
  • added a few more stocks that prevent usage of repeating code inside of the actual show_activity stocks; these stocks include: activity_init (prepares the amx_show_activity cvar for usage, creates a dummy cvar if not found, sets default values for the global variables and returns the amx_show_activity cvar value as an integer), get_activity_prefix (returns the PLAYER or ADMIN prefix depending on the client's admin status) and can_see_admin_name (checks whether the client can see the admin name).

With the changes made, here's an example on how to send a colored chat message using the show_activity functions:

set_activity_rules(true, print_team_default);
show_activity_key("ADMIN_SLAP_1", "ADMIN_SLAP_2", name, name2, damage);

Alternatively, the set_activity_rules function can be used inside plugin_init or some other forward in order to make all show_activity functions in the code use colored messages.

Another huge problem with these messages is that all of them demand the usage of language keys or format the message in a specific way and they don't allow you to send the message to a specific client or all clients at once. To "fix" this problem, I added a completely new stock called show_activity_custom with the following arguments:

stock show_activity_custom(id, const name[], const fmt[], any:...)

This function combines the power of all other show_activity function and allows users to create simple chat messages that obey the amx_show_activity cvar. It allows users to send a message to a specific client or all clients at once. It also supports the usage of language files or a simple string. The way it works is that it simply replaces a special placeholder inside the message with the admin's name or completely removes it if the client isn't supposed to see the name. In addition, it also has an option to use the ADMIN or PLAYER prefix from the other functions. The placeholders by default are $an for the admin name and $pr for the prefix. They are both changeable by the new set_activity_rules stock:

stock set_activity_rules(bool:color, sender = print_team_default, ph_name[] = "$an", ph_prefix[] = "$pr")`

An example of how to use this function to display an activity message to all clients without a language key:

new szName[32], iValue = 100;
get_user_name(id, szName, charsmax(szName));
show_activity_custom(0, szName, "$pr ^3$an ^1set the cool value to ^4%i", iValue)

In addition to this function, I also included another stock that can be used to replace the special keywords in a message:

stock replace_activity_data(id, const name[], buffer[], len)

@OciXCrom
Copy link
Contributor Author

I just got an idea on how to avoid using the huge switch statements in all the functions. I'll update the code tomorrow.

@OciXCrom
Copy link
Contributor Author

OciXCrom commented Jul 25, 2018

I updated the code. The stocks are now 3-4 times smaller than before while their functionallity is kept the same.

@SmileYzn
Copy link
Contributor

Store is_user_admin into a var at stock can_see_admin_name(id)

✌️

@OciXCrom
Copy link
Contributor Author

OciXCrom commented Jul 25, 2018

There's no need to do that. It's used only once per function call since it's in a "case".

@WPMGPRoSToTeMa
Copy link
Contributor

WPMGPRoSToTeMa commented Aug 4, 2018

How can I put $an/$px in the string if I need it? Is there an escape feature for $?
Your show_activity_custom doesn't support LANG_PLAYER.
Your and old show_activity also looks broken for LANG_PLAYER.

@OciXCrom
Copy link
Contributor Author

OciXCrom commented Aug 4, 2018

@WPMGPRoSToTeMa I changed the code a bit. I changed the set_activity_color function to set_activity_rules and added arguments for changing the $an and $px symbols. Actually a bigger problem would be if player names contain these symbols. Any ideas on how to prevent this? I wasn't able to find a function that gets the buffer without formating it.

I'm not sure if LANG_PLAYER can be even implemented here. If you have an idea how to do it, let me know, otherwise I'll add a @note that LANG_PLAYER isn't supported with these functions.

@WPMGPRoSToTeMa
Copy link
Contributor

@OciXCrom you can implement it using SetGlobalTransTarget.

replace_all(buffer, len, activity_ph_prefix, prefix);
}

static cansee; cansee = can_see_admin_name(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you use static here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of my understanding that static should be used in functions that are called each frame. I can see that many people are confused on whether this is true or not - https://forums.alliedmods.net/showthread.php?t=309363

Copy link
Contributor

Choose a reason for hiding this comment

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

Just use new everywhere.

@OciXCrom
Copy link
Contributor Author

OciXCrom commented Aug 5, 2018

@WPMGPRoSToTeMa @rsKliPPy

I changed all the static declarations with new and fixed LANG_PLAYER. Let me know if there's something else that can be improved.

PS: someone should make a proper tutorial on when to use static. Too many people have wrong understanding of it.

static player;

for(new i; i < pnum; i++)
for(new i, player; i < pnum; i++)
{
player = players[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

new should be here.

Copy link
Contributor Author

@OciXCrom OciXCrom Aug 5, 2018

Choose a reason for hiding this comment

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

Hm? It's already new.
// Nvm, that's the outdated review.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OciXCrom I mean inside for-loop block.

new player = players[i];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WPMGPRoSToTeMa It's declared inside the for-loop body.

for(new i, player; i < pnum; i++)

I think you're viewing the old version of the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OciXCrom I mean its declaration should be moved to its first assignment.

Copy link
Member

@Arkshine Arkshine Aug 6, 2018

Choose a reason for hiding this comment

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

You fool, never quote me! But yes, it doesn't matter much here as said KlIPPy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rsKliPPy That's exactly what I did. I declared the variable before the ";" in the loop like I always do, so I don't see a problem. I've always been told that declaring them in the actual loop is bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OciXCrom look the difference diffchecker/mergely/gist.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OciXCrom have you looked into this assembly? Do you have any questions/thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WPMGPRoSToTeMa I can't say that I understand much what that even means. Maybe you can explain? If it's not something extremely important, let's just keep it declared in the for() body for the sake of everyone knowing that that is the better way to do it.

@Arkshine
Copy link
Member

Can you reflect the latest changes in your first post?

@OciXCrom
Copy link
Contributor Author

@Arkshine Sorry, I completely forgot about the description there. It's updated now.

@OciXCrom
Copy link
Contributor Author

I improved show_activity_custom by making it remove the extra whitespace that will appear in the message when the admin's name gets removed.

Before:

ADMIN OciXCrom slap Player
ADMIN  slap Player

After:

ADMIN OciXCrom slap Player
ADMIN slap Player

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants