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

feat(folia): added folia support #292

Merged
merged 2 commits into from
Feb 19, 2025
Merged

Conversation

xCodiq
Copy link

@xCodiq xCodiq commented Feb 17, 2025

No description provided.

@Jikoo
Copy link
Owner

Jikoo commented Feb 18, 2025

From the first request for Folia support:

I should also note so it's documented (in case anyone tries to make a hacky fork that "works"): The way OI works may require a lot more modification under the hood. Currently OI provides a view backed by the same in-memory contents of the player's inventory, which removes the need (in a normal server) to synchronize contents to ensure that inventories are up-to-date and prevent duplications/deletions. I have no idea how Folia handles inventories opened by multiple players in different regions, but actions would have to be synced to the region owning the player (if any, see note above about constructing a ServerPlayer being a complication) to prevent duplications/item corruption/general inventory functionality breakdown by concurrent modifications of the same backing contents list from different threads.

I see this PR does not introduce any mitigation for the above concern. How does Folia handle inventory modification synchronization from players on different threads?

@R00tB33rMan
Copy link

From the first request for Folia support:

I should also note so it's documented (in case anyone tries to make a hacky fork that "works"): The way OI works may require a lot more modification under the hood. Currently OI provides a view backed by the same in-memory contents of the player's inventory, which removes the need (in a normal server) to synchronize contents to ensure that inventories are up-to-date and prevent duplications/deletions. I have no idea how Folia handles inventories opened by multiple players in different regions, but actions would have to be synced to the region owning the player (if any, see note above about constructing a ServerPlayer being a complication) to prevent duplications/item corruption/general inventory functionality breakdown by concurrent modifications of the same backing contents list from different threads.

I see this PR does not introduce any mitigation for the above concern. How does Folia handle inventory modification synchronization from players on different threads?

From testing, I haven't found any discrepancies relative to removing or adding armor/tools to the player in or outside of the global region (nor if the player is in a separate region from the origin player) when experimenting with this build, even through my valiant attempts and with multiple accounts. So, from testing these changes directly, I couldn't really find anything that'd potentially break the synchronization of inventory contents.

@Jikoo
Copy link
Owner

Jikoo commented Feb 19, 2025

Gotcha. I'm still lightly concerned about it, but maybe the existing sync system moves the handling to the correct thread? I'm aware that there is some sync done in vanilla inventory operations, but I didn't think it was extensive enough to handle Folia-level async operations. I'll probably just put in a disclaimer about not having the capacity to test myself in the changelog.

@Jikoo Jikoo merged commit 0db487c into Jikoo:master Feb 19, 2025
2 checks passed
@R00tB33rMan
Copy link

Gotcha. I'm still lightly concerned about it, but maybe the existing sync system moves the handling to the correct thread? I'm aware that there is some sync done in vanilla inventory operations, but I didn't think it was extensive enough to handle Folia-level async operations. I'll probably just put in a disclaimer about not having the capacity to test myself in the changelog.

Of course and if issues arise, we can swiftly patch 'em. We just really couldn't find any with the amount of tested conducted thus far. Thank you for considering this PR!

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.

3 participants