-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add v0 support to the decoder #230
base: master
Are you sure you want to change the base?
Conversation
Potentially needs some refactoring
Yes, like https://github.com/PrismarineJS/minecraft-data/blob/master/data/pc/common/legacy.json that can be added for bedrock in the common folder. I'd recommend using a script to generate a static mapping of the old ID + meta to a block state string like that pc schema and then calling Block.fromString() to get the .stateID from the block state string. |
This pull request will now be reliant on PrismarineJS/minecraft-data#755 getting accepted |
OK, an accompanying update would be necessary in node-minecraft-data to expose that data. In the meantime you can add the file to this repo inplace of the current JSON so we can see if tests are passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing to keep in mind here doing translations for legacy pre-flattening data to new data is that Block.fromString/fromProperties will be looking up blocks against the current block palette, not 1.3/1.4. There have been some block renames since then, and those translations would fail. Are you aware of that?
Using
I am aware of that, though I don't have enough experience with this library to figure out a fix. |
Indeed, prismarine-block would need to be updated to handle this. Likely instead of branching on registry type it can check if the string includes
OK, since this seems like a broader issue for consideration we can handle that after this PR. |
So after I fix the failing test would this be ready for a merge? Or does something else need to be done first? |
First you would need to expose the added data to minecraft-data inside node-minecraft-data, per my comment in PrismarineJS/minecraft-data#755 (comment). Otherwise this data can't be used here. Then you can open a PR to prismarine-block to fix Block.fromString to fix syntax support per my comment above. Then we can move this along. |
Your comment was unclear, can you clarify a bit more? |
As you mention in prismarine-block
I was saying this should be fixed inside prismarine-block so fromString does work for this use case. Instead of branching here on if (version.type === 'pc') {
// ...
} else if (version.type === bedrock) {
// ...
} I was thinking to branch by deducing if the string is in one syntax or the other. Like if (str.includes('["')) {
// other syntax
} else {
// normal other schema
} |
minecraft-data/node-minecraft-data was merged, so seems like this is only blocked by the prismarine-block at the moment. |
@Flonja would you like to finish this? |
So I was trying to finish the PR just now, but realized that there isn't an updated version for |
This is by no means final.
I'm guessing the
block_legacy_id_map.json
file should be in theminecraft-data
repo(?)