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

Add v0 support to the decoder #230

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Flonja
Copy link
Contributor

@Flonja Flonja commented Aug 13, 2023

This is by no means final.
I'm guessing the block_legacy_id_map.json file should be in the minecraft-data repo(?)

Potentially needs some refactoring
@extremeheat
Copy link
Member

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.

@Flonja
Copy link
Contributor Author

Flonja commented Aug 14, 2023

This pull request will now be reliant on PrismarineJS/minecraft-data#755 getting accepted

@extremeheat
Copy link
Member

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.

Copy link
Member

@extremeheat extremeheat left a 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?

src/bedrock/1.3/SubChunk.js Outdated Show resolved Hide resolved
@Flonja
Copy link
Contributor Author

Flonja commented Aug 16, 2023

Another thing to keep in mind here doing translations for legacy pre-flattening data to new data is that Block.fromString ...

Using Block.fromString requires me to reformat the (temp) legacy.json file into a (arguably) worse format.

There have been some block renames since then, and those translations would fail. Are you aware of that?

I am aware of that, though I don't have enough experience with this library to figure out a fix.

@extremeheat
Copy link
Member

extremeheat commented Aug 16, 2023

Using Block.fromString requires me to reformat the (temp) legacy.json file into a (arguably) worse format.

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 [" to see if it's using that other syntax. You can open a PR there to fix that here: https://github.com/PrismarineJS/prismarine-block/blob/master/index.js#L247

I am aware of that, though I don't have enough experience with this library to figure out a fix.

OK, since this seems like a broader issue for consideration we can handle that after this PR.

@Flonja
Copy link
Contributor Author

Flonja commented Aug 17, 2023

So after I fix the failing test would this be ready for a merge? Or does something else need to be done first?

@extremeheat
Copy link
Member

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.

@Flonja
Copy link
Contributor Author

Flonja commented Aug 17, 2023

Then you can open a PR to prismarine-block to fix Block.fromString to fix syntax support per my comment above.

Your comment was unclear, can you clarify a bit more?

@extremeheat
Copy link
Member

extremeheat commented Aug 17, 2023

As you mention in prismarine-block

Using Block.fromString requires me to reformat the (temp) legacy.json file into a (arguably) worse format.

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
      }

@extremeheat
Copy link
Member

minecraft-data/node-minecraft-data was merged, so seems like this is only blocked by the prismarine-block at the moment.

@rom1504
Copy link
Member

rom1504 commented Dec 31, 2023

@Flonja would you like to finish this?

@Flonja
Copy link
Contributor Author

Flonja commented Mar 28, 2024

So I was trying to finish the PR just now, but realized that there isn't an updated version for prismarine-block, so I'll push the changes that I can currently make (+ that also passes tests) and I'll push the rest when there's a version that has the changes that were asked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Almost too old
Development

Successfully merging this pull request may close these issues.

3 participants