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

Book cloning fix #4784

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Book cloning fix #4784

wants to merge 6 commits into from

Conversation

YHDiamond
Copy link
Contributor

This is a bug fix to allow copying books to work 100% of the time and closes #3682. This is an adaption of a patch Camotoy wrote a few years ago, I changed it to work with the modern codebase and fixed an issue or two. This needs more testing to ensure that it works 100% of the time and that it doesn't break any existing crafting recipes. The code can likely be rewritten to be neater and more modular but I did not get around to that yet.

/**
* Book cloning recipe ID; stored separately as its recipe works differently from others.
*/
public static final int BOOK_CLONING_RECIPE_ID = 278;
Copy link
Member

Choose a reason for hiding this comment

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

This can't be hardcoded, as it could differ between two sessions on standalone working with different recipes.

@@ -376,7 +382,7 @@ private void reduceCraftingGrid(boolean makeAll) {
for (int i = 0; i < gridSize; i++) {
final int slot = i + 1;
GeyserItemStack item = getItem(slot);
if (!item.isEmpty()) {
if (!item.isEmpty() && (!handleBookRecipe || item.getJavaId() != session.getItemMappings().getStoredItems().writtenBook().getJavaItem().javaId())) {
Copy link
Member

Choose a reason for hiding this comment

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

The second comparison can now be item.asItem() == Items.WRITTEN_BOOK

Copy link
Member

Choose a reason for hiding this comment

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

Did you mean the third comparison? And != instead of ==?

If the clone recipe is occurring, and then we should only sub items that are not the written book

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 just committed to make the changes the way Konicai described, if that's not what Camotoy intended I can change it back

@onebeastchris onebeastchris added PR: Bugfix When a PR contains a bugfix PR: Under Review When a PR (particularly a large one) is currently being reviewed to see if it can be merged labels Jun 22, 2024
Konicai
Konicai previously requested changes Jul 9, 2024
Copy link
Member

@Konicai Konicai left a comment

Choose a reason for hiding this comment

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

side note, InventoryTranslator is now over 1k lines 🥴

@Konicai Konicai dismissed their stale review July 9, 2024 03:25

addressed

@AJ-Ferguson
Copy link
Member

Did some tests. When cloning multiple books at once the request is rejected because timesCrafted != 1. Also, after cloning a book, attempts to move the original book out of the crafting grid is rejected.

I had to comment out updateCraftingGrid from JavaContainerSetSlotTranslator to consistently reproduce these bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bugfix When a PR contains a bugfix PR: Under Review When a PR (particularly a large one) is currently being reviewed to see if it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot Copy Books
5 participants