Folia Support #2848
Folia Support #2848
Conversation
This is a thoroughly tested version of WorldEdit against Folia. Not only does it introduce the fundamentals that other forks have offered, but it also provides the ability to utilize `-e` tags, butchering entities, and even the `//regen` command on Spigot, Paper, and Folia alike. That said, this pull request is ready for review, so critical changes (if needed) can be made to ensure Folia is sufficient and prepared for WorldEdit upstream. This was also done for FastAsyncWorldEdit: IntellectualSites/FastAsyncWorldEdit#3363
|
Converted into a draft as FAWE's code has been reworked due to external requirements that will be applied here once FAWE determines their preferred codestyle and approach. |
me4502
left a comment
me4502
left a comment
There was a problem hiding this comment.
I understand that you're reworking based on the feedback from FAWE (and I agree with most of the feedback you received there)- just thought I'd add a few notes to help with the reworking.
|
|
||
| @Override | ||
| public boolean regenerate(World bukkitWorld, Region region, Extent extent, RegenOptions options) { | ||
| if (FoliaScheduler.isFolia()) { |
There was a problem hiding this comment.
IMO the regen code here is substantially more complicated than it makes sense to support (given the very low Folia user counts); I'd personally rather at least for now have this throw an unsupported error, and then possibly in the future looking into adding this in a simpler way with less maintenance burden.
There was a problem hiding this comment.
Appreciate the kind responses here! We've actually created a much more simplified approach that'd likely also reduce the # of code changes needed. Once we properly implement everything on FAWE, I'll go ahead and shift all of these changes to WE in an appropriate way.
| int cx = loc.getBlockX() >> 4; | ||
| int cz = loc.getBlockZ() >> 4; | ||
| if (FoliaScheduler.isFolia()) { | ||
| if (Bukkit.isOwnedByCurrentRegion(loc.getWorld(), cx, cz)) { |
There was a problem hiding this comment.
I do notice a lack of handling non-current-region blocks/entities in this code, what exactly happens if someone were to make an edit in a region other than the one they're in, or across two regions?
IMO supporting it isn't necessary, but it'd be better to have some actual proper handling rather than some things silently failing (or causing errors, if that's the case)
There was a problem hiding this comment.
I entirely agree. I haven't ran into issues during testing but now that you mention this, I'll review this closely and will make changes if needed under such conditions
|
|
||
| @Override | ||
| public boolean trySetPosition(Vector3 pos, float pitch, float yaw) { | ||
| if (PaperLib.isPaper()) { |
There was a problem hiding this comment.
While I understand having alternate async behaviour on Folia, the current API constraints imply it's an immediate teleport and the caller will receive the result of it. I don't feel breaking that constraint for all Paper users makes sense, any of this odd alternate behaviour should be for Folia users only
There was a problem hiding this comment.
Would you be more inclined to us having:
- Folia executes this same operation
- Paper executes teleportAsync but without a scheduler
- Spigot of course runs standard teleport
or
- Folia executes this same operation
- Paper + Spigot runs standard teleport?
I feel like in regards to performance, allowing Paper to use flat/standard teleportAsync could be favorable, since I don't see disparties with its raw use on Paper
There was a problem hiding this comment.
The return value is important, so doing it sync is necessary. Paper shouldn't change because it may affect other plugins that use our APIs.
| actor.printInfo(TranslatableComponent.of("worldedit.butcher.killed", TextComponent.of(total), TextComponent.of(finalRadius)))); | ||
|
|
||
| return killed; | ||
| return 0; |
There was a problem hiding this comment.
We shouldn't be limiting behaviour of WorldEdit on all platforms purely for Folia here- if it's not possible for something to work on Folia without breaking behaviour on other platforms, IMO it should be an unsupported operation on Folia.
There was a problem hiding this comment.
I'm thinking the best approach here is to ensure all platforms are supported whilst ensuring the alternate Folia-based implementation is appropriated. I feel like if we're supporting Folia and can support all available commands (in a cohesive, non-janky way), it's worthwhile. We'll look into introducing this change in a way that better suites the project
|
I was wondering, to avoid making a lot of changes, wouldn't it be worthwhile to create a worldedit-folia module similar to worldedit-bukkit? |
Imo from a long term maintenance perspective, a separate module would be substantially worse- it's better to keep it in the bukkit module with as minimal changes as possible |
4 participants
This is a thoroughly tested version of WorldEdit against Folia. Not only does it introduce the fundamentals that other forks have offered, but it also provides the ability to utilize
-etags, butchering entities, and even the//regencommand on Spigot, Paper, and Folia alike. That said, this pull request is ready for review, so critical changes (if needed) can be made to ensure Folia is sufficient and prepared for WorldEdit upstream.Supersedes: #2379
Resolves: #2348
This was also done for FastAsyncWorldEdit: IntellectualSites/FastAsyncWorldEdit#3363