Support for hashed block network ID's #33
Conversation
lib/bedrock/index.js
Outdated
| function fnv1a32(s) { | ||
|
|
||
| //https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function#FNV_hash_parameters | ||
| const FNV1_OFFSET_32 = 0x811c9dc5 | ||
| const FNV1_PRIME_32 = 0x01000193 | ||
|
|
||
| var h = FNV1_OFFSET_32 | ||
| for (let i = 0; i < s.length; i++) { | ||
|
|
||
| h ^= s.charCodeAt(i) & 0xff; | ||
| //h *= FNV1_PRIME_32; //<-- this does not work in nodejs | ||
| h += (h << 1) + (h << 4) + (h << 7) + (h << 8) + (h << 24); | ||
| } | ||
|
|
||
| return h & h | ||
| //return h >>> 0 | ||
| //return h | ||
| } |
There was a problem hiding this comment.
This should be moved to prismarine-block
lib/bedrock/index.js
Outdated
| blockStatesByRuntimeId[hash] = Number(stateId) | ||
| } | ||
|
|
||
| data.blockStatesByRuntimeId = blockStatesByRuntimeId |
There was a problem hiding this comment.
This should be data.blocksByRuntimeId and similarly exposed for mcpc equal to data.blocksByStateId
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
lib/bedrock/index.js
Outdated
| for (const key of Object.keys(block.states).sort()) { | ||
| states[key] = block.states[key] | ||
| } |
There was a problem hiding this comment.
Should that be a field or a function? Should it be only avalieble if blockHashes supported?
There was a problem hiding this comment.
I think i understand, keys should be pre-sorted in pblock
There was a problem hiding this comment.
I don't understand the question. All prismarine-registry should be doing is
function loadHashedRuntimeIds () {
const Block = require('prismarine-block')(this)
data.blocksByRuntimeId = {}
for (let i = 0; i < data.blockStates.length; i++) {
const { name, states } = data.blockStates[i]
const hash = Block.getHash(name, states)
data.blocksByRuntimeId[hash] = data.blocksByStateId[i]
}
}There was a problem hiding this comment.
BlocksByState have stateId excluded, should indexers keep that field/new indexer that keeps indexing field? Or technically since its sequencial index of hash in runtimeIds would give stateId?
There was a problem hiding this comment.
In JavaScript, objects are passed by reference, not by copy. This means when you mutate one reference (like adding a .stateId to it) then all other references will also be updated. This effectively means iterating over blocksByStateId or any other blocks array and mutating a block object in one of them will update it everywhere else.
We don't want updates in prismarine-registry to blocks to affect the static data exposed by node-minecraft-data, so an explicit copy would need to be made before any edits. That's what I was mentioning in the prismarine-chunk comment
|
Still reqires either stateId setter or a map from hash to stateId |
lib/bedrock/index.js
Outdated
| loadHashedRuntimeIds () { | ||
| const Block = require('prismarine-block')(this) | ||
| data.blocksByRuntimeId = {} | ||
| for (let i = 0; i < data.blockStates.length; i++) { | ||
| const { name, states } = data.blockStates[i] | ||
| const hash = Block.getHash(name, states) | ||
| data.blocksByRuntimeId[hash] = { stateId: i, ...data.blocksByStateId[i] } | ||
| } | ||
| }, | ||
|
|
||
| loadRuntimeIds () { | ||
| data.blocksByRuntimeId = {} | ||
| for (let i = 0; i < data.blockStates.length; i++) { | ||
| data.blocksByRuntimeId[i] = { stateId: i, ...data.blocksByStateId[i] } | ||
| } | ||
| } |
There was a problem hiding this comment.
Things that return {}'ed inside this function are apart of the exposed prismarine-registry API. You should put non-API functions outside the returned object
| @@ -1,26 +1,13 @@ | |||
| const buildIndexFromArray = require('../indexer') | |||
|
|
|||
| module.exports = (data) => { | |||
| return { | |||
There was a problem hiding this comment.
I mean put the non-API helper functions before this return statement
There was a problem hiding this comment.
Also if you make a change to the PR that needs review, feel free to comment so we can get notified of it
There was a problem hiding this comment.
Got it! Thank you, will note
There was a problem hiding this comment.
Also if you make a change to the PR that needs review, feel free to comment so we can get notified of it
Might've done it
|
Should anything else be added/modified? |
|
bump |
|
@extremeheat seems fine to me now |
|
/makerelease |
Aims to add support for hashed block network ids.
WIP so more commits to follow