search

Technoguyfication / Minecraft-Unity3D

Vanilla-compatible Minecraft multiplayer client written from scratch in C#
MIT License
64 stars 14 forks source link

PlayerInfoPacket and chunk rendering improvements #3

Closed djgaven588 closed 5 years ago

djgaven588 commented 5 years ago

The goal of my changes is to both improve chunk rendering (handling how generation is done to how they are displayed), and handle the PlayerInfoPacket.

Changes so far:

More details will be added as this is added to.

Technoguyfication commented 5 years ago

Hey, I took a quick look at your code and I'd recommend studying how packets are currently being handled. I see you reimplemented VarInt handling and you're using a BinaryStream for reading the packet. All the tools needed for parsing the packet body are available in the PacketHelper class. We also want to keep all business logic outside of the Packet class. It should only be used as a method of transporting data between the socket and GameManager (think of models in a MVC application).

At some point in the future the packet handling logic will need to be moved out of the Game Manager (and into World, Player, etc. possibly) but it should be put in there for now.

Thanks for the contribution by the way!

djgaven588 commented 5 years ago

I am using BinaryReader for this code, as it is pretty much built for this kind of setup. I did however move all of the actual reading of the streams to a new class called "PacketReader" which has utility functions for reading it. Reading the protocol for this packet, it seems to be a very complex packet, which requires a large amount of code to convert to a usable form in comparison to something like ChunkDataPacket. Unless I am completely missing something, the way I have implemented it so far seems fine. (I could be wrong, but I would assume from the available code I have to read that a SomethingPacket converts the data into what the protocol page displays)

djgaven588 commented 5 years ago

For the above commit. Chat history and player list

djgaven588 commented 5 years ago

We are ready for a merge over here. Let me know if I need to do any last minute changes. This is tested, and I have not had issues with it.

Technoguyfication commented 5 years ago

Quick question before I take a look at the code. Why did you add TextMeshPro twice? It was already in Assets/TextMeshPro

djgaven588 commented 5 years ago

I don't see that for some reason, there is only _Project and Dependencies in my project's asset folder.

djgaven588 commented 5 years ago

Reading the gitignore file, the folder you are talking about is completely ignored.

Technoguyfication commented 5 years ago

Ah you're right. It seems like this was a mistake in the gitignore when I created this project: https://github.com/github/gitignore/pull/3134

Should be fixed in master

djgaven588 commented 5 years ago

Branches should now be mergable again. Unknown if all of your changes persisted.

djgaven588 commented 5 years ago

Position is separate, for it is a type in the Protocol, and I believe it is different from a BlockPos.

Technoguyfication commented 5 years ago

merged changes