-
Notifications
You must be signed in to change notification settings - Fork 171
Added travel using an elytra feature, the RepairToolTask and some edits around that #194
base: main
Are you sure you want to change the base?
Conversation
… of 2) and added fireworks requirement for wip elytra task
…le MobDefenseChain and MLGBucketFallChain, and made a more functionnal GetToXZWithElytraTask
…unexpectedly and some improvements
…it, it will use bottle of exp and will kill some zombies if doesn't have, and some edits to the getToXZWithElytraTask
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.
Nice! Hyped to merge this in, just got some code recommendations to go through first.
src/main/java/adris/altoclef/tasks/movement/GetToXZWithElytraTask.java
Outdated
Show resolved
Hide resolved
return null; | ||
} | ||
private boolean isOnGround(AltoClef mod) { | ||
return mod.getPlayer().isSwimming() || mod.getPlayer().isTouchingWater() || mod.getPlayer().isOnGround() || mod.getPlayer().isClimbing(); |
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.
My fault but this code is duplicated like 3 times, it would be nice to add a utility method to EntityHelper
called isGrounded(Entity entity)
and isGrounded() // defaults to player
|
||
//Equip elytra, if didn't equipped, | ||
setDebugState("Equipping elytra"); | ||
if (StorageHelper.getItemStackInSlot(new PlayerSlot(6)).getItem() != Items.ELYTRA) { |
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.
Please use PlayerSlot.ARMOR_CHESTPLATE_SLOT
instead of PlayerSlot(6)
_fz = 0; | ||
mod.getBehaviour().disableDefence(false); //Enable mob defence | ||
if ((int)dist == 0) { //We are where we need to go ! | ||
_isFinished = true; //End the task |
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 would get rid of _isFinished
and instead add the check in the isFinished
method for whether we're close enough. Also I'd use WorldHelper.inRangeXZ(mod.getPlayer(), _x, _z)
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.
Why is mob defense even disabled? Doesnt that mean phantoms and mobs that shoot can get us mid-flight?
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.
It disable mob defense while flying because the bot try to avoid it's own firework when using it. and i think the player move faster than the phantoms while flying
_isFinished = true; //End the task | ||
return null; | ||
} | ||
if (dist < 128) { //We are near our goal |
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'd add a constant at the top of the file so we don't have magic numbers
ex.
private static final int CLOSE_ENOUGH_TO_WALK = 128;
protected Task onTick(AltoClef mod) { | ||
//We start this task by filtering out every item type that we can't repair : | ||
//All items without mending or with no damage | ||
_toRepair = Arrays.stream(_toRepair).filter(target -> needRepair(mod, target)).toArray(ItemTarget[]::new); |
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'd make _toRepair
final
and make a local variable ItemTarget[] shouldRepair
here or something. Instance variables should be final if they're compared to in isEqual
, because otherwise the task may continuously re-run onStart
|
||
List<Slot> SlotRepair = mod.getItemStorage().getSlotsWithItemPlayerInventory(false, ItemTargetRepair.getMatches()); //And we get a list of every slot with that item | ||
|
||
Slot SlotRepairTarget = new PlayerSlot(-1); //Placeholder slot, will never get used if not replaced |
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.
Why not use null as a placeholder instead? -1 can be mistaken for the cursor slot.
if (ItemTargetOPTRepair.isPresent()) { //If the list is not empty | ||
ItemTarget ItemTargetRepair = ItemTargetOPTRepair.get(); //We get the (real) first item on the list | ||
|
||
List<Slot> SlotRepair = mod.getItemStorage().getSlotsWithItemPlayerInventory(false, ItemTargetRepair.getMatches()); //And we get a list of every slot with that item |
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.
Java uses lowerCamelCase for local variables (slotRepair
, not SlotRepair
)
return null; | ||
} | ||
//Get the nearest experience orb | ||
Optional<Entity> expentityOPT = mod.getEntityTracker().getClosestEntity(mod.getPlayer().getPos(), ExperienceOrbEntity.class); |
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.
This is an annoying gotcha, but avoid always gunning for the closest entity/block. There could be a ping-pong pattern of the bot getting stuck between two targets (https://youtu.be/uROEqwyzn3o?t=6946)
Using DoToClosest ... Task
can fix this, as it checks for this bug.
return new DoToClosestEntityTask(entity -> {
// Make sure our repair item is equipped when we're about to collect the orb!
if (entity.isInRange(mod.getPlayer(), 3)) {
mod.getSlotHandler().forceEquipSlot(SlotRepairTarget);
}
return new GetToEntityTask(entity);
});
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.
Interesting ... maybe this is why my @pickup task likes to shit itself sometimes
} | ||
|
||
//Test if the itemstack have mending on it | ||
public static boolean haveMending(AltoClef mod, ItemStack target) { |
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.
EnchantmentHelper.get(itemStack).containsKey(Enchantments.MENDING);
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.
Nice! Am excited to merge this in, just have some code suggestions to check first.
Please fix and merge! Much love <3 |
Still WIP |
Sounds good! When you're done remove the |
Okay, I made the requested changes, is there still anything else to change in this code :) ? |
Any update on this now that the code has been changed? |
//Way of disabling MLG and defense from a task | ||
public boolean isDefenseDisabled() { | ||
return current().disableDefence; | ||
} |
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.
Consider making mob defence enabled/disabled an interface. We can talk about how to integrate this later. Discord me if you are interested in learning about this,
I started woking on this about a week ago mostly for fun, and i think it's ready
In this pull request :
@elytra <x> <z>
@test repair
And i think i listed everything that i added/edited in that PR.