Forgottenserver: Depot doesn't save contents unless opened

[jerryb1988]

Steps to reproduce (include any configuration/script required to reproduce)

1. Login, DONT OPEN YOUR DEPOT
2. Use an LUA script to do item:moveTo(player:getDepotChest(17,true))
3. Logout
4. Login
5. Item is gone.

Expected behaviour

The item should save.

Actual behaviour

Item is gone, even if the server saved after the item was moved.

More Details

This DOESN'T happen if you open your depot box at least once while logged in.

Application

Using an autoloot script on kill that moves items a player selects to their depot box. The exact line used is pasted above. The script works flawlessly, the only problem I'm having is the items dont save if they dont open their depot. They can open it at any point while logged in, before or after the items are moved, and they will save. If they don't open it, all the stuff disappears, even if the server saved the player in the process. The items never make it to the SQL table.

[jerryb1988]

http://www.tibiaking.com/forum/forums/topic/63838-tfs-1x-autoloot-system/

The only modification being the moveto line.

[pzaj2]

@jerryb1988

I just checked the source code and I believe this piece of code is responsible for your issue:

    if (player->lastDepotId != -1) {
        //save depot items
        query.str(std::string());
        query << "DELETE FROM `player_depotitems` WHERE `player_id` = " << player->getGUID();

        if (!db.executeQuery(query.str())) {
            return false;
        }

It's iologindata.cpp, line 807. Basically it checks if lastDepotId of player is other than -1, that is the initial value. The only code I see that changes it is Actions::internalUseItem method and this would be consistent with what you said (once the player uses depot locker, this value is changed and the deposit items are saved).

As far as I can see TFS does not expose method allowing to set that value from lua. So for now, as a hotfix, you could remove this check and recompile your server. The only drawback is that no matter if any changes were done to player's depo during their online time, it will be saved anyway. I'm not sure how much time and resources of player depot items is taking, therefore it could lead to performance issues (though if most of your players have their depot modified during their online time, it does not make much difference to what I wrote below).

One proper way would be adding a flag to Player class that would be set when item is being moved to player's depot and check this flag on player save.

[jerryb1988]

Thank you so much. I've been banging my head off the desk trying to figure this out for almost a month. I changed it to != -2 (should never realistically get this value, forcing it to always be 'true') and recompiled, and as you said, it works. When the server saves the database updates. Logout the database updates.

[pzaj2]

@jerryb1988 Yes, I think the depotId must be 0+, so using -2 is fine (but you should really just remove the if condition there). Please keep in mind this solution isn't really a proper solution and you may risk some performance issues. Perhaps someone from TFS team can come up with a better idea?

[diath]

You could probably start with DepotChest::postAddNotification() to retrieve the player using Item::getTopParent() (although I'm not sure if the DepotChest class holds the player as top parent) and adjust the flag accordingly (you'd probably need a new method for the player class to retrieve the depot ID via the pointer though). That way the depots would only be saved if items have been added to them through scripts (or other means). I'm not entirely sure if that'd be the best way to go about it.

[joseluis2g]

I think DepotChest class set the depot tile as top parent

[Kamenuvol]

These loading and save on TFS ARE ALL WRONG. It should load everything on start and save everything at some point. Not by login and logoff,

[ghost]

@jerryb1988
Forcing to save depot items always may hit the performance.

More proper solution (until the real one is found) could be to alter the lastDepotId of Player in player:getDepotChest() like this:

if (depotChest) {
    player->setLastDepotId(depotId); //<--
    pushUserdata<Item>(L, depotChest);
    setItemMetatable(L, -1, depotChest);
}

[pzaj2]

@jerryb1988

The idea proposed by @EvulMastah is much more performant assuming you always do call Player::getDepotChest() method before putting anything into depot from lua (otherwise it will not save). Go for this one.

@Kamenuvol
One thing I can agree is that it is all wrong. But loading data on startup and and saving on restart does not solve a thing here. It only use more memory than required and make the save longer. But the persistence layer is a bit dumb, that's right. I believe all objects should be tracked for changes by having state flag(s) to only save what has to be saved. They could then save much faster and could be saved much more often in the background (while server is running)

[jerryb1988]

Thanks everyone for all the feedback. Per the various suggestions on performance, I reverted the -2 option and went with EvulMastah's solution of setting the last depot when doing player:getDepotChest(). Tested and it works.

[TwistedScorpio]

Or simply keep players into memory and just save them on every server save.

[Znote]

Confirmed at latest commit (Sat Dec 30 23:15:56 2017 +0100).

Add to talkactions.xml:

<talkaction words="!depot" separator=" " script="depot.lua" />

Add and (possibly modify position of depot):
depot.lua

-- Transfer all equipped items on player to depot
local depotPos = Position(104,366,6) -- Position of a depot box in default TFS map

function onSay(player, words, param)
    -- Retrieve the depot crate
    local depotItem = Tile(depotPos):getItemByType(ITEM_TYPE_DEPOT)

    if depotItem then
        -- Retrieve the depot chest that is inside the depot crate
        local container = player:getDepotChest(getDepotId(depotItem:getUniqueId()), true)

        -- Loop through player EQ slots
        for i = 1, 10, 1 do
            local item = player:getSlotItem(i)

            -- If the item exist, move it to the depot chest
            if item ~= nil then
                player:sendTextMessage(MESSAGE_STATUS_CONSOLE_BLUE, "Transferring item to depot: " .. item:getName())
                item:moveTo(container)
            end
        end
    end

    return false
end

1: Login with a God character (for convenience, to summon items)
2: DON'T OPEN DEPOT
3: Summon item, etc /i golden armor
4: Execute talkaction !depot
5: Log out, log in.
6: Item is gone.

If you go aginst step 2 and open the depot before or after transferring the item, the item is inside depot and all is working well.

I thought #2258 was related to this (transferring house items), but no. I am unable to reproduce #2258. After house and depot was configured properly (matching town ID above 0), it worked consistently. I tested using the /owner method tho, not sellhouse.

Edit: I tested with !sellhouse as well and it worked fine. Unable to reproduce any issues with it.

[DSpeichert]

As @EvulMastah suggested:
As a workaround (not quite a proper fix but does the job done), can we simply stamp player->setLastDepotId(depotId) whenever player:getDepotChest is called? That would be better for now that losing items.