ValveSoftware / halflife

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

[TFC] Weapon missfire #1070

Closed tehORiON closed 5 years ago

tehORiON commented 11 years ago

The pipelauncher that fires the green/yellow pipes that you can detonate yourself will occationally fire two pipes at once, this seems to happen after you have just detonated some pipes and while holding reload. Confirmed this on both the win32 and linux beta clients. UPDATE: This happens on all weapons. See post below for details.

@alfred-valve This bug should have higher priority as it is quite gamebreaking really, it is also a bug which has been introduced with the new update.

hlstriker commented 11 years ago

I just tested this too and it's very weird. It seems m_flNextPrimaryAttack is getting set to a random value after using any a button mask key. It seems to happen with any weapon in the game.

Button mask key examples: +forward +back +jump +duck +showscores +attack +reload +attack2 etc

Example to replicate it: 1) Tap jump (or any other buttons mask key) once. 2) Immediately shoot a pipe (or other ammo).

The largest amount of pipes I shot at once was about 4.

Looking at this screenshot you see the time between 2 pipes is about 0.05 seconds. The time between pipe shots is supposed to be 0.6 seconds. I've also had it show me a negative time for the next attack.

http://i.imgur.com/M5i99Nn.jpg

hlstriker commented 11 years ago

Here's a video showing the problem: http://www.youtube.com/watch?v=XfqAKT18bo8

ghost commented 10 years ago

please fix this and set it to high priority, is really annoying the double fire bug, i'm using the last updated client and server build(6153 and 6027)

Starscre4M commented 10 years ago

it's for sure really annoying and game breaking, especially soldiers shooting 2 rockets at once

ghost commented 10 years ago

no fix yet? :(

ghost commented 10 years ago

Please? It's impossible to host a server, because i can't download the old engine anymore with HLDSUPdateTool, so i need to use the latest server from SteamCMD (version 6153) and this version has this bug, it's impossible to play the game with this bug like that, it's 7 months old and not fix yet, really? And you are still charging u$s 5 to play this game, really?

There are bugs since the release of the game from the year 2002~ and not fixed yet

Put it free and fix this bugs please.. Stop killing this game and the only players remains from this game please.

We don't like the TF2.

Please fix this HIGH PRIORITY and BLOCKER bug, 7 months and nothing... my servers from tournaments and competition for pickups all are broken

I needed to download the old version(5757 build) from some unnoficial page

Thanks.

@alfred-valve

hlstriker commented 10 years ago

@alfred-valve @triage-valve

This bug is a serious issue and is forcing server operators to use the pre-steampipe dlls (which is also bad because a lot of the fixes you guys did won't apply). This bug needs addressed and released as soon as possible!!!

I'm not sure the exact cause but I have found a way to replicate it every time.

1) Choose soldier. 2) Shoot first rocket (clip now at 3/4). 3) Start to reload as soon as you can. 4) Wait about half way through reload and hold +attack.

This works with any weapon (not sure about weapons with no reserve ammo such as the nailgun). From testing it's not limited to reloading but it does seem to be the easiest way to replicate the bug.

ghost commented 9 years ago

I appreciate TFC is not a massive Valve priority, understandably - but after introducing this and the disguise bug, could you find a way to fixing them, please?

@alfred-valve @triage-valve

Arkshine commented 9 years ago

Issue looks like the same as https://github.com/ValveSoftware/halflife/issues/802 for CS.

For now, changes should be likely reverted too for TFC.

JonasEira commented 9 years ago

I just started playing TFC after a long break and found this peculiar behaviour as well, being a avid skills player. But i'm thought the double shots were tied to reloading? I seem to be getting them if i fire the weapon just before a reload cycle ends and the ammo count goes up. Maybe a mismatch, but info nonetheless.

tehORiON commented 8 years ago

@alfred-valve Any chance anyone could look into this? It's quite annoying having to replace dll's because we can't use the newest one due to this gamebreaking bug. Would also be useful to be able to have some of the bugfixes that is in the newest dll.

SamVanheer commented 7 years ago

I debugged this as much as i could, and my findings confirm hlstriker's.

The Soldier's RPG has a delay of 0.8 seconds. Normally, m_flPrevPrimaryAttack would be 0.7 due to rounding and timing issues, but sometimes it would be a smaller value that allows for much faster firing.

I couldn't tell where the variable was being modified, but it should only ever be occurring in CBasePlayerWeapon::GetNextAttackDelay. This function appears to be identical to the SDK version, barring compiler optimizations. The only thing that could cause this sort of thing short of some data being corrupted or changed when it shouldn't be is if this method is being called for a weapon when it shouldn't be.

It should be trivial to debug with the source code and debug symbols available.

SamVanheer commented 7 years ago

I debugged this again, this time by modifying the RPG's vtable to point to my own function:

float __fastcall GetNextAttackDelay( CBasePlayerWeapon* pThis, float delay )
{
    if( pThis->m_flLastFireTime == 0 || pThis->m_flNextPrimaryAttack == -1 )
    {
        // At this point, we are assuming that the client has stopped firing
        // and we are going to reset our book keeping variables.
        pThis->m_flLastFireTime = gpGlobals->time;
        pThis->m_flPrevPrimaryAttack = delay;
    }
    // calculate the time between this shot and the previous
    float flTimeBetweenFires = gpGlobals->time - pThis->m_flLastFireTime;
    float flCreep = 0.0f;
    if( flTimeBetweenFires > 0 )
        flCreep = flTimeBetweenFires - pThis->m_flPrevPrimaryAttack; // postive or negative

    float lastfiretime = pThis->m_flLastFireTime;

    float prevattack = pThis->m_flPrevPrimaryAttack;

                                                              // save the last fire time
    pThis->m_flLastFireTime = gpGlobals->time;

    float flNextAttack = UTIL_WeaponTimeBase() + delay - flCreep;
    // we need to remember what the m_flNextPrimaryAttack time is set to for each shot, 
    // store it as m_flPrevPrimaryAttack.
    pThis->m_flPrevPrimaryAttack = flNextAttack - UTIL_WeaponTimeBase();
    //  char szMsg[256];
    //  _snprintf( szMsg, sizeof(szMsg), "next attack time: %0.4f\n", gpGlobals->time + flNextAttack );
    //  OutputDebugString( szMsg );
    META_DEBUG( 0, ( "gpGlobals->time: %f, m_flNextPrimaryAttack: %f, m_flPrevPrimaryAttack: %f, m_flLastFireTime: %f\n"
                     "prevattack: %f, lastfiretime: %f, flCreep: %f, flTimeBetweenFires: %f\n"
                     "flNextAttack: %f, delay: %f",
                     gpGlobals->time, pThis->m_flNextPrimaryAttack, pThis->m_flPrevPrimaryAttack, pThis->m_flLastFireTime, 
                     prevattack, lastfiretime, flCreep, flTimeBetweenFires, 
                     flNextAttack, delay ) );
    return flNextAttack;
}
}

This works perfectly on Windows because thiscall and fastcall are the same calling convention as far as parameter pushing and popping goes.

The purpose of this method is to calculate the next attack time, taking into account low server FPS to ensure that attacks are allowed every delay seconds. In effect, when firing rapidly, it computes the next attack time as previous_attack_time + delay.

So after a lot of debugging, i was able to fix the issue by changing how the time between fires is calculated:

float flTimeBetweenFires = min( gpGlobals->time - pThis->m_flLastFireTime, delay );

This clamps the time between fires value to the firing delay. In effect, this means that the code will assume that at most delay time has passed since the last shot. This factors in to the delay never becoming shorter due to the code thinking the last shot time was an unrealistic amount of time ago.

Changing the first if check to this also seems to fix it:

if( pThis->m_flLastFireTime == 0 || pThis->m_flNextPrimaryAttack < 0 )

If the next attack time has passed, it resets the attack time data. I doubt this will hold up in laggy servers, but it does seem to prevent the problem from happening.

I still haven't found the reason why jumping or pressing other buttons triggers this bug, i'm thinking that doing so changes some code's behavior somewhere which in turn affects the think time. That's somewhat worrying because the think time is directly affected by incoming user commands. If pressing buttons can affect the influx of those commands then it can affect server side player simulation speed.

I've ruled out frametime (always ~0.001 seconds) and the next attack time, which is always negative and does not affect weapon firing. The value passed into GetNextAttackDelay is correct as well, though the method does get called for things other than firing weapons, but the state tracked by it is reset in some places, like the Reload method of several weapons.

I've also checked the code path to PrimaryAttack:

The conditions at every step are valid, so it just doesn't make sense.

It's always possible that it's just the impression of pressing buttons that causes it, and that it's really a small delay when pressing those buttons and firing that creates the problem, whether in the input system or in the user's own mind.

For example, the RPG has a delay of 0.8. If you fire again after 0.9 seconds, it'll set the next attack time to -0.1, which lets you fire again immediately. If pressing another button introduces a slight delay somehow, it could account for that.

Regardless, it seems to go away when you clamp the value, so it's a stopgap solution at least. I'd compare TFC's code to the SDK, since the same exact code is in use there and it isn't affected by this issue. I even used the same delay on the Crossbow and it was unaffected by this issue.

EDIT: And i just realized that issue #802 is where this method came from. It was also removed from CS because it caused issues. The old code just used UTIL_WeaponTimeBase() + delay.

SamVanheer commented 7 years ago

I decided to put my hypothesis to the test by programmatically triggering attacks. I set the Crossbow's next attack time to 0.8 seconds and added a command to trigger weapon attacks automatically, firing up to 4 times using different intervals.

const float increments[ 3 ] = 
{
    0.8,
    0.8,
    0.05
};

void CBasePlayer::PreThink()
{
    if( m_uiAttack < 4 && m_flAttacks[ m_uiAttack ] != 0 )
    {
        if( m_flAttacks[ m_uiAttack ] <= gpGlobals->time )
        {
            if( m_uiAttack < 3 )
                m_flAttacks[ m_uiAttack + 1 ] = m_flAttacks[ m_uiAttack ] + increments[ m_uiAttack ];
            m_flAttacks[ m_uiAttack ] = 0;
            ++m_uiAttack;

            pev->button |= IN_ATTACK;
        }
    }

    const int buttonsChanged = ( m_afButtonLast ^ pev->button );    // These buttons have changed this frame
//PreThink continues here

I also added an ALERT in GetNextAttackDelay.

I executed the command a couple times, and every time it had the bug:


] fireweapon 
next attack: 0.800000
next attack: 0.017709
next attack: 0.767950
] fireweapon 
next attack: 0.800000
next attack: 0.017816
next attack: 0.768057
] fireweapon 
next attack: 0.800000
next attack: 0.017816
next attack: 0.768057

But here's the thing: in order to replicate this bug, i had to disable some code: https://github.com/ValveSoftware/halflife/blob/5d761709a31ce1e71488f2668321de05f791b405/dlls/weapons.cpp#L659

if ( !(m_pPlayer->pev->button & IN_ATTACK ) )
{
    m_flLastFireTime = 0.0f;
}

This code is missing from TFC. It goes from the reload check to checking if PrimaryAttack() should be called:

  //Code to get ammo info omitted...
  if ( m_fInReload && m_pPlayer.m_flNextAttack <= 0.0 )
  {
    //Code to handle reloads omitted...
  }

  //Missing check here

  if ( ( m_pPlayer->pev->button & IN_ATTACK ) && m_flNextPrimaryAttack <= UTIL_WeaponTimeBase() )
  {
    //Code to remove Spy disguises omitted...
    if ( m_pPlayer->super_damage_finished > gpGlobals->time )
    {
      if ( gpGlobals->time > m_pPlayer->m_fSuperSound )
      {
        //Super attack sound omitted...
      }
    }
    PrimaryAttack();
  }
  else
  //Other attack handlers omitted...
  if ( ShouldWeaponIdle() )
      WeaponIdle();

When firing automatically this issue doesn't occur, probably because this issue requires precise timing that can't happen when attacks occur the first frame that it's possible.

So i guess adding that missing check should fix it then.

EDIT: i double checked this by replacing ItemPostFrame:

using postframefn = void ( __fastcall * )( CBasePlayerWeapon* );

postframefn postframe = nullptr;

void __fastcall ItemPostFrame( CBasePlayerWeapon* pThis )
{
    if( !( pThis->m_pPlayer->pev->button & IN_ATTACK ) )
    {
        META_DEBUG( 0, ( "resetting fire time" ) );
        pThis->m_flLastFireTime = 0;
    }

    postframe( pThis );
}

I was unable to reproduce the issue with this in place.

pizzahut2 commented 6 years ago

Btw Valve, one of the reasons why the TFC community considers this a major bug is that as soldier, it can happen that the two rockets explode immediately, causing self damage.

pizzahut2 commented 6 years ago

https://www.youtube.com/watch?v=LbYxTKQE0Qs

The rocket count at start is off for some reason, should be 4 but displays 0. The server was created via TFC main menu, this is a demo playback.

Steps:

  1. Fire rocket.
  2. Wait for it to reach the other side.
  3. Reload and during reload, shoot again.

If timed right, two rockets will fire at the same time, causing them to instantly explode.

pizzahut2 commented 5 years ago

@kisak-valve @johndrinkwater @triage-valve There are only two major issues left in Team Fortress Classic, is there any chance that you could fix these please? This bug (weapon missfire) even keeps server owners from using the current TFC library because people avoid servers with this bug if they can.

The other bug which I noticed is that custom sprays can't be exchanged any more using recent versions of the HLDS. It's been reported as a HL/CS bug but seems to be located in the HL engine and thus applies to TFC as well.

https://github.com/ValveSoftware/halflife/issues/1485

Both bugs have been introduced around 2012 / 2013 with the move from the HLDS Update Tool to SteamCMD.

mikela-valve commented 5 years ago

This issue should now be fixed in the beta version released earlier today. If anyone would be so kind to give it a test to ensure things are working fine, I'll update the public version with it early next week.

SamVanheer commented 5 years ago

I tested the current public and beta versions, i was able to reproduce the issue in the public version, the beta version appears to be fixed. @pizzahut2 can you confirm?

pizzahut2 commented 5 years ago

No time for testing right now, sorry. The beta is simply called "beta", yes? https://developer.valvesoftware.com/wiki/SteamCMD#Downloading_an_app

SamVanheer commented 5 years ago

It's called beta - SteamPipe beta so i think the branch name is just beta.

se7entfc commented 5 years ago

I have tested the beta on both Linux and Windows servers and was unable to reproduce the double firing bug.

However, the auto exec for class configs is broken once again :/

mikela-valve commented 5 years ago

Thanks, @se7entfc. The class config exec was moved out of TFC and into the engine in the latest HL1 release for security reasons. Servers that haven’t updated are trying to exec them the old way so the config isn’t executed.

This issue was one of the primary blockers preventing server owners from being able to update their TFC version so it’s hoped that with this fixed server owners will be able to update and class/map config will work as expected.

pizzahut2 commented 5 years ago

Class configs and double shoot bug seem to be fixed, tested on a Linux server.

37.122.208.248:27015 192.223.27.178:27015

Not aware of any problems with this bug fix yet, but I've only tested it briefly though.

mikela-valve commented 5 years ago

This has now been updated in the public release.

iwouldliketohaveanargumentplease commented 5 years ago

https://www.youtube.com/watch?v=iqzN51VhGW8

Here an issue. Fix please. Thank you.

SamVanheer commented 5 years ago

Which version are you running?

It looks like you're also running a plugin of some sort that changes the color of the model and trail, at least. It could be interfering.

I also don't see anything wrong with the fire rate, it seems to be working fine.

iwouldliketohaveanargumentplease commented 5 years ago

"I am using the most up to date libraries on LAN. The client-side custom models/sprites are inconsequential to the issue I am trying to present.

What I noticed was that (after this double-fire bug was fixed) was that on rare occasions I am able to fire yellow/detable pipes and there is a chance that they do not det simultaneously; this is what I hoped to show at the end of this clip. I do not know if this is related and/or caused by the patch, I only know that I now have experienced this. It seems minor but still an unintentional consequence of the update."

SamVanheer commented 5 years ago

That's normal, pipe bombs are primed 0.6 seconds after being created. They can be force detonated by creating too many of them, dying, disconnecting or if pre match cvars are in use, in which case they're all detonated when the match starts.