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

Support for hashed block network ID's #237

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

Conversation

FreezeEngine
Copy link

No description provided.

for (let i = 0; i < paletteSize; i++) {
const runtimeId = stream.readZigZagVarInt()
const block = this.registry.blocksByRuntimeId[runtimeId]
this.palette[storageLayer][i] = { stateId: runtimeId, ...block, count: 0 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stateId and runtimeId are not the same, stateId should be set to block.stateId, runtimeId should be the runtimeId (that should also be inside the block object return by this.registry.blocksByRuntimeId[runtimeId])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should block.runtimeId be added to pblock? Or I can just do stateId: block.stateId? this.registry.blocksByRuntimeId = data.blockStates when hashes are not used

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocksByRuntimeId should look the exact same as blocksByStateId with the only difference being the key for the map being a runtimeID as opposed to a stateID

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block.stateId doesnt seem to exsit, I can add it to pregestry by blocksByRuntimeId[pBlock.getHash(name, states)].stateId = stateId but then data.blocksByRuntimeId = data.blocksByStateId would not have that field, how should this be resolved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it seems node-minecraft-data doesn't expose a .stateId. So it seem the options to fix are:

  1. add .stateId field in node-mcdata on top of .minStateId / .defaultState (this kind of feels wrong as typically node-mcdata maps right from blocks.json)
  2. add in prismarine-registry a .stateId field to blocks objects inside blocksBy* (would require .map copying over mc-data block data)
  3. add some separate map inside p-registry to map stateID <-> runtimeID (kind of messy to have many of these maps)

I'm not sure exactly best approach but 2 seems least problematic, cc @rom1504

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do second for now, then could be reviewed and decided

@rom1504
Copy link
Member

rom1504 commented Apr 28, 2024

what is the state of this? do you know @extremeheat ?

@FreezeEngine
Copy link
Author

Should be working

@extremeheat
Copy link
Member

Blocked by PrismarineJS/prismarine-registry#33

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

Successfully merging this pull request may close these issues.

3 participants