GlowstoneMC / Glowstone

A fast, customizable and compatible open source server for Minecraft: Java Edition
https://glowstone.net
Other
1.88k stars 270 forks source link

BlockData implementation #1076

Closed mosemister closed 4 years ago

mosemister commented 4 years ago

This is my first PR for any public project so sorry if the format isn't correct.

This PR is to solve the issue of https://github.com/GlowstoneMC/1.13-board/issues/28 As this is my first issue any and all comments will be appciated to make the code more to the Glowstone standard.

Implementations currently done:

claassistantio commented 4 years ago

CLA assistant check
All committers have signed the CLA.

mastercoms commented 4 years ago

Hi, welcome to Glowstone! I really appreciate your initiative on this issue. One thing I would like to point out is that statically typed properties like this will prove to be cumbersome when initializing block data from IDs, unless we were to create class specific initializing for every material, or used slow reflection to product block data. Neither approach is preferable, so I'd recommend taking a different approach to tackle this problem. Admittedly, this issue is one of our most difficult, as signified by the high bounty. If this is your first PR, I would recommend tackling one of the smaller issues first, and communicating with us on our Discord to get help on how to get your way around Glowstone!

smartboyathome commented 4 years ago

To add to what @mastercoms said, this solution does not provide a mechanism for translating blockdata to the appropriate over-the-network ID, which is stored in the generated block report (see the linked issue on 1.13-board). Your solution will need to take this into account for that.

mosemister commented 4 years ago

Thats fair. I am very familiar with Sponge API that uses Key Value pairing for this and I know the glowstone project doesn't want anything based off another project.

However I have just seen the classes in the flattening folder, in particular, the BlockData implementation there that uses Maps. Ill expand on that keeping into account the network api

mastercoms commented 4 years ago

Hey, I just wanted to drop by and say I really like how this PR is going! I'll have some more feedback on it soon, but just wanted to let you know you're some awesome work!

smartboyathome commented 4 years ago

I saw you removed the WIP tag. I will see about checking out your code and reviewing it here soon. :)

mosemister commented 4 years ago

I mearly removed the [wip] due to all the blocks being added (except noteblock ... Still working out how to do that one). I still need to make sure the code compiles, make sure all the names are correct and all that. That wont change the code that much

mosemister commented 4 years ago

The updated context push fixes the noteblock and missed code. The only check i need to do now is to make sure the ids are as they are in mc

mosemister commented 4 years ago

Should be complete now. All the ids and defaults match. No implementations I have missed.

The network stuff can be removed if needed, ill leave that to whoever wants to take on that issue (it maybe me)

mosemister commented 4 years ago

I will take a deep look in a few days as I have stuff to do. You guys have been doing an amazing job with glowstone, just glad there is code based on my code that will be in it.

Please take your time reviewing it, in no rush.

As for my brief look on the your code, it looks amazing, easy enough to follow. As mentioned before, ill take a good look in a few days.

smartboyathome commented 4 years ago

Closing, as its superseded by #1087.