Auto generate BlockTypeIds & ItemTypeIds #6313
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I realized not long after 5.0.0 release that we could simply use incrementing IDs in VanillaBlocks/VanillaItems (in the same way that plugins have to), and just generate constants for BlockTypeIds/ItemTypeIds from there. This reduces the number of manual needed to implement new blocks and items, since the autogen can be automated when modifying VanillaBlocks/VanillaItems. Longer term, we should probably explore getting rid of these IDs altogether. I added them to ensure we had a way to compare item types without cloning items from VanillaBlocks/VanillaItems, but in practice this is likely just premature optimization thinking carried forwards from the old ID system, as pointed out by @jasonw4331. For the duration of PM5 support, however, this will now have to be maintained, as we can't remove them before PM6.
this makes vanillaBlocks less ugly, and also removes a manual step when adding new blocks.
dktapps
added
Category: Core
this stops the entire file from changing when adding new blocks.
the diff will instead look something like this:
@@ -713,7 +722,8 @@ final class BlockTypeIds{
public const CHERRY_DOOR = self::CHERRY_STAIRS + 1;
public const CHERRY_BUTTON = self::CHERRY_DOOR + 1;
public const CHERRY_PRESSURE_PLATE = self::CHERRY_BUTTON + 1;
- public const CHERRY_SIGN = self::CHERRY_PRESSURE_PLATE + 1;
+ public const CHERRY_TRAPDOOR = self::CHERRY_PRESSURE_PLATE + 1;
+ public const CHERRY_SIGN = self::CHERRY_TRAPDOOR + 1;
public const CHERRY_WALL_SIGN = self::CHERRY_SIGN + 1;
public const CAULDRON = self::CHERRY_WALL_SIGN + 1;
public const WATER_CAULDRON = self::CAULDRON + 1;
dktapps
added
Category: Dev Workflow
we don't actually care about the specific values, only whether all the blocks and their states have been correctly registered. I'd prefer to track all of the state data permutations, but the APIs for that are private, so tracking the number of permutations will have to suffice (this should be good enough to detect bugs anyway, and also takes way less space).
we can't rely on BlockTypeIds to look up the ID name if the type IDs aren't consistent to begin with... I don't like using VanillaBlocks for this (we might have multiple source registries in the future, e.g. if we decide to pull the education edition blocks into their own registry), but it doesn't seem like we have a choice. For now this is valid anyway.
|
Only remaining issue is how to avoid wacky runtime bugs when type IDs get changed due to adding new things PHPUnit tests will ensure that we don't push broken type IDs to a release, but it could still cause some WTFs in local testing and I'm not sure what should be done to avoid that |
in the longer term we really need to find a better way to define these, but without breaking BC, this is the best we can do.
technically there's no reason why the first reserved ID can't be used directly, but allowing this would require special conditions in the unit tests, so this fix is easier.
|
It seems that longer term, it would make more sense to use strings for type IDs instead of integers. The names used for We'd more or less end up with the same ID system that Minecraft has natively (however, we wouldn't be able to use This would have a few benefits
Downsides:
We would have to dynamically compress these string IDs to integers at runtime for storing on chunks, but that would be far cleaner than the clusterfuck we currently have. However, with all that said, we won't be able to do a change like this until PM6 due to the BC breaks required. |
auto incrementing IDs are a big pain and don't offer any obvious advantages. In PM6, it's looking like using strings for type IDs directly will be the logical course of action However, we can still get some benefit in PM5 from breaking the hard cycle here, and this makes it a bit less inconvenient to implement new things.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction
Currently, implementing new items requires manual changes to both
VanillaItemsandItemTypeIds.The same is true for
VanillaBlocksandBlockTypeIds.This PR removes
VanillaItemsdependency on theItemTypeIds::*constants andVanillaBlocksonBlockTypeIds::*constants.This permits the type IDs to be defined by
VanillaItemsandVanillaBlocksdynamically, and then regeneratingBlockTypeIdsandItemTypeIdsis just an afterthought.This change also makes the code of registering new blocks & items far less obnoxious to read and write, since the information is repeated less times, and the code isn't as wide.
Problems
Adding a new block or item anywhere except right at the end of the
setup()function will cause type IDs of existing blocks & items to change. This causes a few issues:Messy diffs (resolved by making const values relative to previous)block_factory_consistency_checkcan't properly detect changes anymore (needs to be rethought or possibly deleted)I haven't figured out how to deal with these issues yet. Perhaps the type IDs could be derived from the block type name or something of that nature (but this would be a bit more complicated to implement).
Despite these issues, I think this change is probably still worth making, for the reduction in tedious bullshit when implementing new blocks & items.
Relevant issues
Partially addresses #6311
Changes
API changes
None
Behavioural changes
None
Backwards compatibility
Values of type ID constants have changed. Plugins should not be storing these (as per the giant warning at the top of the files), but this will no doubt catch someone off guard anyway.
Follow-up
Consider if we can get rid of the type ID constants entirely. Perhaps redesigning
VanillaBlocksmight provide a way to do this.I've considered the possibility of using enum objects instead of type IDs, which would have various benefits.
Tests
CI tests are sufficient