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

Elevators crushing player/monsters #700

Closed
vsonnier opened this issue Jun 28, 2024 · 23 comments
Closed

Elevators crushing player/monsters #700

vsonnier opened this issue Jun 28, 2024 · 23 comments
Assignees

Comments

@vsonnier
Copy link
Collaborator

vsonnier commented Jun 28, 2024

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
Copy link

Placo commented Jun 28, 2024

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
Copy link
Contributor

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
Copy link

Placo commented Jul 3, 2024

@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
Copy link
Collaborator Author

vsonnier commented Jul 3, 2024

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: d56b859

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
Copy link
Collaborator Author

vsonnier commented Jul 3, 2024

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)

  • I've got that case, if i run host_maxfps 0 while my usual host_maxfps 58 is OK...

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

  • I've got that case, if i keephost_maxfps 58 while host_maxfps 0 mostly fix it...

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

@vsonnier
Copy link
Collaborator Author

vsonnier commented Jul 4, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@vsonnier
Copy link
Collaborator Author

vsonnier commented Jul 4, 2024

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
Copy link
Contributor

j4reporting commented Jul 5, 2024

Not tested @Placo patch yet.

reverting the remaining bits of d2c34b3 in host.c , already partly reverted with 25cebeb , 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
Copy link
Collaborator Author

vsonnier commented Jul 5, 2024

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
Copy link
Collaborator Author

vsonnier commented Jul 7, 2024

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:

  • Added CVAR host_simu_time_quantums_per_sec : the time driving Host_Frame(time) is quantumized by amounts of host_simu_time_quantums_per_sec. 0 means 'realtime' as before.

@vsonnier
Copy link
Collaborator Author

vsonnier commented Jul 8, 2024

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
Copy link
Collaborator Author

I've continued some experiments around Host_Frame(time) including somewhat close to andrei-drexler/ironwail#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
Copy link
Contributor

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
Copy link
Collaborator Author

vsonnier commented Jul 14, 2024

Thanks @j4reporting for your investigation !

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

  • rm_resurgence ( re:Mobilize mod version 1.2b ) in the use case above is now OK with host_maxfps > 72
  • Quake Brutalist Jam 2 Start map elevator is also OK with host_maxfps > 72, no longer hurts the player wihile ascending/descending.

Peril Elevator is of course beyond saving :)

@j4reporting
Copy link
Contributor

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
Copy link
Collaborator Author

vsonnier commented Jul 15, 2024

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 debcaca.

@Novum
Copy link
Owner

Novum commented Jul 16, 2024

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

@vsonnier
Copy link
Collaborator Author

Added commit 58f5bbe 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
Copy link
Contributor

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
Copy link
Collaborator Author

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

@Placo
Copy link

Placo commented Aug 9, 2024

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants