Cataclysm-dda: Can't have friendly animals in vehicles

[railmonkey]

So far, as of build 2779, I've only observed this when I tried to take my cat with me. It stayed within the confines of my car, but once I started moving, it flew right out, getting nabbed by the frame on the way. Fortunately I didn't run the poor thing over, but it still strikes me as odd that there seems to be no provision in the code for passengers. I haven't the faintest idea how feasible it is, but I'm sure I'm not the only one that's been wanting to take our faithful companions with us by anything other than foot.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[KA101]

Yeah, no critters in the car is a known issue, thanks.

[kevingranade]

There's code for NPCs, the last time this was looked at there were no pets.
Ideally we'd hoist at least some of that code to the creature class.

[railmonkey]

Ah, I see. Glad it's known, then. Possibly work the seatbelts into it?

[Oskar636]

Don't we have the crew assignment option now?

[mugling]

Yes

[mugling]

...but this doesn't cover animals

[kevingranade]

This is arguably a bug and if not it's an important feature request.

[mugling]

This is arguably a bug and if not it's an important feature request.

How would the ideal implementation work?

[Coolthulhu]

We "only" need creature IDs to reuse the code NPCs use.

[mugling]

Wouldn't the path finding AI code need updating?

[kevingranade]

At a high level, if the player enters a vehicle a 'friendly following'
animal should pick a valid seat in the vehicle and get in it. how it does
this has options for sophistication, it can teleport there, path there and
teleport if it fails, or complain and give up if it fails.

Implementation wise the animal ai has to handle the above scenario and
suppress the urge to jump out of the vehicle, and the vehicle code should
politely refrain from crushing the animal to a bloody paste.

[l29ah]

It's more than that: you can actually avoid being eaten by a horde of zombies if you succeed to move your car for a tile or two, converting all those zombie brutes having fun with your poor body inside your car into a paste.

[mlangsdorf]

semi-simple proposed work-around:
install livestock into vehicles in a modified cargo space.
ideally, this would work similar to adding a weapon to a turret mount.
I hoped to be able to do this json, but it's going to need a new flag and some additional code.
Basic concept: install a livestock rack (which is a cargo rack + some pipes to act as railings) in your vehicle as a livestock stall. The livestock stall is now a pet carrier for very large animals. Activate the livestock stall as 5-10 minute long action to put a friendly livestock in the livestock stall. Drive off.
I'm hoping I can recycle the existing pet carrier code in a vehicle part; I'll take a look to see.

[Night-Pryanik]

Should use some sort of ramp (or perhaps boom cranes) for cows and other heavy animals to enter the stall.

[mlangsdorf]

A boom crane is overkill. Even a ramp is a hassle that I don't necessarily want to code.

Vehicle parts can't take iuse actions directly =( I'll have to define a new flag, hook it into the vehicles code, and then hopefully call the iuse function. I'd hate to have to duplicate the code.

[Coolthulhu]

Every vehicle part has an item attached to it (the item used to construct it). You can tell an item is a part of a vehicle because it has a tag "VEHICLE". You can check it with item::has_flag (tags "count as" flags).

You could modify the capture critter iuse to have an extra field (if it's an actor) or hack it with a flag on item (if it's not an actor iuse) that quits the iuse and prints a failure msg if the used item doesn't have the "VEHICLE" flag/tag.
Then you'd only need to add vehicle interaction code that activates that iuse. This could be as simple as adding a "USE_PART_ITEM" flag to the new part, then checking it when examining vehicle part, and simply calling the first iuse on the item. This would be very generic and could be useful for other parts too.

I'm not 100% sure if this would work, but it could be a very simple way to achieve the effect. Could be as little as ~50 changed lines and no horrible copypasta hacks.

[mlangsdorf]

Okay, so I think I've almost there. I've defined some flags and some new items/vehicleparts and vehicle_parts. I've added a new iuse function that wraps capture_monster_act with the flag check for vehicles (and may do some stuff on a successful use, not 100% sure yet).

in src/pickup.cpp, in interact_with_vehicle, I've added a generic action boolean and enum and code to add it to the selectmenu. I think, based on the get_turret logic, that I've even figured out how to get the vehicle_part that has the USE_PART_ITEM flag on it and confirm that it's not broken or removed.
What I'm not sure about is how to I invoke the iuse at that point - do I just do part->base->use( p, base, true, pos ) or what?

[Coolthulhu]

What I'm not sure about is how to I invoke the iuse at that point - do I just do part->base->use( p, base, true, pos ) or what?

Something like this should be enough.
You can use itype::invoke( player &p, item &it, const tripoint &pos ). You can get an itype from item::type field.
So in the end, it should look like

item &base = part.get_base();
base.type->invoke( p, base, pos );

But you also need to guarantee that invoke is never called if itype::can_use() returns false. You can check it every time, but it would be much better if you checked it once, on load (in Item_factory::check_definitions()) that any item with your newly defined flag:

• Has at least one use function
• (Optional) Has a vehicle part that uses it (otherwise it will be useless)

If vehicle part base is hard to access (because it's private or something), it would be better if you added a vehicle_part::invoke instead of making base accessible. Vehicle part base items have a bunch of extra check that could be accidentally omitted if the field was public.

[mlangsdorf]

I successfully captured my first cow! Which then disappeared into the ether and I couldn't retrieve it. But progress is progress.

[mlangsdorf]

okay, I added a new get_base() variant that didn't return const item&, because passing a const item& was causing me problems.

item& vehicle_part::get_base()
{
    return base;
}

item& base = vpart.get_base()
base.type->invoke( g->u, base, pos )
// inside invoke, base becomes it, and it->set_var( "contained_name", "test" );
if( ! base->has_var( "contained_name " ) {
    g->u->add_msg_if_player( "well, that didn't work " );
}

always prints the message. base is an item reference, I should be able to change it and have those changes persist. But that's clearly not happening. What am I missing here?

[Coolthulhu]

item& vehicle_part::get_base()

That was the exact thing I told you to avoid.
Return a const pointer, but make a copy on the call site. Then, when done, set_base to the copy.
set_base should ensure that "VEHICLE" item tag is set.
Performance cost of copying an item on explicit use is unnoticeable.

if( ! base->has_var( "contained_name " ) {

If this is ctrl+c+v from your actual code and not hand-written, it looks like you added an extra space to the string.

[mlangsdorf]

it was hand-written code.

So my C++ is super rusty and 20 years out of date, and writing a copy function is not obvious and the tutorials are not sorting it out for me. so I've got these two bits:

interact_results interact_with_vehicle( vehicle *veh, const tripoint &pos,
                                        int veh_root_part )
{
        case USE_GENERIC: {
            auto res = veh->get_parts( pos, "USE_PART_ITEM" );
            if( !res.empty() ) {
               auto vpart = *res.front();
               if( ! vpart.removed && ! vpart.is_broken() ) {
                   item base = item( vpart.get_base() );
                   base.type->invoke( g->u, base, pos );
                   vpart.set_base( base );
               }
            }
            return DONE;
        }
}

const item& vehicle_part::get_base() const
{
    if( base.has_var( "contained_name" ) ) {
        g->u.add_msg_if_player( "found the stored part correctly" );
    } else {
        g->u.add_msg_if_player( "in vehicle part, didn't find the stored base" );
    }
    return base;
}

void vehicle_part::set_base( const item& new_base )
{
    this->base = item( new_base );
    if( base.has_var( "contained_name" ) ) {
        g->u.add_msg_if_player( "stored the part correctly" );
    } else {
        g->u.add_msg_if_player( "in vehicle part, didn't store the base" );
    }
}

The first time I go through case USE_GENERIC, I get "in vehicle part, didn't find the stored base" followed by "stored the part correctly" message, so I assumed that means that new_base got assumed to vpart->base.
The second time I go through case USE_GENERIC, I get "in vehicle part, didn't find the stored base" and there's no cow in the livestock stall to release. So even though I thought I had copied new_base to base, apparently I didn't.

[Coolthulhu]

I found it:
You used auto without specifying that you want a reference. By default, auto strips references, so the vehicle part was copied, the copy got the new base set, then was dropped because it went out of scope. The original vehicle part was left unchanged.

In C++, you only need to assign to copy. In DDA code, items are generally copied by assignment, not by explicit constructor invocation.
I'm not sure if the compiler will figure out it doesn't need to create the item to assign to it, but this function will be called at most once per keypress, so readability > performance.

[mlangsdorf]

That's what I get for using C++ features I don't full understand.
Okay, I'll simplify, make some stuff explicit, and give it another go. Thanks for the review.

[mlangsdorf]

Stashing this logic here:

        case USE_GENERIC: {
            auto res = veh->get_parts( pos, "USE_PART_ITEM" );
            if( !res.empty() ) {
               vehicle_part* vpart = res.front();
               if( ! vpart->removed && ! vpart->is_broken() ) {
                   item base = item( vpart->get_base() );
                   base.type->invoke( g->u, base, pos );
                   vpart->set_base( base );
               }
            }
            return DONE;
        }

because it works, and it's part of an eventual refactoring of interact_with_vehicle(). I'd like to add vpuse.cpp, which would handle use actions for vehicle_parts the same way iuse.cpp handles use actions for items. Then a bunch of special cases and weird conditionals would drop out of interact_with_vehicle, which would simplified to getting the vector of vparts on the tile at pos, building the menu by querying each vpart for its menu options, and then invoking the vpuse item for the selected option.

But absent that framework, USE_PART_ITEM and has_generic_option don't work well for the livestock carrier, because (by design) there's no way to update the vpart with the results of iuse action.

[mlangsdorf]

okay, taking another look at this, because some people want to have their dogs and cats ride around in their vehicles with dealing with onboard pet kennels.

git grep get_passenger src/
src/map.cpp:499: player *passenger = veh.get_passenger( boarded );
src/map.cpp:866: player *psg = veh.get_passenger( ps );
src/map.cpp:1182: player *psg = veh->get_passenger( seat_part );
src/map.cpp:1214: passenger = veh->get_passenger(seat_part);
src/map.cpp:1288: psgs.push_back( veh->get_passenger( prt ) );
src/npcmove.cpp:649: const player *passenger = veh->get_passenger( p2 );
src/vehicle.cpp:2681: if( get_passenger( psg_part ) == &( g->u ) ) {
src/vehicle.cpp:3020:player *vehicle::get_passenger(int p) const
src/vehicle.cpp:6691: const player *p = get_passenger( i );
src/vehicle.h:757: player *get_passenger (int p) const;

okay, so to a first approximation, the reason that pets can't ride around in vehicles is that C:DDA doesn't distinguish between a passenger (mostly sentient being that can take the controls and be assigned a seat) and a rider (a creature along for the ride). And it looks like to me that distinguishing between the two should be pretty easy.

[Coolthulhu]

And it looks like to me that distinguishing between the two should be pretty easy.

There is one big problem with this:
NPCs have IDs, monsters do not.
This means that you can't remember which critter is what, you have to be able to recalculate this every time.

So if you want to allow monsters to ride in vehicles, you will have to rewrite vehicle-critter collision system like this:

• Note all the riders - critters on the vehicle that will move with it. Since you have no ID, you will have to use their pointers, position in monster array, or some other such "internal" ID. Note that those IDs will become invalid every single turn, so you must not rely on saving them anywhere.
• Check and process the collisions while disregarding all collisions with riders.
• When moving the vehicle, recalculate the riders using the same function as you used for collision rider finding. You may reuse previously calculated riders but only if you can pass them between functions (DON'T save them in any class that lasts more than a turn).
• Knowing the new riders, move all the riders. This isn't as simple as it sounds because you must move them in correct order: a critter must not be placed on another critter, it can only be placed on an empty tile.

[mlangsdorf]

Yeah, I got to the mess of compile errors from the lack of monster ID and realized this was not going to be as easy as I hoped.
I have some idea for an algorithm to handle moving the critters, by tracking the frame they're on and then moving them back to that frame after the vehicle moves and doing it from the vehicle corner closest to the direction of movement towards the vehicle corner farthest away, but as I think about it, it's a mess of corner cases and tricky math.
Hacky alternate work-around: promote critters to players and give them IDs when they board a vehicle, use the existing code. When they unboard a vehicle, demote them back to critters.

[Coolthulhu]

Hacky alternate work-around: promote critters to players and give them IDs when they board a vehicle, use the existing code. When they unboard a vehicle, demote them back to critters.

This won't work since they are totally separate classes.

You could start with the simplest possible option. It most likely won't be slow enough to matter and it can be optimized easily if it will:

• Keep a list of pairs: critter and its intended destination
• Loop all over pairs:
• 
  
  • If the destination is free, move the critter to its destination and delete this pair
  
  
• 
  
  • Otherwise go to next pair
  
  
• If you perform a whole loop without moving any critter, that's a bug and needs a debugmsg. It shouldn't happen in normal game, but if it somehow does, it shouldn't loop infinitely and just cancel moving the rest of the critters after debugmsg-ing.
• If the list isn't empty, loop over it until it is (or a bugged loop happens)

It's only quadratic in the worst case, so the player would need 10+ critters on the vehicle to cause trouble.

[mlangsdorf]

So all the critters are displacing roughly the same amount in roughly the same direction, so I think I would try:

for each critter:
  determine destination
  try to displace to destination
  if destination already has a critter in it
    recurse/restart with the new critter

worse case is that you start with a critter at the movement-relative rear of the vehicle, and there's a line of critters in front of him. dunno, I'll have to play with it.

the fact that there's no state makes it somewhat complicated, but it also means that a lot of the get_passenger / board_vehicle state keeping stuff can be avoided.

[kevingranade]

Simpler proposal.

If a monster moves onto a vehicle, set a "boarded vehicle" flag in the
monster.
Un-set it on movement that moves the monster off the vehicle.
Adjust vehicle collision code to ignore monsters with this flag.
Adjust vehicle movement code to shift monsters with this flag along with
the vehicle.

[mlangsdorf]

Sure, I can try that.

[Amariithynar]

Only if you fix creatures (player included) from walking diagonally through solid walls because the vehicle is staggered in rows when moving diagonally.

[Coolthulhu]

Adjust vehicle movement code to shift monsters with this flag along with the vehicle.

Won't help with movement order.
That would require very ugly hacks in vehicle tracker.

[mlangsdorf]

@Amariithynar : please, one hugely difficult problem at a time. This is the issue about letting zombies, pets, and random critters get onto, and move with, a vehicle.

21701 is for discussion about how to implement walls correctly.

[Amariithynar]

@mlangsdorf It's a serious issue, since when a diagonal vehicle is in motion, unsecured creatures will still walk right out of the vehicle and get smashed against the vehicle's wall. Even if it's some kludge like "if two vehicle tiles that prevent movement through them are diagonally adjacent, you are blocked from moving diagonally through them".

[kevingranade]

Won't help with movement order.

That can either be handled by your suggestion of retry until no collisions, or by sorting monsters in a vehicle to move based on direction of vehicle travel, or by simply updating monster position without collision detection since it's already covered by vehicle collision.

Re: gaps in rotated vehicles, I agree that's out of scope for this issue.

[mlangsdorf]

@Amariithynar I think it'll be much easier and simpler to just prevent creature with the on_vehicle flag from pathing off the vehicle while the vehicle is in motion. Hopefully. There is a difficulty that critters (probably) can't know what vehicles they are on, and won't be able to tell if it is in motion, but there is rarely going to be more than 1 vehicle in motion at a time so meh.

@kevingranade is it acceptable if the boarded_vehicle flag is a pointer to the vehicle the creature is on? that would simplify some things, I expect. it would get set to nullptr when the creature gets off the vehicle.

[kevingranade]

If it's a pointer you're going to have to serialize it.
Why not just lookup the vehicle based on the monster location as needed?

[mlangsdorf]

I know creatures in general don't have persistent IDs, and this is probably a good thing given the number of creatures in the game. But the number of PC friendly creatures is probably a more reasonable number, not much more than the number of NPCs, and NPCs have persistent IDs. Would it be acceptable to create a friendly_creature class that inherits creature, replaces a creature when it is made friendly, and has a unique and persistent ID?

[Coolthulhu]

Would it be acceptable to create a friendly_creature class that inherits creature

It would be a massive hack that would be hard to maintain and extend. Either all creatures get IDs or none do, anything else is a problem, because it results in special casing of everything, which is bad practice.

Here are the sane options:

• Before collision check, build a set of positions of creatures that are on the vehicle. Then during collision check, ignore those. Then, either pass or rebuild this set before moving the vehicle, so you know what to move.
• Before collision check, build a set of critter positions in creature list. During collision check, ignore those critters. But you have to remember to rebuild it on every collision with a creature, since the positions change every time a creature is added or removed (say, death or slime split).
• Before collision check, mark all critters to move with some flag. Then ignore those during actual collision check. Finally go through ALL critters and move and unflag those that have the flag. Note: This is likely to result in bugs if you don't make 100% sure that all collision checks end with un-flagging of critters.
• Add IDs to ALL critters, deal with all the problems that ensue.

Remember that all vehicles move sequentially. You don't need to remember all motions, only "critter x is on the vehicle that is moving right now". It should be easy to pass the position list around.

[kevingranade]

Regardless of unique IDs, a new class for one purpose is not ok. We avoid
subclassing like the plague it is.

It should be sufficient to add a creature tag like "boarded_vehicle" when a
creature steps onto a vehicle, and handle removing it each time the
creature moves and when a vehicle frame is destroyed. This flag would
exclude the creature from vehicle collisions entirely, because once
boarded, collision is handled by the vehicle/vehicle collision code.

[mlangsdorf]

I had other potential uses for friendly_creature, but it was not a good idea and will be withdrawn.