Novum / vkQuake

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

Elevators crushing player/monsters #700

Closed vsonnier closed 3 months ago

vsonnier commented 4 months ago

From a vkQuake-vso reported issue:

Hello, Good job on keeping vkQuake alive! I use your port as my main quake engine, together with Ironwail. Is it ok if I report vkQuake bugs to you here? If not feel free to remove this bug report.

This one is a physics bug I have first noticed last summer while playing through the mod Peril (https://www.slipseer.com/index.php?resources/peril3-0.253/), it is quite noticeable there because the mod makes extensive use of Quake elevators and trains.

The bug is that when on an elevator moving up or down sometimes it stops and begins to crush the player or the monsters that are on it, even if there are no obstacles. The player can jump to stop the crushing and have the elevator resume its course but most of the time the crushing resumes a few seconds later. I have had this bug in other, more regular Quake mods as well, for example I've had it occur on the elevator in the start map of Quake Brutalist Jam 2. But admittedly it is rarer to have it occur with slower, less long elevator rides yet I still encounter it from time to time.

For the mod Peril, the bug is present in Ironwail as well, but in Ironwail there is a workaround: enabling v-sync fixes it completely, even with target framerates well above 72fps (this is very strange and feels more like a bug itself than anything else but hey it fixes the issue). I have tried messing with vsync or setting host_maxfps below 72 in vkQuake to see if it wasn't a physics/rendering decoupling leftover bug but nothing makes it work. The bug is not present in QSS.

I have linked a savegame to just before the bug occurs in the first level of the Peril mod (it's at the start of the level anyway). elevator crushing bug Peril savegame.zip


Hello @Placo,

Good job on keeping vkQuake alive! I use your port as my main quake engine, together with Ironwail. Is it ok if I report vkQuake bugs to you here?

Thank you for your kind words, happy to see this port helps you.

This one is a physics bug I have first noticed last summer while playing through the mod Peril The bug is that when on an elevator moving up or down sometimes...etc.

Oh I know this one ! This is also known to by the mod author Balgorg and somewhat mitigated in the latest release (3.0c now) that can be found here : https://www.moddb.com/mods/peril/downloads. Otherwise, the author just advise to use QSS.

Amazing mod, right ? I'm now in Geothermal and stopped here for now. I secretly hope Balgorg will update the mod again and I can replay it from the begining.

The player can jump to stop the crushing and have the elevator resume its course but most of the time the crushing resumes a few seconds later.

Yes, it triggers every time on the long descending elevator of the first level, an occurs multiple times.

I always play with v-sync off, with limiting refresh rate in game instead by host_maxfps 58 (monitor is 60Hz)

My workaround for this physics problem is to unlock the maxfps by host_maxfps 0 temporarily. Even with that, it can still occur on the descending elevator once or twice.

To be more practical, I've bound that to a shortcut 'p' in autoexec.cfg like this:

alias al_physicstoggle_on "echo Physics workaround on (DISABLE fps limit); host_maxfps 0 ; scr_showfps 1 ; bind p al_physicstoggle_off"
alias al_physicstoggle_off "echo Physics workaround off (normal fps limit); host_maxfps 58; scr_showfps 0 ; bind p al_physicstoggle_on"
bind p al_physicstoggle_on

The bug is not present in QSS.

I know, but unfortunatly I've not stumbled (yet) on the magical line of code in QSS responsible.

Placo commented 4 months ago

Thanks for copying the issue here! Since then I have tried to look around the code myself but since it's my first foray into a real game engine I don't think I stand much chance of fixing this myself. But hey, at least I'm learning :)

My hunch was that it was some kind of float precision issue around the SV_PushMove method but I've not yet looked up how I can even debug that. And comparing the code with QSS and Ironwail now I'm not so sure that's the cause of the issue

j4reporting commented 4 months ago

Might be related. Noticed something similar in rm_resurgence (re:Mobilize 1.2), when taking the lift up from level 0 to level 1 in the trident building, Lift will reverse the direction shortly before reaching the destination. However, the player suffer no damage. You need to jump shortly before this happens or disable renderer/network isolation host_maxftp <= 72

Lift has 2 more levels, and they do work fine though. Can't reproduce with ironwail.

Placo commented 4 months ago

@j4reporting I've tested it and indeed it is the same issue. I have located the culprit line of code, it's in the calculation of host_frametime in _Host_Frame (in host.c)

I don't get what vkquake is trying to do here but when I replace that line: host_frametime = sv.active ? (listening ? q_min (accumtime, 0.017) : host_netinterval) : accumtime; with (the calculation from Ironwail and QSS) host_frametime = q_max(accumtime, host_netinterval); It fixes the issue with elevators. Not sure what doing this breaks though...

vsonnier commented 4 months ago

Thanks @Placo for this find. Alas there has been some significant host.c changes in vkQuake which make it diverge from both QS and QSS from which it picks up some change or other.

For instance, that particular line you picked points on a change by @temx: d56b85937a8d54c2f57239c6a25789813e23b3a4

I'll test your suggestion and try to understand what this commit does..

Another thing I wanted to test, is adding a CVAR to always force the physics to tun at the fixed rate = 72 Hz, i.e. which effect is the same as host_maxfps N where N is either > 72 or 0 (infinite), i.e. forcing host_netinterval= 1/ 72.f whatever the fps.

The question is rather : is there any desavantage at runing the physics at a fixed rate, irrespective of the host_fps ?

vsonnier commented 4 months ago

Another thing I wanted to test, is adding a CVAR to always force the physics to tun at the fixed rate = 72 Hz, i.e. which effect is the same as host_maxfps N where N is either > 72 or 0 (infinite), i.e. forcing host_netinterval= 1/ 72.f whatever the fps.

The question is rather : is there any desavantage at runing the physics at a fixed rate, irrespective of the host_fps ?

Nevermind, it breaks things 1 side, while fixing the other...

About the given examples:

for example I've had it occur on the elevator in the start map of Quake Brutalist Jam 2 (a very fast elevator down)

Peril : the long, slow, elevator down at start.

@Placo So 2 exact opposite cases I'll test vs. your suggestion when I understand what it does.

vsonnier commented 4 months ago

Another thing I wanted to test, is adding a CVAR to always force the physics to tun at the fixed rate = 72 Hz, i.e. which effect is the same as host_maxfps N where N is either > 72 or 0 (infinite), i.e. forcing host_netinterval= 1/ 72.f whatever the fps.

Pushed a commit for a new CVAR host_phys_cst_ticrate [0 - 72] to have this as an experiment, default = 0 unchanged behaviour.

j4reporting commented 4 months ago

Another thing I wanted to test, is adding a CVAR to always force the physics to tun at the fixed rate = 72 Hz, i.e. which effect is the same as host_maxfps N where N is either > 72 or 0 (infinite), i.e. forcing host_netinterval= 1/ 72.f whatever the fps.

Pushed a commit for a new CVAR host_phys_cst_ticrate [0 - 72] to have this as an experiment, default = 0 unchanged behaviour.

won't this break physics when host_maxfps <> host_phys_cst_ticrate?
host_maxftp {1-72] does this already. The engine moves/rolls forward physics each frame by its frametime i.e ~16ms for 60Hz.

you can try vanilla quake or QS what happens with higher values for host_maxfps. QS will print only a warning that things will break but does not have implemented the renderer/network isolation that keeps the physics at save frametimes.

j4reporting commented 4 months ago

just noticed: the elevator arrives on level 1 in rm_resurgence in a coop game ( one player connected ),

vsonnier commented 4 months ago

host_maxftp {1-72] does this already. The engine moves/rolls forward physics each frame by its frametime i.e ~16ms for 60Hz.

Yeah you are right, a better name would have been host_phys_max_ticrate . The only usefulness is to experiement something like a slow physics update like 35Hz (like Doom , why not) with for instance unlimited fps.

Not very usefull indeed, at the end I'll probably revert this.

Not tested @Placo patch yet.

j4reporting commented 4 months ago

Not tested @Placo patch yet.

reverting the remaining bits of https://github.com/Novum/vkQuake/commit/d2c34b38b3fe566873a414e28dea4ef1f0ddafd9 in host.c , already partly reverted with https://github.com/Novum/vkQuake/commit/25cebeb1e05ddee6484f04b78b41c2eb6396665e , brings this closer to QSS and ironwail again. lift in rm_resurgence is not bugged, and lift in peril works with vsync enabled like in ironwail.

there is at least one other commit, that might depend on this reworked version, though ( dedicated server ).

CVAR to switch between different codepaths? kiddin'

vsonnier commented 4 months ago

CVAR to switch between different codepaths? kiddin'

Hey thanks for testing for that roolback. But all those modifications were done in good faith to improve things I suppose, so I would rather try to understand and reproduce the problem on the current to actually fix it, before considering reverting.

Now do not hesitate to publish that change on a branch or through a PR of yours so we can experiment (and critisize:)

The problematic part of this investigation it is that the interlocking of farmerate, simulation time, and real time elapse in general : it means the moment you execute that in a debugger or worse, stops using a breakpoint, the beahaviour is modified.

Now for testing if we stub Sys_DoubleTime () to control Time (Whoa...) increments, that would ease the debugging : stop and run time arbitrarily.

Another thing to test for fun : always make Sys_DoubleTime () increment by an integer of of a minimal quantum value of 1 ms (for instance) to see what happens. I feel like having a discrete time source would make the computations less prone to float roundings along the way.

vsonnier commented 4 months ago

FYI I've reverted my previous experiments from master, and push an experimental branch for us to sandbox : https://github.com/Novum/vkQuake/tree/issue_700_experiments

From Master:

vsonnier commented 4 months ago

Right @j4reporting I'm lacking time (ah!) to really tickle all this, so I'll gladly take your PR, if you have any. This will deserve a new 1.31.1 release as well.

vsonnier commented 4 months ago

I've continued some experiments around Host_Frame(time) including somewhat close to https://github.com/andrei-drexler/ironwail/pull/312 but nothing works right, the bench being the blighted Peril elevator down, working OK with QSS. (it was developped with it)

Alas bringing QSS changes graviting around Host_Frame() or SV_Physics() brought nothing of value or so it seems.

j4reporting commented 4 months ago

maybe someone with more insight could comment what's happening here?

The liff in rm_resurgence ( re:Mobilize mod version 1.2b ) will work with slight less precision for host_netinterval. The culprit seems to be the door in the elevator shaft blocking access to the basement from floor 0. The door will open when player gains access to the basement through another entrance.

this is also reproducible with QS, QSS and Ironwail with command host_framerate.

override host_frametime with command host_framerate ( default = 0 )

setpos  2662 -2601 -1767   0  0  0     ( teleports you to the basement of the buildung )

host_framerate 0.013888     => lift reverts on the way up      floor  B-> 0        
host_framerate 0.0138888    => lift reverts on the way down    floor  1-> 0  and floor 0 -> B
host_framerate 0.01388888   => lift reverts on the way up      floor  B-> 0  and floor 0 -> 1   
host_framerate 0.013889     => no issues    => 0.01388    with some safety margins 

so something like host_netinterval = floor ( 100000.0 / 72 ) / 100000; in Max_Fps_f() prevents the platform from reverting its direction.

vkQuake's timings are simply too accurate, because with host_maxfps 72 values for host_framerate are ~ 0.013888 and the lift will also bug out with renderer/network isolation disabled.

Why does the platform revert its direction? Is it just a combination of the speed of the platform and the retracted door? Are we already trigering the physics bugs that renderer / network isolation is supposed to prevent?

vsonnier commented 4 months ago

Thanks @j4reporting for your investigation !

I've committed the small change above, according to your findings. With this:

Peril Elevator is of course beyond saving :)

j4reporting commented 4 months ago

I didn't knew that QBJ2 was also bugged.

For peril ( at least the first elevator ) you would need to bring back the host_phys_cst_ticrate cvar and set it to 50 ot 51 ( again with less precision in case of 51).

you can reproduce this also in QSS again with host_framerate

host_framerate 0.0196078        bugged 51 FPS
host_framerate 0.01960          ok
host_framerate 0.02             50 FPS

Or bring back the old less frame rate consistent physics suggested by @Placo and make it available by a new CVAR (no archive). Not sure if you really want to go there...

QSS and ironwail (vsync enabled) works only because host_frametime = accumtime and it seems most of the time accumtime is in the range of 0.02 - 0.019

vsonnier commented 4 months ago

For peril ( at least the first elevator ) you would need to bring back the host_phys_cst_ticrate cvar and set it to 50 ot 51 ( again with less precision in case of 51).

Ah yes, I've noticed this before. Fine, I've brought back host_phys_max_ticrate (no archive, default = 0 = disabled) for those edge cases :)

Anyway, I've noticed that the Mjölnir elevator at MJ4M3 start no longer hurt the player when host_maxfps > 72 with commit https://github.com/Novum/vkQuake/commit/debcaca0c94a1d927d0bedfeb9313db290304f13.

Novum commented 4 months ago

Time ideally should be integer fixed point, but I never got around to changing this everywhere.

vsonnier commented 3 months ago

Added commit https://github.com/Novum/vkQuake/commit/58f5bbe6e20ac3134c290bcd016a239279e6b925 from @andrei-drexler , thank you very much ! The infamous Peril Elevator is now fixed.

I'm keeping both host_phys_max_ticrate and #define MAX_PHYSICS_FREQ (71.9990) as extra seatbelts.

j4reporting commented 3 months ago

I have the impression that MAX_PHYSICS_FREQ (71.9990) is a bonus overall. Need to test more, but with this tweak the player's movement feels more responsive. With MAX_PHYSICS_FREQ(72) there seems to be some kind of delay/lag sometimes. Hard to describe. I hope I'm not just imagining it.

vsonnier commented 3 months ago

I think we can close this now. Thanks @j4reporting @andrei-drexler (and others) for the help !

Placo commented 3 months ago

Hello, sorry for reopening this issue but the fix only works on vertical pushers. Peril's "blimp" map is still bugged because monsters are standing on flying platforms that are moving up/down and to the sides and are crushed by them (not the big ship/first wave of enemies) but some waves later in the level. Another good test is the start of Peril's "mountain" level. The cable cars used to crush the player, now it's fixed unless you stand against a wall (the cable car's movement is not just up but also to one side), Also now the health kit and ammo in the cable car clip though it near the start and fall to the ground.

I'm not sure trying to wiggle each edicts standing on a pusher into position with each increment of its movement would be a good solution though...

j4reporting commented 3 months ago

a pusher is also a platform that moves horizontally (e.g. in DM2 ) or any other direction.

Can you pls try if you can reproduce this in QSS? I tried the mountain map in qss and player is also taking damage. Have not tried blimp.

Objects clipping through, Yeah I think I witnessed this with the red armor near the end of vm8 ( Seismic Ventures ).

It's probably better to change the default of sv_gameplayfix_elevators to 1 instead of 2. Need to test again with that level.

cvar_t sv_gameplayfix_elevators = {"sv_gameplayfix_elevators", "2", CVAR_ARCHIVE}; // 0=off; 1=clients only; 2=all entities

UPDATE: the red armor in VM8 falling off the platform into the water below is something else.

Placo commented 2 months ago

Those specific cases I mentioned work well in QSS (latest march 2024 release), I have no issue with it. Are you sure you don't have other things/config messing with it? The mod was developed for QSS (albeit an earlier version) so it seems strange you have such issues.

All these problems are gone when testing in my vkQuake with the host_frametime calculation change (from QSS) I mentioned earlier in this thread. I get that it might break some quake standard timing somewhere (still haven't got around to looking into it proper), and I imagine it might break some networking thing or even some demos, but for my own personal use I don't notice anything unusual in the gameplay (movement, jump distances, that sort of things), except that I didn't get any problem in my replay of Peril and that with its performance and particle effects it is by far the best engine for it.

j4reporting commented 2 months ago

I guess it depends on the HW. I can definitely reproduce the issues with QSS ( latest git ) on Linux and WIndows. Different results with various host_maxfps settings. Player blocks platform/cable car with host_maxfps 71 1 - 2 seconds after the platform started to move. Later on, player get stuck again at various points, e.g when the ship appears and before reaching the destination.

Enemies get stuck sometimes on the second platform. You can see the cable car stops moving then continue again. Sometimes it stops for good and won´t move again. host_maxftp 40 seems to fix most of this issues on this level on my HW, although player still blocks the platform sometimes.

It seems the author micro optimized this mod to QSS and his HW.

vkquake: does sv_gameplayfix_elevators 0 bring back the issues on your patched version?

Maybe it's worth to experiment with DIST_EPSILON and multiply it with an integer factor in sv_phys.c and let the edict jump just a little higher...

--- a/Quake/sv_phys.c
+++ b/Quake/sv_phys.c
@@ -565,7 +565,7 @@ void SV_PushMove (edict_t *pusher, float movetime)
                        // try moving the entity up a bit if it's blocked by the pusher while also standing on it
                        if (riding && block == pusher && (sv_gameplayfix_elevators.value >= 2.f || (sv_gameplayfix_elevators.value && e <= svs.maxclients)))
                        {
-                               check->v.origin[2] += DIST_EPSILON;
+                               check->v.origin[2] += DIST_EPSILON * 2 ;
                                if (!SV_TestEntityPosition (check))
                                        continue;
                        }

btw. the health pack and ammo do fall through the floor with sv_gameplayfix_elevators 0 too.

j4reporting commented 2 months ago

Sorry I did the tests on Linux only. QSS works on Windows (same HW). Also tried the included qss windows version from 2022 (the failback version available on triptohell ) This version reports lower fps, but they may be calculated differently. This older version also works much better on Linux.

Taking the fps values from the old qss version as a guide, I tried the following in vkquake in map mountain

sv_gameplayfix_elevators 0
host_phys_max_ticrate  30 / or 35    ( from QSS) 

and look and behold, the ammo and health pack no longer fall through the floor on the cable car. And overall, player and enemies take much less damage riding on the platforms. Enabling sv_gameplayfix_elevators 1 or 2 may help here.

host_phys_max_ticrate 40 and both items fall through again.

Unfortunately, it seems that the value forhost_phys_max_ticrate needs to be adjusted for every peril3 map. The FPS values shown by the old QSS value seem to be a good starting point.