Draft: Feature/replace blockstorage

[test137E29B]

All reviews and updates from others welcome on the branch - feel free to open a PR targeting the branch in my repo if you'd like to contribute to this change. It's a big one.

Changes Checklist

• Update BlockStorage to use BlockDataService internally
• Update BlockDataService to use CustomBlockData third party lib for storing all data (not just TileEntity data)
• Update Inventory storage to store on the BlockDataService with custom serializer, also storing the preset
• Use static keys for inventory preset and prefix slot identifiers to prevent "0" being a key incase it gets used elsewhere.
• Update geo scanning / chunk resource scanning to store in ChunkDataService
• Create ChunkDataService to handle data storage on Chunks, and use BlockStorage as a wrapper/helper for simplicity in access
• Convert Universal Inventories if possible
• Conversion script on load of BlockStorage if LegacyBlockStorage files exist, moving these to a backup folder so they can be restored if something goes wrong
• Loading of data where required. There are problems noted in TODO's here - mainly potentially needing to load chunks to get CustomData blocks. Probably requires a good bit of testing to ensure performance isn't horrible and chunks are not perm loaded if they are required to be loaded in the first place.

Description

BlockStorage causes a lot of problems. Not great performancewise, and can desync stored data so blocks convert to their vanilla counterparts. This is a much needed overhaul, though it will require a lot of testing and reviewing.

Proposed changes

Proposing to use PersistentDataContainer - directly for Chunks to store their Geo-Scan resource status, but also using https://github.com/mfnalex/CustomBlockData for storing data in ALL blocks, not just tile entities. Internally it works by storing the data on the chunk if it has to. This is a much better and more consistent way of storing data.

Related Issues (if applicable)

If we listed issues we'd be here all day.

Checklist

• I have fully tested the proposed changes and promise that they will not break everything into chaos.
• I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
• I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
• I followed the existing code standards and didn't mess up the formatting.
• I did my best to add documentation to any public classes or methods I added.
• I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
• I added sufficient Unit Tests to cover my code.

[github-actions]

Your Pull Request was automatically labelled as: "🎈 Feature"
Thank you for contributing to this project! ❤️

[test137E29B]

This will likely be removed since LegacyBlockStorage wont be needed at all.

[J3fftw1]

A worry I have with this a chunk can hold x amount of data before it resets Minecraft has never solved it only increased the limit. Won’t we run into this problem?

[test137E29B]

This probably needs to be the shaded dependency

[test137E29B]

Should probably be shadowed import, not the original? Who knows, Java is strange 🤷

[test137E29B]

Convert to use BlockStorage.addBlockInfo instead of directly accessing this in such a horrible way

[test137E29B]

same here

[test137E29B]

Clear with BlockStorage class directly, dont access CustomBlockData so directly

[test137E29B]

FYI You can literally convert anything with this exact same Class template - just update the ItemStack type to whatever the data type is... crazy

[test137E29B]

There's no global key anymore.... so this test is kinda impossible. Maybe there's a better way - you're a smart reviewer, you tell me how to do this cause I'm an idiot :))

[test137E29B]

A worry I have with this a chunk can hold x amount of data before it resets Minecraft has never solved it only increased the limit. Won’t we run into this problem?

@J3fftw1 This is also something I'm a bit weary of. Individual blocks storing their small state here like power should be okay, but my worry is blocks with inventory storage - a whole inventory, even if byte[] is much larger than a single integer. I guess only one way to find out by doing it and then figuring out how much stuff we can fit into a chunk before Paper complains it's >16MB.

I think too one of the problems with BlockStorage is everything is stored as a string. PersistentData stuff is much more powerful and can support serialising basically anything to a byte[] and deserialising it back to the original instance without loss (see ItemStackSerializer that's been added).

Absolutely open to all thoughts and suggestions on this.

[test137E29B]

bro pls

you can just commit them all at once why are you reviewing your own code xD

@SchnTgaiSpock Viewing it in GitHub file change view makes it a bit easier to review it "separately" in my mind. I'm also aware the linter in my IDE has decided imports no longer need to be in blocks, which is annoying and I would not notice without the file change view here. 🤷 You don't have to look at the PR, come back after a week when I have my shit together :))

[SchnTgaiSpock]

@SchnTgaiSpock Viewing it in GitHub file change view makes it a bit easier to review it "separately" in my mind. I'm also aware the linter in my IDE has decided imports no longer need to be in blocks, which is annoying and I would not notice without the file change view here. 🤷 You don't have to look at the PR, come back after a week when I have my shit together :))

yeah ok and you pinging me just resubscribed me :))

[ybw0014]

so many reviews why not using formatter in IDEA

[WalshyDev]

Hey,

Thank you so much for putting this up and taking the time to do this!

This is an area I've been thinking about a lot lately, I like the general idea but am not sure about the execution. I also want to make sure we start on something smaller first, BS is a huge huge thing (as your PR shows) with a very large consequence when things go wrong.

I recommend reading my doc here for my plan: https://github.com/WalshyDev/Slimefun4/blob/e491dda6e5397e5d70366ebdb5eba4082f8c1ffe/docs/adr/0001-storage-layer.md
(The API proposed is a very quick think and will 100% change, I know for a fact it already doesn't work for what we need - the goal is to continue to update and work on it till we get it into a good place, I don't expect us to have a refined API documented before we actually implement it and are comfortable with it -- there's just far too much to consider within Slimefun and the ecosystem)

I'd absolutely love your feedback and general help on this, it very much needs to be a collaborative effort and I really appreciate the work put in here. I want to make sure it isn't wasted but that we do it the right way.

If you aren't already, I'd recommend joining the Discord, discussion is best done real-time there - https://discord.gg/slimefun

[Seggan]

TBH I'm not really comfortable with using a third-party lib to handle the storage of data, we don't have as much control over the API/etc in that case.

[test137E29B]

TBH I'm not really comfortable with using a third-party lib to handle the storage of data, we don't have as much control over the API/etc in that case.

You could write exactly the same thing again as a Slimefun data storage version if you wanted.... but then again why use built in MC data storage since you don't have control over it's API etc. Seems like a bit of a stupid argument imo. Maybe coming from NodeJS background to me it's more normal to use other dependencies - something like this though I think is perfectly fine to use.

[JustAHuman-xD]

Seems like a bit of a stupid argument imo.

Definitely not a stupid argument....? It's a valid point regardless of the context. And kinda disrespectful to call it stupid even if it wasn't one. Using any APIs not controlled by ourselves leaves us vulnerable to a lot more breaking changes, unwanted behavior, lack of control over updates, etc etc.

We don't know what all we want this api to do which is why Walshy is trying to have those discussions. More than likely there won't be any existing APIs that will fit our needs perfectly or better than something we could create ourselves.

This is something that needs to be done collaboratively and the best way to do that is not with a pre-existing dependency, it's with one made by those collaborating.

[test137E29B]

As Per Discord, this isn't how contributors want to implement a fixed version of BlockStorage, so not much I can do to help out. Storing block data in SQL is extremely dumb though, and if you do it, I guarantee you're not going to solve any problems - in fact you might even cause more. I eagerly await the updated BlockStorage and hope it fixes the issues I have to deal with on servers I'm playing on.