Update grinding handler to use in-world crafting for grindstone or obsidian #277

2023-07-10T15:42:43Z

NathanDSimon commented

2023-07-10 15:42:43 +00:00

(Migrated from github.com)

Crying obsidian also works and has a 1 in 3 chance to double the output (for grains of infinity) or cut the input cost by 66% (for coal dust).

Description

Since the grindstone craft was crashing, this implementation relies on the right click block event, such that a player can sneak and right-click a grindstone or obsidian block to perform the craft.

Fixes issue(s): #196

Crying obsidian also works and has a 1 in 3 chance to double the output (for grains of infinity) or cut the input cost by 66% (for coal dust). # Description Since the grindstone craft was crashing, this implementation relies on the right click block event, such that a player can sneak and right-click a grindstone or obsidian block to perform the craft. Fixes issue(s): #196

Trytoon commented

2023-07-10 15:49:16 +00:00

(Migrated from github.com)

Hey !
A new contributor :)

I can't review code but I'm pretty sure that they are going to notice that:
The if/else block should be written in java as

if (condition) {

}

instead of what you wrote

if(condition)
{
}

Hey ! A new contributor :) I can't review code but I'm pretty sure that they are going to notice that: The if/else block should be written in java as if (condition) { } instead of what you wrote if(condition) { }

NathanDSimon commented

2023-07-10 15:51:13 +00:00

(Migrated from github.com)

Darn it, I did it again.
I can't believe it.
This is the C++ programmer in me, old habits die hard.
XD

Darn it, I did it again. I can't believe it. This is the C++ programmer in me, old habits die hard. XD

NathanDSimon commented

2023-07-10 15:59:52 +00:00

(Migrated from github.com)

I fixed the formatting, also streamlined a couple of if statements

justliliandev (Migrated from github.com) requested changes

2023-07-10 16:54:42 +00:00

src/main/java/com/enderio/base/common/handler/GrindingHandler.java

					
				@ -3,53 +3,59 @@ package com.enderio.base.common.handler;

				import com.enderio.EnderIO;

justliliandev (Migrated from github.com) commented

2023-07-10 16:47:48 +00:00

instead of the Blocks.OBSIDIAN check use target.is(Tags.Block.OBSIDIAN)

instead of the Blocks.OBSIDIAN check use `target.is(Tags.Block.OBSIDIAN)`

justliliandev (Migrated from github.com) commented

2023-07-10 16:49:24 +00:00

Split this guard clause into two

        if (!player.isCrouching())
            return;
        if (target != Blocks.OBSIDIAN && target != Blocks.GRINDSTONE && target != Blocks.CRYING_OBSIDIAN) || !(offhand.is(Items.FLINT)))
            return;

Split this guard clause into two ```suggestion if (!player.isCrouching()) return; if (target != Blocks.OBSIDIAN && target != Blocks.GRINDSTONE && target != Blocks.CRYING_OBSIDIAN) || !(offhand.is(Items.FLINT))) return; ```

justliliandev (Migrated from github.com) commented

2023-07-10 16:52:58 +00:00

cancel it for all cases, to mark that we have done something and others don't do further interactions

NathanDSimon commented

2023-07-10 17:08:20 +00:00

(Migrated from github.com)

I did all the above, and I also moved the crouching check to before the getter calls to save a few instructions in the manifold cases where the player clicks a block without crouching

justliliandev (Migrated from github.com) requested changes

2023-07-10 19:05:26 +00:00

justliliandev (Migrated from github.com) left a comment

most of the checks are kinda wrong now.
you don't check the target anymore for the 2 valid blocks and the valid tag, resulting in all blocks being a valid shift right click target.
you don't check if you are serverside for the inventory manipulation, which could result in desynced inventories and duplicated item removal in creative mod. Add a check for that before you manipulate the inventory.
the mainhand check in the guard clause is not useless.
cancel the event only if the mainhand items are correct, not in general after the big if/else block

most of the checks are kinda wrong now. you don't check the target anymore for the 2 valid blocks and the valid tag, resulting in all blocks being a valid shift right click target. you don't check if you are serverside for the inventory manipulation, which could result in desynced inventories and duplicated item removal in creative mod. Add a check for that before you manipulate the inventory. the mainhand check in the guard clause is not useless. cancel the event only if the mainhand items are correct, not in general after the big if/else block

NathanDSimon commented

2023-07-10 19:28:43 +00:00

(Migrated from github.com)

Whoops, I didn't mean to delete the target block check...lol.
Also I don't know how the event set canceled call ended up outside the if/else block, that was also probably an editing error.
Added a serverside check at the very beginning of the method.

Whoops, I didn't mean to delete the target block check...lol. Also I don't know how the event set canceled call ended up outside the if/else block, that was also probably an editing error. Added a serverside check at the very beginning of the method.

Rover656 commented

2023-07-10 21:41:49 +00:00

(Migrated from github.com)

(Not to be a pain, could you also add your changes into CHANGELOG.md? Cheers!)

(Not to be a pain, could you also add your changes into `CHANGELOG.md`? Cheers!)

justliliandev (Migrated from github.com) requested changes

2023-07-10 21:56:40 +00:00

justliliandev (Migrated from github.com) left a comment

After this and the changelog this looks fine

src/main/java/com/enderio/base/common/handler/GrindingHandler.java

					
				@ -3,53 +3,59 @@ package com.enderio.base.common.handler;

				import com.enderio.EnderIO;

justliliandev (Migrated from github.com) commented

2023-07-10 21:56:16 +00:00

Replace the obsidian check with the obsidian Tag check. The TagKey can be Found in Tags.Blocks.OBSIDIAN

justliliandev (Migrated from github.com) reviewed

2023-07-10 22:00:25 +00:00

src/main/java/com/enderio/base/common/handler/GrindingHandler.java

					
				@ -3,53 +3,59 @@ package com.enderio.base.common.handler;

				import com.enderio.EnderIO;

justliliandev (Migrated from github.com) commented

2023-07-10 22:00:25 +00:00

oh, and can you check, that there is a space between the if and the (

oh, and can you check, that there is a space between the `if` and the `(`

NathanDSimon (Migrated from github.com) reviewed

2023-07-10 22:19:54 +00:00

src/main/java/com/enderio/base/common/handler/GrindingHandler.java

					
				@ -3,53 +3,59 @@ package com.enderio.base.common.handler;

				import com.enderio.EnderIO;

NathanDSimon (Migrated from github.com) commented

2023-07-10 22:19:53 +00:00

C++ Coder habits again 😛
Let me know if what I wrote in the changelog is too wordy

C++ Coder habits again 😛 Let me know if what I wrote in the changelog is too wordy

justliliandev (Migrated from github.com) reviewed

2023-07-10 22:24:48 +00:00

src/main/java/com/enderio/base/common/handler/GrindingHandler.java

					
				@ -3,53 +3,59 @@ package com.enderio.base.common.handler;

				import com.enderio.EnderIO;

justliliandev (Migrated from github.com) commented

2023-07-10 22:24:48 +00:00

that one was close, crying obsidian is not in the obsidian tag, so that check needs to persist

justliliandev (Migrated from github.com) approved these changes

2023-07-10 22:31:51 +00:00

Rover656 commented

2023-07-11 07:27:35 +00:00

(Migrated from github.com)

Looks great, thanks a bunch!! Also thanks again agnor for the review

Rover656 commented

2023-07-11 07:28:28 +00:00

(Migrated from github.com)

Oh looks like some conflicts have shown up, if you're happy to wrangle them, feel free, if not i can do that at merge after work :)

Rover656 (Migrated from github.com) reviewed

2023-07-11 11:30:06 +00:00

Rover656 (Migrated from github.com) left a comment

Actually, I just realised - we need to update JEI 😅. Will need to at least remove the old recipe stuff for the grindstone but maybe add something for this too if you could. If you can't, don't worry; I've an idea on a way it could look.

Sign in to join this conversation.

No reviewers

justliliandev