Possible issue using GetItem() in SharedDatabase::SetStartingItems()

[Caryatis]

I have to say there seems to be an overriding assumption here that * I am wrong *

Because you did something retarded by removing the table completely instead of say, cleaning it out and just adding the 10 items to it. You think its cleaner to edit the source but that just shows your inexperience.

Ever think of making the charges column signed? Probably wouldn't dissolve your computer if you tried.

[c0ncrete]

confirmed that adding an item with a click to the starting_items table with a item_charges value of 0 doesn't create the item with its max charges, as SharedDatabase::CreateBaseItem() appears to indicate. the item clicked says it is out of charges. setting item_charges to 1 resolves the issue, however. it could be that it is only an issue with items that have unlimited charges.

[Tabasco]

On my first server I started everyone with a charm that had a click-effect, it's actually very common practice, so if this was broken it would have to be a very recent regression.
That's why I've been inclined to suspect that we're all missing something, myself included. I apologize if I came across as abrasive, it wasn't my intent.

The function does exactly what it's supposed to do, the key is understanding how items with click effects work. The SQL I listed earlier is wrong, you would need a 1 in the charges field.
The item itself has a maxcharges of -1, which means we never run out of charges, but the item instance has to have at least 1 charge if it is going to do anything. That's where this became confusing. Charges can't get set to maxcharges unless maxcharges is a positive value. The -1 in maxcharges just keeps Mob::CastSpell from reducing the charge count, it still won't fire the spell unless it has at least one charge in the item instance.

By passing SharedDatabase::CreateBaseItem() a 0 for charges, you are effectively setting the charge amount to 0 if maxcharges is -1. Set maxcharges to -1 in the db and charges to 1 in the instance and it will work.

[sorvani]

There should be a follow up check in CreateBaseItem to handle a negative charge value resulting from item->MaxCharges before creating the item, otherwise the -1 will get passed all the way down the chain.

as everything from this point on is a SINT16 for item charges, one can only assume the intent was to let items potentially have negative charges.

The information that comes from SetStartingItems would have restricted it to 0-255 as the DB is an UNSIGNED TINYINT

[sorvani]

this would resolve the -1 from max charges getting stuck in as the charge amount. whie preserving the ability to actually pass negative charges that the system currently accepts.

Index: common/shareddb.cpp
===================================================================
--- common/shareddb.cpp	(revision 2266)
+++ common/shareddb.cpp	(working copy)
@@ -1402,8 +1402,11 @@
 ItemInst* SharedDatabase::CreateBaseItem(const Item_Struct* item, sint16 charges) {
 	ItemInst* inst = NULL;
 	if (item) {
-		if (charges == 0)
+		if (charges == 0) {
 			charges = item->MaxCharges;
+			if (charges == -1)
+				charges = 1;
+		}
 
 		if(item->CharmFileID != 0 || (item->LoreGroup >= 1000 && item->LoreGroup != -1)) {
 			inst = new EvoItemInst(item, charges);

[Tabasco]

From what I'm seeing, it should be ok if charges is -1 because the tests look specifically for 0 or greater charges. I wonder if it's getting zeroed somewhere along the way.

[ghanja]

Line 66, sharddb.h ?

Or due to datatype changes taking place?

[sorvani]

i think you missed the point. a item with a maxcharges = -1 means it will never run out of charges, that is correct.

but when creating the item if the charges value passed to CreateBaseItem equals 0 then it is grabbing the amount of charges from the maxcharges of the item. This works for items with finite charges, but for an item with -1 maxcharges, the item that gets created will have -1 charges and be unusable.

[sorvani]

Originally Posted by ghanja

Line 66, sharddb.h ?

That is the declaration and it simply states if you pass no charge value is sets it to 0

Originally Posted by ghanja

Or due to datatype changes taking place?

there are no datatype changes happening at any point after CreateBaseItem is called. everything is a sint16.

[ghanja]

Originally Posted by sorvani

i think you missed the point. a item with a maxcharges = -1 means it will never run out of charges, that is correct.

but when creating the item if the charges value passed to CreateBaseItem equals 0 then it is grabbing the amount of charges from the maxcharges of the item. This works for items with finite charges, but for an item with -1 maxcharges, the item that gets created will have -1 charges and be unusable.

items->maxcharges of -1 tells eqemu that there are unlimited charges

starting_items->item_charges is defined a tinyint datatype (0-255) as you said in PM

How would one go about placing an item in starting_items with unlimited charges in that case? Or impossible?

Curious, why does starting_items have anything more than itemid, gm and slot? I'm sure there is a perfectly good reason beyond my limited comprehension of the source, genuinely curious is all.

[ghanja]

Originally Posted by sorvani

That is the declaration and it simply states if you pass no charge value is sets it to 0


there are no datatype changes happening at any point after CreateBaseItem is called. everything is a sint16.

Fair enough, was thinking maybe shareddb.cpp line 395 was culprit. :/ Back to the books and source study.

I tried, I failed.. but learned something so. Appreciate the responses.

[Tabasco]

I get that, in fact I explained it above, but what I talking about is this:

else if ((item->Click.Type == ET_ClickEffect) || (item->Click.Type == ET_Expendable) || (item->Click.Type == ET_EquipClick) || (item->Click.Type == ET_ClickEffect2))
            {
                if (inst->GetCharges() == 0)
                {
                    //Message(0, "This item is out of charges.");
                    Message_StringID(13, ITEM_OUT_OF_CHARGES);
                }

If inst->GetCharges() returns -1 we shouldn't get an out of charges message.

Elsewhere only maxcharges is tested, so unless the client has a particular problem with it, the rest of the server appears to be expecting the possibility.

[sorvani]

looks like everything in SharedDatabase::SaveInventory uses an unsigned long for the charges of an item.

[Tabasco]

That would certainly do it, but it looks like there's already a check there also.

            int16 charges = 0;
            if(inst->GetCharges() >= 0)
                charges = inst->GetCharges();
            else
                charges = 0x7FFF; //32767

[sorvani]

seen that in VerifyInventory also. there is definitely something going on here.