[WIP] [CR] Item Bags

[DavidKeaton]

See #10926
This is still WIP, even though it's basic. I still need some minor kinks sorted out before this can be merge-able, and I would massively prefer to be able to merge in changes over time in successive PRs, so that way all the additions aren't in a massive commit and a total bitch to merge.

Things that need fixed

• Items that contain liquids [need to test with containers as a whole] delete contents, thus destroying liquid.
• Need to synchronize additions to JSON and saving/loading.
• Currently only backpack can do this.
• Fix up the basic UI to not be as jank, or how I learned to love the uimenu_cb.
• Not selecting an object still selects object [should be fixed by above].

[DavidKeaton]

I know that this is the cause of removing an item even if you don't select it [via escape to quit], but I put this here to prevent OOB errors with vector for the time being.

[DavidKeaton]

I want to perhaps make unopened bags a color and just make 'known' bags regular.

[DavidKeaton]

Prob just migrate this logic into get_remaining_capacity.

[BevapDin]

You do not want to change the id of an existing item type, ever. This member is only written when new items types are created during loading of the world data.

Just return the default created item and later check the returned item with item::is_null().

[BevapDin]

How about using game::inv() or game::inv_for_filter instead of this whole block.

[BevapDin]

Why don't you print color tags for ltgray? This seems unnecessarily and potentially wrong (you don't know which default color is used to display the text).

[BevapDin]

This looks like a simple cache of the total contained volume. It can be recalculated when needed for the few items that require it instead of storing it for every item in existence.

[BevapDin]

I have the feeling it should be <=

[BevapDin]

Some style issue: I know the temptation is big, but function definitions inside the class definitions are automatically inline, so there is no need to add the inline keyword there, it's redundant.

An index in a vector is of type size_t (or pedantically std::vector<T>::size_type), not unsigned int. Those might be the same on many, but not necessarily on all platforms. And saying size_t gives a better hint about its usage.

Some item functions could be const (has_space_for). Some functions should accept a const item reference to avoid coping the item object. New item members should probably got to the other private members.

And someBoolean == true is unnecessarily verbose and redundant, which means it adds more code that others have to read and understand, but that is not needed and that is someone one can live without it, removing the comparisons with the constant boolean won't change the meaning. Same with return (someBoolean) ? true : false or the inverse.

[DavidKeaton]

Duly noted! I appreciate the clarification on a few things (namely, didn't know const prevents shadow copy and all that, nor about inline not needing to be declared). I can slip these in as I got some more code to do to this anywho. Thanks Bevap!

[BevapDin]

const prevents shadow copy

What exactly are you referring to with "shadow copy"?

[DavidKeaton]

I am referring to the fact it makes a copy of the item as its argument, or is this notion I have not right?

[BevapDin]

has_space_for(item it) will enforce a full copy, so does has_space_for(const item it) (a copy of all members - the contents vector, the item_vars map etc.)

has_space_for(item &it) won't copy (but restricts the caller to only use non-const items as argument), has_space_for(const item &it) is the optimal, no restriction on the caller, no copy.

[vache]

const doesn't prevent that, using a reference does. It's pass by reference vs pass by value. By reference you pass it THE object, by value you just pass the value contained in the object.

[kevingranade]

Might be good temporally, but eventually if it has storage it will be a container.

[kevingranade]

A bool is insufficient for this, we need to know if a particular player has opened the bag. Since there's no stable way for a player to refer to a bag, this should probably be a set of player identifiers.

[kevingranade]

There are two options here, either issue every item a unique index or use a set of tiered indices.

I haven't thought out the implications of both yet.

[DavidKeaton]

re: the two options

As far as what the pathname will look like? I have a rough draft if you want to see it.

[DavidKeaton]

re: learning about C++

Thanks for the info! :-)

[DavidKeaton]

Okay so the basic implement was the easy part, been having to rewrite chunks at a time (this ain't dead yet).

[Rivet-the-Zombie]

I'm looking forward to seeing how this one turns out.

[Barhandar]

I hope at least a majority of container management will be in background instead of explicit. I want to play a scavenger roguelike, not inventory management game.

As in, inventory is still looking mostly the same as now, except if you take a worn container with background-assigned items off and you have no space for the entire container on you, you're presented with the option to 'w'ield the container, and if you refuse, to drop the items on the floor (if container itself can still fit) or drop it altogether (if it can't).

So you don't have to waste time putting things into backpacks, bags, pouches and pockets manually or switching their container if you take one of them off.

However there still should be option to exempt a container from background assignment, resulting in it being manually-managed (or even auto-stocked for things like wielding items! Put a knife in a sheath, wield it, take it off, it goes back in that sheath instead of any other container).

[DavidKeaton]

I don't think that's quite the direction I'm going mate.

[kevingranade]

Hopefully a miscommunication, that's roughly what we discussed what was
needed. If you pick an item up, it goes into an automatically-determined
bag by default, unless you've earmarked it for a particular container with
auto-pickup. You'd also be able to manually move items to different
containers as needed.

[DavidKeaton]

Wait so now I'm confused?

[DavidKeaton]

If you mean the auto sort items yes, but i think he meant the system staying close to similar.

[narc0tiq]

Yeah, even with automatic containerization, it won't be that similar -- for
one, you won't be able to split an item across multiple containers (unless
it's something that counts by charges, like a stack of arrows).

On Tue, Jan 27, 2015 at 7:21 PM, Davek notifications@github.com wrote:

If you mean the auto sort items yes, but i think he meant the system
staying close to similar.

—
Reply to this email directly or view it on GitHub
#10927 (comment)
.

[DavidKeaton]

My aim is to make it as transparent as possible, but items will be in containers. Not the same volume system as before.

[kevingranade]

Yes, but there should be an interface that works more or less like the
current one does, in other words a flat list of items in your inventory,
and a flat list of worn items. You shouldn't need to navigate through a
hierarchy of containers to select an item for activation, dropping,
wielding, etc.
When you do pickup, drop, activate, etc an item, whether it fits somewhere
will depend on whether there's space, and the move cost for retrieval will
be based on container logic.

[Barhandar]

Yes, but there should be an interface that works more or less like the current one does, in other words a flat list of items in your inventory, and a flat list of worn items. You shouldn't need to navigate through a hierarchy of containers to select an item for activation, dropping, wielding, etc.

Yep, that's exactly what I mean!

Having a non-basic inventory mode that shows nested containers with some data, like example below and working like a 2D layering interface, would be very nice in addition to the basic one. But by no means required!

>military rucksack (worn)
  >cardboard box
    >flour (10)
  >canvas sack
    >raw potato (fresh) (60)

Aaaalso, speaking of ideas. This PR can pave the way for advanced decay mechanics, for example, honey rotting if it's placed in a container that doesn't seal (IRL honey starts to decay when it absorbs enough water to drop sugar content below antibacterial threshold, and so needs to be stored in airtight containers) or tin cans that have been opened not preserving their contents.

[DavidKeaton]

Ah okay! Gotcha! Yeah man, it won't be having to open a container to get the item. That was a big misconstrued notion I picked up. Sorry aboot that.

[Rivet-the-Zombie]

Any news on this front?

[DavidKeaton]

Late reply, didn't ever show up as a notification. Still in the works, check out #11182