Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring container interactions to avoid overwriting adds/removes #786

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

Grantapher
Copy link

@Grantapher Grantapher commented Feb 19, 2023

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
Copy link
Contributor

MSchmoecker commented Feb 19, 2023

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
Copy link
Author

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

@Grantapher
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants