PrismarineJS / prismarine-chunk

A class to hold chunk data for Minecraft
MIT License
61 stars 59 forks source link

prismarine-chunk or prismarine-chunk-column ? #16

Closed rom1504 closed 7 years ago

rom1504 commented 9 years ago

Looking at http://wiki.vg/SMP_Map_Format a chunk is 16x16x16 and a chunk column is 16x256x16

This module is currently handling a chunk column, that probably shouldn't be the case.

Not sure actually, I guess it makes some sense to store a column, related to #8 anyway.

rom1504 commented 9 years ago

Alright, I looked at mineflayer code and spock code, and both are storing a array of 16 chunk for columns instead of just one big buffer. That is easier to use for #8 I think we should switch to that at least internally if not in the API.

https://github.com/andrewrk/mineflayer/blob/master/lib/plugins/blocks.js#L60

https://github.com/SpockBotMC/SpockBot/blob/007963c2b7eddaefec549694aca453bfc455009f/spockbot/plugins/tools/smpmap.py#L182

rom1504 commented 9 years ago

Is it really needed for .dump() to be very fast or could we just use a nicer representation like mineflayer one (https://github.com/andrewrk/mineflayer/blob/master/lib/plugins/blocks.js#L349) and just build the ugly buffer in dump() ? (or dumpChunk(y=5))

roblabla commented 9 years ago

My personal opinion is that it doesn't need to be fast. Sending a chunk isn't an operation done very often.

rom1504 commented 9 years ago

Hmm, in mineflayer we definitely don't need dump() to be fast since we never send chunks in mineflayer. In flying-squid we might send 24*24=576 chunks shortly after login (viewsize = 12). It's expected to see the viewsize around you in at most a few seconds. To send these chunks in 1 second (for example), we need dump() to take about 2ms. I guess we'd need to do some bench to compare the current approach (store the chunk as a buffer) with the mineflayer way (store the chunks in a nice representation, and generate the buffer in dump()).

roblabla commented 9 years ago

@rom1504 this is under the assumption we only dump one minichunk at a time, where in truth it is possible to asyncify it ;)

rom1504 commented 9 years ago

asyncifying (:p) it wouldn't change much: the problem is the time it would take the cpu to generate the buffers.

(but it's totally possible that this time is low enough)

roblabla commented 9 years ago

@rom1504 it does change a lot, because generating the buffers is done by reading from the backend, a purely IO task, which can be done asynchronously, many times at the same time

rom1504 commented 9 years ago

@roblabla but that is not doable if we store chunk as not-buffer as mineflayer do then ;)

roblabla commented 9 years ago

@rom1504 Because mineflayer does it in memory, right ? Well we're going to have to make some kind of trade-off somewhere. Mineflayer doesn't need to dump() anyway. I think it's reasonable to expect at least each minichunk to be bufferizable in "parallel"

rom1504 commented 9 years ago

Hmm. Let me re-explain from the beginning. There are mostly 2 costs:

There are several representations of a chunk:

The current representation make whole chunk reading/writing very fast (nothing to do), but block by block a little bit slower. Mineflayer representation make block by block reading/writing fairly fast, but make whole chunk reading/writing a bit slower.

It would be possible to have any kind of intermediate between these two, including an array of 16 "mini-chunks", or not storing anything in memory (that would be close to mineflayer representation in term of block-by-block operation, and might be faster in term of whole-chunk thanks to async stuff)

Mineflayer/not storing anything in memory has the advantage of making things simpler for most functions of prismarine-chunk except 2 : load() and dump() that would have to deal with generating/loading the buffer.

Do we know what minecraft does internally ?

roblabla commented 9 years ago

AFAIK in 1.9, what is sent on the wire is how the chunk is represented internally. Before that, it was generated. Don't take that for granted though, I'm not sure.

rom1504 commented 8 years ago

okay so indeed ThinkOfDeath is saying vanilla represent chunks in the buffer way internally in 1.9 So I think we should keep doing that.

rom1504 commented 8 years ago

So yes we should keep doing that.

But we're not actually doing it as vanilla currently. As indicated by http://wiki.vg/Pre-release_protocol#Chunk_Data and http://wiki.vg/Pre-release_protocol#Chunk_Section vanilla split the sections.

I think we should do the same thing, that would allow us to fix #8 easily and it would allow us to not store empty sections in ram.

So basically :

rom1504 commented 8 years ago

An idea that's not completely unrelated: currently world generation have to use setBlockType for every position in the chunk. That means computing the position in the buffer 256*16*16 times, not efficient. A better way would be to provide a good constructor for the Chunk that would take a (x,y,z) => {} function and use it to set things efficiently.

(might be interesting to setup a benchmark for the generations before doing this though)

rom1504 commented 8 years ago

Might be worth looking into how voxeljs does it, and if https://github.com/scijs/ndarray could be useful here.

rom1504 commented 7 years ago

done in 1.8 and pe 1.0, the rest might be done later if needed