Add Crafting command

[rycbar0]

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]

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

You're a real coding wizard!

[ZacSharp]

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]

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]

altoclef is gonna be obsolete soon

[rycbar0]

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]

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.

[ZacSharp]

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

[ZacSharp]

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.

[ZacSharp]

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

[rycbar0]

i understand the logic behind the first comment but not the second.

[ZacSharp]

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.

[rycbar0]

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

[ZacSharp]

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

[ZacSharp]

You should use

baritone/src/main/java/baritone/behavior/InventoryBehavior.java

Line 182 in 26a2945

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.

[rycbar0]

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.

[ZacSharp]

It's made to select throwaway items, yes.

[ZacSharp]

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.

[ZacSharp]

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.

[rycbar0]

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.

[ZeroMemes]

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

[rycbar0]

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

[ZacSharp]

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

[ZacSharp]

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.

[ZacSharp]

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.

[rycbar0]

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.

[ZacSharp]

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.

[ZacSharp]

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

[rycbar0]

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.

[ZacSharp]

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.

[rycbar0]

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.