fix grabbing dig you down

[GuardianDll]

Summary

None

Purpose of change

Fix #79854
Generally grabbing calculate gravity check once, in the end, but not when grabbing moves you across a tiny map, in this case grabbing calls map update, which itself calls a gravity check. For magical reason calling two gravity check dig you down one level below

Describe the solution

Remove manual map update - undocumented and unexplained, i don't really know why it was here in the first place

Describe alternatives you've considered

Make grabbing more 3d aware - currently it just moves you in two dimensions and then just rely on gravity check for you to fall down
Find why calling gravity check twice do not update the character position properly - good approach, but try_fall() code is terrible, barely explained, and i have difficulties following it

Testing

Used my test where i had a setup that digged me under the ground quite often, saw zero cases of me digging down, also saw no another oddities, which made me think it's safe to backport it

Additional context

[PatrikLundell]

The purpose of the update_map() stuff is to keep the PC at the center submap of the reality bubble at all times. Any movement of the PC that takes the PC out of that submap should result in the reality bubble being shifted to center around the PC's reality bubble submap.

I don't know what the consequences of removing the code are. Maybe the next movement of the PC causes the shift to happen then (while the reality bubble remaining lopsided until that happens), in which case the damages are limited as long as the distance moved is short. However, this cannot be accepted without an analysis of what the consequences would be.

If update_map() calls gravity_check() somewhere it would cause the target to fall on the in-air pull that crossed a submap boundary, only to have the position being returned to the air (but, presumably, still taking damage from the fall). I fail to see where this causes the PC to end up underground (which shows there's something lacking in my reading of the situation, not that the bug cannot occur).

The crux here would be tracking of the PC's Z level and the call chain of it dropping below ground in particular.

[GuardianDll]

I actually tried to debug the try_fall() function that respond for character falling, but what i debugged made me super confused

Cataclysm-DDA/src/map.cpp

Lines 10079 to 10092 in 3d07400

	int height = 0;
	tripoint_bub_ms where( p );
	tripoint_bub_ms below( where + tripoint_rel_ms::below );
	creature_tracker &creatures = get_creature_tracker();
	while( valid_move( where, below, false, true ) ) {
	where.z()--;
	if( get_creature_tracker().creature_at( where ) != nullptr ) {
	where.z()++;
	break;
	}
	below.z()--;
	height++;
	}

DEBUG : where: (60,63,0), below: (60,63,1)

[PatrikLundell]

That's definitely weird.

That code could be changed to test for a creature at "below" rather than "where" and decrement both only when past that point rather than decrement and then increment "where" on a bailout, but assuming the compiler works correctly, it shouldn't change the result, as far as I can see.

Having try_fall take the position of the creature as well as the creature is also weird, but the call we're interested in seemed to feed in pos_bub() of the critter, which, again, wouldn't matter.

"below" isn't updated in lockstep with "where" after its last use, but that should at most cause the two to become equal for the case where the PC falls on a critter.

The PC is eventually moved by g->vertical_move() which is controlled by "height". That code takes the PC's current position rather than a parallel position position, but as far as I can see all calls to try_fall() just gets the current position of the critter, and so should work out.

It can be noted vertical_move() is called with "force" being true, which is probably what allows the PC to be placed underground, but the "height" value shouldn't have made that possible,

OK. I think I've found where things go wrong in this crowd surfing situation. In vertical_move(), on the second call (when crowd surfing) there's a weird calculation of z_after, which returns -1 rather than 0, and that's later used to set the Z level. There's even a TODO comment, but no reason for why it hasn't been done rather than just writing a comment.
OK, testing that change causes the PC to end up in the same place as the zombie when the zombie ought to have been move to the same time the PC occupied (i.e. on top of the PC), I suspect the zombie fall code doesn't allow the zombie to crowd surf.
Yes, hacking the creature below to end up to the side (+ {30, 30, 0}, just to get it out of the way) allowed the PC to become visible (and die, as it was attacked by everyone around, but debug invulnerability showed the result without getting a tomb stone.

So, the problem seems to be that vertical_move() contains a hack to avoid the PC being placed under a zombie (with a cryptic comment about the correct code, but no mentioning of why the wrong code is there), but correcting that code causes the the PC to end up under the zombie and become invisible (the zombie is critter displayed). I still think that's less bad than ending up underground, though. It can also be noted that the PC isn't invulnerable in that location: if attacking (and presumably doing anything advancing time) the PC is killed by the surrounding zombies.

Edit: I debugged a roof on the other side of the horde (but on the same submap), teleported there, and then waited. The result was that the PC was dragged down and ended up invisible under a zombie, but without any call to vertical_move(). Given that this happens in the regular case, I don't see any reason for not fixing the weird miscalculation of z_after so the behavior is consistent. Of course, there might be some other reason for the weird miscalculation...

[rtxyd]

I still think it's because of #78254, there's no this kind of issue before it.

Edit: If its because of gravity check, why not to just try deleting the added gravity check here by #78254
https://github.com/Procyonae/Cataclysm-DDA/blob/10373910aaa983776c003725297211e0f4e3eb56/src/mattack_actors.cpp#L570
this way the code doesn't need to be removed, and trigger 1 less gravity check

[PatrikLundell]

@rtxyd: Well, removing that call causes the PC to crowd surf in the bug report save, but what happens when the grabber stands in a window of an above ground level floor of a building? I would expect that would cause the PC to levitate outside that window, which would shift the problem there instead.

The root cause of the trouble is that there is currently no good way of handling the case of the PC ending up on top of a sea of zombies. The corrected vertical_move() code causes the PC to end up in the same tile as the zombie. There is code in try_fall() that moves the zombie to the PC's place above it, but apparently the zombie falls down on top of the PC when it gets processed.

Changing try_fall() to give up when the critter fallen upon cannot be pushed to the side and leave the PC crowd surfing in that case, e.g. by replacing

            critter->setpos( c->pos_abs() );
            add_msg( m_bad, _( "You fall down under %s!" ), critter->disp_name() );

with

            add_msg( m_bad, _( "Your fall is broken by %s, and you're blocked from reaching the ground!" ), critter->disp_name() );
            return false;

would leave the PC crowd surfing while not messing up the other cases where a gravity check is warranted.

[GuardianDll]

So what's your opinion @PatrikLundell? is it okay to merge it in this state, or there should be some different approach? because again, i tested it manually, and i didn't saw something that would make me instantly think it's a bad idea

[PatrikLundell]

Unfortunately, I still think it's a bad choice. It removes functionality because of a side effect in it, and it's unclear what the effects of removing the functionality are. I would expect obscure bugs that are hard to track down, because the reality bubble generated by a loaded save isn't the same as it was when the save was made.

I still think there are two main alternative soultions:

1. Change the line with the weird comment to do what the comment says it should be changed to. I don't have any idea of why the comment was made rather than actually changing the code, though, but there presumably was to prevent something unwanted from happening. This alternative causes the PC to end up under the pulling zombie, invisible to the player (only the zombie is displayed). However, that's the current behavior when the PC is pulled into a crowd from within the same submap, so that part is consistent. This achieves the same result as the removal of the map shifting code, but retains that functionality. Again, we have the uncertainty of why the code wasn't corrected but an incomplete comment was left instead. Still I think that's better to try to fix the side effect rather than removing functionality due to it.
2. Change the code for handling the case where the PC is trying to fall when on top of a zombie that can't be pushed to any adjacent tile because of the lack of space to leave the PC on top of the zombie rather than having the PC and zombie switch places and then have the zombie drop onto the same tile as the PC. This results in the PC crowd surfing in this situation, which is the same as happens to a zombie on top of a crowd (that isn't the PC, although I don't know if the behavior is different when the crowd consists of Characters rather than monsters). I think this is the "safe" option.

There is also the expansive option of trying to deal with crowd surfing for monsters on monsters and PC on monsters of option 2. How? I don't know. Maybe find a way to allow multiple critters on the same tile, but that is probably quite a task, including the need to show it graphically.

[rtxyd]

How about my previous suggestion? Though your concerning is true, but when the grabber is on the edge and pulling you its certain that its the only monster on the edge, and as the physical force keeps inflicting by it during the grabbing, I think its reasonable to levitate at the mid-turn of the whole grabbing procedure (if it finally grabs you up to the next floor).

[PatrikLundell]

@rtxyd: I think changes to #78254 are better discussed with the author of that PR. However, as a minimum I'd expect you to test the current implementation against your proposed change in a variety of situations to verify the results are reasonable in all of them.

Note that the call is done after the dragger has pulled its victim to the final destination for that turn, and I believe that destination is adjacent to the dragger, not to the dragger's tile, which would mean hovering in the air when the dragger is at the edge of a ledge (ending up above the dragger presumably only happens when there's no free tile adjacent to the dragger on the same Z level).

Now, you could argue that the dragger presumably is still holding on to the victim, thus preventing it from falling, but you'd end up with a number of oddities that would need to be checked if that's the case.

• Will the victim fall of the grabber's grip is escaped (my guess is that there's no gravity check on a release, but only on a character move)?
• Can the victim move while grabbed, to where, and should it be able to? And what happens when the victim moves there (assuming it could move)? If gripping now prevents falling when dragged, it should keep preventing it on movement to an adjacent unsupported tile, and releasing the grip should result in a fall unless the victim has managed to move to a supported tile.

[kevingranade]

I don't have the total context, but Patrik is right that update_map MUST be called ASAP if the player is moved out of the central submap of the map. The player always being in that central submap is a bedrock assumption made throughout the code and there's no telling what will break if that assumption is violated.