Prospect patch FS#2443 - Wrong Stacking Behavior

Patch by kougaro, fixing strange stacking behaviour with dropped items, by removing a check that appears to be unneeded.

[Lanarel, 2008-11-10 21:40:27]

Prospect patch FS#2443 - Wrong Stacking Behavior , 2nd try for patch

[Lanarel, 2008-11-10 21:41:59]

http://codereview.appspot.com/7769/diff/202/3
File src/server/bulkobjects/psitem.cpp (right):

http://codereview.appspot.com/7769/diff/202/3#newcode1249
Line 1249: //    if (GetGuardingCharacterID() !=
otheritem->GetGuardingCharacterID())
According to Kougaro this check is not needed, as for dropped items this is done
before.

[Kayden, 2008-11-11 18:33:55]

On 2008/11/10 21:41:59, Lanarel wrote:
> Line 1249: //    if (GetGuardingCharacterID() !=
> otheritem->GetGuardingCharacterID())
> According to Kougaro this check is not needed, as for dropped items this is
done before.

This seems like the right place for such a check.  We definitely want to check
that for world items—otherwise people could drop a similar item to steal your
stuff.  If things were stackable in containers (which they're not ATM), then
this would also check it.

Rather than just disabling this, I would look into why exactly they won't stack
in inventory—what are the guardingCharacterIDs involved?  I suspect that it's
simply not clearing them when adding items to inventory.  There should be no
reason to have a guardingCharacterID set if the item actually has an owner. 
Perhaps even having SetOwner clear guardingCharacterID would be reasonable.

[Lanarel, 2008-11-11 19:11:03]

I wonder if this is a similar problem to
http://www.hydlaa.com/flyspray_upgrade/index.php?do=details&task_id=2457
for which I just put a possible fix in
http://codereview.appspot.com/8647/show

In that case people could not add items to containers, also probably because
they seemed guarded. WHat was missing for sure was a check if the guard was
nearby (which I added), and it seems missing here as well. On the other hand,
there may be a more fundamental problem in both.

[Kayden, 2008-11-11 19:23:38]

It's not really missing here—these are psItems, not gemItems.  They very often
won't have a position.

Another potential pitfall: if one is guarded and another is 0, this won't work. 
If one ID or the other is invalid, it probably ought to work—but the final
result should be guarded by the valid ID.

[kougaro, 2008-11-11 20:59:01]

Ok, so here is the situation : when you pickup an item, the guardingCharacterID
is set to 0. And the items you dropped on the ground have your ID as
guardingCharacterID. When you try to pickup the item, and the game verify if he
can stack the items together in your inventory(was already asserted that you can
pick it up), it found two different guardingCharacterID, and do not stack the
items.
The check to decide if you can pick up an item is made around lines 1294-1335 of
server/gem.cpp

[Kayden, 2008-11-12 08:07:45]

On 2008/11/11 20:59:01, kougaro wrote:
> Ok, so here is the situation : when you pickup an item, the
guardingCharacterID
> is set to 0. And the items you dropped on the ground have your ID as
> guardingCharacterID. When you try to pickup the item, and the game verify if
he
> can stack the items together in your inventory(was already asserted that you
can
> pick it up), it found two different guardingCharacterID, and do not stack the
> items.

Okay, good.  That did seem missing from it.  So a check like

if (guardingCharacterID.IsValid() &&
otheritem->GetGuardingCharacterID().IsValid() && guardingCharacterID !=
otheritem->GetGuardingCharacterID())
    return false;

would allow this case...but then CombineStack possibly should inherit whichever
of the guarding IDs actually -is- valid.  This is sounding obnoxiously complex
though...your idea may be better.

> The check to decide if you can pick up an item is made around lines 1294-1335
of
> server/gem.cpp

I'm not so worried about picking items up...it's autostacking when -dropping-
items.  Namely, if there's a Carrot on the ground, and you drop another Carrot
near it...they stack (or at least used to).  What happens if Joe is guarding a
Gold Ore, and I drop another Gold Ore near it?

I would think that this CheckStackable test would cause it to fail, disabling
autostacking, and simply creating a new, separate entity.  Without it, I'm
afraid that the two will stack, and my ore will now belong to Joe.

[Kayden, 2008-11-12 08:19:45]

Looking at it again, IMO, the notion of guardianship doesn't belong in psItem at
all—it's not an intrinsic property of the item.  One guards entities, not items.
 So it really ought to live in gemItem and possibly gemContainer...presumably
it's here for saving purposes.

That said, fixing up the gem*/ps* hierarchy is too big a project to be
undertaken here.  So...back to the problem at hand, I guess.