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

Add Crafting command #3960

Draft
wants to merge 44 commits into
base: master
Choose a base branch
from
Draft

Add Crafting command #3960

wants to merge 44 commits into from

Conversation

rycbar0
Copy link
Contributor

@rycbar0 rycbar0 commented May 28, 2023

I want to be honest, as it is, its unusable. Maybe someone has a idea how to salvage it. My main problem is that items from blocks like oak planks and spruce planks or stone and granite, are the same item and i dont know how to handle them. Also some items have recipes that allow different ingredients (example chests) and i havnt yet figured out how to pick whats available instead of a fixed ingredient. If this problems cant be solved i cant see this ever working. some problems like checking if we have the resources to craft are related to this, others like placing a crafting table when non is available arnt a priority aslong as first one exist.

Whats missing (list not complete):

  • place crafting table if non is available
  • cancel operation and inform player if low on resources
  • proper way to get a item from a string
  • proper way to get a recipe from a item

What it can do:

  • walk to a crafting table and craft. (only works with some items, example #craft stick 200, #craft stone_axe)

PS: deconstructing recipies is not planed. if you want a wooden_pickaxe but you only have log's it wont automaticly craft planks and sticks.

@strubium
Copy link

strubium commented Jun 2, 2023

This is looking really nice! I'll test it out tomorrow.

You're a real coding wizard!

@ZacSharp
Copy link
Collaborator

ZacSharp commented Jun 7, 2023

Forgot to include in my review:
The scope looks simple and small. That's good.
Not sure what that autoCraft setting is supposed to be though. So far you don't seem to plan on automatically starting crafting (I think that's good) and from it's documentation it seems to be a setting to disable #craft?

@strubium
Copy link

strubium commented Jun 8, 2023

Forgot to include in my review: The scope looks simple and small. That's good. Not sure what that autoCraft setting is supposed to be though. So far you don't seem to plan on automatically starting crafting (I think that's good) and from it's documentation it seems to be a setting to disable #craft?

I kinda just threw that in. It was meant to make baritone walk over to a crafting table and then open it, letting the user craft their own thing. It probably will be removed.

Nevermind its been removed

@UltraBlackLinux
Copy link

altoclef is gonna be obsolete soon

@rycbar0
Copy link
Contributor Author

rycbar0 commented Jun 12, 2023

this is what it can do at the moment. i will need help with coding the "placing crafting tables" part. i thought about adding something similar for furnaces but they are extremely slow and idk how i could split the smelting on multiple furnaces. i dont think brewing, smithing, enchanting or trading are needed.
https://youtu.be/W6w_uHJijzw

@ZacSharp
Copy link
Collaborator

For placing I know brute force is inefficient (very limited range if you don't want to cause a lagspike) so I'd suggest just doing something like what BuilderProcess uses to determine placeable positions and if that fails just walk away a bit. This covers fewer positions but is much lighter on performance and I can't really imagine why it would do much worse. In most cases the first scan will find a position and that's it, in most other cases it will be able to place were it was standing or after breaking a block for tunneling.

Two problems with current error handling:

  1. If an unexpected screen or no screen is open, either fail with a clear error message or wait a couple seconds and then close it and attempt to open the crafting screen again. If the unexpected screen is chat then log a single message instead of closing it and wait until it is closed. That way the user has time to cancel the task and also knows this is intentional. Canceling while Baritone is opening 10+ GUIs per second is basically impossible without alt + f4.
  2. If it can't click the table despite being next to it, log a message and fail. Don't crash the game for no reason.

Copy link
Collaborator

@ZacSharp ZacSharp left a comment

Choose a reason for hiding this comment

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

You said it's ready so here we go. Running a formater won't hurt and I also won't nitpick about wording of chat messages yet.

One thing you should definitely do (and probably before anything else I mention here) is stripping down the code copied from BuilderProcess. When I said "doing something like what BuilderProcess uses" I didn't mean "using the exact same code as BuilderProcess". We don't need to place arbitrary blocks here so there's no need for all that general purpose placing code. We already know we have a valid item (the crafting table) and that it will place the correct thing and don't need to worry about managing hotbar slots. All we need to know is where to place that specific item.

When reading the code for item parsing I also wondered why you fixed it to crafting one specific recipe instead of a list of recipes. CraftCommand would create a list of all recipes producing a correct item and CraftCommand would use whichever of them is possible and fail if no recipe is possible but it still has to craft more.

In general I had a hard time figuring out the control flow here. Like, why does placeCraftingtableNearby call getACraftingTable after it is done? (because getACraftingTable doesn't get crafting tables, it walks to them and opens them)
Your field names also have similar naming problems (e.g. placeCraftingTable stores whether it is placingCraftingTable) and using the type of goal to distinguish between walking to a crafting table and placing one deserves at least a comment. The better variant would be using fields with just a single purpose which can then be reflected by their name.
If you instead want an alternative way of structuring the code without an actual state machine you could also check things like "crafting gui open? -> (items in place? -> take result. else move items) else if crafting table known? -> walk there and klick it else try placing".

int amount = args.hasAny() ? args.getAs(Integer.class) : 1;

if (item == null) {//this is hacky, but it gets the job done for now. also we arnt interested in non craft-able items anyway
itemName = itemName.replace("_", " ");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not reverse the order to be [quantity] <item> like for #mine? That way you can try getting an integer (is there something called args.getAsOrDefault?) and then an item and if the item fails you can do args.rawRest instead of replacing underscores with spaces.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better to first get the item by id or name and then do the same for both. You should at least move the item-by-name thingy into a helper method which returns just an item (no logging etc.).

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 understand the logic behind the first comment but not the second.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Basically I mean you should first get the item by either method (id or name) and then continue along the same code path for both cases.
Currently this is

get item by id
if that failed
      get by name
     handle all the resource counting
else
     handle all the resource counting slightly differently

and I think it should be

get item by id
if that failed
    get item by name
handle all the resource counting (the same for both)

Why should there be a difference between #craft wooden_sword and #craft wooden sword?
Not sure how easy that is though, due to the way you use items in the id case and recipes in the name case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it maybe doesnt make that much sense with a wooden sword as there only exist 1 wooden sword.
take the example of a red bed

if you type: #craft bed it will craft any bed this can be a white bed, a blue bed or a red bed.
if you type #craft red bed it wont find a item that is named like that so it searches a recipe that produces a item stack with that display name. note here that there are 2 recipes for red beds, one with red wool and one dying a white bed.

i use crafItem() if i want any instance of that item crafted and i use craftRecipe() if i want baritone to use that specific recipe to craft

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you get all recipes that produce the desired item (be it by name or id) and then use that collection of recipes from that point on? That way there is only one case (n recipes) instead of two. In general even if the user specified one exact item there can still be multiple recipes for it and the current recipe switching mechanism is sketchy (doesn't it even bypass what you said about #craft red bed because redBedRecipe.getRecipeOutput().getItem() == Items.BED and getting a recipe for that doesn't need to return redBedRecipe again?).

src/main/java/baritone/process/CraftingProcess.java Outdated Show resolved Hide resolved
}
}

private void selectCraftingTable() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should use

public boolean throwaway(boolean select, Predicate<? super ItemStack> desired, boolean allowInventory) {
instead. It handles all of the searching and respects allowInventory and inventoryMoveOnlyIfStationary and ticksBetweenInventoryMoves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done but this method name is not representative of what it is doing. its probably a historical reason why its called that but throwaway sounds like it will throw this items out of its inventory. something like hasAndOptionalSelect() would be more fitting.

Copy link
Collaborator

@ZacSharp ZacSharp Jun 20, 2023

Choose a reason for hiding this comment

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

It's made to select throwaway items, yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can take time though (inventory movement delay or waiting until stationary) so you should make sure you only use the item once it's actually there.

src/main/java/baritone/process/CraftingProcess.java Outdated Show resolved Hide resolved
src/main/java/baritone/process/CraftingProcess.java Outdated Show resolved Hide resolved
src/main/java/baritone/process/CraftingProcess.java Outdated Show resolved Hide resolved
src/main/java/baritone/process/CraftingProcess.java Outdated Show resolved Hide resolved
private IRecipe recipe;
private Goal goal;
private boolean placeCraftingTable = false;
private boolean clearToPush = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This variable is weird. It is set right from the start but there isn't even anything to push until the crafting gui is opened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it exists to prevent update lag/desync of the inventory and what we are counting. if you have a better solution to do that im open for suggestions.

Copy link
Member

@ZeroMemes ZeroMemes left a comment

Choose a reason for hiding this comment

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

Didn't look over thoroughly, just suggesting changes to fix mc references

src/main/java/baritone/process/CraftingProcess.java Outdated Show resolved Hide resolved
src/main/java/baritone/process/CraftingProcess.java Outdated Show resolved Hide resolved
@rycbar0
Copy link
Contributor Author

rycbar0 commented Jun 17, 2023

How should i resolve the merge conflict? upstream merge with master?

@ZacSharp
Copy link
Collaborator

Merging master into your branch again is probably the easiest way, yes.

# Conflicts:
#	src/main/java/baritone/Baritone.java
Copy link
Collaborator

@ZacSharp ZacSharp left a comment

Choose a reason for hiding this comment

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

I didn't look at it very closely again, since there's still things from the previous review remaining.
If a review comment is dealt with you can mark it as resolved so it's easier to see what's still open.

public void execute(String label, IArgConsumer args) throws CommandException {
int amount = args.getAsOrDefault(Integer.class, 1);

Item item = args.getAsOrNull(Item.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I'll take that as my fault. The method I wanted was getDatatypeForOrNull (basically the same but works without the wrapper arg parser) and I should have looked it up instead of guessing a name which ended up existing with slightly different functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no IDatatypeFor for Integers so i would have to add that before i could use getDatatypeForOrDefault() which in my opinion is nonsense. For the item, what is wrong with getAsOrNull()? i will do it but i dont understand why.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can freely mix datatypes and arg types so you can use getAsOrDefault for the integer and getDatatypeForOrNull for the item. That way you don't need to create a new parser for either of them.

Comment on lines +104 to +105
} catch (Exception e) {
logDirect("Error! Did you close the crafting window while crafting process was still running?");
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can easily detect a closed crafting screen so no need to assume it as the reason of any exception.

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 original added this try catch block to catch casting errors from generic Container to CraftingContainer, this however should no longer happen because the casting now checks beforehand if it can cast. (see getInputCount()) in theory the try catch block could be removed but i left it in should there be a edge case i didnt think of.

}

private List<BetterBlockPos> blockPosListSortedByDistanceFromCenter() {
//this is so stupid idk why im doing this
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try just creating the full list and then sorting it? If that's too slow (doubt it) there's ways to generate the positions in order.

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 will probably be the last thing i change for the simple fact that it works with or without a sorted list. it doesnt matter if the table is placed right next to the player or 2 blocks over. this is a pure quality of life feature and will simply be kicked if it isnt up to standards. also i dont care about the efficiency of this method as it only ever be called once if you need to place a table.

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

Successfully merging this pull request may close these issues.

5 participants