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

Infighting and indirect kills not accounted to players in netgame #778

Open
dirkd opened this issue Nov 7, 2021 · 2 comments · May be fixed by #784
Open

Infighting and indirect kills not accounted to players in netgame #778

dirkd opened this issue Nov 7, 2021 · 2 comments · May be fixed by #784

Comments

@dirkd
Copy link

dirkd commented Nov 7, 2021

else if (!netgame && (target->flags & MF_COUNTKILL) )
{
// count all monster deaths,
// even those caused by other monsters
players[0].killcount++;
}

Since the above code does only attribute kills to player 1 in singleplayer when playing cooperative multiplayer kills get "lost" to infighting and indirect kills (monsters shooting barrels).

This affects the both level stats and tally screen when doing UV Max co-op runs - making them a little unpleasant QoL-wise.

It would be preferable to have kills always be attributed to a player (or have an option to at least force this behavior). What's your thoughts about this?

I don't know how original executables attributed those.
PrBoom+/DSDA-Doom attribute kills to the last enemy of the dying object if it was a player or the first player who is still ingame otherwise.

https://github.com/coelckers/prboom-plus/blob/e08f129f557cae6e529d8f9ff151750a2345a16a/prboom2/src/p_inter.c#L704-L723

Demo (solo-net) showing the behavior: infight-tally-00000.lmp (2:18 min, all kills @ 1:30)

@fabiangreffrath
Copy link
Owner

I am surprised that the PrBoom derived family of ports deviate from the Vanilla behaviour here, since one way to track demo sync is to compare the tally screen - and thus level stats - output with the same run in Vanilla. Otherwise I agree that not properly counting kills in multiplayer runs is unpleasant to say the least.

Could you please preparea PR with the minimum required code change that is necessary for the tally screen and level stats output to match PrBoom+?

@dirkd dirkd linked a pull request Nov 18, 2021 that will close this issue
@dirkd
Copy link
Author

dirkd commented Nov 18, 2021

There's the PR but I think there's some room for discussion before proceeding (after thinking about this for a while I'd rather see #784 rejected in its current state, see below).

The behavior PrBoom+ is showing is indeed surprising with regard to demo verification purposes. But coop demos are probably sufficiently rare that nobody has ever bothered with it not aligning to the vanilla executables nor Choco/Crispy.

The PR in #784 consists of two commits implementing two stages of PrBoom+' behavior:

The first one dirkd@3e3fe6b implements attributing kills at all - which DOOM 1.9 doesn't so this is breaking visual compatibility with Vanilla in coop netgames.
I'd argue that since Crispy also provides a tally after Map 8 of each Episode in doom.wad this is actually in-line and a simple QoL change.
With this commit every kill on the map is accounted for and attributed to a player (percentages on tally might still not sum up to 100% due to rounding errors; can see this side-by-side in this timestamped VoD of last weekend's DoomRave - left side patched, right side unpatched Crispy 5.10). BUT the tally doesn't align with PrBoom+ with this change only.

The second commit dirkd@db100ff pulls in some stuff that was introduced in Boom I think - namely the lastenemy field in mobj_s.
With this commit the tally actually matches PrBoom+ and every kill should be accounted exactly the same way - i.e. to the same player - PrBoom+ does it (validated against ep1c1003, e2cm2122 and e3cm1659 from DSDA). Whether this is a good thing is up for debate since this might've bled into the lower complevels without notice in the past in PrBoom itself (just guessing from the git log of coelckers/prboom-plus@b8291d6#diff-91371af3fe6ea63c1cbac23d5e0f05210e035f443c48ccec1964932010fe2d49R650-R683 here; change was introduced in prboom-plus-2.2.6-19 and is more or less a copy of the complevel>=10 behavior substituting the random distribution for the first player still in-game choice).

In conclusion merging only dirkd@3e3fe6b would introduce yet another way of counting kills (neither matching vanilla nor PrBoom+) while merging both commits pulls behavior into Crispy one might consider "not faithful to the original".
On top adding the additional field to mobj_s breaks savegame compatibility which is probably a no-go as per Crispy's project synopsis.
The latter could possibly be worked around by storing the field OOB so it wouldn't be saved/loaded. Since it is only used for aligning the detailed kill attribution for netgames and not anything else (the rest of the uses in PrBoom+ are complevel-guarded and not applicable to anything below Boom-compatibility). Also loading is not compatible with either recording demos or netgames anyways. I could implement this if this is actually considered for inclusion.
I would argue that pulling in the first commit only plus an additional change to add either a commandline flag or crispness option (or both) to toggle this behavior so people who want this can opt-in/opt-out (depending on the default) would be the cleaner solution implementation-wise (less code, more predictable behavior/outcome). Making the second one work in a non-disruptive way (fixing save/load by pulling it out of mobj_s) would introduce a lot of code just for the niche of coop netgames and introduces behavior that is actually quite finicky to reason about (only attributes to lastenemy if it's an alive player at the time of the monster dying or the first player still in-game otherwise + lastenemy can change only if it's not set, currently not alive OR not a player at the time of receiving damage, esp. the alive/dead part in both code paths is tricky in the context of multiplayer games).

What's your take on this?

side note:
For validation purposes I've used a makeshift implementation of PrBoom's extended levelstat.txt format showing per-player-kills at dirkd/crispy-doom@53d05ab.
Which led me to the question: Is there interest in having this in Crispy - i.e. per player stats in levelstats.txt and/or on HUD (I have a working implementation for the latter, see DoomRave VoD above for visuals)?

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

Successfully merging a pull request may close this issue.

2 participants