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

Modernize and improve imessage.sma #575

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

Conversation

OciXCrom
Copy link
Contributor

@OciXCrom OciXCrom commented Sep 12, 2018

The changes include:

  1. Added new cvar amx_imessage_only_dead to control whether or not the messages will show only to dead players (requested in About the scrollmsg and imessage plugins #571).
  2. Added new cvar amx_imessage_x_pos to set the X position rather than having it hardcoded in the .sma file.
  3. Added new cvar amx_imessage_y_pos to set the Y position rather than having it hardcoded in the .sma file.
  4. Added new cvar amx_imessage_holdtime to set the holdtime rather than having it hardcoded in the .sma file.
  5. Usage of create_cvar and bind_pcvar_* functions instead of the old ones.
  6. Change show_hudmessage to ShowSyncHudMsg to prevent overlapping messages if the repeat time is shorter.
  7. Consistent variable names, semicolons and brackets.

@all85100
Copy link

And scrollmsg plugin display only dead fix it?

@OciXCrom
Copy link
Contributor Author

@all85100 It will be done later in a separate PR. This one is only for imessage.

@Arkshine
Copy link
Member

Just a note about the next time you do a lot of changes in a plugin, please try to break down in multiple commits. As much as you can. It will be easier to review. Here, I would have made a commit for the style, the moving code, bind_cvar, etc.

@OciXCrom
Copy link
Contributor Author

@Arkshine Got it.

@IgnacioFDM
Copy link
Contributor

You should use #pragma semicolon 1 if you're going to use semicolons.

@OciXCrom
Copy link
Contributor Author

@IgnacioFDM That's not necessary. There's no need to force users to use semicolons. It's just a coding style. Half of the plugins have semicolons, so it's just right to add them where they're missing.

@IgnacioFDM
Copy link
Contributor

I mean, if the entire plugin is going to have semicolons, for consistency it's best to tell the compiler to keep it that way. Otherwise you end up with what you are trying to fix with this PR: Some lines have semicolons, some don't.

@OciXCrom
Copy link
Contributor Author

@IgnacioFDM I added #pragma semicolon 1 before I submitted the PR to check if I missed semicolons somewhere, so no, there's not any missing. Future PRs should follow the same style and be careful when adding semicolons. The same thing is with the .inc files - there's no #pragma in them, but they all have semicolons.

@rsKliPPy
Copy link
Contributor

rsKliPPy commented Sep 13, 2018

There's no #pragma semicolon in .inc files because that directive isn't local to the file, it will apply to any files including that one. It's okay to add it in this case. It's clear that the style that is agreed upon is to use semicolons consistently; same in SM.

@OciXCrom
Copy link
Contributor Author

Yes, but when beginners want to make changes to the file for their server, they probably won't know that they must use semicolons. It's not that big of a deal really. Let's see what @Arkshine has to say.


bind_pcvar_num(create_cvar("amx_imessage_only_dead", "0", _, "Set to 1 to show info messages only to dead clients", true, 0.0, true, 1.0), g_amx_imessage_only_dead);
bind_pcvar_float(create_cvar("amx_freq_imessage", "180", _, "Frequency in seconds of info messages", true, 0.0), g_amx_freq_imessage);
bind_pcvar_float(create_cvar("amx_imessage_x_pos", "-1.0", _, "X position for info messages", true, -1.0, true, 1.0), g_amx_imessage_x_pos);
Copy link
Contributor

Choose a reason for hiding this comment

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

Bound logic doesn't fit good here, because the valid values are 0.0 <= x <= 1.0 || x == -1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but it's the only available way to set the boundaries. Anyways, values from -1.0 to 0.0 won't output any errors so it's not a big deal. It can be done via hooking the cvar and checking it there, but I don't know if it's necessary to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OciXCrom this can be an example for custom validation. And I think it would be better to add a param for custom filter in create_cvar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this may be very useful. The params in create_cvar definitely need some rework.

@mlibre2 mlibre2 mentioned this pull request Jul 7, 2023
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.

6 participants