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

Issues with hit detection #1408

Open
jackeri opened this issue Mar 15, 2020 · 39 comments
Open

Issues with hit detection #1408

jackeri opened this issue Mar 15, 2020 · 39 comments
Assignees
Labels
🐞 Bug Something isn't working cat: general Category engine (client/server) cat: mod QAGAME Category mod (qagame) ❓ Feedback Further information is requested P1: Urgent Priority 1
Milestone

Comments

@jackeri
Copy link
Member

jackeri commented Mar 15, 2020

The "feeling" when shooting is somehow delayed. The way it manifests is just simply: you press the shooting button -> the hit event may or may not register at all or it registers delayed.

Things to look for: antiwarp code, antilag code and the "updated" client & server download code made in ~2015 ish.

@jackeri jackeri added 🐞 Bug Something isn't working P3: Normal Priority 3 cat: general Category engine (client/server) cat: mod QAGAME Category mod (qagame) labels Mar 15, 2020
@rmarquis rmarquis added this to the 2.77 milestone Mar 15, 2020
@rmarquis
Copy link
Contributor

Related UDP download speed change was done in 108ca85. See #330.

@rmarquis
Copy link
Contributor

Some initial changes done in f73b9c8.

Early testing indicates better results already.

@rmarquis rmarquis added P1: Urgent Priority 1 and removed P3: Normal Priority 3 labels Apr 19, 2020
@rmarquis
Copy link
Contributor

See also #1201 to potentially improve hit "feeling" by having hitsounds generated server side.

@rafal1137
Copy link
Member

Point of interest
ETPub Replace servertime value with attackTime and test the results.

@rmarquis
Copy link
Contributor

For the record: some heavy testing was done back in March, with various combination of etded/etlded and Jaymod/Legacy mod (since Jaymod is compatible with etlded, has same hitbox, etc.). The issue has been pigeonholed to the mod code, especially antilag part.

Server Mod Client     Jacker's notes Spyhawk's notes
etded Jaymod 2.60b     With both antilag and antiwarp disabled I was consistenly still hitting the players where I expected to hit. I'm not used to jaymod, so I played a bit to get the feel of it. No noticeable difference between 2.60b and ETL clients. (lag 0, warp 0): There is little lag noticeable. Aiming at or very close to the target pretty much hits most of the time.
    Legacy master        
etlded Jaymod 2.60b     Pretty much the same as the vanilla server (lag 0, warp 0) can't see diff between etlded vs etded (both Jaymod). I'm less sure about the client, but that might be placebo.
    Legacy master        
etlded Legacy 2.60b     With antilag 0 the hitboxes were totally out of place. Headshots were 0.5 meters ahead of the player mode. With antilag 1 the effect was mitigated to some degree but hitboxes were still very inconsestent. (lag 0, warp 0) As one should expect, you need to aim in front of the target to hit it without antilag. (lag 1, warp 0) Legacy antilag does its job, but feels... inconsistent? It feels like only 1 bullet on 3 gets a HS, unlike Jaymod where 3 HS in a row are not an issue. I couldn't feel any difference between 2.60b and ETL client
    Legacy master        

The last change done above however gave mixed results: while it felt better on the testing server, in some way, the feeling was worst once deployed on a populated server.

I do believe reworking the antilag/antiwarp from scratch would be easier than finding out where the original issue has been introduced. Legacy mod implements Zinx antiwarp from etpro (shared publicly), and might have used etpub as a base for its implementation back in ~2013.

However, afaik some people mentioned etpub (and derived mods) implementation has some issues, but Jaymod implementation should be rather independent from etpub codebase family. It might be worth looking at it since it's been opensourced since then by Jaybud.

@rmarquis
Copy link
Contributor

From @Aranud, and related to @rafal1137 comment above:

rafal1137 added a commit that referenced this issue Jul 26, 2020
@rmarquis
Copy link
Contributor

rmarquis commented Jul 26, 2020

A few extra resources about existing implementations:

g_skipCorrection [1|0]
Set this to 1 to enable Neil Toronto's unlagged2 skip correction. This will smooth out the movement of players with high packet loss (to a degree). This is similar to etpro's antiwarp, but has some differences. Neil likes this version better, bani likes his better.
This replaces g_smoothClients from etmain.
You can find a demo that shows g_skipCorrection in action at: http://et.tjw.org/etpub/skipCorrection/

Defaults to 1 (on)

g_maxWarp [integer]
This allows you to control the amount of "warping" that players with high packet loss can do. The [integer] is the number of server frames that you allow a player to miss before their next movement is put in check.

A server frame is 50ms on a typical server (sv_fps set to 20). This means that if you set g_maxWarp to 5 you won't allow players to warp from point A to point B if that distance takes an normal player 1/4 of a second to travel. Setting this to 1 is a good way to drive off just about everyone from your server.

As far as I can tell, 1000ms is allowed by default in the game, so setting this to any value higher than 39 should have no effect if sv_fps is set to 10.

You can find a demo that shows g_maxWarp in action at: http://et.tjw.org/etpub/skipCorrection/

Defaults to 4

@rafal1137
Copy link
Member

rafal1137 commented Jul 27, 2020

Interesting note in NQ code
// tjw: save ucmd->serverTime before it gets mutilated further // tack on 2 server frames time for "50ms built-in lag"

Propably worth implementing it

Ref:

@rmarquis
Copy link
Contributor

I'd heavily suggest to try understanding the code before pushing "fixes", knowing etpub and derived might have issue of their own (see above comment).

In that case, tjw was an etpub coder, and never directly participate to NQ dev afaik. NQ being derived from an old version of etpub, that part of the code might have been dropped from later etpub version for good reason.

@rafal1137
Copy link
Member

@rmarquis Yeah this reply from

you fixed a problem you were having giving you 50 ms lag all the time

Might be a reason why it was removed.

@Aranud
Copy link
Contributor

Aranud commented Sep 7, 2020

Other differences between ETL / ETPub / NQ for attackTime assignment :

On Pub the value from g_antilagDelay is added (by default the value is set to 0).
On NQ, the value added is computed by the delta of current server frame time previous server frame time.

@Aranud
Copy link
Contributor

Aranud commented Sep 10, 2020

Another differences but on Jaymod side this time : there is one different regarding the implementation of unlagged in ETL / ETPUB / NQ compared to Jaymod and it's there : https://github.com/budjb/jaymod/blob/bbe44e0cca806d6b51260d48d4c11eab2b85bfe9/src/cgame/cg_ents.cpp#L2340-L2349

Aranud added a commit that referenced this issue Oct 21, 2020
Aranud added a commit that referenced this issue Oct 23, 2020
To ensure the client size values changes are take into account for colision detection, we must link the entity before doing the trace.
Aranud pushed a commit that referenced this issue Oct 26, 2020
Bots don't get their position adjusted, so we have to apply the syringue hitbox height change outside this function.

Also, only modify the syringue hitbox if the player is wounded or prone, otherwise on standing player, the syringue could not hit.
@ryzyk-krzysiek
Copy link
Contributor

Thanks for the feedback. One important question: did you play on server with sv_fps 20 or 40?

Yes, we are playing on sv_fps 40 almost daily for around 2 months now.

Just to clarify here: I am not saying it couldn't be advantageous, but that it does mask the underlying antilag issue, that we know exist. As such, it is not a proper fix but more of a band aid solution - which might be sufficient, but indeed not optimal. Having both would be advantageous, if the reserves of Neil are proved to be unworthy or obsolete with modern hardware and bandwidth.

In my opinion the antilag issue is fixed too, doesn't matter if server uses sv_fps 20 or 40. The hitbox change and various other improvments did the job. If it somehow isn't fixed then I don't know personally, I've been playing on public server too and everything seemed fine, but maybe it's something I just can't see like the "delay".

@rmarquis
Copy link
Contributor

All right, but my point is that we don't know if it is fixed for sure, and testing with sv_fps 40 doesn't help to know. With all due respect, I kinda don't want to rely on "opinion" here because we've seen many times how opinions and methodical testing gave different results.

Maybe we'll need to set up test servers with Jaymod/Legacy as we did previously.

@ryzyk-krzysiek
Copy link
Contributor

ryzyk-krzysiek commented Aug 23, 2021

From games I played on internet servers and from what I tested on local dedicated server (sv_fps 20) with various sv_packetdelay and cl_packetdelay the antilag works how it is supposed to. Which is, it sets every player to a postion the attacker saw them and does hit registration, and it works, I can play on 200 ping and hit people where I see them on my screen, doesn't matter what is their real position.

I don't know how it could be better than this, but maybe it can somehow, maybe there is still some bug that no one noticed to this day, but I believe we got to a point it might not matter, if one day someone comes with an example that something didn't work, then that's great. If someone wants to keep looking for things to improve then that's cool too. It's been a while since I've heard someone complain about hitreg/hitboxes.

The only thing I'm worried is that delay issue which is of course a matter of ping mostly and I'm yet to find the cause it would be different between mods. So far I can prove that legacy is the same or has lower delay than etpro. cg_debugevents clearly shows it's the same or faster on sv_fps 40. I've tried comparing with just a stopwatch and nothing. I've tried to see if SDL or OpenAL makes a difference and nothing. I recorded 60fps clips and checked frame by frame and nothing but I want to try this again soon, because since then I got some new ideas. At this point I think it's either just in the heads of people or my hearing is impaired and I'm dumb.

I'm not really saying you should trust me, I don't consider myself a good programmer, but I've spent literal days on this already, so unless someone comes to me with evidence (or at least claims that I can check if they are true) I say it's fixed and better than etpro, be it hit registration or hitboxes, because not only it feels better for me, but we fixed issues that are still in etpro.

@rmarquis
Copy link
Contributor

rmarquis commented Aug 23, 2021

I'm not really saying you should trust me, I don't consider myself a good programmer, but I've spent literal days on this already, so unless someone comes to me with evidence (or at least claims that I can check if they are true) I say it's fixed and better than etpro, be it hit registration or hitboxes, because not only it feels better for me, but we fixed issues that are still in etpro.

Yes, and again: this isn't about not trusting you, nor dismissing the work you did (and my apologies if any of the above message made you think otherwise). I saw your changes (which are all great), but none of them fixed anything in the antilag interpolation (afaik) which we factually know to be somewhat borked with the above test from last year. I have no doubt hitboxes and hit feeling are better now compared to the past, but this isn't my direct concern here.

Rather, I'd like to see that underlying issue be fixed, and ideally be certain it is fixed by measuring it somehow (isolating variables is a nice start). All the rest of the changes is a nice bonus on top, obviously.

Edit: also, antilag is obviously kinda hard to test locally. It's also what I did above, but only at a lack of having any better way to test it.

@ryzyk-krzysiek
Copy link
Contributor

Yes, and again: this isn't about not trusting you, nor dismissing the work you did (and my apologies if any of the above message made you think otherwise).

No need to apology, I'm not taking it as dismissing my or anyone elses work.

And don't get me wrong, I would like to fix it and be 100% sure it is fixed too, but I don't know how it can be accomplished as I tried many things that only point to it working and I'm out of ideas how to prove it otherwise. But I'm open to any ideas or examples or evidence, but just by reading the code for the 20th time in the hopes of spotting something I believe I will not get anywhere anymore personally.

For the antilag interpolation, the code for interpolation on client and antilag is the same. The user command with the time player made a shot is sent (and so we know what time on client the interpolation was done while he shot) and that time is used to interpolate between two saved snapshots that were sent to the player. The only thing I see that could interfere and cause issues is antiwarp as it can change the command time, but I've yet to find evidence it actually does (in a normal game scenario where you don't seem have internet issues at all) and messes up antilag in the process.

@rmarquis
Copy link
Contributor

All right. I realize we might not all be on the same page, as that issue has been discussed on Discord long time ago and some information I'm taking as granted might not be clear to everyone. I'll try to write a recap here, and hopefully you might understand my thought process a bit better.

Back in the early days (~2012-2014), ETL devs added many features and many optimizations here and there. The code started back from the original W:ET and was lagging significantly behind all existing mods. Many stuff were added, and along them also many regressions - over time, we fixed many but it took literally years of backtracking. Some doubt about the hit "feeling" were issued, but dismissed. However, with time it became clear something was fundamentally wrong so we eventually set up a series of test to understand what's going on.

Since we had no idea where the issue was located, we tested both the server/client part and the mod part, using Jaymod as reference.
Jaymod had the advantage of

  • running on both etded and etlded (unlike etpro)
  • have the same hitbox height as Legacy (allowing an easier comparison)
  • being open source (allowing implementation comparison)
  • being independant from the etpub lineage (we've heard etpub antilag code has some issue by itself - and so might any of its derivative). Silent devs also said at some point they fixed some issue, but their code not being available it is hard to know what they fixed exactly.

Jacker set up a few servers with a combination of server/mods as described in the table above. We then we did some lengthy and boring test remotely over a few evenings. Since we couldn't use bot to properly to test antilag, we basically used scripted clients running straight, with us as additional clients to shoot them under various angle. Shoot, let them respawn, repeat. Do that hundreds of time. Change one server variable at a time. Repeat again. Boring as fuck, but the idea was to test each component and cvars in isolation, and minimize randomness as much as possible (which is the opposite of what's happening on live servers).

The results showed the issue was located mod side, specifically when g_antilag was enabled (confirmed at least by Jacker, RaFal and myself). We've all gone through that antilag code dozens of time, compared it to existing implementation, made some minor tweaking here and there, but essentially: the issue was still present, and we couldn't find it.

So yeah, we were all pretty sure the "antilag works how it is supposed to" because we've checked its implementation dozens of time by at least 4 people (5 including you), but also: we factually knew the antilag doesn't work how it is supposed to.

Forward a few months, with many additional and positive hitboxes improvements and what not, but when I read something that basically amount as "it feels better, so it's fixed", but that not much was actually changed on the antilag level, I have more than doubt. I mean, I don't have any doubt all the extra work improved the hit feeling, but logic says the underlying issue hasn't been fixed at all.

Hopefully you follow the reasoning without much difficulty, and kinda understand why I'm generally very skeptical of "opinions about feeling" ;)


Now, let's look at that last reverted commit 31ee385 (introduced in 8a7d58a and extended by a few others).

This looks like an over zealy optimization:

  • it dropped temporary entities created by the antilag immediately. G_SwitchBodyPartEntity() might thus not return the expected result, and consequently the POSITION_READJUST macro wouldn't act on the right component.
  • it was only active when the debug hitboxes weren't displayed. Each time we tried to see what's happening with hitboxes, the issue would disappear. This is Heisenbug level.
  • it's so experimental that it required the addition of a special cvar to disable it. A warning about network game was also added.
  • it was enabled by default, and nobody knew it was there (IR4 vanished some years ago)
  • it matches the time period of the crazy days (Dec 2013)

All of these factors make me think there is a high possibility of it being responsible for that antilag issue.
I have trouble understanding how exactly the effects of that code translate in practical terms, but at first sight it might explain why headshots were feeling jaggy (ET_TEMPHEAD not reajusted correctly?). It's also not exactly in the interpolation code itself, but something in a deeper layer that feeds the hit detection server side, and hidden in g_utils.c rather than g_antilag.c.

Now, I really don't want to rejoice too early and consider the issue fixed without any somewhat objective feedback. We've all spent so much time on that specific issue that it is kind of ridiculous, and I'd rather be 100% certain it's gone once and for all (if only to really consider etpro as being the subpart mod it actually is :P). I guess with you guys used to play with sv_fps 40 in scrims, you might not be able to clearly make the difference: it's already subtle in essence, and theoretically twice as hard to feel with a doubled server rate. I am not sure if asking the more casual players on public TM is very useful (if I understand it correctly, the main server still uses sv_fps 20), because well... getting useful feedback from regular folks is not exactly easy.

Maybe spinning 2 servers with 2.77.1 and a recent snapshot might help here, so we could do a systematic comparison with a low sv_fps. Opinion @jackeri?

@Aciz
Copy link
Member

Aciz commented Aug 24, 2021

Both of you have very valid opinions about this, I think @ryzyk-krzysiek has excellent POV on this issue due to the fact that he has played actively through these changes for the past year or so and has the end-user opinion on things (which is ultimately the most important thing), however his opinion might be unconsciously skewed a bit since he has done things fix the issue along the way.

However @rmarquis has the background knowledge on how things have evolved over the years, and this is very important for the sake of knowing and validating if the issues are truly fixed (btw I have no idea how actively you play).

I think more methodological testing is definitely warranted regarding this, and I'm down to help with that.

@ryzyk-krzysiek
Copy link
Contributor

All of these factors make me think there is a high possibility of it being responsible for that antilag issue.
I have trouble understanding how exactly the effects of that code translate in practical terms, but at first sight it might explain why headshots were feeling jaggy (ET_TEMPHEAD not reajusted correctly?). It's also not exactly in the interpolation code itself, but something in a deeper layer that feeds the hit detection server side, and hidden in g_utils.c rather than g_antilag.c.

This is what I don't understand either how it could help because G_SwitchBodyPartEntity() and POSITION_READJUST is called before G_FreeEntity in G_DettachBodyParts and as far as I understand, it should be released asap, it's not needed after for anything. Those temp body parts are not sent over network even if g_debugPlayerHitboxes is on, instead new temp event is created (EV_RAILTRAIL). If it isn't released it shouldn't matter I think because list->client->tempHead is overwritten in G_AttachBodyParts next time G_Trace is called.

This also shouldn't affect the actual trace to check for if head was hit which is here:

qboolean IsHeadShot(gentity_t *targ, vec3_t dir, vec3_t point, meansOfDeath_t mod, grefEntity_t *refent, qboolean newRefent)

Where even if G_FreeEntity was missing it wouldn't cause issues for future traces I think? It would be memory/too many entities problem because there is a limit of 1021(?). That's my understanding and maybe I'm wrong.

TM public useses sv_fps 20 and I've been playing on and off there too, the clip that sparked the body hitbox change was done by me playing on TM main server. But that issue would happen anywhere and it's still an issue in etpro.

I agree I might be biased and by the amount of time I've spent on it I could be blind to some things.

@rmarquis rmarquis modified the milestones: 2.78, 2.79 Sep 28, 2021
Aciz added a commit to Aciz/etlegacy that referenced this issue Sep 28, 2021
…etlegacy#1408

This was added as a test some 9 months ago, but basically ever since then, `g_playerHitBoxHeight` has just been set to __48__ on servers, so let's just remove the cvar and use the vanilla hitbox height, as it's what's currently being used.
jackeri pushed a commit that referenced this issue Nov 13, 2021
Aranud pushed a commit that referenced this issue Nov 26, 2021
…#1408

This was added as a test some 9 months ago, but basically ever since then, `g_playerHitBoxHeight` has just been set to __48__ on servers, so let's just remove the cvar and use the vanilla hitbox height, as it's what's currently being used.
@jackeri jackeri modified the milestones: 2.79, 2.80 Dec 23, 2021
@Aranud Aranud closed this as completed Apr 11, 2022
@Aranud
Copy link
Contributor

Aranud commented Apr 11, 2022

Reopen, failure

@Aranud Aranud reopened this Apr 11, 2022
@Aranud Aranud modified the milestones: 2.80, 2.81 Apr 11, 2022
@Aranud Aranud modified the milestones: 2.80.1, 2.81.0 Apr 29, 2022
@Aciz Aciz modified the milestones: 2.81.0, Future May 3, 2022
Aranud pushed a commit that referenced this issue May 13, 2022
…refs #90 #1408

1. As far as I know the part of the antiwarp design was to start allowing all incoming user commands if the player stopped moving, because - as I understand - he can't warp if he is not moving. This is done by checking user move commands in G_CmdScale, but the issue is player doesn't come to a stop as soon as those commads stop, which was causing player to warp for the remaining commands.
2. Antiwarping bots makes no sense.
3. If I understand correctly, as of now and just like in etpro, players are allowed to warp for the 75ms worth of commands LAG_MAX_DELTA. From what I tested warping is a bit visible still because of it. Could be maybe adjusted in the future but for now I left it be, because I'm not 100% sure if current fix in PR will be good and if changing that 75ms to lower value will not make playing less enjoyable for players with bad connections.
rafal1137 pushed a commit that referenced this issue Sep 13, 2022
…refs #90 #1408

1. As far as I know the part of the antiwarp design was to start allowing all incoming user commands if the player stopped moving, because - as I understand - he can't warp if he is not moving. This is done by checking user move commands in G_CmdScale, but the issue is player doesn't come to a stop as soon as those commads stop, which was causing player to warp for the remaining commands.
2. Antiwarping bots makes no sense.
3. If I understand correctly, as of now and just like in etpro, players are allowed to warp for the 75ms worth of commands LAG_MAX_DELTA. From what I tested warping is a bit visible still because of it. Could be maybe adjusted in the future but for now I left it be, because I'm not 100% sure if current fix in PR will be good and if changing that 75ms to lower value will not make playing less enjoyable for players with bad connections.
Aranud pushed a commit that referenced this issue Mar 2, 2023
This fixes PMF_TIME_KNOCKBACK never being set unless head was hit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug Something isn't working cat: general Category engine (client/server) cat: mod QAGAME Category mod (qagame) ❓ Feedback Further information is requested P1: Urgent Priority 1
Projects
None yet
Development

No branches or pull requests

9 participants