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

% symbol is not being displayed through third party plugins on latest beta branch. #2611

Closed
metita opened this issue Jul 21, 2019 · 72 comments
Closed
Assignees

Comments

@metita
Copy link

metita commented Jul 21, 2019

Title.

Beta:
%d%% ->[100 ]

Stable version:
%d%% ->[100%]

@SamVanheer
Copy link

#2531 is resurfacing?

@afwn90cj93201nixr2e1re
Copy link

@mikela-valve the best way is just replace % and # to unicode analog. But then we can get bug with non displayed symbols on xp.

@metita
Copy link
Author

metita commented Jul 21, 2019

#2531 is resurfacing?

Looks like.

@mikela-valve
Copy link
Member

Is this only happening on CS or for other games as well? #2531 was a fix to the SayText handler exclusively for CS.

This might be some combination of message processing that is causing this. The SayText handler by itself does not handle non-string format specifiers, so %d%% must be getting handled earlier, replaced with 100% which then has the % stripped by the SayText handler.

Before I tightened up the SayText string handling in #2531 you were allowed to have % characters at the end of the input string even though that should be invalid as a format specifier, so when I fixed that it may have broken this. I'll update the beta later today with a change that lets SayText have a bare % at the end of the string again and see if that fixes this.

@metita
Copy link
Author

metita commented Jul 22, 2019

@mikela-valve I don't have a way to test it on another game besides CS. If someone else can test it on another game that would be really helpful.

@metita
Copy link
Author

metita commented Jul 23, 2019

@mikela-valve was the supposed update to the beta postponed?

@mikela-valve
Copy link
Member

I'm just waiting on some changes to build now. I had a few more changes to make for #2237.

@metita
Copy link
Author

metita commented Jul 23, 2019

Thanks, I will update this asap once the update hits the beta branch.

@mikela-valve
Copy link
Member

Thanks, it should be updated now, build 8304.

@oaus
Copy link

oaus commented Jul 23, 2019

@mikela-valve Still not appearing with the new build 8304

@metita
Copy link
Author

metita commented Jul 23, 2019

@mikela-valve This is still happening on latest beta branch.

@SamVanheer
Copy link

Typing %d%% into chat prints this:

Solokiller :  d %
Solokiller :   d %

Note that it prints to console twice and it adds a newline on the second print. It's also inserting another space in the second print.

Normal chat text does seem to work with %%, but i haven't tested it with programmatically inserted strings.

@basuritashka How are you sending the %d%%? Through SayText or some other print function?

@metita
Copy link
Author

metita commented Jul 23, 2019

@SamVanheer client_print / client_print_color.

@SamVanheer
Copy link

Ok so that's still ending up in SayText eventually.

@Arkshine
Copy link

client_print uses TextMsg. client_print_color uses SayText if CS, otherwise TextMsg.

@metita
Copy link
Author

metita commented Jul 23, 2019

I was testing some things with AMXX and these are my results:

image

image

image

@SamVanheer
Copy link

What happens when you type in %% in chat yourself on the same server? Does it also completely remove them then?

@metita
Copy link
Author

metita commented Jul 23, 2019

@SamVanheer
say %%
say %

image

@SamVanheer
Copy link

Ok, i did some checking in AMX's code, it looks like this problem is caused by how AMX interacts with the new SayText code.

Here are some links:
https://github.com/alliedmodders/amxmodx/blob/1cc7786a4c260ca9ad55fa9fd1c8c415115ead89/amxmodx/amxmodx.cpp#L308-L402
https://github.com/alliedmodders/amxmodx/blob/75cf5f55f9b06e778fc9a87c0bbc84d00be7b805/amxmodx/string.cpp#L38
https://github.com/alliedmodders/amxmodx/blob/2559fcf00aecb19fb6e6f8de846ae37cbcd6d572/amxmodx/CLang.cpp#L269
https://github.com/alliedmodders/amxmodx/blob/5a257a7a42ce34de055d087e5460e2190a5c2ed2/amxmodx/format.cpp#L549-L811

AMX will format your text first before sending it, since SayText doesn't support complex format arguments.

Among other things it converts %% to %:
https://github.com/alliedmodders/amxmodx/blob/5a257a7a42ce34de055d087e5460e2190a5c2ed2/amxmodx/format.cpp#L777-L782

So when you pass %% it only sends %.

Now we can see that a single one is still supported when entered through chat so the question is why it's being ignored. I see that Host_Say has code to check for % to prevent passing format inputs to the client, but it doesn't do anything when the % is followed by a space or nothing.

I do see code to append '\n' at the end of the text, so the client is always going to find one more character at the end of the string, even if it ends with %.
This appears to be consistent in AMX as well:
https://github.com/alliedmodders/amxmodx/blob/1cc7786a4c260ca9ad55fa9fd1c8c415115ead89/amxmodx/amxmodx.cpp#L356-L357

I don't immediately see a cause of this problem but this should help @mikela-valve track things down.

I'd suggest trying to send %%%% to see if that does print, if so it means that repeated printf formatting is halving the number of them every time until it encounters only one and ignores it. If not, double the number of them until you see something.

If nothing shows up after 64 i think it's safe to say it isn't caused by repeated printf calls.

Now as to why it does work in the public version. In that version of the SayText handler it scans the input for %, and it replaces the character that comes after with a space if it isn't either the end of the input or a space already:

for( int i = 0; i < strlen( szMsg ); ++i )
{
	if( szMsg[ i ] == '%' )
	{
		if( szMsg[ i + 1 ] && szMsg[ i + 1 ] != ' ' )
		{
			szMsg[ i + 1 ] = ' ';
		}
	}
}

So %% becomes % here. It looks to me like AMX converts %% to % already, and the client then sees %\n and this is somehow ignored.

@Arkshine If you know of any way to log the data being sent using SayText we can see exactly what string is being sent to the client both by AMX and the game when using chat so we can figure out which data is causing this result.

I think there might be a Metamod plugin that does this but i don't know if there's anything readily available to run.

@mikela-valve
Copy link
Member

Ok, all of these explanations are starting to make sense.

So, first, the reason %'s work in in-game chat is that in-game chat is first filtered and written into a temporary string that is then inserted into the chat buffer through a loc token that resolves to that temporary string. That temporary string is not used as a format string so it is not stripped of format specifiers.

The reason that client_print and client_print_color are not working is that they send strings directly to the SayText and TextMsg handlers. The first string parameter in the message is used as a format string that is allowed up to four %s format specifiers for dynamic replacements. I fixed the validation of that format string in #2531 which is what broke this for the client_print functions as it started filtering out single percent signs in the format string as they are not valid format specifiers.

This can be fixed as @SamVanheer suggested by sending %%%% through client_print_color which should resolve to %% in the SayText handler and be kept in the input string as a single %. The other fix would be for AMXX to send the formatted string (as it's clearly pre-formatting the string and sending it formatted) as a second string parameter to SayText with "%s" as the first parameter. Then any text in the second string parameter would be printed verbatim.

@metita
Copy link
Author

metita commented Jul 23, 2019

@mikela-valve

Testing with /say3color and /say4color:

image

image

image

@SamVanheer
Copy link

@Arkshine What's your opinion on this? Do you think this should be fixed in AMX's code? It seems like the best place to put it, since it reduces the amount of changes required to just the implementations for those message sending functions.

@Arkshine
Copy link

I did not really follow the last changes lately. I remember vaguely I wanted to use %s as first argument to fix exploits, but could not because Alfred filtered it (or maybe it was TextMsg?). Sorry, I'm a bit out of the loop. Either way, can we now use %s for both messages? If so, I don't see any problems to change it in AMXX (at least I don't see potential breakage at first glance).

@SamVanheer
Copy link

@mikela-valve says you can send two strings, %s followed by the actual message to print it without the checks against % interfering with printing.

@mikela-valve
Copy link
Member

@Arkshine Currently what @SamVanheer is describing is for SayText only based on the changes I made in #2531. The format string for SayText is allowed to have 0-4 plain %s format specifiers to match the 0-4 replacement strings you can send in that message.

@mikela-valve
Copy link
Member

I'm also not sure why @basuritashka's last test printed two %'s. I looked through the AMXX code and everything looked like it worked the way that I thought it would, i.e. %%%% would be replaced with %% which would then be let through the SayText handler and printed as % in chat, but it looks like the entire %%%% came through?

@SamVanheer
Copy link

That's why it would be very helpful to see what was actually sent to the client once the string made it through AMX's code, so we can see what exactly is getting received. I'm not aware of any plugin that actually does this, however.

@Arkshine
Copy link

Random test with:

    client_print_color(id, print_team_default, "100%");
    client_print_color(id, print_team_default, "[100%]");
    client_print_color(id, print_team_default, "--");
    client_print_color(id, print_team_default, "100%%");
    client_print_color(id, print_team_default, "[100%%]");
    client_print_color(id, print_team_default, "--");
    client_print_color(id, print_team_default, "100%%%");
    client_print_color(id, print_team_default, "[100%%%]");
    client_print_color(id, print_team_default, "--");
    client_print_color(id, print_team_default, "100%%%%");
    client_print_color(id, print_team_default, "[100%%%%]");

What AMXX sends (which is expected):

%% -> %
%EOS -> %
%unconsumed character ->

image

Latest CS stable | AMXX SayText with %s:
%s can't be used.

image

Latest CS stable | AMXX SayText without %s (default):
Is it expected? I would have expected at least one occurrence with a %.
image

Latest CS beta | AMXX SayText without %s (default):

image

Latest CS beta | AMXX SayText with %s:
Looks like it's the way to go. Once moved to stable, I can make the change in AMXX.
Though the extra lines in console are odd. Doesn't appear in the chat.
image

Highlighted to see the spaces.

I'm quite confused about the current stable version. I've used latest without beta on server/client (3 Apr 2019) and I'm not sure how OP can see %. Or did I miss something?

@metita
Copy link
Author

metita commented Jul 24, 2019

@Arkshine This is being tested on latest beta branch.

image

@Arkshine
Copy link

I know, I tested on latest stable and beta. I'm just concerned how you see % on the stable version when in my test it doesn't appear. But I guess it doesn't matter since eventually the beta will be moved to the stable branch.

@metita
Copy link
Author

metita commented Jul 28, 2019

If I understood it correctly, do you mean this?

image

Both CenterPrint and CenterPrintString are being displayed correctly on latest stable and latest beta branch.

@Arkshine
Copy link

No, that's not what I mean. If AMXX is patched, you can't send language keys with print_center (you can with others print_).
You can't test unless you compile AMXX with the patch, which is just:

image
(actually, you can test by sending directly the message, with message_begin, etc.)

@metita
Copy link
Author

metita commented Jul 28, 2019

Oh okay, I just misunderstood your comment thinking it was related to actual AMXX builds (1.9 - 1.10).

@mikela-valve
Copy link
Member

mikela-valve commented Jul 31, 2019

@Arkshine The print_center issue will be fixed in the next beta.

I also checked out why the extraneous newlines were being shown in the console. Are there embedded newlines in the string arguments being passed to Saytext in AMXX? The format string is stripped of newlines but the string parameters are not being stripped in the version printed to the console. If it is that, I'd rather leave the version printed in the console identical to the input so I'm going to leave it like it is for the moment unless there are reasons to change it to be printed differently.

@Arkshine
Copy link

Arkshine commented Jul 31, 2019

@mikela-valve Thanks, much appreciated.

About the newline, it seems historically AMXX has always appended a new line before sending SaytText message:

msg[len++] = '\n';	// Client expects newline from the server

Old code. I can't really say if it was necessary. Is it still needed now? If it's safe, then I don't mind removing it.

@mikela-valve
Copy link
Member

Yep, I don't think that's necessary anymore. The message string from say does get sent to SayText with a newline on it but I tested stripping all newlines from the SayText string parameters and everything still worked as expected, with fewer newlines in the console as expected.

@mikela-valve
Copy link
Member

print_center should now work as expected in the current beta.

@di57inct

This comment has been minimized.

@Arkshine
Copy link

Arkshine commented Oct 25, 2019

@mikela-valve

It seems new lines are actually stripped when using TextMsg with
It seems that new line at the end of string is stripped when using TextMsg with

WRITE_STRING("%s");
WRITE_STRING(msg);

New lines are still allowed in the middle of a string.
So, if you do message\n\n, the last \n will be stripped and new line will happen with the first \n.

It doesn't happen with SayText.

Is something can be done for TextMsg, please?

@metita
Copy link
Author

metita commented Oct 25, 2019

@mikela-valve @Arkshine

Not sure if related but a empty space is created when using client_print_color and sometimes a couple of strings (via console_print) will get merged into a single one (look at the "space" text)

Note that console_print was used as:

console_print( id, "hello")
console_print( id" "hello2") 

(That should be enough to test that)

image

There is also a problem with how % is getting displayed through print_center. (Cannot confirm this)

@Arkshine
Copy link

@basuritashka AMXX inserts by default a newline. For SayText it's no more needed with latest game version. Just a matter to update AMXX. I've created an issue alliedmodders/amxmodx#766

@mikela-valve
Copy link
Member

@Arkshine Unfortunately without examining all of the translation strings I can't remove that newline stripping as I'm unsure of what in the game resources might unknowingly have them. It looks like the same single newline stripping is done to all four of the string inputs, could you double the newline on any string args passed through to TextMsg?

@Arkshine
Copy link

Alright. I can double the newline, it's not that dramatic. Thanks for answering.

@metita
Copy link
Author

metita commented Oct 25, 2019

@Arkshine Multiple console_print in the same function are not getting displayed correctly, is that related? Because when using more than 1 console_print in the same function the game will just release a single string with everything merged on it. (Look screenshot from latest post from me)

@Arkshine
Copy link

Arkshine commented Oct 25, 2019

@mikela-valve Sorry to bother you again.

Just tested WRITE_BYTE("%s"); WRITE_STRING(msg); with TextMsg under DoD and it doesn't work for print_center and print_chat (it shows %s). It works for print_notice and print_console.
The others mods looks fine.

Also, under CS, with TextMsg and print_center, newlines are stripped. It works on the others mods.

@mikela-valve
Copy link
Member

It looks like DoD doesn't currently support that through TextMsg. A few of the mods have their own implementations of these message handlers. Report that as a new issue and I'll fix it when I have a chance.

For CS, are you referring to stripping of newlines in the format string or the message args?

@metita
Copy link
Author

metita commented Oct 25, 2019

@Arkshine print_center is also displaying %s on CS NS version, I know it is not supported here but its worth mentioning.

@Arkshine
Copy link

@mikela-valve
About CS, message args.

E.g.

WRITE_STRING("%s");
WRITE_STRING("Hello \n World");

@Arkshine
Copy link

print_center is also displaying %s on CS NS version, I know it is not supported here but its worth mentioning.

I guess I will have no choice to check mods and use message args only for the official ones. 😩

@mikela-valve
Copy link
Member

@Arkshine It looks like TextMsg only treats carriage returns as valid line splitting characters. See if converting \n’s and \f’s to \r’s works.

Were ordinary newlines working for TextMsg before when you were passing them in the primary string?

@Arkshine
Copy link

@mikela-valve

\r works for CS.
\r and \n seems to work for the others games.

For CS, with or without format string, \n is stripped. I can't test on previous build.
But I'm pretty sure it was working before because looking at some plugins, they use \n.
It can be fixed in AMXX, but if possible, can \n be allowed again?

@mikela-valve
Copy link
Member

@Arkshine I've been looking back through the changes for TextMsg and I don't see how \n ever worked for HUD_PRINTCENTER. There's explicit checks that allow \r to be used as a line separator and to skip \n so it looks to me like it should always have worked this way. Additionally the checks are via integer values, not characters (i.e. 10 rather than \n) so it shouldn't be the case that CS is interpreting those characters differently.

It looks like all of the other modes of TextMsg either properly interpret \r and \n or auto-convert all \r to \n. Is it possible the plugins you checked were using a different HUD mode?

@Arkshine
Copy link

Arkshine commented Oct 28, 2019

@mikela-valve

Honestly, I can't remember. I know plugins using \n for sure.
Here some result from an old plugins dump:
image
print_center = 4 and ^n = \n

But a fast search on our forum, and I see reference in comments where ^n doesn't work and ^r needs to be used (2012 and 2017). So, you may be right.

Either way, what about converting \n to \r then? The point is having the same behavior for all mods.

That's said, I've already pushed a fix in AMXX to convert them for CS only. Ideally, I would prefer a fix in the game for the sake of consistency if possible, but it would not be dramatic otherwise.

@djearthquake
Copy link

djearthquake commented Oct 29, 2019

so it shouldn't be the case that CS is interpreting those characters differently.

The Linux CS I reported does not have 'console' Carriage Return phenomenon while OP4 does. Matching Amxx 5260 ⌘773. Buffalo, New York. 5263 Resolves this. Thank you.

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

No branches or pull requests

9 participants