valheimPlus / ValheimPlus

A HarmonyX Mod aimed at improving the gameplay and quality of life of the game Valheim.
http://valheim.plus
GNU Affero General Public License v3.0
967 stars 236 forks source link

Refactoring container interactions to avoid overwriting adds/removes #786

Closed Grantapher closed 2 months ago

Grantapher commented 1 year ago

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.

MSchmoecker commented 1 year ago

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.

Grantapher commented 1 year ago

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

Grantapher commented 1 year ago

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.