Update to 1.18 #514

Iaiao · 2022-01-10T16:27:35Z

Update to 1.18

Status

Ready
Development
Hold

Description

This PR adds 1.18 (and 1.18.1) support, multiworld and multidimension handling (not fully implemented and tested because we don't have portals yet, but changing EntityDimension component should work) as entities with components Dimensions, WorldName, and WorldPath, whede Dimensions is just a Vec<Dimension>, and Dimension is like old World that contains chunks, blocks, etc., full rewrite of generators in rust (except for blockstate generator, it was already written in rust), moved feather-blocks to libcraft-blocks because those were just different implementations of one thing, refactors biome logic, now there's a Arc<BiomeList> resource that is initialized at startup from worldgen json files extracted from vanilla server (probably should be changed to Rc<BiomeList>) that maps numerical IDs to biome identifiers and other information (temperature, particles, music, etc.), implements anvil format version 2865, also implements global palette serialiation/deserialization (#445), creates constants/ directory with to easily update to newer minecraft versions (changes version strings in ping packet, brand packet, protocol version, anvil version, data generators version, etc.), and adds support for varying height and negative y coordinate. Information about dimension height is taken from .json file of that dimension. Chunk-relative coordinates start with y=0, world-relative coordinates start with y=<min y>. Readable trait for Chunk Data and Chunk Light packets is currently not fully implemented because it's up to client to know how many chunk sections does the packet contain. Also fixes a few bugs and typos that I encountered during development.

What is left:

Update tests.
Default world generator (I deleted everything except superflat, I don't know why, but I think I need to update it to 1.18).
Plugin support. I have no idea how to test plugin support because rust-native plugins != wasm plugins and wasm plugins only work on rust 1.51, does that mean I should rewrite everything without newer features of rust in order to test plugins?
Per-world seed and generator in config.toml.

Good luck reviewing >100,000 lines of diff :)

Related issues

Closes #505
Closes #459
Fixes #445

Checklist

Ran cargo fmt, cargo clippy --all-targets, cargo build --release and cargo test and fixed any generated errors!
Removed unnecessary commented out code
Used specific traces (if you trace actions please specify the cause i.e. the player)

Defman

This is amazing, but maybe next time break it into a few smaller and easier to manage PRs.

I would suggest generate the code at build time via build.rs, such that we don't have to review the generated code but instead just the build scripts.

Maybe a few more tests here and there, since I have mostly looked at code quality.

Defman · 2022-01-11T15:43:31Z

constants/PROTOCOL_VERSION

@@ -0,0 +1 @@
+757


Why files instead of using const variables?

Because I wanted to create a "one place for all constants" so we don't forget to update constants (protocol version number in initial_handlers, version string "Feather 1.18.1" in initial_handlers, anvil format number in base/anvil/region, and other constants in different places), but I completely forgot that I could create a constants.rs file.

Defman · 2022-01-11T15:47:01Z

data_generators/src/generators.rs

+mod items;
+mod simplified_block;
+
+pub fn generate_all() {


Would it be too much to ask, if we moved this to a separate PR... since there are some stuff I would like to discuss, for instance moving to on build generated files, using build.rs.

Yeah, I think this pr should've been split into many PRs for worlds, dimensions, generators, 1.18, etc., but I was updating it like "old dimension_codec.nbt doesn't work, then I need to implement multidimension and biome loading, the packet doesn't work - rewrite proxy, a packet couldn't be deserialized - fix a bug in hematite-nbt" and so on, and I didn't want to stop if something doesn't work. I'm not sure if this would be compatible with old generators, but I think I can try to move it.

I think it's better to leave generators in this PR because:

1.16 requires a biome generator, which is not implemented here (it extracts biome information at runtime rather than hardcode the Biome enum in generators) and I don't see any solutions other than "write a generator and then delete it" or "it doesn't work, but it'll work when 1.18 is merged" and have CI failing.

main doesn't have feather_base::consts, and this pr won't be able to use data_generators::extract_vanilla_data, so we'll need to have duplicate code across 2 PRs and then create another pr to remove it (or forget about it and have 2 different implementations, like we had feather-blocks and libcraft-blocks)

minecraft-data has removed Item::Air from their 1.16 data, but main still uses it as a default item, that'll create even more problems.

Should I replace libcraft-blocks with feather-blocks in this PR or in generators PR? Both variants have obvious issues such as merge conflicts in use statements.

Defman · 2022-01-11T15:47:50Z

feather/base/src/anvil/entity.rs

@@ -200,7 +189,7 @@ impl From<&ItemData> for ItemStack {
 fn from(item: &ItemData) -> Self {
 ItemNbt::item_stack(
 &item.nbt,
- Item::from_name(item.item.as_str()).unwrap_or(Item::Air),
+ Item::from_name(item.item.as_str()).unwrap(),


This would cause a panic on loading an invalid world?

This would cause a panic on loading an invalid player.dat file. minecraft-data removed Item::Air and other non-item blocks from items.json, and I don't think panicking on invalid world saves is bad. Anyway, it's better than just silently wiping out a player's inventory.

Would it be better to only implement TryFrom?

Defman · 2022-01-11T15:50:15Z

feather/base/src/anvil/region.rs

-/// to 1.16.5.
-const DATA_VERSION: i32 = 2586;
+/// The data version supported by this code
+const DATA_VERSION: i32 = unwrap_ctx!(parse_i32(include_str!(


Why use files, instead of a constants module?

Defman · 2022-01-11T21:16:55Z

feather/base/src/chunk/packed_array.rs

 }

 fn values_per_u64(&self) -> usize {
- 64 / self.bits_per_value
+ if self.bits_per_value == 0 {


Can bits_per_value be zero?

I don't know, but bits_per_value is not NonZeroUsize

Defman · 2022-01-11T21:48:36Z

feather/server/src/main.rs

+ }
+}
+
+fn init_biomes(game: &mut Game) {


This can only panic on non-utf8 file names, files not ending with .json, and invalid biome .json files, I think that failing fast on startup is better than making a user think "why doesn't my biome work?" because of a trailing comma.

non-utf8 thing is just sad but whatever,
invalid biomes should ideally not panic if the biome is never used but also passable,
files not ending with .json immediately stops working on mac. you can rely on the OS to insert a .DS_Store file anywhere you look. also why not have texts or images or any other crap hidden in datapacks, who does it hurt?

P.S, yes, this is my first appearance on feather gh

You're right, it should ignore .DS_Store, but images or text files shouldn't really exist in biomes/. Also, worldgen/ is not a datapack, though it would be great if we extract the full vanilla datapack when we implement the datapack support, so it's rather a temporary solution.

Feather (or any non-prototype rust code for that matter) should only panic on programming errors, never on I/O errors, which can and will definitely occur in normal usage. init_game already returns anyhow::Result so you don't even have to make big changes through a lot of code to implement proper error handling. Just pass the error up the chain by using ? and letting init_biomes return anyhow::Result<()>. This also applies for init_dimensions and anywhere else (I have not reviewed the PR yet).

Nice work, looks good

Defman · 2022-01-11T21:52:59Z

feather/worldgen/Cargo.toml

-
-[dev-dependencies]
-approx = "0.3"
+log = "0.4.14"


Remove path and or minor.

Defman · 2022-01-11T21:55:25Z

libcraft/blocks/Cargo.toml

@@ -5,16 +5,11 @@ authors = ["Caelum van Ispelen <[email protected]>"]
 edition = "2018"

 [dependencies]
-libcraft-core = { path = "../core" }
+once_cell = "1.9.0"


Remove patch and or minor.

Defman · 2022-01-11T21:57:09Z

libcraft/blocks/src/directions.rs

@@ -0,0 +1,162 @@
+// use crate::blocks::{AxisXyz, FacingCardinalAndDown, FacingCubic};


An entire file commented out?

Defman · 2022-01-11T22:05:28Z

libcraft/blocks/src/lib.rs

+ /*
+ /// Returns the vanilla fluid ID for this block in case it is a fluid.
+ /// The fluid ID is used in the Tags packet.
+ pub fn vanilla_fluid_id(self) -> Option<u16> {


Commented out code.

feather/feather/blocks/src/lib.rs

Lines 96 to 99 in 57ca709

/*

/// Returns the vanilla fluid ID for this block in case it is a fluid.

/// The fluid ID is used in the Tags packet.

pub fn vanilla_fluid_id(self) -> Option<u16> {

Defman · 2022-01-11T22:10:28Z

What's the difference between world and dimension?

Iaiao · 2022-01-11T23:37:14Z

In vanilla, each world has 3 dimensions: Overworld, Nether and The End. There are plugins that manage multiple worlds on a single spigot server, and they're popular, then why not to add it with built-in support? Also, I thought that the client has support for servers with multiple worlds because the Respawn and JoinGame packets had "world_name" and "dimension_codec" fields, and a note "is world_name resolving the same-dimension issue?", but it turned out that they both mean dimensions.

Defman · 2022-01-12T08:40:07Z

In vanilla, each world has 3 dimensions: Overworld, Nether and The End. There are plugins that manage multiple worlds on a single spigot server, and they're popular, then why not to add it with built-in support? Also, I thought that the client has support for servers with multiple worlds because the Respawn and JoinGame packets had "world_name" and "dimension_codec" fields, and a note "is world_name resolving the same-dimension issue?", but it turned out that they both mean dimensions.

Can we not just treat each dimension as a separate world, why do we need to group dimensions into worlds?

Schuwi · 2022-01-12T11:17:53Z

Can we not just treat each dimension as a separate world, why do we need to group dimensions into worlds?

If you enter a nether portal it has to know which dimension to send you to for example. How would it know which nether to send you to without grouping into worlds?

Plecra · 2022-02-07T19:46:35Z

That's a relationship between two dimensions, it doesn't need to be represented as a hierarchy. Any dimension that has a PortalDestination(WorldId) component could handle it without all the extra structure.

comblock · 2022-06-15T18:09:27Z

Any update on this? Also, how does the 1.19 release impact this pull request?

Supermath101 · 2022-06-15T19:01:24Z

This is amazing, but maybe next time break it into a few smaller and easier to manage PRs.

Or, you could follow this guide and split the main commit into a bunch of smaller commits. That way you don't need to manage a dozen or so PRs, but still enable reviewers to do a git diff for each segment of the overall changes.

Iaiao added 12 commits January 10, 2022 17:48

Update to 1.18

b97c953

Remove hardcoded .nbt files

a4834df

Fix a typo

4305587

Fix dimensions in Server::broadcast_with

dea6486

Update tests and add test for PalettedContainer

78a60db

Merge main

57c5704

Fix clippy warning

e947675

Remove openssl-sys from reqwest dependencies, use rustls instead

57b9574

Use IndexMap to fix generated item order

a7d4a2f

Fix clippy

d02da81

Remove unused biome code

d4c2113

Change "value".parse().unwrap() to quote! syntax in generators

66508b8

Defman requested changes Jan 11, 2022

View reviewed changes

Iaiao added 6 commits January 12, 2022 01:54

Change dependency version format in all crates

7fcd37c

Uncomment libcraft/blocks/directions.rs

b86c2ce

Take EntityKind by value in feather_server::entities::add_spawn_packet

eff0dc6

Move constants/ to feather_base::consts

ce4d351

Make PalettedContainer.bits_per_value NonZeroUsize

4c0f80d

Fix tests and clippy

2d0d90e

Iaiao added 5 commits January 13, 2022 18:42

Remove From<ItemData> for ItemStack

e8803df

Allow non-.json extensions in worldgen/namespace/worldgen/biome/

0df9ca1

Replace for if let with .flatten()

373e4f6

Make Paletteable::{MIN,MAX}_BITS_PER_ENTRY NonZeroUsize

1d2c0d2

Improve error handling in biome and dimension loading

46be6ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to 1.18 #514

Update to 1.18 #514

Iaiao commented Jan 10, 2022 •

edited

Defman left a comment

Defman Jan 11, 2022

Iaiao Jan 11, 2022

Defman Jan 11, 2022

Iaiao Jan 11, 2022

Iaiao Jan 13, 2022

Defman Jan 11, 2022

Iaiao Jan 11, 2022

Defman Jan 13, 2022

Defman Jan 11, 2022

Defman Jan 11, 2022

Iaiao Jan 12, 2022

Defman Jan 11, 2022

Iaiao Jan 11, 2022

MalbaCato Jan 12, 2022

Iaiao Jan 12, 2022

Schuwi Jan 12, 2022

Schuwi Jan 21, 2022

Defman Jan 11, 2022

Defman Jan 11, 2022

Defman Jan 11, 2022

Defman Jan 11, 2022

Iaiao Jan 11, 2022

Defman commented Jan 11, 2022

Iaiao commented Jan 11, 2022

Defman commented Jan 12, 2022

Schuwi commented Jan 12, 2022

Plecra commented Feb 7, 2022

comblock commented Jun 15, 2022

Supermath101 commented Jun 15, 2022

		@@ -0,0 +1,162 @@
		// use crate::blocks::{AxisXyz, FacingCardinalAndDown, FacingCubic};

	/*
	/// Returns the vanilla fluid ID for this block in case it is a fluid.
	/// The fluid ID is used in the Tags packet.
	pub fn vanilla_fluid_id(self) -> Option<u16> {

		@@ -0,0 +1 @@
		757

Update to 1.18 #514

Are you sure you want to change the base?

Update to 1.18 #514

Conversation

Iaiao commented Jan 10, 2022 • edited

Update to 1.18

Status

Description

Related issues

Checklist

Defman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Defman commented Jan 11, 2022

Iaiao commented Jan 11, 2022

Defman commented Jan 12, 2022

Schuwi commented Jan 12, 2022

Plecra commented Feb 7, 2022

comblock commented Jun 15, 2022

Supermath101 commented Jun 15, 2022

Iaiao commented Jan 10, 2022 •

edited