Import extra region folders into slime format

[vjh0107]

Plugins like CraftEngine keep their per chunk data in a separate region folder inside the world
directory, such as /craftengine. The anvil reader only reads /region and /entities,
so that data is lost on import.

Added:

• /swm import <path> <data-source> [name] --extra-region craftengine
• java -jar asp-importer.jar <world> --extra-region craftengine

[davidmayr]

Thank you for your PR. But, I really don't know what to think of this tbh. This sets a lot of expectations that I personally think are sort of hard to follow. Although this is in theory quite abstract, it's probably too opinionated for any widespread use. This comment is either a no nor a yes. I think this one might need some discussion. I'll ask around what others think of this.

The first major issue I see here is that it expects plugins to follow the .mca file format. Considering plugins are free to do whatever they please, this on its own probably only holds true for a handful of plugins.

Its second limiter is that it expects plugins to follow the same format when using slime as they do in vanilla worlds. It also expects said data to just be stored under a key with e.g. the plugin name. That's a reasonable assumption, yet again very implementation dependant.

However, I can personally see the value of the existence of this within the slime plugin.

Whilst for the API itself, its (in my opinion) a risk for API clutter and a maintenance burden in the future (not as in effort, but as in: ugh, we still have that method and people might use it, what do we do with it). I think any plugin developer, especially someone already integrating the MCA format into their plugin is capable of just adding this on their own after the import world call. Read the vanilla world, read your MCA files and add the extra data. Might be a little more effort for them, but gives them full control. If they are calling import world manually anyway, they'd have to specify all the possible import options so they might as well just implement that code manually.

Yet on the plugin front, that's not that easy. Whilst I do think the ASP Plugin shouldn't be used in production environments, I can see the value in users using such feature for compatible plugins.

The question now is rather, what plugins can actually benefit from this? Only Craftengine? What other plugins have their data in the world folder and in an MCA region file? And more specifically, for which plugins would the specific key → world data actually work 1:1?

Whilst writing this, I have also thought about plugins being able to register import helpers for these kinds of things, yet the API is intended to be stateless (unlike the old 2x API). So theoretically we would have to make this plugin only, and we would need an additional plugin API (next to the existing server bundled API), which would be an extra step for us, an extra step for devs and probably not a good experience either.

Then again, we could maybe exploit the server Event API for this and make importing Anvil Worlds an Event where plugins can inject data. This would probably solve this for every usecase, yet it might cause issues of plugins expecting other plugins not to intervine with their world imports as the API is probably expected to be consistant between calls (but maybe I'm overcomplicating this) :/

I do understand the lack of such feature as a problem. The question is if this approach is the right one, if this should be plugin exclusive or for the whole API, if there are other options entirely or if this should be done on our end in the first place. Dont jump to conclusions just because of this comment, this is just my input on this for now.

[vjh0107]

Those are fair points, and "which other plugins would benefit" is the right question to ask. I went looking, and the honest answer is that CraftEngine is the only implementation I can verify. So I would rather turn this into a question than defend the patch as it stands.

The PR makes two assumptions, and I think only one of them is new.

The slime side already exists. Per-chunk extra data is where plugin data goes today: ASP itself stores the chunk PDC there as ChunkBukkitValues, on import in AnvilWorldReader#convertChunk and at runtime in NMSSlimeChunk#updatePersistentDataContainer, and SlimeChunk#getExtraData documents it. CraftEngine reads its own key out of the same map:
https://github.com/Xiao-MoMi/craft-engine/blob/79eba6084ba46ba77ce5fd8dbb7d2fe80bae560e/bukkit/compatibility/src/main/java/net/momirealms/craftengine/bukkit/compatibility/slimeworld/SlimeWorldDataStorage.java#L64
Neither the storage slot nor "the importer copies a known key into extra data" is something this PR introduces.

The anvil side is the new part. "Per-chunk data living in //r.x.z.mca belongs under extra data key " is a convention that exists nowhere. CraftEngine happens to match it. A flag does not solve that; it just moves my assumption into your API, which I agree is exactly the clutter you are describing.

So the question I would rather ask is whether this is a convention ASP wants to define.

If it is, the change gets smaller instead of bigger. No flag, no API addition, no plugin name in the code:

• the importer detects region-shaped folders at the top level of the world directory
• region, entities and poi keep their dedicated handling
• anything else is preserved under its folder name, with the payload never parsed
• SLIME_FORMAT documents the contract, so it becomes a target rather than something CraftEngine happens to do

If it is not, that is a reasonable call and I will keep the importer out of tree. I can also look into the import event you suggested, though it would not cover the standalone importer, where no plugins are loaded. Either way, I would rather hear which direction you consider right before writing more code than guess.