0
\$\begingroup\$

I have a game where you walk around picking up items. They are supposed to go into the next empty slot in your inventory, but when I pick up an item all the empty inventory spots get filled with it.

How can I only have the first empty one be filled by the item that is being picked up:

Here is my code:

for (int i = 0; i < c.toArray().length; i++) {
  c.get(i).tick();

  for (int i1 = 0; i1 < play.items.p.toArray().length;i1++) {
    if (c.get(i).holdingid == 0) {
      // check if the player walks onto the item which means it should 
      // be picked up:
      if(play.items.p.get(i1).r.intersects(play.p.r)) {
        // remove the item from the ground:
        play.items.p.get(i1).remove = true;
        // change the empty inventory spots to be filled with the item 
        // that has been picked up
        c.get(i).holdingid = play.items.p.get(i1).holdingid;
      }
    }
  }
}

The problem is it fills all the inventory spots how can I make it only fill the first inventory spot.

\$\endgroup\$
  • 2
    \$\begingroup\$ Your code is messy and hard to understand. For instance the line play.items.p.get(i1).r.intersects( play.p.r ) is really killing me, that's a very bad practice on writing code, especially if you're asking for help. Other people will not know your hierarchy. Take a step back to explain what you're doing first. \$\endgroup\$ – aslg Aug 13 '15 at 10:20
  • \$\begingroup\$ ok I will edit it \$\endgroup\$ – user2279603 Aug 13 '15 at 10:23
  • \$\begingroup\$ If you add a break after assigning the item to the empty slot, you would break out of the inner loop and only fill the first empty slot found instead of all. \$\endgroup\$ – Kagemusha Aug 13 '15 at 10:54
  • \$\begingroup\$ break didn't work.. \$\endgroup\$ – user2279603 Aug 13 '15 at 10:58
  • \$\begingroup\$ So c is the array of items you're holding, and play.items.p is an array of items on the world/floor? \$\endgroup\$ – aslg Aug 13 '15 at 11:31
0
\$\begingroup\$

It looks like you're incorrectly assuming that the item is removed from the ground immediately upon picking it up. However, it isn't, and the player keeps intersecting with the same item, picking it up multiple times. Before pickup, you should check if the item has already been put in an inventory slot:

  if(play.items.p.get(i1).remove != false &&
     play.items.p.get(i1).r.intersects(play.p.r))

The above should fix the problem, but I wouldn't keep going this route.

I agree with the comments & answers that the original code is structured a little strangely, in a way that is hard to articulate. It goes something like this: the items on the ground have no relationship to the number of slots you have in your inventory, so they should not be tied together.

A cleaner method would be to go through the list of items the player is intersecting with - just once, not for every inventory slot. Every time you find an item the player is intersecting with, find the first empty inventory slot. Put the item in that slot.

\$\endgroup\$
1
\$\begingroup\$

Assuming you have a class named Slot.

Public class Slot
{
    Public Item item;

    Public bool IsEmpty ()
    {
        If (item == null)
            Return true;
        Return false;
    }
}

Then make a method to return the next empty slot.

Public Slot GetEmptySlot()
{
    For (int slotIndex = 0; slotIndex < inventorySlots.Length; slotIndex++)
    {
        Slot currentSlot = inventorySlots[slotIndex];
        If (currentSlot.IsEmpty()
            Return currentSlot;
    }
    Log ("Unable to find an empty slot... Inventory is full!");
    Return null;
}

There are several ways to optimize this but I won't be showing you because that isn't the question.

This is just a super basic example of how to find an empty slot and it assumes you have an array of "Slot" which is used as your inventory.

You could then store the item in the current slot like so...

Slot emptySlot = GetEmptySlot();
If (emptySlot != null)
    emptySlot.item = pickedupItem;

Understand?

Note: I wrote this on my phone so it is untested and is semi c# / pseudocode.

\$\endgroup\$
1
\$\begingroup\$

Make the inventory a 1D Array and just use:

void onpickup(Item item)
{
     for(int i = 0; i < inventory.length; i++)
         if(inventory[i]==0) //using an int array for item ids where 0 is null
         {
             inventory[i]=item.id;
             break;
         }
}
\$\endgroup\$
  • \$\begingroup\$ Two small changes: inventory[i] = item.id; and you have to add a break after this line to assign the id only to the first empty slot. \$\endgroup\$ – Kagemusha Aug 13 '15 at 10:52
  • \$\begingroup\$ @Kagemusha oh you Arme totally right, Mc autocorrection on the iPad doesnt make Thinges easier. \$\endgroup\$ – Addi Aug 13 '15 at 10:54
  • \$\begingroup\$ So, I am on my PC right now. What I wanted to say was: "Oh, you are totally right, my autocorrection on the iPad doesn't make things easier." \$\endgroup\$ – Addi Aug 13 '15 at 12:30
  • \$\begingroup\$ You can of course also make an array of Item or Slot (much like Salvon did in his answer). Which would be useful, since you can not store cutsom attributes in an int array. \$\endgroup\$ – Addi Aug 13 '15 at 12:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.