Novum / vkQuake

Vulkan Quake port based on QuakeSpasm
GNU General Public License v2.0
1.82k stars 221 forks source link

[AD] reloading save breaks player bullets #734

Closed bk4928412 closed 2 weeks ago

bk4928412 commented 3 weeks ago

Describe the bug

on AD mod, reloading saves somehow causes weird lag and player's bullets stuck in mid air for a second. it gets worse with every reload. im not quite sure but it seems it only affects player bullets, i am unable to deal damage. at first, i thought maybe it was just qc related but then tried with ironwail, it doesnt have this issue.

To Reproduce

simply reload a save and try shooting an enemy.

Expected behavior

unable to deal damage, bullets wont fly

Screenshots

vkquake0002

Desktop (please complete the following information):

Mod

AD 1.8

vsonnier commented 3 weeks ago

I've seen similar things, but not on the few AD maps I played.... And in those cases reloading a save would make those 'stranded' items disappear. Moreover, those are more likely 'free' items, because if you do r_showbboxes 1, none of them have a box, which means they 'don't exist' anymore for the game, except for the renderer, as if there was a de-syncho between the two. Do you have Fast loading = on ? I've seen strange things with it on some maps only. Does switching it off improves the situation ?

Anyway, I need more info:

bk4928412 commented 3 weeks ago

i forgot fast loading exists. yes, it works fine without fast loading. and indeed some maps does not affected by fast reload. ad_e1m1 is one of those i think

s0.zip

j4reporting commented 3 weeks ago

Moreover, those are more likely 'free' items

this is a major bug that can lead to softlocks. I saw enemies not moving anymore or force fields still visible although the enemy was killed etc. I re-tried to reproduce and managed to prevent the last arena fight to commence! The first wave have not spawned!

s15 is the original savegame, taking the red armour will start the first wave. fastload, jump down to the secret, through the slipgate (teleports to the red armour) , and move to the left,

softlock.sav the enemies will never spawn. s_meatjam.zip https://www.slipseer.com/index.php?resources/meat-jam.392/

vsonnier commented 3 weeks ago

I can confirm that Fast loading = on is actually breaking some maps, and not only diplaying artifacts... For instance, in Snack Map 3 map sm3_sze (Overcast Citadel) you cannot access to the last area because the big lift never activates.

I'm now replaying all the pack with Fast loading = off and everything looks OK.

The fast loading feature was already present, but not exposed as a setting, in 1.30.1. I don't know if it is fixable at all, I don't understand enough the way it works and have too little time to study it.

On the other hand, this feature is just too pratical for very big levels, (when it doesn't break things !) I would like to keep it... Maybe as disabled, non-archived setting (CVAR_NONE), so it is always off at start, that way the player have to activate it each time it actually wants it ?

j4reporting commented 3 weeks ago

I'll restart the meatjam level with current git master. The save game s15 has a modify date of 08. Sep. and pre-dates this commit https://github.com/Novum/vkQuake/commit/26004e8dc2fdc62095b2e53509909c8b1e88e4d9 I saw the frozen edicts before, and I'm quite sure I didn't use fastload at that time. Usually I leave fastload disabled.

There have been so many changes to the edict handling recently.

bk4928412 commented 3 weeks ago

The fast loading feature was already present, but not exposed as a setting, in 1.30.1. I don't know if it is fixable at all, I don't understand enough the way it works and have too little time to study it.

present in what way, do you mean it fastloads maps automatically without any settings, or do you have to fastload manually

asking this because i realized i didnt check 1.31.0 yesterday and checked 1.31.1 2 times instead. my mistake..

missclick...(sorry again)

edit: 1.31.0 seems working fine.. i did fastload through cmd just to make sure. may need a double check though

j4reporting commented 3 weeks ago

this save game ad_dm1.zip triggers an assertion (Linux)

vkquake_git: ../Quake/pr_edict.c:89: ED_Alloc: Assertion!e->free' failed.`

fastload the savegame, open the door, attack the enemies

sometimes it's enough to play for a few seconds, then fastload and shoot to trigger the assertion

Interesting: I get the same assertion with the meatjam savegame! I've never seen this with VStudio builds on Windows!

Testing with the meatjam map on Linux with the save game from 08. Sept. Fedora 40 / gcc (GCC) 14.2.1 20240912 (Red Hat 14.2.1-3)

1.31.0 seems to be safe with fastload / first wave spawns and it seems that no edict are stuck 1.31.1 first issues, 1st wave of enemies spawns, but stuck edicts (enemies, rockets, etc ) current git master triggers the assertion.

j4reporting commented 2 weeks ago

quick update: I can't reproduce the assert on Windows even with mingw builds (MSYS2 / and w64devkit with gcc13 or gcc14 ). But it's exactly the point when things break on the map meat_f15h00k.
The assert should trigger when player picks up the MH and steps on the button in the secret cavern.
The button spawns a Quad in the arena above. It's not possible to pick up the quad at all, and all projectiles nails, grenades rockets etc. are stuck and won't move.

On Linux this situation is prevented by the assert ( => crash ) while on Windows program unfortunately continues...

vsonnier commented 2 weeks ago

Thanks @j4reporting for your tests... again ! The assertion error you encountered is actually very interesting, and we may have a something of a lead this time.

It is pure luck I added that assert in that code path, where ED_Alloc goes when it has to allocate a new edict, that is when it cannot re-use one on the free-list (for good reason) and actually raise the number of 'live' edicts. (num_edicts) Now the code (dating from 1.30.0 no less) assumed it was just new initialized memory (to 0), including e->free = false (0). Right ? Wrong ! The loading / fast-loading may change num_edicts a bit all the time, so there is no garantee that a new 'live' edict is properly reset, ...and we may even create one wrongly marked as free, like here.

I've made a branch (not published) bringing back the rebuilding of free-list (because we definitily cannot trust the current state of it after a load, fast or not), AND a proper reset of the new edict in ED_Alloc... which coincidentally is exactly what 1.22.3 did, but it get dropped after that point (1.22.3 pre-dates use of the original free-list as linked-list)

I'm now replaying Snack Pack 3 , Replicon, and soon Gibtropolis. I've also started Meat Jam and will test /re-test your particular save.

About the assert triggering or not, this is no mystery: on the Windows builds it is only enabled on Debug builds, as expected. I'm actually supprised it is present on Linux release builds, but hey !

vsonnier commented 2 weeks ago

The branch is here : https://github.com/Novum/vkQuake/tree/vso_fix_734. So far during my testing, no more frozen items 🤞

vsonnier commented 2 weeks ago

@bk4928412

present in what way, do you mean it fastloads maps automatically without any settings, or do you have to fastload manually

fastloading was, and is to be activated manually, but the mechanism is the same. Before 1.31.0 it was called Game > Load last save = fast I simply separated it in a distinct option Fast loading = on but the process is the same.

j4reporting commented 2 weeks ago

So far during my testing, no more frozen items 🤞

Things are looking good so far. I have not been able to trigger the assertion with the meatjam savegame.

asserts: can be disabled by the preprocessor option NDEBUG. It disables the assert macro in /usr/include/assert.h It's defined in common.make, it's also set in VStudio ( project C/C++ preprocessor ). The linux build was compiled with meson/ninja and the meson.build file does not define NDEBUG.

Maybe it's a good idea to have a check for !e->free and print something meaningful to console and inject a disconnect command?

vsonnier commented 2 weeks ago

Maybe it's a good idea to have a check for !e->free and print something meaningful to console and inject a disconnect command?

No point now, at that particular place ED_Alloc new edicts are, by construction garanteed to be non-free.

Our particular problem was litterally there since 1.22.3, hidden first by the 'leak'-y management of edicts, fixed aftewards, and marginally hidden by the broken free-list implementation as a linked-list.

The Fast loading = on by its very nature was much more prone to trigger this ED_Alloc problem, while having at the same time its own problems now completly solved (or blanked) by forcing a rebuild of the free-list after each load/save.

So with all the cleanups addded after a load/reload + plus this particular fix, I'm quite confident the edicts memory lifecycle is now OK.

Meme_Always_has_been

I've played a handful of maps now, no frozen (free) entities seen... Sometimes monsters get stuck in geometry, especially Fiends, but I've seen that before.