-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix GT tools not working for some mod actions #1706
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -21,12 +21,14 @@ | |||
import net.minecraft.world.World; | ||||
import net.minecraftforge.common.util.FakePlayer; | ||||
|
||||
import java.util.ArrayList; | ||||
import java.util.Collections; | ||||
import java.util.List; | ||||
import java.util.*; | ||||
|
||||
public class ToolJackHammer extends ToolDrillLV { | ||||
|
||||
private static final Set<String> HAMMER_TOOL_CLASSES = new HashSet<String>() {{ | ||||
add("pickaxe"); add("hammer"); | ||||
}}; | ||||
Comment on lines
+28
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing as other comment, perhaps also should be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It uses "hammer" in canMineBlock, so it should be consistent:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All these if statements could be changed to something like:
It will be slightly less efficient, but easier to maintain in future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I clearly meant the name of the constant, not the value of the strings in the Set.
Not sure what this has to do with this block of code; perhaps create a separate "conversation" linked to the code you're talking about so we have the appropriate context. |
||||
|
||||
private static final ModeSwitchBehavior<JackHammerMode> MODE_SWITCH_BEHAVIOR = new ModeSwitchBehavior<>(JackHammerMode.class); | ||||
|
||||
public enum JackHammerMode implements ILocalizationKey { | ||||
|
@@ -202,4 +204,9 @@ private static BlockPos rotateVertical(BlockPos origin, int x, int y, EnumFacing | |||
default: return BlockPos.ORIGIN; | ||||
} | ||||
} | ||||
|
||||
@Override | ||||
public Set<String> getToolClasses(ItemStack stack) { | ||||
return HAMMER_TOOL_CLASSES; | ||||
} | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to use
ImmutableSet.of("hammer", "pickaxe")
or similar here. If the Set itself is mutable then a caller could modify it, and the double-brace approach will also create an anonymous inner class which is probably not desirable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think these should be immutable from our side to keep them safe. Regarding double braces they have to be addressed too.