Bedrock support

[CreeperG16]

This PR adds Bedrock Edition item support to the Item class.
Todo:

• Implement Stack ID in Item class This should be moved to prismarine-registry later
• Add support for older bedrock versions, which might have different formats
  • Add more features to features.json for bedrock in minecraft-data, this way testing if the registry is bedrock could be skipped entirely in some cases -> PrismarineJS/minecraft-data#696
• Items with different namespaces - mostly affects block names in CanPlaceOn and CanDestroy this can be addressed later if there are problems
• anvil - minor, could do later?
• toNotch and fromNotch functions
  • < 1.16.220
  • >= 1.16.220
  • canPlaceOn and canDestroy
  • stack ID
  • blockingTick
• Docs
• Types
• Tests
  • Handle Stack ID in PC tests
    • Temporary solution: setting the same stack ID
    • Better solution: Stack ID is null if the version doesn't support Stack IDs (right now just test if bedrock, but could be added as an mcdata feature maybe?)
    • LT solution: use Item.equal() in tests - need to implement NBT equal function to work properly
  • Bedrock tests

[socket-security]

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

[CreeperG16]

    // TODO:
    // - tests
    // - docs
    // - add support for older versions:
    //   need to see how items are handled in older bedrock protocol versions,
    //   and probably update features.json in minecraft-data with useful data
    // - Stack ID generation
    // - Block runtime ID field
    // - Setting canPlaceOn and canDestroy with block/item objects?
    // - Merge with Item class in ../index.js?

I'm probably forgetting some things

[CreeperG16]

Just realized that I missed #85 completely
Would it be better to close this and continue #85? Or would that complicate things even more? Sorry, I'm still a bit new to GitHub

[extremeheat]

Remember the API needs to be the same as the pc implementation

[extremeheat]

Since everything seems mostly identical to pc, with the exception of import/export functions, so one class seems to make sense if enchantments are the same (minus integer vs string IDs). Are they roughly the same?

[CreeperG16]

other than the stack ID stuff, yes, they seem to be pretty much the same

[extremeheat]

they seem to be pretty much the same

Very different from what I see in the current PR. Base class and extension for pc/bedrock function implementation may make sense, or separate

[CreeperG16]

What I've noticed is a lot of things could already work with the java code if the same features were there in bedrock's features.json in minecraft-data. The spawn egg mob names for example, the getter would need no modification if the features were in minecraft-data
*no modification for versions above 1.16.100, I'm not sure about below, but it's probably similar to the older java system with metadata (obviously would need more research)

[extremeheat]

bedrock's features.json in minecraft-data. The spawn egg mob names for example, the getter would need no modification if the features were in minecraft-data

Right, you can open a PR to add it to the features

[extremeheat]

Remove all the unnecessary stackId defining and ?? operations, including from the tests. The only time it should be specified in the tests is making sure there is equality. and if so it should be set to null.

[extremeheat]

What's the status of this? Stale PRs are not good as they will accumulate merge conflicts over time

[CreeperG16]

sorry, haven't had much time the past few weeks but should have some now... The prismarine-registry PR is pretty much done (just need to do some testing) so there's just this left. I might be able to finish it this weekend next week but I'll see what I need to fix

[CreeperG16]

Only anvil left, hopefully anvil usage doesn't differ very much from java to bedrock.

[rom1504]

I think it would be nice to get this in

1. how do we check it works for your purpose in bedrock ?
2. how do we check it doesn't break pc ?

anvil support is minor

[extremeheat]

LGTM, left some comments on the current code. For the anvil handling code, maybe we can do that in a separate PR and leave the Item.anvil methods as undefined for bedrock for now

[extremeheat]

I'm thinking this should return a [block name, properties] tuple instead of raw block strings for future proofing. It does seem odd that the matching is just done by block name here. But I'm not sure if we should block on that.

[extremeheat]

Returning a prismarine-block isn't what I meant. I meant returning properties associated with a block. Like stone[variant=andersite] --> [["stone", {"variant":"andersite"]] as opposed to just ["stone"].

Since we don't actually have the prop data as a discriminator for these fields, returning undefined as a second param for now makes sense to me for now [["stone"]].

[CreeperG16]

ah, ok

[CreeperG16]

Not sure how to type/document it, if it never returns those properties anyway as there isn't a way of getting them, should they be documented to return those properties at all?

[rom1504]

Hey, does it work ?

Does it break PC? Will it help for bedrock?

[CreeperG16]

It works as far as I've tested, and I've written bedrock tests for it too. PC tests are also working fine, but I haven't tried PC much outside of that, though I made sure to keep the original functionality intact. As for helping with bedrock, apart from anvil, it implements all methods and properties PC has.

[rom1504]

/makerelease 1.13.0