Game inventory system

Question

I am trying to build an inventory system in C++ for a game that I am working on. However, there is a bug in the inventory system when I call Inventory::AddItem(Item i), no item gets added and that slot still stays blank. Currently, I handle the inventory through std::vector<Item>, where Item is a struct containing the type, if it is stackable, the maximum number of blocks in a stack, the current number of blocks in the stack, and a couple of objects for animation. Moreover, I automatically fill the inventory in with 40 slots of air blocks, which have the ID of INVENTORY_EMTPY_SLOT_ID.

Here is the code:

    typedef struct item {
        int type;    // this is whether the block is a foreground of background tile
        int id;      // use this for the id of objects
        bool stackable;   // true indicates that the block can be stacked
        int max_num;      // maximum number of blocks in a stack
        int num;          // the current number of blocks in the stack
        Animation* use_animation;    // the animation of the block or item when it is being used
        Animation* other_animation;  // secondary animation of item in case it is necessary
    } Item;

How I initialize empty slots:

    for (size_t x = 0; x < INVENTORY_MAX_SLOTS; x++) {
        Item i = {0, INVENTORY_EMPTY_SLOT_ID, true, 1, 1, NULL, NULL};
        this->items.push_back(i);
    }

Adding items

/*********BUG HERE:******************/
void Inventory::AddItem(Item item) {
    // find all indexes with the same item.id
    std::vector<size_t> indexes_w_same_item;
    for (size_t i = 0; i < this->items.size(); i++) {
        if (this->items[i].id == item.id) {
            indexes_w_same_item.push_back(i);
        }
    }

    // find the next empty slot
    int next_empty_slot = -1;
    for (size_t i = 0; i < this->items.size(); i++) {
        if (this->items[i].id == INVENTORY_EMPTY_SLOT_ID) {
            next_empty_slot = i;
        }
    }

    // go through all the indexes with the same item.id
    // and see if at least one of them is empty.
    // if one is empty and has sufficient capacity,
    // add the item and return. if it isn't, keep moving forward
    for (size_t x = 0; x < indexes_w_same_item.size(); x++) {
        if (item.id == this->items[indexes_w_same_item[x]].id) {
            if (this->items[indexes_w_same_item[x]].num + item.num <= this->items[indexes_w_same_item[x]].max_num) {
                this->items[indexes_w_same_item[x]].num += item.num;
                return;
            }
        }
    }

    // if there is an empty slot, make a new stack
    if (next_empty_slot >= 0) {
        this->items[next_empty_slot].id = item.id;
        this->items[next_empty_slot].max_num = item.max_num;
        // clamp item.num so it doesn't exceed item.max_num
        if (item.max_num > item.num) {
            this->items[next_empty_slot].num = item.num;
        } else {
            this->items[next_empty_slot].num = item.max_num;
        }
    }
}

Answer 1

I know you have found the error, but there are many issues in your code that lead to this error, and I wanted to help you understand how to write better code, so next time it will be easier for you to find it (and maybe even avoid it!).

1. You should divide the logic into as small pieces as possible - modularity is a key for more clear and clean code, which was helping you to understand the error much faster.
2. Instead of making a clear flow, you made two distinct flows on and off. The code is much clearer when you exhaust one possible flow, and only then start the other (look at the functions add_item_to_existing_stack_if_possible and add_item_to_new_stack_if_possible.
3. Your variables/functions/classes names must represent what they are standing for, it wasn't the case with the original code! Look at the Item struct now - it is much clearer what each member represents, without comments! (personally, I am not using comments at all)
4. C++ is not C with classes - things like typedef should not appear in your code, you should use operator<< to std::cout instead of printf and so on.
5. Make sure you add const specifiers as possible, it can help find many mistakes on compile time (and make your program run faster).
6. Performance related - you should pass objects as references as much as possible, it is much faster to pass an uint64 (memory location) than copy your entire Item object.

#include <vector>
#include <array>
#include <iostream>

struct Animation;

struct Item {
    int type;
    int id;
    bool is_stackable;
    int max_num_blocks_in_stack;
    int curr_num_of_blocks_in_stack;
    Animation* used_animation; // if it is non nullable, you should consider to use it without a pointer (possibly a reference)
    Animation* secondary_animation; // nullable - can be a pointer or std::optional
};

class Inventory
{
public:
    bool add_item(Item&& item);

private:
    bool is_slot_empty(size_t idx) const { return items[idx].id == INVENTORY_EMPTY_SLOT_ID; }
    std::vector<size_t> find_indexes_of(const Item& item) const;
    size_t find_next_empty_slot() const;

    bool add_item_to_existing_stack_if_possible(const Item& item);
    bool add_item_to_new_stack_if_possible(Item&& item);

    void print() const;

    static constexpr size_t MAX_INV_SIZE = 40; // can transform into a class template!

    std::array<Item, MAX_INV_SIZE> items;

    static constexpr int INVENTORY_EMPTY_SLOT_ID = -1;
};

std::vector<size_t> Inventory::find_indexes_of(const Item& item) const
{
    std::vector<size_t> indexes{};

    for (size_t idx = 0; idx < MAX_INV_SIZE; ++idx) 
    {
        if (items[idx].id == item.id) 
        {
            indexes.push_back(idx);
        }
    }

    return indexes;
}

size_t Inventory::find_next_empty_slot() const
{
    for (size_t idx = 0; idx < MAX_INV_SIZE; ++idx) 
    {
        if (is_slot_empty(idx)) 
        {
            return idx;
        }
    }

    return MAX_INV_SIZE; // invalid value!
}

void Inventory::print() const
{
    for (size_t i = 0; i < MAX_INV_SIZE; ++i) 
    {
        if (this->items[i].id != INVENTORY_EMPTY_SLOT_ID)
        {
            std::cout << "Inventory slot: " << i << "\n"
                      << "Item ID: " << items[i].id << "\n"
                      << "Item Num: " << items[i].curr_num_of_blocks_in_stack << "\n"
                      << "Item Max Num: " << items[i].max_num_blocks_in_stack << std::endl;
                      //<< "Item Texture: " << textures[items[i].id] << std::endl;
        }
    }
}

bool Inventory::add_item_to_existing_stack_if_possible(const Item& item)
{
    auto indexes_with_same_item = find_indexes_of(item);

    for (auto idx : indexes_with_same_item) 
    {
        if (item.id == items[idx].id) 
        {
            if (items[idx].curr_num_of_blocks_in_stack + item.curr_num_of_blocks_in_stack <= 
                items[idx].max_num_blocks_in_stack) 
            {
                items[idx].curr_num_of_blocks_in_stack += item.curr_num_of_blocks_in_stack;
                return true;
            }
        }
    }

    return false;
}

bool Inventory::add_item_to_new_stack_if_possible(Item&& item)
{
    size_t next_empty_slot = find_next_empty_slot();

    if (next_empty_slot >= 0) 
    {
        this->items[next_empty_slot] = std::move(item);

        return true;
    }

    return false;
}

bool Inventory::add_item(Item&& item) 
{
    bool was_possible_to_add_to_existing_stack = add_item_to_existing_stack_if_possible(item);

    if (!was_possible_to_add_to_existing_stack)
    {
        return add_item_to_new_stack_if_possible(std::move(item));
    }

    return false;
}