ValheimPlus icon indicating copy to clipboard operation
ValheimPlus copied to clipboard

Refactoring container interactions to avoid overwriting adds/removes

Open Grantapher opened this issue 2 years ago • 3 comments

Updates any container modification to check is the container is in use by a player. If it is, then the container is ineligible for modifications. If it is not, it is set to "in use" while the mod changes the contents, and then sets it back to not in use after.

These changes aren't very well tested, I did verify that chests being used by players are skipped, but I couldn't reproduce a player being locked out of a chest while the mod changes it (since the time window is so small).

This is mostly an example of what I was thinking in my comment on #675 to avoid players putting items into a chest, only for them to be overwritten by the mod also depositing items.

Grantapher avatar Feb 19 '23 00:02 Grantapher

Container.IsInUse() is not synced in multiplayer, only for the owner of the container this check can ever be true.

The most reliable way is to check for ownership with Container.IsOwner() and only access if if this is the case. I also discourage to use Container.m_nview.ClaimOwnership(), since this can potentially delete/duplicate items when another player interacts with the chest, even if additional checks are made on the non-owner side. One of the reliable and not too complex solutions is to ask for ownership, like vanilla does with the RequestOpen RPC. There will be a short dely between request and response but the auto deposite doesn't have to be instant anyway.

MSchmoecker avatar Feb 19 '23 11:02 MSchmoecker

Thanks for the info, I'll re-evaluate the code.

Grantapher avatar Feb 19 '23 20:02 Grantapher

Is the server ever the owner? Or is it always a player who is the owner? Either way, it seems like adding IsOwner() checks may be sufficient, but I have a feeling it is more complicated than that.

Grantapher avatar Feb 19 '23 21:02 Grantapher