ValveSoftware / halflife

Half-Life 1 engine based games
Other
3.57k stars 596 forks source link

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

Closed metita closed 4 years ago

metita commented 4 years ago

Title.

Beta: %d%% ->[100 ]

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

SamVanheer commented 4 years ago

2531 is resurfacing?

afwn90cj93201nixr2e1re commented 4 years ago

@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 commented 4 years ago

2531 is resurfacing?

Looks like.

mikela-valve commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

mikela-valve commented 4 years ago

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

metita commented 4 years ago

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

mikela-valve commented 4 years ago

Thanks, it should be updated now, build 8304.

oaus commented 4 years ago

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

metita commented 4 years ago

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

SamVanheer commented 4 years ago

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 commented 4 years ago

@SamVanheer client_print / client_print_color.

SamVanheer commented 4 years ago

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

Arkshine commented 4 years ago

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

metita commented 4 years ago

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

image

image

image

SamVanheer commented 4 years ago

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

metita commented 4 years ago

@SamVanheer say %% say %

image

SamVanheer commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@mikela-valve

Testing with /say3color and /say4color:

image

image

image

SamVanheer commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@Arkshine This is being tested on latest beta branch.

image

Arkshine commented 4 years ago

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 commented 4 years ago

@Arkshine That's weird. That was tested on several servers on latest stable version and % is being printed correctly, I am testing it right now on a hosted server with ReHLDS and it is not being displayed on latest stable version.

Latest stable version:

Console output: image

Code: image

In-game:

image


Latest stable version from a friend server. ~>[I]nz'KiN.- (ID: 783) DISCONNECTED (Mode: Nightmare, Human Sharpshooter Lvl. 8 [100.0% HP], 24/07/2019 - 10:33:00)

metita commented 4 years ago

We did some test with @oaus

Code [1]: image

Output [1]: image


Code [2]: image

Output [2]: image


Code [3]: image

Output [3]: image


Code [4]: image

Output [4]: image

mikela-valve commented 4 years ago

Ok, I should have this fixed in the next beta. I spent some time debugging through the SayText handler and formatting code and noticed that certain format strings were incorrectly being routed to the localization formatter and not the printf formatter which was why 100%% was being printed as 100%% and not 100%.

I also replaced the old format-stripping process in TextMsg with the new %% and %s validation that SayText has, so both of those paths should work the same in the next beta.

metita commented 4 years ago

Great, I will update this once beta hits the beta branch and will close this if fixed. Thanks for the hard work!

metita commented 4 years ago

@mikela-valve There was an update to the Beta Branch for CS 1.6 but looks like everything still the same, or it wasn't included in this one?

mikela-valve commented 4 years ago

Build 8308 should have the fix in it for properly handling the format string. Does it not print properly for %d%%%% now? It won't be fixed for %d%% until AMXX is updated to send the print string as a parameter instead of as the format string.

metita commented 4 years ago

Gotcha, good to know about %d%%.

%d%%%% its doing his job flawlessly.

image

image

I think I will leave this open for a while till AMXX release an update about being able to use %%.

Arkshine commented 4 years ago

Tried both message with %s using https://github.com/ValveSoftware/halflife/issues/2611#issuecomment-514780120

SayText image

TextMsg image

Thanks for adding support in TextMsg. Are the extra newlines in SayText from the console can be fixed somehow?

mikela-valve commented 4 years ago

I’ll look more into that. It must be something that the SayText handler is doing that the TextMsg handler is not as they’re both routed to the same functions later to handle formatting and printing the messages.

Arkshine commented 4 years ago

It looks like some plugins are using game language key such as client_print(id, print_center, "#Cstrike_TitlesTXT_Not_Enough_Money"); (it uses TextMsg)

It seems with %s, it's still working with print_chat and print_console but not with print_center (it shows %s). If possible, is something can be done, at least for the sake of consistency, please? that would be great.

It's not that dramatic because there are not much references (and there is alternative solution like client_printex), but if AMXX uses %s, I would prefer to avoid any breakage and obscure checks to keep compatibility.

di57inct commented 4 years ago

If I'm not mistaking, the same problem is present in the stable version too. Once this is properly fixed, can you push it on it too please? Thank you.

metita commented 4 years ago

If I'm not mistaking, the same problem is present in the stable version too. Once this is properly fixed, can you push it on it too please? Thank you.

There is a way to display % properly on the latest stable version but seems to not be working at all (I am a bit confused with this method), this should be added in the next official release and if you are using AMXX you need will need to wait till Arkshine release an update to allow %% usage to display %, otherwise you will need to use %%%%

metita commented 4 years ago

I will be closing this because it is fixed. @Arkshine feel free to update this whenever you want when AMXX release an update regarding the usage of %%.

Arkshine commented 4 years ago

Is print_center fixed as well? I can't test it atm. Also, if an update happens, it will be only when CS changes are available on the stable branch.

justgo97 commented 4 years ago

I guess something should be added to the new client build that allows AMXX to know which build number the client is to allow it to handle both old and new builds correctly ?

Arkshine commented 4 years ago

That's not the point. It will work with print_chat and print_console ; but not with print_center. If anything, print_center should use the same logic as those both.

metita commented 4 years ago

@Arkshine What do you mean with print_center? Not displaying %s correctly?

Like this? image

Arkshine commented 4 years ago

^ https://github.com/ValveSoftware/halflife/issues/2611#issuecomment-514962589

metita commented 4 years ago

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 commented 4 years ago

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.)