Inventory problem?

[danielmccarthy]

Hi I am developing an inventory class and items, and weapons.

Can anyone see the issue with my code the program crashes I think its a segmentation error. Could anyone tell me a better method of doing this if they know of one?

#include <iostream>
#include <stdio.h>
#include <string.h>

using namespace std;

/* Here we store some definitions */
enum
{
    MAX_INVENTORY_ITEMS = 20
};

/* The Item class will represent an item in our game*/
class Item
{
public:
    string getName()
    {
        return name;
    }
protected:
   string name;
};

class Weapon : public Item
{
 public:
     int getDamage()
     {
         return damage;
     }
 protected:
    float damage;
};

class BasicSword : public Weapon
{
public:
    BasicSword()
    {
        name="Basic Sword";
        damage=2.0;
    }
};

class Inventory
{
public:
    Inventory()
    {
        currentItems=0;
    }
    bool addItem(Item* itm)
    {
        if (currentItems < MAX_INVENTORY_ITEMS)
        {
            for (int i=0; i < MAX_INVENTORY_ITEMS; i++)
            {
                /* If the item slot is free */
                if (item[i] == NULL)
                {
                    item[0] = itm;
                    return true;
                } else
                    return false;
            }
        }
    }

    Item** getItems()
    {
        return item;
    }
private:
    /* Maximum of 20 items */
    Item* item[MAX_INVENTORY_ITEMS];
    /* total amount of items */
    int currentItems;
};
int main()
{
    Inventory bag;
    BasicSword s;
    bag.addItem(&s);
    bag.addItem(&s);

    bag.getItems()[0]->getName();
    printf("Current items in inventory: ");

   // for (int i=0; i<MAX_INVENTORY_ITEMS; i++)
    //{
      //  if (item[i] != NULL)
//           cout << item[0][1]->getName();

    //}
    return 0;
}

[S G H]

It crashes because Items, by default, may not be null.
Set all of them to null in the constructor, or rely on the currentItems.
(FYI, the items aren't even getting added. I suggest you the first solution)
Also you should increase currentItems when you add them.

[ne555]

http://www.cplusplus.com/forum/general/112111/

foo.cpp: In member function ‘bool Inventory::addItem(Item*)’:
foo.cpp:68:3: warning: control reaches end of non-void function [-Wreturn-type]

$ valgrind ./a.out
==31401== Memcheck, a memory error detector
==31401== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==31401== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
==31401== Command: ./a.out
==31401== 
==31401== Conditional jump or move depends on uninitialised value(s)
==31401==    at 0x400B97: Inventory::addItem(Item*) (foo.cpp:60)
==31401==    by 0x400979: main (foo.cpp:84)
==31401== 
==31401== Conditional jump or move depends on uninitialised value(s)
==31401==    at 0x400B97: Inventory::addItem(Item*) (foo.cpp:60)
==31401==    by 0x40098F: main (foo.cpp:85)
==31401== 
==31401== Use of uninitialised value of size 8
==31401==    at 0x4EEE658: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::string const&) (in /usr/lib/libstdc++.so.6.0.19)
==31401==    by 0x400A7C: Item::getName() (foo.cpp:19)
==31401==    by 0x4009B0: main (foo.cpp:87)
==31401== 
==31401== 
==31401== Process terminating with default action of signal 11 (SIGSEGV)
==31401==  Bad permissions for mapped region at address 0x5119C48
==31401==    at 0x4EEE6A9: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::string const&) (in /usr/lib/libstdc++.so.6.0.19)
==31401==    by 0x400A7C: Item::getName() (foo.cpp:19)
==31401==    by 0x4009B0: main (foo.cpp:87)
==31401== 
==31401== HEAP SUMMARY:
==31401==     in use at exit: 36 bytes in 1 blocks
==31401==   total heap usage: 1 allocs, 0 frees, 36 bytes allocated
==31401== 
==31401== LEAK SUMMARY:
==31401==    definitely lost: 0 bytes in 0 blocks
==31401==    indirectly lost: 0 bytes in 0 blocks
==31401==      possibly lost: 36 bytes in 1 blocks
==31401==    still reachable: 0 bytes in 0 blocks
==31401==         suppressed: 0 bytes in 0 blocks
==31401== Rerun with --leak-check=full to see details of leaked memory
==31401== 
==31401== For counts of detected and suppressed errors, rerun with: -v
==31401== Use --track-origins=yes to see where uninitialised values come from
==31401== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 1 from 1)

you didn't initialize Item* item[MAX_INVENTORY_ITEMS];


you may use an {unordered_,}set holding the appropriate smart pointer (shared, unique)

[danielmccarthy]

Yeah, I just figured that out just then I thought they would be defaulted to zero because I am not initialising them to the heap and they are being stored on the executable. Never mind its 1 am here pretty tired. I fixed up the code is this a decent way of structuring items and inventory in a game? If not or if their is a better way could you please state it thanks here is the revised code:

#include <iostream>
#include <stdio.h>
#include <string.h>

using namespace std;

/* Here we store some definitions */
enum
{
    MAX_INVENTORY_ITEMS = 20
};

enum ITEM_FLAGS
{
    IS_EQUIPMENT   = 1 << 0,
    IS_CONSUMABILE = 1 << 1
};
/* The Item class will represent an item in our game*/
class Item
{
public:

    char getFlags()
    {
        return flags;
    }

    bool isEquipment()
    {
        if ((flags & IS_EQUIPMENT) == IS_EQUIPMENT)
                return true;
            else
                return false;
    }

    bool isConsumabile()
    {
        if ((flags & IS_CONSUMABILE) == IS_CONSUMABILE)
                return true;
            else
                return false;
    }
    string getName()
    {
        return name;
    }
protected:
   string name;
   char flags;
};

class Potion: public Item
{
  public:
    Potion()
    {
        flags |= IS_EQUIPMENT | IS_CONSUMABILE;
        name = "Potion";
    }
};

class Weapon : public Item
{
 public:
     Weapon()
     {
        flags = IS_EQUIPMENT;
     }
     int getDamage()
     {
         return damage;
     }
 protected:
    float damage;
};

class BasicSword : public Weapon
{
public:
    BasicSword()
    {
        name="Basic Sword";
        damage=2.0;
    }
};

class GodSword: public Weapon
{
public:
    GodSword()
    {
        name="God Sword";
        damage=100.0;
    }
};

class Inventory
{
public:
    Inventory()
    {
        currentItems=0;
        Empty();
    }
    bool addItem(Item* itm)
    {
        if (currentItems < MAX_INVENTORY_ITEMS)
        {
            for (int i=0; i < MAX_INVENTORY_ITEMS; i++)
            {
                /* If the item slot is free */
                if (item[i] == NULL)
                {
                    item[i] = itm;
                    currentItems++;
                    return true;
                }
            }
        }
        return false;
    }

    Item* getItem(int slot)
    {
        return item[slot];
    }

    Item** getItems()
    {
        return item;
    }

    void Empty()
    {
        /* Emptying all items */

        for (int i=0; i< MAX_INVENTORY_ITEMS; i++)
        {
            item[i] = NULL;
        }
    }
private:
    /* Maximum of 20 items */
    Item* item[MAX_INVENTORY_ITEMS];
    /* total amount of items */
    int currentItems;
};

Inventory bag;
void OutputInventory()
{
     Item** item = bag.getItems();

    for (int i=0; i<MAX_INVENTORY_ITEMS; i++)
    {
        if (item[i] != NULL)
        {
            cout << item[i]->getName();
            if (item[i]->isConsumabile())
            {
                cout << " (can be eaten or drank)";
            }
            cout << ",";
        }

    }
}
int main()
{
    Potion p;
    BasicSword s;
    GodSword gs;

    bag.addItem(&p);
    bag.addItem(&s);
    bag.addItem(&gs);


    printf("Current items in inventory: ");
    OutputInventory();

    return 0;
}

[giblit]

class GodSword: public Weapon
{
public:
    GodSword()
    {
        name="God Sword";
        damage=100.0;
    }
};

this seems questionable if the only difference between a godsword and a normal weapon is the damage and name you may want to consider using a map.

std::map<std::string, Weapon>
map["God Sword"] = Weapon(100);

then just overload the constructor to take damage.

[danielmccarthy]

I have a big game project I am working on so I am dividing small tasks into new projects before I implement them into the real thing. GodSword is just a basic example in the future their would be virtual methods in the Weapon class such as

virtual void Attack(ATTACK_TYPE attktype);

Is this bad implementation doing it like that?

The game could then play the required animation and maybe the damage code could be done some where else?

[danielmccarthy]

BUMP

[AbstractionAnon]

I would stronly suggest you use a standard container for your Inventory class. A vector or map will automatically adjust as items are added or deleted eliminating the need to track them yourself.

You also have the potential for a memory leak in your Inventory class. Your Empty function overwrites item pointers with NULL. Thats works now when the items are allocated on the stack in main, but is going to be a problem if you start allocating items on the heap.

As for whether your individual classes are a bad design, it depends on how complex your derived classes are going to be. If all you want to do is play an automation, it would seem like that would be a function of the base class and the animation to be played an attribute of that base class since every derived object would share these. But the decision is really up to you as the game designer. There is inherently nothing wrong with using derived classes in this manner as long you correctly place what is common to all derived classes in the base class. As giblit pointed out, so far your derived classes aren't doing anything that can't be done in the base class.