CleverRaven / Cataclysm-DDA

Cataclysm - Dark Days Ahead. A turn-based survival game set in a post-apocalyptic world.
http://cataclysmdda.org
Other
10.65k stars 4.18k forks source link

Container Inventory Management #10067

Closed Robik81 closed 9 years ago

Robik81 commented 10 years ago

I finished CIM and am opening this issue to find, if there is interest / support in having it in the mainline. Change list and link to branch can be found in the forum here http://smf.cataclysmdda.com/index.php?topic=8234.0

The branch CIM also contains commits from CataTweaks branch (layering changes), but in eventual PR these would not be included, so you can ignore them.

kevingranade commented 10 years ago

You already have your answer. We have a plan for supporting this, and it does not include mandatory actions for interacting with containers, or only supporting interacting with containers via one of the inventory interfaces. If you've moved away from those two, we can talk, otherwise it's pointless.

To be mainlined, a container interaction interface needs to support both regular inventory and AIM, and automatically insert/remove items in containers as needed (both for inventory interaction and crafting).

Robik81 commented 10 years ago

don't know what you mean by automatically insert and remove... removing obviously works when you use the stuff when eating, reloading or crafting, automatic insertion ?

kevingranade commented 10 years ago

When I say mandatory, I mean if you want to use one of these containers, you must use the manual interface to decide where it goes. If you pick up an item the inventory system should find somewhere to put it for you, and allow that choice to be overriden.

Robik81 commented 10 years ago

Hmm, in system I have it goes straight into your inventory, not into any container. Asking the user every time he grabs something, if he wants to put it in inventory, container A or container B would be annoying as Windows. To make it bearable, you would have to make each container configurable, what items to put there and what items never put there.

Shoes01 commented 10 years ago

That would be cool if you could indeed identify your messenger bag as your "med pack" and only "medicine" goes in there. I agree that that would be pretty intricate. Perhaps simply having a "do not put in" flag that the player can set can be a good compromise. My messenger bag would not have items automatically put into it, and then I would manually move medical items into it that I want in there.

I could imagine if your inventory is full, you are prompted "Really put in your messenger bag? Y/N". Or even more generic in the case of having multiple flagged containers, "Really put in a flagged contained? Y/N"

Robik81 commented 10 years ago

Actually, I added feature to label your items, so you can name the container, like canvas bag, "med pack" and use it just like you described. That is exactly how I am using it, having "med kit" and "food bag" containers in inventory.

Messenger bag, as a wearable, is not a container though. That is something I plan to add later. And I would like to wait until Kevin implements something he mentioned on forum:

By the way, the solution (which will take a while, but be worth doing) is to make itypes "composable", that is an itype defines a way you can interact with an item, e.g. firing it, wearing it, activating it. Then guns with a sling would simply have a "wearable" itype attached to them. This does require overhauling every item definition in the game though, so not something I'm looking forward to :P

Regarding compromise, I am not sure if Kevin wants to reach one. My impression is, that he wants perfect container system, done in one PR and done how he thinks is right. Right now, I have a system that allows people who want to use containers to do so, and people who don't want to mess with containers to play exactly as they are playing now, without any penalty. I don't see any logical reason why such feature could not be merged and improved later by people, who want such improvement to be done.

On top of that, am afraid that anything I will do would not be sufficient enough, or will be dismissed as badly implemented. So, with that risk on mind, I am not too keen to work on something I am not going to use myself regardless of if it will be merged or not.

inverimus commented 10 years ago

So what is the difference between inventory and containers? Is inventory just from certain clothing and backpacks, bags, etc are containers? Doesn't it make more sense to make everything containers?

Robik81 commented 10 years ago

Container is everything specified in containers.json, these items have some container specific properties that other items do not have.

Inventory space is just calculated number from all worn items with storage property combined.

Does it make sense to make everything container? IMO no, it would be to much hassle. I personally plan to change only some worn items to work like containers. After these changes implemented, it would be possible to make container from rest of them with mod though. This is something different than this issue is about though.

kevingranade commented 10 years ago

I'm not insisting on a single PR that solves every problem, but your PR doesn't get us any closer to the correct implementation, and will have to be dismantled when the complete system is done.

If people cannot interact with certain items without using your system, it is in no way optional.

Another point, yes it makes sense for every container to use the same system. It becomes very confusing otherwise, because the distinction between them is arbitrary.

The problem with adding an alternate system like this is:

  1. If it's not a complete solution, people will try to use it for everything and complain to me when it becomes a hassle to use.
  2. You're proposing multiple different inventory systems that work in different ways. If putting it that way doesn't make it obvious what the problem is, I don't know what else would make it clear.
  3. Your solution will actively interfere with the more complete solution, as they would be implemented differently. On Nov 18, 2014 10:11 AM, "Robik81" notifications@github.com wrote:

Container is everything specified in containers.json, these items have some container specific properties that other items do not have.

Inventory space is just calculated number from all worn items with storage property combined.

Does it make sense to make everything container? IMO no, it would be to much hassle. I personally plan to change only some worn items to work like containers. After these changes implemented, it would be possible to make container from rest of them with mod though. This is something different than this issue is about though.

— Reply to this email directly or view it on GitHub https://github.com/CleverRaven/Cataclysm-DDA/issues/10067#issuecomment-63516982 .

Robik81 commented 10 years ago

I'm not insisting on a single PR that solves every problem, but your PR doesn't get us any closer to the correct implementation, and will have to be dismantled when the complete system is done.

Yeah, I said that my implementation will be dismissed as bad, did you actually checked the code?

If people cannot interact with certain items without using your system, it is in no way optional.

Look, unless they actively put item into the container themselves in AIM, then the item is not in the container and they don't have to use my system at all, thus, it is completely optional.

Another point, yes it makes sense for every container to use the same system. It becomes very confusing otherwise, because the distinction between them is arbitrary.

Every container does use same system and rules. Please, don't mix wearables into it, these are not containers - they never were and changing that is not part of the CIM.

  1. If it's not a complete solution, people will try to use it for everything and complain to me when it becomes a hassle to use.

It is pretty much complete solution for stuffing things into containers. If people finds it hassle, they will not use it, which is what I am predicting. Some people don't want to organize their inventory in such way and others do. And there is always someone who complains about something on the forum, question is if it is valid complaint or not.

  1. You're proposing multiple different inventory systems that work in different ways. If putting it that way doesn't make it obvious what the problem is, I don't know what else would make it clear.

Not in CIM. There is nothing new, wearables are with "storage", as always were, and containers with "contains" as always were. Only new thing is, that you can put non-liquids into containers.

  1. Your solution will actively interfere with the more complete solution, as they would be implemented differently.

Yes, if you have different idea how to do it, it will interfere with each other. Although, it might be easier to alter my code, than altering current code, or even reuse something. Did you check the implementation and found it unacceptable?

All in all, it is probably my fault, I just seem not able to explain what this thing does and what it doesn't do.

Rivet-the-Zombie commented 10 years ago

Every container does use same system and rules. Please, don't mix wearables into it, these are not containers - they never were and changing that is not part of the CIM.

Part of the whole 'container inventory system' idea is that the storage containers one wears (like backpacks and pouches, and yes, they are objects whose stated purpose it to contain other objects - even though they're not listed under 'containers' in the code) should operate under the same rules as storage containers one carries, like bags - which this issue doesn't seem to care about.

So what's the actual gain here? I'm not in favor of adding extra button-pushing with no tangible benefit, or adding a superfluous system that nobody will use because it doesn't actually do what it says on the tin.

As a final note, I haven't checked out the code, because I see no actual code here, just a discussion.

Robik81 commented 10 years ago

Okay, I am going to answer your post seriously, even though I feel that getting answers is not what you want.

Part of the whole 'container inventory system' idea is that the storage containers one wears (like backpacks and pouches...

Yes, part of it. Few posts up I talk about wanting to add this after Kevin adds "composable" itypes, as he calls it. In forum thread (linked in first post) I am talking about this as stage 2, or separate project.

they are objects whose stated purpose it to contain other objects - even though they're not listed under 'containers' in the code) should operate under the same rules as storage containers one carries, like bags

Yeah, in real life, and hopefully in game too, one day... but for now, these are not container type items in code and as such, do not abide same rules as container type items.

So what's the actual gain here?

Less cluttered and better organized inventory, mainly. For folks that care about such things. Thoughts about it can be found here and in the forum thread.

I'm not in favor of adding extra button-pushing with no tangible benefit, or adding a superfluous system that nobody will use because it doesn't actually do what it says on the tin.

o_0

As a final note, I haven't checked out the code, because I see no actual code here, just a discussion.

You can find the link to branch with code in forum thread, read the first post.

Rivet-the-Zombie commented 10 years ago

Thank you for answering my question somewhere in the middle of your response. I've no idea what the 'o_0' emoticon means, though.

I can dig the goal of making a simplified inventory, but why not work out a way to have it also work in the basic inventory display as well?

Robik81 commented 9 years ago

I could tell you what o_0 means, but since you prefer response only to sentences with question mark on the end, I am not going to :wink:

I can add simple pull out and put in actions on container, that would work in basic inventory. Would not work with nested containers though (in one step, you could always pull out nested container first) and for only one item at a time.

Didn't make it because I think it would be inferior to AIM container handling. But could be useful at times, so I will add that.

Is such solution acceptable, or you had something different on mind?

kevingranade commented 9 years ago

I've already outlined what we're looking for, a unified system. If you're not working toward that, the changes aren't going in.

Robik81 commented 9 years ago

Your outline is pretty vague and sucks. I am not going to make changes based on it and pray it will be good enough for you.

Thanks for keeping this open source though, at least, I can alter the game as I see fit for my personal use.