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

Implementing new blocks is a pain in the ass #6311

Open
dktapps opened this issue Mar 29, 2024 · 1 comment
Open

Implementing new blocks is a pain in the ass #6311

dktapps opened this issue Mar 29, 2024 · 1 comment
Labels
BC break Breaks API compatibility Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP

Comments

@dktapps
Copy link
Member

dktapps commented Mar 29, 2024

Description

The current design of the blocks system is currently disastrously complex and painful to maintain, and needs to be redesigned.

Many people have criticized the system (for the wrong reasons), but those people weren't entirely wrong. The current system is an awful mess, and it's not one of my finer works.

Justification

The current system is awful to work with.

  • Information of how to translate NBT into PM internal structures is repeated twice per block
  • We have to edit minimum 4 enormous files to add a new block (and that's before thinking about StringToItemParser et al)
  • We end up with giant repeated blocks of crap like this because there's no way to dynamically (e.g. fetch a certain type of wood block)
  • We end up with giant repeated blocks of crap like this because we have no way to extract information from the ID, even when the PM internals support dynamically setting said information (e.g. color)
  • Implementing a new block also requires manually declaring a new type ID in BlockTypeIds, adding it to VanillaBlocks, and adding it to StringToItemParser

It ended up this way because it was being designed all the way back in 2019, and took 3 years to come to fruition. I attempted to solve several other problems at the same time (such as the inability to provide dynamic APIs for stuff like glazed terracotta due to the 1:1 linking of block type <-> minecraft block id, the heavy dependence on minecraft internal meta). While this redesign was partially successful (e.g. runtime state data is now far easier to work with than in prior versions), it's undeniable that the current system is not nice for maintenance or extension.

For what it's worth, I knew that the system I started to build out in 2019 was going to be a hunk of shit, but I pushed through with it anyway once LB got behind me, because it needed to be done, and I knew I wouldn't be able to see how better to do it until making the first version.

What we gained from the new system:

  • Internal system no longer directly depends on Mojang's awful mess of a block system, allowing us to implement nicer APIs (e.g. GlazedTerracotta->setColor())
  • We don't have to break BC when Mojang keep changing the block IDs one by one each month (for the love of god, please just get it over and done with)
  • Internal block property storage can now handle extra non-vanilla things, such as the recent optimisation for farmland
  • Plugins can't hardcode random numbers all over their code and make it a pain to read
  • Block classes are cleaner due to not being loaded up with serialization crap, allowing them to focus on actual gameplay logic

Design constraints which turned out to be unnecessary:

  • Blocks to have deserializers without serializers - I thought this would be useful for upgrading old data, but it turned out that it was far easier to build out BedrockBlockUpgradeSchema and the systems around it
  • Be able to implement layered deserializers to support many different versions' blockstates without polluting the Block classes to kingdom come - again, it turned out to make far more sense to maintain latest only and use BedrockBlockUpgradeSchema to deal with the upgrading step
  • Easy (?) to extend a system like this to support Java blockstates without having to make any internal changes - in practice it makes more sense to just translate the blockstates at the data layer using Geyser mappings or something

What we lost:

  • New blocks need changes in several giant files instead of one
  • It's easy to accidentally forget to do something like putting a new block/item in StringToItemParser making it unavailable to commands
  • Plugins now have to do more work to register new things (although this can be wrapped into a simple method)
  • Comparing blocks is a bigger pain now (needs instanceof plus various block-specific APIs, e.g. getColor())
  • Conflicts between PRs adding new BlockTypeIds

Forward steps

The following is a non-exhaustive list of stuff that could be done to improve upon this mess:

  • Provide central APIs for registering blocks
  • Have BlockTypeIds auto-generated, or perhaps get rid of it altogether - perhaps a VanillaBlocks redesign that allows fetching the ID without cloning the block is a good idea? - sadly this means even more BC breaks
  • Improve block APIs to allow unified serializer/deserializer designs similar to ItemSerializerDeserializerRegistrar (e.g. getColorWrapper() : Reference or something of this nature - essentially allowing pairing of internal properties with NBT properties, since this what we need 95% of the time) - this would allow deduplicating most of the logic in the serializers & deserializers
  • Develop a system to allow pattern-matching IDs like colored blocks so the color can be extracted from the ID and dynamically set
  • Develop a way to e.g. get a Planks block with a certain WoodType (WoodType must stay immutable, so this is a bit more difficult than colored stuff)

Alternative methods

@dktapps dktapps added Category: API Related to the plugin API Category: Core Related to internal functionality BC break Breaks API compatibility Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Mar 29, 2024
@Bqleine
Copy link

Bqleine commented Mar 30, 2024

After programming the Structure Void and Structure block as an inexperienced contributor, I agree with what you said but I'd like to add that going forwards, making a documentation file on how to add a block would be much friendlier towards contributors and make it easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Breaks API compatibility Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

No branches or pull requests

2 participants