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

Update to 1.18 #514

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Update to 1.18 #514

wants to merge 23 commits into from

Conversation

Iaiao
Copy link
Contributor

@Iaiao Iaiao commented Jan 10, 2022

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)

Copy link
Member

@Defman Defman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -0,0 +1 @@
757
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why files instead of using const variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mod items;
mod simplified_block;

pub fn generate_all() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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)
  3. 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.
  4. 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.

@@ -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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would cause a panic on loading an invalid world?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to only implement TryFrom?

/// 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!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use files, instead of a constants module?

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can bits_per_value be zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but bits_per_value is not NonZeroUsize

}
}

fn init_biomes(game: &mut Game) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, looks good


[dev-dependencies]
approx = "0.3"
log = "0.4.14"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove path and or minor.

@@ -5,16 +5,11 @@ authors = ["Caelum van Ispelen <caelum12321@gmail.com>"]
edition = "2018"

[dependencies]
libcraft-core = { path = "../core" }
once_cell = "1.9.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove patch and or minor.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An entire file commented out?

/*
/// 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*
/// 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
Copy link
Member

Defman commented Jan 11, 2022

What's the difference between world and dimension?

@Iaiao
Copy link
Contributor Author

Iaiao commented Jan 11, 2022

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
Copy link
Member

Defman commented Jan 12, 2022

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
Copy link
Member

Schuwi commented Jan 12, 2022

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
Copy link

Plecra commented Feb 7, 2022

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
Copy link

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

@Supermath101
Copy link

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.

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