Quiver Autoloading

[MattAmmann]

Autopickup and regular pickup will now autoload any equipped quivers. See below notes for more information. Fixes #7164.

Note - please add ammo to be picked up to the autopickup rules for your character. Otherwise, it'll just go into your inventory.

CODE TODO
[✔] change quivers container so that it holds maxCharges int as well
........[✔] remove unnecessary call to max_charges_from_flag() after that's sorted
[✔] change "choice" logic to loop through quivers container instead
[✔] iuse::quiver() small refactor
[✔] change multiple quiver autopickup to fill both quivers
[✔] prune commented out code
[✔] remove AutoPickup dependency
[✔] fix arrow pickup behavior when player volume is full

✔ - working
O - In Progress/Partially working
X - Not working yet

TESTING
[✔] Empty quiver
[✔] Full quiver
[✔] Partially-full quiver
[✔] Non arrow/bolt ammo (do nothing)
[✔] More arrows than quiver holds (fill quiver, rest in inv)
........[✔] Autopickup
........[✔] Regular pickup
[✔] Multiple non-full quivers (rest in inv)
........[✔] Autopickup
........[✔] Regular pickup
[✔] maxCharges still works properly in iuse::quiver
[✔] Arrow volume on ground exceeds player max volume
........[✔] Hands full
........[✔] Hands free

[MattAmmann]

Everything I planned to implement is now in the PR, so I've removed the WIP tag.

[kevingranade]

Sweet, code looks good except the line of commented out code you left in
the method to add arrows to quivers.

[MattAmmann]

Sorry about that! Those are now cleaned up.

[kevingranade]

Not sure what's up, didn't work when I tested it...
Also a minor issue, if you don't have room for the arrows it'll wield them instead. Unfortunately that looks a bit complicated to sort out.

[MattAmmann]

Not sure what's up, didn't work when I tested it...

Did you add the ammo to the autopickup rules for your character? It only works with items on the autopickup list, so that the player has control over what gets quivered and what doesn't.

Also a minor issue, if you don't have room for the arrows it'll wield them instead. Unfortunately that looks a bit complicated to sort out.

I see the problem. In game.cpp, the volume check happens before my code to check if the ammo is an arrow/bolt. I'll work on a fix to this.

[kevingranade]

I missed the autopickup dependency, and I don't agree with it. It means
this feature won't trigger except for people in the know, and it overloads
what autopickup means. If I didn't figure it out even looking at the code,
it seems a stretch for players to figure it out.
It's a no-brainer that if you already have arrows in a worn quiver and you
pick up matching arrows you'd want to put them in the quiver.
It's a really good heuristic that if you pick up arrows and you have an
empty quiver you want to put them in. I think it's a good enough heuristic
that we should do it by default, all you need to do is activate the quiver
and pull them back out to reverse the process.
The only refinement i'd ask for would be to prefer matching quivers instead
of empty ones.

[MattAmmann]

Ok, that reasoning makes sense. I'm not really sure why the autopickup dependency is there, but I'll work on getting rid of it. Iterating over partially-filled quivers before empty ones is definitely doable. I'll mark this as WIP again, while I make the changes.

[MattAmmann]

I changed the quivers container to a vector containing std::pair, so that I can sort by quiver charges. However, I'm running into a compile error that I can't seem to shake. I posted about it here, if someone wouldn't mind taking a look: http://smf.cataclysmdda.com/index.php?topic=6192.msg110090.

EDIT: Nevermind, I figured it out.

[MattAmmann]

Made all requested changes.

[kevingranade]

Damn pickup is such a mess. You missed the chunk of code that picks up just one item. That method needs some serious housecleaning (not saying you need to do it, just... damn)
However, at this point we're looking at like 4 places that are doing the same thing, so probably a good idea to extract the whole "handle quiver insertion" thing to a helper.

[MattAmmann]

Do you mean this conditional? if (here.size() <= min && min != -1)? That only evaluates to true if the AutoPickup option is false. I left it out of that conditional intentionally, so that the player wouldn't be forced to retrieve ammo (and lose turns) if in a critical situation. I tested this to be sure; a single arrow/bolt will be quivered whether AutoPickup is true or false. Do we still want it in that conditional?

[kevingranade]

If you pick up an arrow from a pile, it'll quiver it, if you pick up an arrow off the ground, you don't quiver it, that doesn't really make sense.
I see your point about move cost, but stepping back, I'd think a quiver should have a discount to moves compared to jamming it in a backpack or some other weirdness. (a quiver that holds them in a loose way anyway, not so sure about the modern ones where you have to socket them into place)

[MattAmmann]

I'll make a helper function and modify the code so that when that conditional does get called, the quiver handling will be there.

[MattAmmann]

Things are going smoothly, and I've tamed the game::pickup function quite a bit. How many moves do we want each action to take? From what I understand of the existing code, it detracts a flat 100 moves per map item, regardless of charges. This is pretty unrealistic.

In this PR, I'm subtracting 10 moves/arrow for adding to a quiver, and 100 moves flat (as it was before), for adding to the inventory. Should I keep these values?

[kevingranade]

I wouldn't want to mess with the moves/action in general here, it's a
separate issue, for arrows, how about 10/arrow, but capped at 100 so it's
no worse with a quiver than without?

[MattAmmann]

I've made those changes, and game::pickup is much more palatable now. Still needs a bunch of testing, but I'm pretty optimistic about it's stability.

[kevingranade]

Thanks for the feature, sorry it was such a protracted process.