Player snaps back if teleport fails

[RenechCDDA]

Summary

Bugfixes "Additional code safety if player's long-range teleport fails"

Purpose of change

• Closes Ran out of available textures in the pool when long range teleporting #54088

Describe the solution

Check the teleport succeeded by comparing the player's actual coordinates before to after. If they are the same, the teleport has failed somehow. Run map update again to shift all the coordinates back. Re-run the function to make sure we safely unload and then reload the map.

Describe alternatives you've considered

Testing

Video depicts an attempted teleport to a modified art_gallery which is packed with every square containing a monster.

2024-05-19.08-04-28.mp4

Additional context

[PatrikLundell]

• There's no need for z_level_shifted, as you bail out if tele_from != tele_to, so it's always going to be false.
• The reason you fail to restore the proper position is because you perform a recursive call to place_player_overmap, and it has an OMT resolution, not a submap one (and it then places you in the middle of that OMT anyway). You can get the correct coordinate from tripoint_abs_ms abs_ms_tele_from = u.get_location();, and you can perform your recursion by introducing a place_player() operation that takes the tripoint_abs_ms coordinate you want to go to, factor out most of the code from place_player_overmap() and place it in the new operation and have place_player_overmap call place_player with the map_sm_pos coordinates (although the variable name is crap, as the resolution is mapsquare, not submap).

If this explanation is too messy to be understood, let me know and I'll make a new attempt.

[RenechCDDA]

There's no need for z_level_shifted, as you bail out if tele_from != tele_to, so it's always going to be false.

Fair.

• The reason you fail to restore the proper position is because you perform a recursive call to place_player_overmap, and it has an OMT resolution, not a submap one (and it then places you in the middle of that OMT anyway). You can get the correct coordinate from tripoint_abs_ms abs_ms_tele_from = u.get_location();, and you can perform your recursion by introducing a place_player() operation that takes the tripoint_abs_ms coordinate you want to go to, factor out most of the code from place_player_overmap() and place it in the new operation and have place_player_overmap call place_player with the map_sm_pos coordinates (although the variable name is crap, as the resolution is mapsquare, not submap).

Do you want to PR this onto my branch? I'm not super-enthused about working with our coordinates systems and that way you can get full authorship for the commit.

[PatrikLundell]

Sorry, I don't know how to hook up to others' PRs, and I don't care about credits (beyond people pounding their chests and bragging about things they've not actually done).

Thus, here's the code:

void game::place_player_overmap( const tripoint_abs_ms &ms_dest, bool move_player )
{
    if( ms_dest == project_to<coords::ms>( u.global_sm_location() - point( HALF_MAPSIZE,
                                           HALF_MAPSIZE ) ) + u.pos() ) {
        return; // Already there
    }

    // if player is teleporting around, they don't bring their horse with them
    if( u.is_mounted() ) {
        u.remove_effect( effect_riding );
        u.mounted_creature->remove_effect( effect_ridden );
        u.mounted_creature = nullptr;
    }
    // offload the active npcs.
    unload_npcs();
    for( monster &critter : all_monsters() ) {
        despawn_monster( critter );
    }
    if( u.in_vehicle ) {
        m.unboard_vehicle( u.pos() );
    }
    for( int z = -OVERMAP_DEPTH; z <= OVERMAP_HEIGHT; z++ ) {
        m.clear_vehicle_list( z );
    }
    m.rebuild_vehicle_level_caches();
    m.access_cache( m.get_abs_sub().z() ).map_memory_cache_dec.reset();
    m.access_cache( m.get_abs_sub().z() ).map_memory_cache_ter.reset();
    // Set this now, if game::place_player fails we'll need it to recover.
    const tripoint_abs_sm tele_from = u.global_sm_location();
    // The subtraction is to get the reality bubble NW corner from the center position.
    const tripoint_abs_sm map_sm_pos =
        project_to<coords::sm>( ms_dest ) - point( HALF_MAPSIZE, HALF_MAPSIZE );
    const tripoint_bub_ms player_pos( u.pos_bub().xy(), map_sm_pos.z() );
    load_map( map_sm_pos );
    load_npcs();
    m.spawn_monsters( true ); // Static monsters
    update_overmap_seen();
    // update weather now as it could be different on the new location
    weather.nextweather = calendar::turn;
    if( move_player ) {
        place_player( player_pos.raw() );
    }
    const tripoint_abs_sm tele_to = u.global_sm_location();
    if( tele_from != tele_to || !move_player ) {
        return;
    } // else tele_from == tele_to !!!
    // We've failed to teleport for some reason (probably monsters occupying destination squares).
    // Let's try to recover gracefully. But also throw a warning, this is bad!
    tripoint_abs_ms org = project_to<coords::ms>( tele_from - point( HALF_MAPSIZE,
                          HALF_MAPSIZE ) ) + player_pos.raw();
    debugmsg( "Failed to place player at destination. If you see this outside of debug teleporting it is a bug." );
    update_map( u, ms_dest.z() != tele_from.z() );
    // This recursive call safely calls map::load_map() again after making sure everything has been unloaded properly.
    // Basically, its only purpose it to reset the z-level to the z-level you teleported *from*. Otherwise, it's redundant after update_map
    // Again, translate the reality bubble reference to a mapsquare one.
    place_player_overmap( project_to<coords::ms>( tele_from - point( HALF_MAPSIZE,
                          HALF_MAPSIZE ) ) + player_pos.raw() );
}

void game::place_player_overmap( const tripoint_abs_omt &om_dest, bool move_player )
{
    // Project the bubble reference to a submap reference.
    tripoint offset = u.pos() - point( 5 * SEEX, 5 * SEEY );

    // And then on to an overmap one.
    if( abs( u.global_sm_location().x() ) % 2 == 1 ) {
        offset.x += SEEX;
    }
    if( abs( u.global_sm_location().y() ) % 2 == 1 ) {
        offset.y += SEEY;
    }

    place_player_overmap( project_to<coords::ms>( om_dest ) + offset );
}

plus:
void place_player_overmap( const tripoint_abs_ms &ms_dest, bool move_player = true );
for game.h.

I've done some basic testing, moving to different submaps within the starting overmap tile and verifying I ended up in the corresponding location in the target overmap tile (that didn't work previously). I also spawned a breather and tried to teleport into it. That caused an infinite loop where, for whatever reason, the code tried to teleport the PC back to the tile it was on already. I blocked that with the check for the destination being the same as the source location at the top (which makes sense anyway).

Can't say it's great fun trying to juggle the various coordinate systems and manually adjust between their projections.

Edit: Come to think about it, I don't think the recursion is needed. The PC is already at the start location when the teleporting failed, and my sanity check causes the recursion call to bail out immediately without actually doing anything, and my character remained at the start location (I didn't check what happens when I try to move, etc. afterwards).

[PatrikLundell]

A ping comment, in case this was put in the "I'm busy right now, I'll deal with it later" pile.

[RenechCDDA]

A ping comment, in case this was put in the "I'm busy right now, I'll deal with it later" pile.

The pile gets cleaned up, eventually!

[RenechCDDA]

I ran this last push locally and it seemed to be all that was needed. Here's to hoping it works on CI as well.

[mqrause]

The issue comes from this change. game::vertical_shift sets abs_sub on the map object. And apparently Creature::setpos does not, so the map still thinks it's at z-level -1 when the avatar moves in subsequent tests, which sets the avatar z-level to -1 when game::update_map is called.

[kevingranade]

avatar_action::move() wants an offset, not a coordinate, so you should provide just tripoint_west as the third argument.
No idea why it's ending up at z= -2 though?

[kevingranade]

Again, this wants an offset like tripoint_east, not a coordinate like left.