Item contents refactor (holster refactor 2 of 3) #36660

KorGgenT · 2020-01-03T03:07:30Z

Summary

SUMMARY: Infrastructure "Refactor item contents to use item pockets"

Purpose of change

This is the most invasive part of the large amount of changes necessary for the nested container project. item::contents was previously a std::list. not only that, but it was a public member. this means there are many instances in which the list was treated exactly like that: a list. As such, some extra encapsulation was required in order to use the new classes, and some new functions that essentially act like psuedo-wrapper functions needed to be created.
Of note: there are some functions that are either all_items() and all_items_ptr(). this is due to needing to act on some arbitrary number of items in an arbitrary point in the list, not knowing what item you wanted to act upon in the first place. These functions aren't meant to be permanent and used as often as they are, but using them reduces the line count and complexity for now.
Additionally, there are many item functions that pick a specific item in order to determine whether it's a liquid container, or what name to display, etc. This also needs to be eventually moved away from in the future. The migration function was surprisingly easy to do, and additional changes to it should be fairly doable as well. Especially using savegame versions.

There are another two notable additions/changes that are important:

a new item_location type called container. this item_location stores its parent, and will be used for things such as obtain_cost.
visitable was adjusted in order to use item::contents properly.
I would appreciate those who are acquainted with these two things to double-check my work -- i'm fairly sure i've got them right, but I'm only using the item_location in one place for now, and plan to use that more with inventory code in the near future.

Testing

This PR was tested as part of the batch of all three PRs by several volunteers. It might be possible that some slipped through in this particular stage, and it is probably a good idea to test this PR individually as well.

Additional Notes

relies on #36656 and has the commits from that PR

KorGgenT · 2020-01-04T06:14:43Z

note Consider adding a new test to tests/iteminfo_test.cpp for your new iteminfo features.

BevapDin · 2020-01-04T12:55:20Z

src/savegame_json.cpp

+            contents.insert_legacy( it );
+        }
+    } else {
+        optional( data, false, "contents", contents );


Suggested change

optional( data, false, "contents", contents );

data.read( "contents", contents );

Or is there an advantage to using optional here?

BevapDin · 2020-01-04T13:04:52Z

src/item_location.cpp

+            if( !target() ) {
+                return std::string();
+            }
+            return _( string_format( "inside %s", container->tname() ) );


Same translation problem as in the other PR.

Suggested change

return _( string_format( "inside %s", container->tname() ) );

return string_format( _( "inside %s" ), container->tname() );

BevapDin · 2020-01-04T13:07:56Z

src/item_location.cpp

+
+            const int container_mv = container->contents.obtain_cost( *target() );
+            if( container_mv == 0 ) {
+                debugmsg( string_format( "ERROR: %s does not contain %s", container->tname(), target()->tname() ) );


Suggested change

debugmsg( string_format( "ERROR: %s does not contain %s", container->tname(), target()->tname() ) );

debugmsg( "ERROR: %s does not contain %s", container->tname(), target()->tname() );

debugmsg does string formatting.

BevapDin · 2020-01-04T13:15:41Z

src/iuse.cpp

@@ -7942,10 +7937,11 @@ int iuse::foodperson( player *p, item *it, bool t, const tripoint &pos )
 int iuse::radiocar( player *p, item *it, bool, const tripoint & )
 {
    int choice = -1;
-    auto bomb_it = std::find_if( it->contents.begin(), it->contents.end(), []( const item & c ) {
+    std::list<item> all_items{ it->contents.all_items() };


This list contains copies of all the items within the pockets. bomb_it contains a reference to one of those copied items.

Later you call it->contents.remove_item( *bomb_it ), but remove_item does removal based on the item pointer (see item_pocket::remove_item) , which is now a pointer to the copy stored in the all_items list. It won't remove anything.

This problem might exist at other places that use the result of all_items.

BevapDin · 2020-01-04T13:18:41Z

src/iuse.cpp

@@ -8439,7 +8433,7 @@ int iuse::autoclave( player *p, item *it, bool t, const tripoint &pos )

        bool empty = true;
        item *clean_cbm = nullptr;
-        for( item &bio : it->contents ) {
+        for( item &bio : it->contents.all_items() ) {
            if( bio.is_bionic() ) {
                clean_cbm = &bio;


And this will lead directly to UB. all_items returns a std::list<item>, so the loop iterates over that list. But the list is never stored, so after the loop finishes, it's deleted (including all the contained items), and clean_cbm will now point to a deleted object.

BevapDin · 2020-01-04T13:24:15Z

src/itype.h

@@ -923,6 +924,9 @@ struct itype {
        /** Weight difference with the part it replaces for mods */
        units::mass integral_weight = -1_gram;

+        // information related to being able to store things inside the item.
+        std::vector<pocket_data> pocket_data;


Regarding the error thrown by the build bot:

pocket_data now has two meanings: the local member, and the type. Either rename the member, or use a fully qualified type name:

Suggested change

std::vector<pocket_data> pocket_data;

std::vector<::pocket_data> pocket_data;

(This makes it unambiguous: pocket_data is the member, and ::pocket_data is the global type.)

BevapDin · 2020-01-04T13:25:19Z

src/item_factory.cpp

@@ -17,6 +17,7 @@
 #include "catacharset.h"
 #include "debug.h"
 #include "enums.h"
+#include "generic_factory.h"


This seems to be unused.

KorGgenT · 2020-01-04T16:48:08Z

This PR was merged with the other one due to the code needed in order for anything to happen

KorGgenT added [C++] Changes (can be) made in C++. Previously named `Code` 0.E Feature Freeze labels Jan 3, 2020

KorGgenT force-pushed the item-contents-refactor branch from 6c88f8c to ded1eb9 Compare January 3, 2020 04:55

KorGgenT added 2 commits January 3, 2020 22:25

class definitions for item_pocket and item_contents

e47a2a1

migrate item::contents to new item_contents object

a869e6d

KorGgenT force-pushed the item-contents-refactor branch from ded1eb9 to a869e6d Compare January 4, 2020 03:46

BevapDin reviewed Jan 4, 2020

View reviewed changes

KorGgenT mentioned this pull request Jan 4, 2020

Item contents refactor (holster refactor 1 of 2) #36656

Closed

KorGgenT closed this Jan 4, 2020

This was referenced Jan 5, 2020

Weekly Digest (29 December, 2019 - 5 January, 2020) #36719

Closed

Weekly Digest (29 December, 2019 - 5 January, 2020) #36738

Closed

KorGgenT deleted the item-contents-refactor branch August 13, 2020 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Item contents refactor (holster refactor 2 of 3) #36660

Item contents refactor (holster refactor 2 of 3) #36660

KorGgenT commented Jan 3, 2020 •

edited

Loading

KorGgenT commented Jan 4, 2020

BevapDin Jan 4, 2020

BevapDin Jan 4, 2020

BevapDin Jan 4, 2020

BevapDin Jan 4, 2020

BevapDin Jan 4, 2020

BevapDin Jan 4, 2020

BevapDin Jan 4, 2020

KorGgenT commented Jan 4, 2020

	optional( data, false, "contents", contents );
	data.read( "contents", contents );

	return _( string_format( "inside %s", container->tname() ) );
	return string_format( _( "inside %s" ), container->tname() );

	debugmsg( string_format( "ERROR: %s does not contain %s", container->tname(), target()->tname() ) );
	debugmsg( "ERROR: %s does not contain %s", container->tname(), target()->tname() );

	std::vector<pocket_data> pocket_data;
	std::vector<::pocket_data> pocket_data;

Item contents refactor (holster refactor 2 of 3) #36660

Item contents refactor (holster refactor 2 of 3) #36660

Conversation

KorGgenT commented Jan 3, 2020 • edited Loading

Summary

Purpose of change

Testing

Additional Notes

KorGgenT commented Jan 4, 2020

BevapDin Jan 4, 2020

Choose a reason for hiding this comment

BevapDin Jan 4, 2020

Choose a reason for hiding this comment

BevapDin Jan 4, 2020

Choose a reason for hiding this comment

BevapDin Jan 4, 2020

Choose a reason for hiding this comment

BevapDin Jan 4, 2020

Choose a reason for hiding this comment

BevapDin Jan 4, 2020

Choose a reason for hiding this comment

BevapDin Jan 4, 2020

Choose a reason for hiding this comment

KorGgenT commented Jan 4, 2020

KorGgenT commented Jan 3, 2020 •

edited

Loading