bradharding / doomretro

The classic, refined DOOM source port. For Windows PC.
https://www.doomretro.com
GNU General Public License v3.0
698 stars 88 forks source link

Jittery interpolation of dropped items on rising platforms #501

Closed bradharding closed 6 years ago

bradharding commented 6 years ago

The z-position of dropped items isn't interpolated correctly when on a rising platform. The weird thing is, this problem only occurs once for those items. For example, a zombieman drops a clip on a lift while the lift is down. As the lift goes up, the clip jitters. Lower the lift. As it goes up a second time, the clip no longer jitters. This can easily be tested using the following map:

interpolate.zip

@fabiangreffrath and @JNechaevsky, this may be of interest to the two of you, as the same thing occurs in Crispy Doom. (I suspect Russian Doom as well, but haven't tested.) I will continue to work out why this is occurring, but haven't had any luck yet.

JNechaevsky commented 6 years ago

I see, thanks for notifying! It feels like the thing, that's wasn't initially spawned on (interpolated) sector doesn't have z-interpolation until sector's vertical movement is started. No idea how it should be fixed...

Perhaps, some additions should be done in R_AddSprites, which calls R_ProjectSprite with interpolation info?

bradharding commented 6 years ago

I'm not sure. This issue has really stumped me. It also occurs in PrBoom+, and that handles interpolation a bit differently to how our ports do.

fabiangreffrath commented 6 years ago

Well, I'd say this is because dropped items are spawned at the sector's floorheight whereas the sector's floor itself is rendered at its interpolated interpfloorheight. The first chance these two values become equal again is when the sector stops moving,

https://github.com/bradharding/doomretro/blob/master/src/p_mobj.c#L738

This is just as untested assumption, but should it prove true, I am afraid this is nothing I could fix in Crispy, as I may not change an item's z-coordinate.

JNechaevsky commented 6 years ago

Is it because of changing to interfloorheight may be unsafe for demos? Another possible case to care - something odd may happen in uncapped mode, but not sure, AFAIR there is a place where was something like this: sector->floorheight = sector->interpfloorheight.

I'm extremely intrigued, but will be able to check only in the evening, there are four and half hours left at day job.

JNechaevsky commented 6 years ago

Nope, not worked for me, and that's really odd. Changing initial mobj->interp to true in P_SpawnMobj (or P_SpawnMobjSafe) does not helps. I've even tried to make some changes in P_KillMobj:

mo = P_SpawnMobj (target->x,target->y,ONFLOORZ, item); replaced to mo = P_SpawnMobj (target->x,target->y,target->subsector->sector->interpfloorheight, item);

...and still the same. Adding mo->interp = true; into P_KillMobj also not working. :confused:

fabiangreffrath commented 6 years ago

How bout this (in Crispy's code, please adapt to your code changes)?

--- a/src/doom/p_mobj.c
+++ b/src/doom/p_mobj.c
@@ -654,8 +654,8 @@ P_SpawnMobjSafe
     // set subsector and/or block links
     P_SetThingPosition (mobj);

-    mobj->floorz = mobj->subsector->sector->floorheight;
-    mobj->ceilingz = mobj->subsector->sector->ceilingheight;
+    mobj->floorz = mobj->subsector->sector->interpfloorheight;
+    mobj->ceilingz = mobj->subsector->sector->interpceilingheight;

     if (z == ONFLOORZ)
        mobj->z = mobj->floorz;
bradharding commented 6 years ago

Thanks for the suggestion Fabian. Unfortunately it doesn't help. AFAIK, interpfloorheight will equal floorheight every real tic, rather than just when a lift reaches its destination height.

fabiangreffrath commented 6 years ago

Then it's probably in P_CheckPosition() in p_map.c, else I have no further idea, sorry.

JNechaevsky commented 6 years ago

Me neither have any luck with various experiments in P_SpawnMobj(Safe). You know what's even more confusing? Even if zombieman is killed while platform is moving up, dropped clip will still be jittered.

JNechaevsky commented 6 years ago

Hell yes, I've figured out something. Look into boolean P_ThingHeightClip and do this:

    if (onfloor)
    {
    // walking monsters rise and fall with the floor
+++ if (thing->type != MT_CLIP)
    thing->z = thing->floorz;

This will fix clip's jittering. But obviously, this is not right condition, it should be something like... "If (mobj doesn't have flag MF_DROPPED)", but excuse me, I can't remember necessary symbols/combination (|=, &, etc). : (

Edit: Errrr... Is it? if (!(thing->flags & MF_DROPPED))

Edit 2: Still not good. No jittering while platform going up, but when will go down again, clip will be "jumping" from air to floor. Guess now it's time for me to pass the baton.

bradharding commented 6 years ago

Thanks for attempting to look into this. I've scoured the relevant code (P_ThingHeightClip, P_CheckPosition, etc.) and I'm at a complete loss. It's not because an item has a dropped flag, because in DR if I spawn anything through the console while a lift is down, the same thing happens. Something somewhere in the item's mobj_t struct changes once the lift its on reaches the top. It doesn't occur when interpolation is off.

JNechaevsky commented 6 years ago

Yeah, capped framerate working fine, and it feels like z coordinate is fighting in standard/interpolated positioning. This give a little thought - perhaps, in P_ThingHeightClip should be added something like this: thing->z = thing->old = thing->floorz;. AFAIR, oldz is used somewhere in interpolation code.

fabiangreffrath commented 6 years ago

Is it just me or is the entire sector moving jittery?

bradharding commented 6 years ago

Seems smooth to me, both in DR and Crispy.

JNechaevsky commented 6 years ago

Yep, sector interpolation seems to be fine with safe condition and without (when player standing on the edge of the platform and have a delay on view height update).

fabiangreffrath commented 6 years ago

The clip simply doesn't get interpolated, though it has not been teleported nor is it connected to the player's map thing. Strange!

fabiangreffrath commented 6 years ago

No, wait, that's not it. If I comment out thing->z = thing->floorz; in P_ThingHeightClip() it gets rendered properly, but other stuff breaks, of course.

fabiangreffrath commented 6 years ago

It depends on the order in which thinkers are executed!

When the sector's floor is lowered, the T_PlatRaise() thinker is still active and counts the tics until it is time to raise the floor back again. When you kill the zombieman during this time, the clip that he drops spawns its own P_MobjThinker(), which is then executed after the sector's T_PlatRaise() thinker.

This is why the dead zombieman isn't affected by the jitter, but the dropped clip is.

Once the sector's floor has reached back to its original position, its T_PlatRaise() thinker gets removed. If you trigger the switch a second time, of course a new T_PlatRaise() thinker gets spawned, but this time it gets executed after both the dead zombieman's and the dropped clip's P_MobjThinker()s.

To proof my point I have created a patch which makes sure that during each tic all the thinkers of type P_MobjThinker() are executed first and then all the others after them. Applying this patch entirely fixes the issue of the jittery interpolated rendering of the dropped clip, but it is most probably not safe for network or demo compatibility:

https://gist.github.com/fabiangreffrath/bd0203aa1d5b1674b6f501dd5c04fd76

bradharding commented 6 years ago

Thanks for taking the time to look into this Fabian! The thought that it might be a thinker order issue did actually occur to me too, and I tried running all mobjthinkers first, but must've done something wrong. Just arrived at work, so won't be able to try your patch for a while. I'll let you know how I go. Thanks again!

bradharding commented 6 years ago

Works beautifully @fabiangreffrath. See commit d0bc80dd7562818ec5adff83592130affece4a53 if you're interested. Thanks again!