Skip to content

Refactor such that libcraft contains all generated files (almost).#461

Merged
Defman merged 56 commits into
feather-rs:mainfrom
Miro-Andrin:refactor
Sep 27, 2021
Merged

Refactor such that libcraft contains all generated files (almost).#461
Defman merged 56 commits into
feather-rs:mainfrom
Miro-Andrin:refactor

Conversation

@Miro-Andrin

@Miro-Andrin Miro-Andrin commented Sep 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Moved generated code into libcraft.

Status

  • Ready
  • Development
  • Hold

Description

I have moved the code from feather/generated into libcraft/generated and made everything depend on that version instead. I have not moved feather/blocks into libcraft, but might do that in a different pull request.

Related issues

#459

What's missing in this pull request:

ItemStack in libcraft was different in libcraft then in the original generated module. I have no idea why. The original version allowed for ItemStacks containing zero items, but the new version does not. This broke a lot of small things, but especially feather/common/window.rs. I tried to quickly adapt it to the new ItemStack methods in libcraft, but i have not gotten it to work. It compiles, but the test don't pass .

Checklist

  • [ x] Ran cargo fmt, cargo clippy --all-targets, cargo build --release and cargo test and fixed any generated errors!
  • Removed unnecessary commented out code (Ongoing, still some in window.rs)

@Miro-Andrin

Miro-Andrin commented Sep 16, 2021

Copy link
Copy Markdown
Contributor Author

My bad hygiene when doing commits has bitten me in the ass. So it's a very messy commit history, but currently the
code is in a finished state.

Things that we did:

Moved /generated into /libcraft by moving most of the generated files into feather/core
and the generators into /libcraft/generators. All the generators have been updated
such that they pull data from the right folder and output to were I moved the generated
files. 

All files in /feather that had 'use generated:{...' now point to an equivalent file 
in /libcraft. This went mostly without any issues. 

ItemKind and ItemStack had a version in /generated and /libcraft. The ItemKind was identical, but the ItemStack in libcraft had a difference. The libcraft version does not allow for the creation of a stack containing zero items. Therefore every case where feather used the old ItemStack was a bit tricky. 

My guess is that someone was in the middle of moving the code to the libcraft version, but just stopped for some reason. The function bodies in /feather/common/src/window.rs had to be completely rewritten, and the InventorySlot struct had to fleshed out as well in libcraft.

ambeeeeee
ambeeeeee suggested changes Sep 26, 2021
View reviewed changes

@ambeeeeee ambeeeeee left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall in the right direction, I know you guys were likely planning to do this but please pull out the tags stuff into its own branch for later.

For the future: please try to use relevant and meaningful commit messages.

Comment thread feather/base/src/anvil/entity.rs Outdated
Comment thread feather/base/src/anvil/entity.rs Outdated
damage: Some(damage),
}) => ItemStackBuilder::with_item(item)
.count(count as u32)
.damage((*damage).try_into().unwrap())

@ambeeeeee ambeeeeee Sep 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is unwrapping here okay? Should we be returning a Result or can this never panic?

@Miro-Andrin Miro-Andrin Sep 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, this is an issue. I am going to change this.

@Miro-Andrin Miro-Andrin Sep 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made damage on ItemStack be a i32, reflecting the fact that vanilla minecraft allows negative damage amounts, making the anvil damage and itemstack damage congruent.

Comment thread feather/base/src/anvil/entity.rs
Comment thread feather/common/src/entities/player.rs Outdated
Comment thread feather/datapacks/src/recipe.rs Outdated
Comment thread feather/datapacks/src/tag.rs Outdated
Comment thread feather/protocol/src/io.rs Outdated
Comment thread feather/server/src/systems/player_leave.rs Outdated
Comment thread libcraft/inventory/Cargo.toml Outdated
Comment thread libcraft/items/src/item_stack.rs

pub struct ItemStackBuilder {
item: Item,
count: NonZeroU32,

@ambeeeeee ambeeeeee Sep 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this a U32 when count is a u8?

@Tracreed Tracreed Sep 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

count is a u32.

@Defman Defman Sep 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Minecraft uses i32?

@Miro-Andrin Miro-Andrin Sep 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It might be the case that ItemStacks should allow for negative stack sizes. However,
i believe this is outside the scope of this refactor. That was how ItemStack were modelled when we started. This "issue" should be revisited on a separate pull request.

Defman
Defman requested changes Sep 27, 2021
View reviewed changes
Comment thread feather/server/src/packet_handlers/inventory.rs Outdated
Comment thread feather/server/src/systems/player_leave.rs Outdated
@Defman Defman merged commit 1605738 into feather-rs:main Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Defman Defman Defman approved these changes

+2 more reviewers

@Tracreed Tracreed Tracreed left review comments

@ambeeeeee ambeeeeee ambeeeeee requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Miro-Andrin @Defman @ambeeeeee @Tracreed