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

Use reflection to locate BlockTypeIds and ItemTypeIds for VanillaBlocks/VanillaItems #6498

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

dktapps
Copy link
Member

@dktapps dktapps commented Nov 12, 2024

Instead of explicitly specifying type ID for each entry in-line, this change allows the type ID to be located via reflection using the registry entry's name.

Does this mean you're coming back?

Nope! I'm just wrapping up some of the stuff I was working on before I stepped down, since I'm the only one who understands what the point of most of this stuff was.

Justification

Since BlockTypeIds and ItemTypeIds are derived from VanillaBlocks and VanillaItems respectively anyway (they only exist to allow identifying blocks/items without having to create instances of them), this hack is probably OK, and reduces the chances of mistakes. Previously it was explored to have these IDs generated by auto-incrementing in VanillaBlocks/Items and have the constants generated that way, but this proved to be too problematic because of unstable diffs no matter how we chose to sort the elements. See #6313 for previous research on the subject.

In summary, this hack:

  • Reduces the chances of mistakes (e.g. using the wrong type ID constant)
  • Reduces obstacles for quick & dirty local testing of new features
  • Makes the code way less wide & more readable
  • Allows deleting a whole bunch of shitcode related to wood-like block handling

Relevant issues

Changes

API changes

No plugin-facing API changes.

However, the code for registering blocks in VanillaBlocks & VanillaItems is now different.
In addition, there is a new requirement that every VanillaBlock & VanillaItem must have a corresponding TypeId constant with an exactly-matching name. Previously, this wasn't technically required due to the ID being explicitly specified.

Behavioural changes

Type IDs are now located using reflection and the registry entry name for VanillaBlocks & VanillaItems.
As written above, this is less prone to mistakes.

In addition, it is now possible to register new things in VanillaBlocks & VanillaItems without defining a new type ID constant. This is intended to ease the testing workflow by reducing the amount of technical bullshit needed to get from "hacking code together" to "playtesting server".
Note that the unit tests will still fail if the constants are not properly defined; however, this will no longer make it impossible to run the server.

Backwards compatibility

Follow-up

This is obviously not a desirable hack to keep long-term. In the future it will probably make sense to redesign VanillaBlocks like so:

enum VanillaBlocks { ... }
VanillaBlocks::STONE; // (the type ID)
VanillaBlocks::STONE->new(); // (to create a block)

This would allow getting rid of BlockTypeIds and ItemTypeIds entirely, but poses some new challenges.
More research is needed on this. It might cause complications for inter-thread communication, and also might annoy plugin devs as creating a block would become more verbose. There's also the question of what to do about stuff like wood-like blocks where the cases are dynamically defined in a loop.

Tests

I tested this PR by doing the following (tick all that apply):

  • Writing PHPUnit tests (commit these in the tests/phpunit folder)
  • Playtesting using a Minecraft client (provide screenshots or a video)
  • Writing a test plugin (provide the code and sample output)
  • Other (provide details)

…ks/VanillaItems

since BlockTypeIds and ItemTypeIds are derived from VanillaBlocks and VanillaItems respectively anyway (they only exist to allow identifying blocks/items without having to create instances of them), this hack is probably OK, and reduces the chances of mistakes.
Previously it was explored to have these IDs generated by auto-incrementing in VanillaBlocks/Items and have the constants generated that way, but this proved to be too problematic because of unstable diffs no matter how we chose to sort the elements. See #6313 for previous research on the subject.

This is obviously not a desirable hack to keep long-term. In the future it will probably make sense to redesign VanillaBlocks like so:

enum VanillaBlocks { ... }
VanillaBlocks::STONE (the type ID)
VanillaBlocks::STONE->new() (to create a block)

However, more research is needed on this, as I'd prefer not to make block creation any more verbose.
@dktapps dktapps requested a review from a team November 12, 2024 16:56
github-actions[bot]
github-actions bot previously approved these changes Nov 12, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 12, 2024
@dktapps dktapps added Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Nov 12, 2024
github-actions[bot]
github-actions bot previously approved these changes Nov 14, 2024
@dktapps dktapps merged commit 33a7b46 into minor-next Nov 14, 2024
29 checks passed
@dktapps dktapps deleted the reflect-link-type-ids branch November 14, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants