search

ValveSoftware / halflife

Half-Life 1 engine based games
Other
3.71k stars 625 forks source link

HL - Weapons events are sometimes called twice #1621

Open JoelTroch opened 9 years ago

JoelTroch commented 9 years ago

On vanilla Half-Life, weapons events are sometimes called twice, this means that a single shot can eject 2 shells, play the firing sound twice and/or paint 2 decals.

This issue mostly happens with firearms (Glock, Python, MP5 and Shotgun), other weapons may be affected.

I haven't been able to reproduce this on Blue Shift and Opposing Force.

I made a video to understand the issue better, you can view it on YouTube here : https://www.youtube.com/watch?v=WKEImTN9H3o

Notice that I have fired 2 whole magazines (100 shots in total) and that the counter indicates that the event has been called 104 times.

If you take a very close look at 9 seconds on the video when I fire the 74th bullet, you can see the counter reporting "Event has been called 76 times" and 2 shells being ejected. You can see the same at 11 seconds when I have 5 bullets left in the second magazine.

dtugend commented 9 years ago

I am guessing that this is a time interpolation issue due to the client time being adjusted to match the server time on the client side (client.dll).

I other words m_flNextPrimaryAttack gets 0.0 or less two or more times on the client. You could check for warping in gpGlobals->time on the client.dll (it is updated in cl_dll/hl/hl_weapons.cpp/HUD_WeaponsPostThink also m_flNextPrimaryAttack and so on are updated there).

You will notice that the code for reacting to m_flNextPrimaryAttack and so on is quite different on the client and serverside (check cl_dll/hl/hl_weapons.cpp/CBasePlayerWeapon::ItemPostFrame and dlls/weapons.cpp/CBasePlayerWeapon::ItemPostFrame).

I am not sure how to fix this properly without changing really much, also would be interesting if the original Half-Life had the same issues.

I have no explanation why this doesn't happen in Blue Shift and Opposing Force, maybe they have different default networking settings, or your network test environment was different?

dtugend commented 9 years ago

For possible solutions one would need to keep in mind that people can rewind and forward the demo in the viewdemo demoplayer.

JoelTroch commented 9 years ago

I am using default settings everywhere except for keys binding, video, sound, player (name/model) settings.

malortie commented 9 years ago

@JoelTroch

While I cannot make a new branch, at this time, I would like to suggest you the following: in dlls/weapons.cpp, find CBasePlayerWeapon::ItempostFrame. Find the following two code blocks:

This is the code for handling secondary attack events.

if ((m_pPlayer->pev->button & IN_ATTACK2) && CanAttack( m_flNextSecondaryAttack, gpGlobals->time, UseDecrement() ) )
    {
        if ( pszAmmo2() && !m_pPlayer->m_rgAmmo[SecondaryAmmoIndex()] )
        {
            m_fFireOnEmpty = TRUE;
        }

This is the code for handling primary attack events.

else if ((m_pPlayer->pev->button & IN_ATTACK) && CanAttack( m_flNextPrimaryAttack, gpGlobals->time, UseDecrement() ) )
    {
        if ( (m_iClip == 0 && pszAmmo1()) || (iMaxClip() == -1 && !m_pPlayer->m_rgAmmo[PrimaryAmmoIndex()] ) )
        {
            m_fFireOnEmpty = TRUE;
        }

Add a return statement after both m_fFireOnEmpty seen above.

m_fFireOnEmpty = TRUE;
return;

Additionally, Seeing this line after SecondaryAttack():

SecondaryAttack();
m_pPlayer->pev->button &= ~IN_ATTACK2;

I recommend adding this equivalence in the primary attack block, after PrimaryAttack():

PrimaryAttack();
m_pPlayer->pev->button &= ~IN_ATTACK;

Hope it helps.

dtugend commented 9 years ago

The return statement would make it not play the empty sound I guess.

Also I am guessing / hoping that @JoelTroch only checked / printed after the check for if the clip is empty in PrimaryAttack / SecondaryAttack.

Also not sure if the button state trickery is a good idea. Also you are only adjusting the server side, not the client side.

JoelTroch commented 9 years ago

I tried @malortie's code on a clean SDK, it just make the bug more "visible" and as @ripieces stated, the empty sound is no longer. I tried the same fixes client side and it doesn't work.

@ripieces The "event counter" you see in the video is coded client side, it's just a CVAR in CHud which is incremented and displayed at the end of EV_FireMP5 in ev_hldm.cpp.

JoelTroch commented 9 years ago

UPDATE: This issue isn't present if you compile without the CLIENT_WEAPONS preprocessor, so I guess it's related to client prediction

malortie commented 9 years ago

Thanks for trying the suggestion.

Glad to see that a bit more of the nature of the problem was identified.

@JoelTroch, Just to be sure that I correctly understand the situation:

When not compiling using CLIENT_WEAPONS, the issue (events called twice) is no longer a problem, or are you speaking about the fact the the empty sound is not playing?

JoelTroch commented 9 years ago

@malortie I was talking about the issue.

The empty sound not playing happens if you compile with CLIENT_WEAPONS with your suggested fixes (return after m_fFireOnEmpty and input tricks)

malortie commented 9 years ago

I setup up two variables, m_iPrimaryAttackCount & m_iSecondaryAttackCount which hold values on how many times both PrimaryAttack() & SecondaryAttack() get called.

When I was testing, without return statements added as in my suggestions, changes made , I noticed that when you keep holding the IN_ATTACK button (primary attack), m_iPrimaryAttackCount keeps increasing, even when out of ammunition.

Having put the return statements and using a weapon(.i.e: 9mmAR), m_iPrimaryAttackCount gets called iMaxClip() times, as opposed to then no return statements were added.

Here is a link to a quick branch I created from master. Have a look for places where I put preprocessor directives HL_DLL_FIX;HL_DLL_FIX_WEAPON_EVENTS.

ghost commented 5 years ago

Hey, how come whenever I look to make sure my thread isn't a duplicate, I never find anything? I looked all over, and the guy who recommended I post the bug report here said no one else had ever posted anything like this. It's just my luck, I swear! I never get things right...

Maxi605 commented 5 years ago

Hey, how come whenever I look to make sure my thread isn't a duplicate, I never find anything? I looked all over, and the guy who recommended I post the bug report here said no one else had ever posted anything like this. It's just my luck, I swear! I never get things right...

It's fine, either way you attract attention to the original topic.

Also, about this issue, i seem on CS 1.6 a similar issue with the weapon showing shooting 2 shots when you're only shooting one.

SamVanheer commented 3 years ago

After a lot of digging i think i've found the cause of this.

This code here: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/weapons.cpp#L1192-L1221

Is used to calculate the next attack delay for predicted weapons. This involves a lot of floating point math. Sometimes the resulting value will have a tiny amount of extra value added, so for example when the next attack time is 0.1, it could actually be 0.1000012344.

Because of this, when the server side weapons code checks if it can attack: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/weapons.cpp#L627-L641

This resolves to false: attack_time <= 0.0

On the client side however, the delay is always constant (0.1). So there is no floating point math error that creeps in and results in slightly larger values.

For example this is data i was getting:

cl:  checking for attack: 0.00999999977648258209, no
sv: checking for attack: 0.01000001467764377600, no
cl:  checking for attack: 0.00000000000000000000, yes
cl:  fired mp5 2 times at 0.000000, next at 0.100000
sv: checking for attack: 0.00000001490116119385, no

Note how the client value is actually slightly smaller than a whole value. Instead of 0.01 it's 0.00999999 which is also a rounding error, but in the other direction which doesn't cause this problem.

The client value is decremented down to 0 and thus passes the condition to fire. The server does not.

Every now and then this results in the client getting an extra shot in.

Once i added in rounding logic to constrain the values returned by GetNextAttackDelay and changed CanAttack to compare against 0.0001 instead of 0 the problem stopped appearing:

cl:  checking for attack: 0.00000000000000000000, yes
cl:  fired mp5 425 times at 0.000000, next at 0.100000
sv: checking for attack: 0.00000001490116119385, yes
sv: fired mp5 425 times at 0.000000, next at 0.100000

So to fix this the code that calculates the next attack delay needs to round its resulting value down to a few decimals, and the code that checks if you can fire needs to account for the value being a very small positive value as a result of rounding errors. Combined this should eliminate the problem altogether.

The required changes are: In GetNextAttackDelay:

float flNextAttack = (std::floor((UTIL_WeaponTimeBase() + delay - flCreep) * 1000) / 1000);

And in CanAttack:

return ( attack_time <= 0.0001 ) ? TRUE : FALSE;

And additionally the cmath header also needs to be included for std::floor.

I've tested this at both a tick rate of 100 and 500 and both produce correct results.

SamVanheer commented 3 years ago

Looks like this fix doesn't work for every weapon. The glock at least can still glitch out with this in place. I'm investigating to see what can be done.

SamVanheer commented 3 years ago

Ok, i figured out what's wrong with my solution. The above fix was rounding the m_flNextPrimaryAttack variable, which is then networked to clients. As a result it still causes the problem depending on the values it produces.

To avoid this, the rounding should be performed on the server side without allowing the rounded value to be networked to clients.

This is done by only rounding in CanAttack (the rounding done in GetNextAttackDelay as suggested above should not be performed):

BOOL CanAttack( float attack_time, float curtime, BOOL isPredicted )
{
#if defined( CLIENT_WEAPONS )
    if ( !isPredicted )
#else
    if ( 1 )
#endif
    {
        return ( attack_time <= curtime ) ? TRUE : FALSE;
    }
    else
    {
        return ( (static_cast<int>(std::floor(attack_time * 1000.0)) * 1000.0) <= 0.0 ) ? TRUE : FALSE;
    }
}

This emulates the rounding that occurs when a value is networked to clients. As specified in delta.lst:

DEFINE_DELTA( m_flNextPrimaryAttack, DT_FLOAT | DT_SIGNED, 22, 1000.0 ),

m_flNextPrimaryAttack is multiplied by 1000, converted to an integer and then has its 22 most significant bits along with the sign bit sent to the client.

The client then reads it as an int along with the sign bit, divides it by 1000 and assigns it to the weapon data m_flNextPrimaryAttack variable, which eventually gets passed to the prediction code.

By performing the same operations on the value passed into CanAttack this produces the same values on the client and the server.

Note that this will increase the rate of fire by a small amount. It's not very significant though.

Also, i checked Source to see what it does. It differs in 2 important ways: