CleanstoneMC / Cleanstone

Springboot based Minecraft Server
MIT License
127 stars 13 forks source link

This is broken #58

Closed GotoFinal closed 5 years ago

GotoFinal commented 5 years ago

As someone who was playing around creating own implementation of minecraft engine... I can see a lot of very important issues here, that just make this project near useless and as broken as mine or more:

  1. Your chunk idea is just bad https://github.com/CleanstoneMC/Cleanstone/blob/master/src/main/java/rocks/cleanstone/game/world/chunk/ArrayBlockDataTable.java#L11 Storing data inside reference of objects will increase memory usage by... a lot, even if we ignore block data itself (and just count size in memory of empty Block[][][] array). Also I don't see any chunk section, so seems that you ar storing every chunk as Block[16][256][16] this means that every chunk in this engine will be around 512KiB + (lights etc, then 640), so for typical single player with default rendering we need 75MiB just for the block type data, so imagine 300 players on server, where at least 100 different places are loaded. Other implementation, including old minecraft server (new one is a bit more tricky https://github.com/Diorite/Diorite-old/blob/master/diorite-core/src/main/java/org/diorite/impl/world/chunk/ChunkBlockData.java) so I will do calculations for old one, as it is good enough, so first: minecraft does divide each chunk into sections 161616 if section is empty then it isn't even created. (and most of the map does not go above 7-8th section, that already saves over 50% of memory) and each block is saved as short in short[16*16*16] for each section (note that this is 1D array) And this means that 1 chunk is at most around 128KiB for typical map with light etc, this is 5X less, for server this is a lot, as chunk and block data is one of largest portion of data.

Also operating on simple short[] array is A LOT faster than Block[][][], using 2d/3d arrays in java is just broken, as in reality is it array of arrays of arrays, so your CPU to get blocks[1][1][1] needs to:

  1. Find reference to blocks
  2. Find reference to blocks[1]
  3. Find reference to blocks[2]
  4. Find reference to blocks[3]
  5. Find reference to block where with short[] all you need is to do simple calculation to get index and just get the data.

New minecraft save is even smaller - but a bit slower. As block types are mapped, so if you only have 4 block types on section (pretty popular for simple stone below map surface) then it is using only 2 bits per block for that section. + mapping data.

https://github.com/CleanstoneMC/Cleanstone/blob/master/src/main/java/rocks/cleanstone/game/world/chunk/ArrayBlockDataTable.java#L88-L91 And this is just broken, this is not even returning the right type.

https://github.com/CleanstoneMC/Cleanstone/blob/master/src/main/java/rocks/cleanstone/game/world/chunk/MapBlockDataTable.java#L18 And you are duplicated a lot of that data here... so your chunks are probably at least 1MiB and this means over 100MB just to store data of map for single active player in unique location.

  1. There is no reason to focus on hosting multiple servers in single application, as this is just over complicated solution that does not give you any benefits. Big server will prefer to use separate servers anyway, as it is easier to recover from failure ;) Single one with multiple worlds is more than enough. People can always run it multiple times on this same host and add some proxy.

  2. https://github.com/CleanstoneMC/Cleanstone/tree/master/src/main/java/rocks/cleanstone/game/entity Just don't, been there, done that, this is bad idea that will break before you will even finish this project. Use something more similar to ECS, like sponge, it does need to be exacly like sponge but just don't use inheritance for entities, this never works good, and bukkit proved this multiple times. I did same mistake and needed to rewrite half of stuff after jus 2 minecraft updates. (and bukkit is just already so broken that half of stuff isn't added or deprecated) PS: health in minecraft is not an int.

  3. Adding plain getters and setter for everything will not help you make this multi threaded, and will only increase amount of issues and places where it might break. Like you allow to just get position of entity and then change that returned instance without even informing entity about that.

And you just miss a lot of basic stuff that will be near impossible to do with current idea, like usage of spring events. Priority of events is just basic and very important thing for mc plugins, and you use block break event to send a packet, what if it will be cancelled after this? And just what if pluginA needs to be after pluginB? Usage of spring to everything here seems to be pointless, especially that game engine is something different that typical application.

Also nothing about your code is ready for usage from multiple threads. And this is something you need to think from beginning, as making code thread-safe it not just something that can be added at any time, code needs to be already designed in safe way, as this often require special API and a lot of additional stuff.

WesleyVanNeck commented 5 years ago

It's wip stop complaining

GotoFinal commented 5 years ago

@YasunaMC yep, and this is why I'm writing this, so they can work on this a fix it, this is how all projects are made, you need to get some feedback to be able to create better solutions, this is not "hate" or anything like that. I just want to talk and help.

WesleyVanNeck commented 5 years ago

Ok

LeonMangler commented 5 years ago

Thank you for your detailed feedback. It's great to see how someone else thinks about what choices we've made so far so we can keep learning and improving. I agree with the current memory state of chunks being bad when it comes to memory-efficiency. We are working on new formats and this has already been a topic for quite some while. The implementation of our in-memory storage is abstracted by the BlockDataTable interface itself and the only reason we chose a 3d block array in the beginning is that it was simple.

I dont't see how entity inheritance is generally bad. We have paid close attention to making it backwards-compatible even if we want to change something that's being inherited by using our VersionedCodecs. We can change the fields and encoding of LivingEntity anytime without losing backwards-compatibility. Also, this is our internal format and it will not be sent to the minecraft clients like this. Generally, we distance ourselves from the minecraft protocol and from the way minecraft works itself. We probably wont even have the same entities that vanilla minecraft has. I personally would much rather prefer an rpg-like mob system with random bosses and special abilities.

Increasing the height limit in the minecraft protocol is definitly not impossible and I'm sure that we can overcome any challenges that come with it. It has been done before in bukkit actually (https://www.youtube.com/watch?v=EYUKawc43u8). Since we have full control over the server side it will be much easier and I think that it will be practical as well.

We are not trying to recreate a minecraft server. Instead we are building a Cleanstone server with a separate Minecraft protocol attached to it so you can join with minecraft clients. As such, we dont need to have a double for health and if the minecraft protocol needs a double then we'll simply convert it into one on the protocol layer, but not in our own game definition. What we are actually building is an open-source open-world game server which you can join with any compatible client you choose. Minecraft and MCPE are simply the first protocols we'll support since they match the idea of an open-world server perfectly. In the future we may create a Cleanstone client to gain more control over the client side, or maybe Hypixel's work-in-progress standalone game will be somewhat similar to minecraft and we can work towards supporting it in the future.

I disagree with Spring being a bad choice. In my opinion Spring is not about what kind of application you're trying to build, but rather about how you want to build it. It's a universal style of programming applications and its scalability and encapsulation is already benefiting us a lot, not just with the many libraries that make our lives easier but also with our code style itself.

Our event system works differently - not necessarily in a bad way though. Currently, we have cancellable and non-cancellable events. When a cancellable event is cancelled, no more listeners will be called. No need to check whether the event is cancelled in a listener. Also, you can only cancel events before listeners with MONITOR priority are being executed in the PREVENT or MODIFY event priority ranges. As such we do have an event priority system. I've been working with bukkit events for a really long time and our event system seems to solve all the issues bukkit ones has by actually enforcing the fundamentals of how the event system works without removing important event design patterns.

Thread-safety isnt easy and we have already thought about making Position immutable. I agree that there is a lot of work to do regarding thread-safety and we will have to change some design patterns. However, Spring's improved encapsulation will make this a lot easier for us. The reason that it probably hasnt been addressed as much as it should have been is that we dont want to think too much about thread-safety in e.g. our block storage model before it's at least close to being somewhat finalized. It's true that you need to think ahead and design for thread-safety and we will need to do quite some refactoring, but right now we simply want to get more features out first as that's what actually keeps this project alive. Refactoring will happen and it will be a lot of work, but not an impossible amount of work because of DI (in my opinion). If the Async annotation wont quite cut it then we'll simply use something else.

We're doing this to gather this experience and to solve problems just like this. Regarding that this project is only a few months old I think that with feedback like this and enough contributors we can definitly fulfil our goal and make cleanstone practical.