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

Heyas,

I add some items to players inventories during character creation and everything works fine except for an item that has a click effect on it.

In the SharedDatabase::SetStartingItems() method I add a charm for each class like this.
The item appears in the player inventory correctly, however when I try to right click the item I get the message 'Item is out of charges'. If I delete the item and #si it again, the click effect works fine.

This makes me think that something is not getting initialised correctly when calling GetItem() at this point. Any ideas? I looked at the code but I can't find anything which would cause this.

EDIT: I am using HoT client and rev 2214 of the source.

this doesn't address the issue you're having, but why not just edit the starting_items table in the database?

Because it is /much/ easier (for me) to change 10 lines of code in the source. The starting_items table offers flexibility I frankly don't need.

in SetStartingItems(), it looks as though CreateBaseItem() is called on the result of GetItem(), and then that item instance is passed to inv->PutItem().

I changed my code to use CreateBaseItem() however the result is still the same. I thought you had it solved c0ncrete! Thank you :):)

EDIT: New code FYI:

that looks like exactly what SetStartingItems() is doing now. i'm at a loss.

What is MaxCharges set to on the item?

Did you try putting the item in the starting_items table? Did it work?

Honestly, I can't imagine a situation where having to recompile the code to change the items is easier than just adding them to the database, but do what you like.

Hi lerxst2112,

maxcharges is -1 in the db. I cant put items into the starting_items table because it is *gone*, deleted. As I stated, I do not need the flexibility offered by the starting_items table, I do not change what items players start with now that they are set.

The concept of putting starting_items into a table is just a design choice, probably to make customisation easier for people not comfortable changing the source. The devs just chose to put starting items in a table and decided to hardcode starting stats yet both can be customised.

You're Telling CreateBaseItem to create an item instance with 0 charges.

As an aside, using the starting_items table is good design. You may not need the flexibility, but what about the convenience?
Once you have your build environment it's pretty easy to recompile but using the table in the database is going to be faster to do in any case, fewer keystrokes, and less opportunity for errors that spawn support threads. In terms of time alone you're at a significant net loss.

Where parameter charges is 0, it presumably uses the value maxcharges from the DB.

You're right, I didn't dig into it deep enough, but in every other case in SetStartingItems charges is set, so it might be worth a try to specify your charges.

You could also revert and just do this:

FYI I downloaded the rev2214 binaries again and the rev2214 DB, made a new DB and set it all up cleanly. I add JBoots to starting_items, create a new toon, right click the boots and get 'Item is out of charges'.

The charges field in starting_items is type tinyint, if I wanted to start with a clicky item I would have to set it to zero anyway. I tried -1 as the parameter before I posted earlier. I have to say there seems to be an overriding assumption here that * I am wrong * and I should just do it some other way. If I find a bug good, I will save some folks some time in the future, if it is my error, even better, I learned something new.

There is no HoT client. There is Titanium (dics), SoF (discs,unfortunately), SoD (Steam), and UF (Steam) among the stable builds.

Sorry, I am using UF.

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.

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.

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.

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

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.

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.

Line 66, sharddb.h ?

Or due to datatype changes taking place?

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.

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.

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.

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.

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

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.

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

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

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

You could create a new character and then look at the inventory entry in the database before clicking enter world. At least you could verify what the initial
save value is.



EDIT: Another thing to look at is how SummonItem modifies its charges:

It creates the item first, and then goes back and modifies the inst charges.

There are already provisions for it in most places, but the loss occurs in Database::StoreCharacter(), which is called right after starting items are populated.

The charges field in the inventory table is unsigned and the item charges are written without any testing.

Hi Caryatis, thanks for your feedback. I have no problems admitting my inexperience with the eqemu source, however I am not inexperienced in software development and design.

Your assumption that "I think it's cleaner" is incorrect. I looked at my requirements and considered the options available. I chose the option that satisfied my requirements and was both simple and pragmatic.

If you read through the thread you see we have covered this already, it will not fix the issue. I agree with you though that it probably wont dissolve my computer.

Necro!

I came across this issue again and the following code now works as a work-around.

CreateBaseItem must be called, only calling GetItem will result in the item being out of charges.

I'm looking at the current code in SharedDatabase::SetStartingItems, and it does call CreateBaseItem.

The code you posted has a memory leak since the ItemInst* returned from CreateBaseItem is never deleted.

Good catch, thank you!