Skip to content

/Cataclysm-DDA

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] [CR] Nested Item Containers #11182

Closed
wants to merge 17 commits into from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@DavidKeaton @Zireael07 @KA101 @BevapDin
@DavidKeaton
Copy link
Contributor

commented Feb 9, 2015

SUPER NOTE

I have begun working from scratch due to a fresh approach and merge conflicts that have accrued over time. I will be setting up a new PR for this. If you want to take a peek at the new branch, please refer to https://github.com/DavidKeaton/Cataclysm-DDA/tree/item_bags in the meantime. This branch aims to more or less refactor and rework the item class to make all the fun item stuff less of a hack approach and more of a correct one.

Thanks all!
-Davek

END OF SUPER NOTE

_Note!_
This PR is in desperate need of some peer review, but I know ya'll been busy. Whenever someone gets a chance, I'd love to make sure I haven't borked anything bad! :-)

Items all up in Items!

  • contents.add() to add items.
  • contents.rem() to remove items.
  • contents.get() to vector of all contents.
  • contents.has_* functions to retrieve certain characteristics of contents [has_food, has_liquid, ...]
  • various overloads to access it the same as before.

I have placed the storage class in item.h, to reduce clutter for fs and headers. contents is now class storage instead of a simple std::vector, but is accessible most of the same way as before (see above).

_TODO_

  • Finish Container Filters (for autonomous item storage)
  • Implement in AIM
  • Better display for inventory_ui

Update 1:

  • removed self pointer to item
  • moved functions that determined space to item class

Update 2:

  • reverted contents back to std::list, not invstack (why I did this, God only knows)
  • basic interface in inventory_ui (mostly for testing stuff atm)
  • implementation of filter functions
  • only allowing storage to containers that can store

Update 3:

  • basic ui fixes in inventory for storing/removing (may still have some bugs) [for testing containers]
  • possible idea for filtering items based on certain criteria (to be expanded further)
  • some other stuff I'm sure, can't remember (go diff checks!) :-)
  • forgot to mention that I have added push_back and erase as per @BevapDin

Update: Also as an aside, I am doing some smaller PRs to take a minor break from this so I don't burn out on it. Still in the works!

@DavidKeaton DavidKeaton changed the title Nested Item Containers Update 1 [CR] Nested Item Containers Update 1 Feb 9, 2015

@DavidKeaton

DavidKeaton reviewed Feb 9, 2015
View changes

src/iuse_actor.cpp Outdated
it->contents.push_back(item(target_id, calendar::turn));
target = &it->contents.back();
it->contents.add(item(target_id, calendar::turn));
target = &it->contents[it->contents.size()];

This comment has been minimized.

Sign in to view
Copy link
@DavidKeaton

DavidKeaton Feb 9, 2015

Author Contributor

I realized now this is redundant, doip.

This comment has been minimized.

Sign in to view
Copy link
@KA101

KA101 Feb 9, 2015

Contributor

Side note: that comment up there looks Precarious if one stores perishables. Careful with this one.

This comment has been minimized.

Sign in to view
Copy link
@DavidKeaton

DavidKeaton Feb 9, 2015

Author Contributor

Wait, wasn't this a bug in the current open issues?

This comment has been minimized.

Sign in to view
Copy link
@KA101

KA101 Feb 9, 2015

Contributor

Possibly, but a quick search for "container" didn't turn up anything that looked relevant.

This comment has been minimized.

Sign in to view
Copy link
@DavidKeaton

DavidKeaton Feb 9, 2015

Author Contributor

noted, I will take a peek!
@BevapDin

BevapDin reviewed Feb 9, 2015
View changes

src/item.cpp Outdated

storage::storage(item *self, item_iterator start, item_iterator stop)
{
me = self;

This comment has been minimized.

Sign in to view
Copy link
@BevapDin

BevapDin Feb 9, 2015

Contributor

Linking back to the parent object is usually not a good approach. This implementation will give you crashes as the pointer is not updated by the copy constructor nor by the assignment operator.

item a = ... ; // a.content.me == &a
item b = ... ; // b.content.me == &b
a = b; // a.content.me == &b

This comment has been minimized.

Sign in to view
Copy link
@DavidKeaton

DavidKeaton Feb 9, 2015

Author Contributor

yeah I actually removed this altogether. lol.
@BevapDin

BevapDin reviewed Feb 9, 2015
View changes

src/item.h Outdated
// removes the item (from iterator)
item_iterator rem(item_iterator iter);
// removes a range of items
std::vector<item> rem(item_iterator start, item_iterator stop);

This comment has been minimized.

Sign in to view
Copy link
@BevapDin

BevapDin Feb 9, 2015

Contributor

Why not use the std::vector function names push_back and erase, you don't have to update all the code that uses it.

This comment has been minimized.

Sign in to view
Copy link
@DavidKeaton

DavidKeaton Feb 9, 2015

Author Contributor

I had done just that, but wanted to make the names more sane for their purpose.
@BevapDin

BevapDin reviewed Feb 9, 2015
View changes

src/item.h Outdated
// items stored in this bag
std::vector<item> items;
// holds any extra information we need about storage
std::map<std::string, void *> var;

This comment has been minimized.

Sign in to view
Copy link
@BevapDin

BevapDin Feb 9, 2015

Contributor

And who manages the pointer stored here? Who deletes them?

This comment has been minimized.

Sign in to view
Copy link
@DavidKeaton

DavidKeaton Feb 9, 2015

Author Contributor

it's a generic type, not solely a void pointer, used for type casted data. prob gonna remove.

@DavidKeaton DavidKeaton changed the title [CR] Nested Item Containers Update 1 [CR] Nested Item Containers Feb 12, 2015

@DavidKeaton DavidKeaton force-pushed the DavidKeaton:item_nested_containers branch Feb 15, 2015

@DavidKeaton DavidKeaton force-pushed the DavidKeaton:item_nested_containers branch Mar 8, 2015

@DavidKeaton DavidKeaton force-pushed the DavidKeaton:item_nested_containers branch Mar 15, 2015

@DavidKeaton DavidKeaton force-pushed the DavidKeaton:item_nested_containers branch to 69a5d56 Mar 17, 2015

@DavidKeaton DavidKeaton referenced this pull request Mar 20, 2015

Closed

[WIP] [CR] Item Bags #10927

0 of 5 tasks complete

@DavidKeaton DavidKeaton changed the title [CR] Nested Item Containers [WIP] [CR] Nested Item Containers Mar 20, 2015

@Zireael07

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 29, 2015

@DavidKeaton: Any news on this?
@DavidKeaton

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 29, 2015

tbh the aim bit took most of my time, but I want to finish this before the next release. :-)
@DavidKeaton

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented May 29, 2015

I also have been considering other avenues for doing it, so it's not on the backburner completely! :-)
@DavidKeaton

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 29, 2015

Please read the description as to why this is closed. (new branch)

@DavidKeaton DavidKeaton closed this Jun 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.