clipping issue with sprite heights and moving floors (e.g. lifts)

[jmtd]

Background

Version of Crispy Doom:

5.4 (debian 5.4-3)

Operating System and version:

Debian stable

Game: (Doom/Heretic/Hexen/Strife/other)

Doom (2)

Any loaded WADs and mods (please include full command line):

none

Bug description

Observed behavior:

MAP17, going down on the lift to the room wtih the spectres and a hell knight. I dropped down the hole that the lift made and landed on a spectre, still within the lift sector. When the lift returned, it passed me vertically and I ended up clipped out below the floor of the sector.

Expected behavior:

When the lift height raised above the spectre height I should have been carried up (possibly the spectre too, but I'm not sure whether it was within the lift sector or not)

[fabiangreffrath]

This was with "walk over/under monsters" enabled, right?

[JNechaevsky]

I was able to reproduce it, requires some luck! I was having a "walk over/under monsters" enabled, but I've also catched two another problems - pay attention of how player (1) and Spectre (2) was jammed on the lift! Can it be a consequences of recently introduced code of monster's heights?

• Video: https://youtu.be/qYy5qGyBgr0
• Savegame (slot 0): doomsav0.zip

And by the way, my kindest greetings to jmtd!

[fabiangreffrath]

Thank you very much for the Savegame! What we actually need here is a demo reproducibly triggering this behaviour, but well, since this feature isn't demo compatible in the first place... 😉

I guess what we see here is P_ThingHeightClip() and PIT_CheckThing() fighting for the proper values for thing->floorz and thing->ceilingz. As a rule of thumb, if I stand on someone else's head, I can only lower his ceiling and he can only raise my floor. I am sure if we consider this in the code, this bug will vanish.

[JNechaevsky]

I can take a part of MAP17 and with luck and patience can replicate it on demo. But am I got you right by the fact that "over/under" is a singleplayer only, so it's always deactivated in demo recording?

[fabiangreffrath]

But am I got you right by the fact that "over/under" is a singleplayer only, so it's always deactivated in demo recording?

Yep, it's forcefully disabled in demos, because it changes game physics significantly.

I will apply a proposed fix soon and we need to see if the bug is triggered when repeating the same steps e.g. 10 times. Just as you did to replicate the original bug here.

[JNechaevsky]

Ah, good to know! Please notify me in the commit or provide a patch, I'll try to catch it on my side.

[fabiangreffrath]

This should be it:

--- a/src/doom/p_map.c
+++ b/src/doom/p_map.c
@@ -392,7 +392,8 @@ boolean PIT_CheckThing (mobj_t* thing)
         (thing->flags & (MF_SOLID | MF_SPAWNCEILING)) == (MF_SOLID | MF_SPAWNCEILING) &&
         tmthing->z + tmthing->height <= thing->z)
     {
-       tmceilingz = thing->z;
+       if (thing->z < tmceilingz)
+           tmceilingz = thing->z;
        return true;
     }

@@ -400,6 +401,7 @@ boolean PIT_CheckThing (mobj_t* thing)
     if (critical->overunder &&
         tmthing->player && thing->flags & MF_SHOOTABLE)
     {
+        fixed_t newfloorz, newceilingz;
         // [crispy] allow the usual 24 units step-up even across monsters' heads
         // but do not allow if this height has been reached by "low" jumping
         fixed_t step_up = tmthing->player->jumpTics > 7 ? 0 : 24*FRACUNIT;
@@ -407,16 +409,20 @@ boolean PIT_CheckThing (mobj_t* thing)
         if (tmthing->z + step_up >= thing->z + thing->height)
         {
             // player walks over object
-            tmfloorz = thing->z + thing->height;
-            thing->ceilingz = tmthing->z;
+            if ((newfloorz = thing->z + thing->height) > tmfloorz)
+                tmfloorz = newfloorz;
+            if ((newceilingz = tmthing->z) < thing->ceilingz)
+                thing->ceilingz = newceilingz;
             return true;
         }
         else
         if (tmthing->z + tmthing->height <= thing->z)
         {
             // player walks underneath object
-            tmceilingz = thing->z;
-            thing->floorz = tmthing->z + tmthing->height;
+            if ((newceilingz = thing->z) < tmceilingz)
+                tmceilingz = newceilingz;
+            if ((newfloorz = tmthing->z + tmthing->height) > thing->floorz)
+                thing->floorz = newfloorz;
             return true;
         }

[JNechaevsky]

For now, falling into the floor seems to be fixed. But there is still small problem with clipping, take a look:

• https://youtu.be/2kQ_y-AOXTM

Lift can normally get up if I'm standing on monster's head, but later it becomes jammed and will not get down until monster is killed.

Edit: maybe it's because of only radius of first thing is being checked by lift? Player's radius is 16*FRACUNIT so he fits, but Demon's radius is 30*FRACUNIT which making him and lift stuck?

Can't explain my thought more clear, it's sill morning. 😴

[fabiangreffrath]

Lift can normally get up if I'm standing on monster's head, but later it becomes jammed and will not get down until monster is killed.

Yes, I know. Somehow I cannot save the spectre from getting slammed into the wall, but I talked to him and he said he is fine with it. 😁

No, seriously, this is the course of events -- at least up to the point where I fail to understand it:
As long as the sector moves, P_ChangeSector() is called during each tic which calls PIT_ChangeSector() for each mobj that is affected by the sector's movement. The first thing this function does is call P_ThingHeightClip(), which in turn calls P_CheckPosition().
P_CheckPosition() first sets tmthing to the mobj that tries to move and then sets tmfloorz and tmceilingz to the entered sector's floorheight and ceilingheight, respectively. Then it calls PIT_CheckThing() for all the other things within tmthing's reach.
PIT_CheckThing() is the function that contains our overunder code. If tmthing is the player and it is on top of another thing, it adjusts the tmfloorz value for the player to the other mobj's "head" and the thing->ceilingz value for the other mobj to the player's "feet".
Now comes the crucial part. Back in P_ThingHeightClip() the thing->floorz value is set to tmfloorz which is correct for the player, but the thing->ceilingz value is also set to tmceilingz which is again correct for the player (because it is the moving sector's ceilingz value). Somehow the ceilingz value for the monster is overridden here with a value that should rather apply to the player, I guess...

[JNechaevsky]

😦 I know that you are an expert in Black Magic of the Code, but speaking with demons... My-my. Good thing it was a Demon, not a Cyber Demon! Otherwise we was forced to extend some areas in some maps where lifts are placed. Through the code, not through the maps editing!

I have only a basic understanding of P_ and PIT_ code, so this puzzle looks almost unreadable for me. But anyways, preventing player from falling into the ground and stucking between sectors are far more than good enough for this, let's just say, uncanonical feature. Thanks!

[fabiangreffrath]

Indeed, I am tempted to close this bug with the fix I committed today. Let's see if I can iron out the last glitch in this implementation...

[fabiangreffrath]

This may be a solution:

--- a/src/doom/p_map.c
+++ b/src/doom/p_map.c
@@ -403,39 +403,32 @@ boolean PIT_CheckThing (mobj_t* thing)
                }

                // [crispy] allow players to walk over/under shootable objects
-               if (tmthing->player && thing->flags & MF_SHOOTABLE)
+               if ((tmthing->player && thing->flags & MF_SHOOTABLE) ||
+                   (thing->player && tmthing->flags & MF_SHOOTABLE))
                {
                        fixed_t newfloorz, newceilingz;
                        // [crispy] allow the usual 24 units step-up even across monsters' heads,
                        // only if the current height has not been reached by "low" jumping
-                       fixed_t step_up = tmthing->player->jumpTics > 7 ? 0 : 24*FRACUNIT;
+                       fixed_t step_up = (tmthing->player && tmthing->player->jumpTics > 7) ? 0 : 24*FRACUNIT;

                        if (tmthing->z + step_up >= thing->z + thing->height)
                        {
-                               // player walks over object
+                               // tmthing walks over thing
                                if ((newfloorz = thing->z + thing->height) > tmfloorz)
                                {
                                        tmfloorz = newfloorz;
                                }
-                               if ((newceilingz = tmthing->z) < thing->ceilingz)
-                               {
-                                       thing->ceilingz = newceilingz;
-                               }
-                               return true;
+                               return tmthing->player != NULL;
                        }
                        else
                        if (tmthing->z + tmthing->height <= thing->z)
                        {
-                               // player walks underneath object
+                               // tmthing walks underneath thing
                                if ((newceilingz = thing->z) < tmceilingz)
                                {
                                        tmceilingz = newceilingz;
                                }
-                               if ((newfloorz = tmthing->z + tmthing->height) > thing->floorz)
-                               {
-                                       thing->floorz = newfloorz;
-                               }
-                               return true;
+                               return tmthing->player != NULL;
                        }

                        // [crispy] check if things are stuck and allow them to move further apart

[fabiangreffrath]

Things I have learned so far: When on a moving platform, things have to re-evaluate their floor/ceiling values each tic. It is not sufficient to clip the player's floor/ceiling values against the monster it stands on, the monster will also need to clips its own floor/ceiling values against the player:

--- a/src/doom/p_map.c
+++ b/src/doom/p_map.c
@@ -403,12 +403,12 @@ boolean PIT_CheckThing (mobj_t* thing)
                }
 
                // [crispy] allow players to walk over/under shootable objects
-               if (tmthing->player && thing->flags & MF_SHOOTABLE)
+               if ((tmthing->player && thing->flags & MF_SHOOTABLE) || thing->player)
                {
                        fixed_t newfloorz, newceilingz;
                        // [crispy] allow the usual 24 units step-up even across monsters' heads,
                        // only if the current height has not been reached by "low" jumping
-                       fixed_t step_up = tmthing->player->jumpTics > 7 ? 0 : 24*FRACUNIT;
+                       fixed_t step_up = (tmthing->player && tmthing->player->jumpTics > 7) ? 0 : 24*FRACUNIT;
 
                        if (tmthing->z + step_up >= thing->z + thing->height)
                        {

By doing so, though, we allow the monster to run away underneath the player's feet, just as the player is able to walk across its head. The former will leave the player hanging in the air until his next attempts to move or the sector's next attempt to change its heights. So this is exchanging one bug with the other.

However, this brought me to another idea: How about removing the tmthing->player restriction entirely and allowing any monster to move over/under any other monster?

--- a/src/doom/p_map.c
+++ b/src/doom/p_map.c
@@ -403,12 +403,12 @@ boolean PIT_CheckThing (mobj_t* thing)
                }
 
                // [crispy] allow players to walk over/under shootable objects
-               if (tmthing->player && thing->flags & MF_SHOOTABLE)
+               if (thing->flags & MF_SHOOTABLE)
                {
                        fixed_t newfloorz, newceilingz;
                        // [crispy] allow the usual 24 units step-up even across monsters' heads,
                        // only if the current height has not been reached by "low" jumping
-                       fixed_t step_up = tmthing->player->jumpTics > 7 ? 0 : 24*FRACUNIT;
+                       fixed_t step_up = (tmthing->player && tmthing->player->jumpTics > 7) ? 0 : 24*FRACUNIT;
 
                        if (tmthing->z + step_up >= thing->z + thing->height)
                        {

This has become theoretically possible by the code I added in the last commit, the one that made sure that running over/under monsters will only raise the floor and lower the ceiling. I have a savegame here of MAP29 which leads to several Cacodemons piling up and Lost Soul flying through them if you lure them a bit.

My idea is to turn this from an On/Off choice into a three choice feature: Off, Player (current code), All (unleashed code presented above).

[fabiangreffrath]

By now, it appears that I have the choice between two evils on how to proceed in moving sectors:

Either, I let the player clip his floor/ceiling values against the monster and the monster against the player. This way I successfully circumvent that any of both clips into the walls, but the sector won't move at all anymore! This happens, because the monster (given that the player stands on its head) has already reached its ceiling by this way and so prevents the sector's floor from taking it any higher. Thus, both monster and player are stuck in the lift until one of both moves away (nb: there isn't always enough space around a lift for any of them to move, so this may be a game breaking situation).

The other solution would be to let only the player clip his floor/ceiling values against the monster, but ignore the monster's floor and ceiling values (i.e. they will get set, but then reset to the moving sector's floor/ceiling values in the code path I outlined here #381 (comment)). Then, the lift will move as expected, but chances are that the monster will get stuck in any of the adjacent walls (but, in contrast to the bug reported here, not the player -- anyway, this may lead to a game breaking situation as well).