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

Importing maps does not work #506

Open
iyusdi opened this issue Jun 8, 2021 · 23 comments
Open

Importing maps does not work #506

iyusdi opened this issue Jun 8, 2021 · 23 comments

Comments

@iyusdi
Copy link

iyusdi commented Jun 8, 2021

I'm trying to import a map into flying-squid. I've tried three so far, none of which work.

There were a couple of bugs in the block palette logic (palette in the sections can be an empty array which was not accounted for) which prevented the spawn point from being calculated, but I fixed those.

Now I'm stuck on bad data getting passed to the prismarine-web-client. All of the map_chunks get rejected by the web-client with an 'unexpected end tag'.

Can someone with more knowledge of the code try this? It's very simple to replicate, just copy the level.dat file and regions from a downloaded map to the flying-squid world directory, and then connect with prismarine-web-client.

The maps I tried are Future City 4.5 and Notre_Dame_and_Medieval_City. The latter is fairly new, I would suggest starting with that one.

I'll keep debugging, but any help would be appreciated.

@iyusdi iyusdi changed the title Import world does not work Importing maps does not work Jun 8, 2021
@extremeheat
Copy link
Member

What version is the map? Do you have a link to the map?

@iyusdi
Copy link
Author

iyusdi commented Jun 8, 2021

Here is a link to the map, thanks.

https://www.minecraftmaps.com/city-maps/notre-dame-and-medieval-city

@callmeteus
Copy link

callmeteus commented Jun 9, 2021

I'm actually also having a lot of similar compatibility issues with all PrismarineJS packages.

The major problem is that most of the code is not documented, and the main problem is that the code is splitted in module.export functions, and not proper classes, so it's even harder to take a closer look and fix or do something in the top of flying-squid.

I actually needed rewritte the entire prismarine-chunk and prismarine-provider-anvil to make this work properly with the minecraft-protocol package :/

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

Can you share the code?

@callmeteus
Copy link

callmeteus commented Jun 9, 2021

I'm afraid I can't :(
The code is all messy for now, I'm still working on it, still got some issues to solve like the lightmap (it's not working for now)
Also I'm writing everything in TypeScript, it's a bit different than the flying-squid code and has some incompatibilities with itself (I'm implementing it using only the minecraft-protocol package)

But yeah I want to make the package public and open source in the future, just keeping it private for now because it's still an alpha.

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

I'm ok with alpha code!

I'm basically doing the same thing. The problem is definitely in the read/write nbt code. The buffer seems to get transferred to the browser, but the readNbt for the chunk fails on the browser.

Rather than both of us duplicating the effort, let's work together to fix it. If you can send what you have working, I'll reciprocate with any fixes or enhancements.

@extremeheat
Copy link
Member

extremeheat commented Jun 9, 2021

The modules are designed to be modular, so they can be independently tested and used for many different purposes. I agree that the docs and how all the packages work together isn't very well documented, it's something to work on. You're free to help contribute.

module.export is the way to export commonjs modules. Classes are a different topic, they offer encapsulation and inheritance, and they're definitely also used in the packages. I think you maybe mistaken for the loaders: they're there to ensure multi-version support, and also allow for supporting other editions of the game, like the bedrock edition.

For more info on design philosophy, see The Node Way.

@extremeheat
Copy link
Member

extremeheat commented Jun 9, 2021

I tested your world with the prismarine-viewer electron example, and it seems to work fine. What coordinates do you have trouble at? Make sure you set the correct world version (1.14.4).

image

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

It's not the coordinates I was having trouble with. The map_chunks sent to the browser were all causing errors. (unexpected end tag).

It could be the world version. How do I set the world version in flying-squid/prismarine-web-client?

I'll take a look at electron, thanks.

@extremeheat
Copy link
Member

extremeheat commented Jun 9, 2021

prismarine-provider-anvil doesn't have the code currently to automatically upgrade worlds. So the version you set the server as, is the version the world is loaded as. PWC will deduce the version based on version what the server is running as. But the server and map version must be the same.

If you need your server to support a newer version, you need to load the world in a vanilla client or server, have it update all the chunks and then you can load it in flying squid.

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

Yeah, after I had to mess with the version in the web client, but the map_chunks are coming in now. Still having an issue with

'Attempted to read beyond the bounds of the managed data'

Thanks for the help, great product.

@callmeteus
Copy link

callmeteus commented Jun 9, 2021

The modules are designed to be modular, so they can be independently tested and used for many different purposes. I agree that the docs and how all the packages work together isn't very well documented, it's something to work on. You're free to help contribute.

module.export is the way to export commonjs modules. Classes are a different topic, they offer encapsulation and inheritance, and they're definitely also used in the packages. I think you maybe mistaken for the loaders: they're there to ensure multi-version support, and also allow for supporting other editions of the game, like the bedrock edition.

For more info on design philosophy, see The Node Way.

I think it's more a difference of coders and coding way, but some things are really hard to deal with, like the exports I've said; not complaining "The Node Way" since I use it in some of my projects, but it's hard to keep following a callstack when there's variables being declared to an object everywhere instead of having them inside one single local and then writing them along the way. This is the reason I said about classes, they can be very handful when dealing with big projects.

The hardest thing in all PrismarineJS packages is to keep track of things, since they're declared or set everywhere, and that's what I'm actually complaining here. The lack of documentation also doesn't help too much to contribute with the project.

Using myself as an example; I've wanted to use flying-squid to develop a server, but it's barely readable due to the things I've said below, so I started coding a new package using just minecraft-protocol, since it is more readable and documented.

I want to contribute with, but it's hard to contribute to something I can't keep track of what's going on :(

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

I've made good progress but am still having one problem. The map_chunks are coming in the browser, but I always get an

'Attempted to read beyond the bounds of the managed data'

It looks like this happens when it's reading the biomes.

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

Yes, each map_chunk now gets this error 'Attempted to read beyond the bounds of the managed data'. It's in the ChunkColumn when it tries to read the biomes.

// read biomes
if (fullChunk) {
const p = { x: 0, y: 0, z: 0 }
for (p.z = 0; p.z < constants.SECTION_WIDTH; p.z++) {
for (p.x = 0; p.x < constants.SECTION_WIDTH; p.x++) {
this.setBiome(p, reader.readInt32BE()) <-- fails here
}
}
}

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

It's the same offset every time: p.z = 15, p.x = 2

Still looking - any ideas?

@extremeheat
Copy link
Member

If you get an error there it’s most likely a chunk version issue. You should load the entire map on a server or use a map pregenerator to make sure all the chunks are a consistent version.

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

I think it's an off by one error. It was always reading one to many sections. I changed the bitMap test in ChunkColumn:

(bitMap >> y) & 1 ===> (bitMap >> (y + 1)) & 1

and it works. I'm getting the world in the browser now.

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

Still not totally there. When I move around, I eventually hit bounds error. Both the client and server are using the 1.14 chunks.

Progress though. Will keep plugging away. If anyone comes up with something, please let me know.

@iyusdi
Copy link
Author

iyusdi commented Jun 9, 2021

The problem turned out to be a bug in the anvil provider. There were sections that only had a SkyLight and nothing else. This threw off the serialization logic. There is already logic to detect empty sections via a palette check - I just added a check for an empty array to ignore those sections.

In chunks.js in the 1.14 dir, change:

    if (chunkSection.palette.length === 1 && chunkSection.palette[0] === 0) {

to:
if (chunkSection.palette.length === 0 || (chunkSection.palette.length === 1 && chunkSection.palette[0] === 0)) {

It seems to work. The entire world is getting rendered now, and no more client errors.

Ignore the bitMap hack above, that is not needed now.

@iyusdi
Copy link
Author

iyusdi commented Jun 10, 2021

As a side note, its really impossible to hack prismarine effectively in it's current form. I agree that you need separate npm packages in order to share code, but maintaining them separately will be a nightmare.

For me to start hacking, I had to basically copy all the npm packages into prismarine-web-client and flying-squid. It was very easy to debug after that.

The problem is you have cross dependencies for all the packages, and you'll never be able to keep the versions in sync.

I would strongly suggest that you use lerna to manage the project as a whole. You still have separate npm packges for everything, but you can coordinate the package releases easily, and still make fixes to separate packages if needed.

Lerna also makes development of the project much easier since it just looks like one large src directory from the developers point of view.

Anyway, this is a great project, nice work from everyone involved!

@callmeteus
Copy link

Lerna is a really great project orchestrator, I agree with using it on Prismarine

@extremeheat
Copy link
Member

You can join the discord for further discussion. The packages are not supposed to be heavily connected to each other, they’re intended to be independent modules reusable for different purposes. As always, you can make modifications inside your node_modules folder just as with any module. Making a monorepo is helpful when you want a closed environment for a specific broader use case. If you’re just making a server or a client for example, sure everything under one umbrella makes sense.

But bundling in hundreds of source files for intentionally distinct projects into one monorepo isn’t much different than distinct submodules, and it makes maintaining more difficult (Just imagine how many CI or test runs per commit). In the current form, it’s easy to fork and depend on a specific custom module from git, something that’s not easy to do with a monorepo. I’ve worked at places where we do use monorepos, but for very specific purposes, mostly because we want to keep things in house. Having different repos gives hard isolation that’s a good thing.

If you have an issue with one of the deps, feel free to open a PR to fix an issue. You can require the dep from git and as long as the version is consistent, it will be overwrite any other versions for the dep. The current system works just how node/npm was designed to work as yes, indeed there’s alternatives but I think the current system works pretty well.

@iyusdi
Copy link
Author

iyusdi commented Jun 10, 2021

I get your points, there is no perfect solution. I come at it from a devs point of view - I want to be able to hack it easily if there are bugs. Hacking node_modules will work in some cases, but not others. Forking and updating packages is very time consuming. Lerna sort of does most of those things for you.

Just a suggestion - I'll join the discord to discuss more.

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

No branches or pull requests

3 participants