Nasty parts of Baritone

[leijurv]

I am going to write down the nasty stuff. I won't include small issues, this is more of big structural problems. For example, InventoryBehavior could be better about figuring out what to put where on the hotbar, and it could be less hardcoded about the pickaxe and the throwaway blocks. This could be fixed without too much effort, just within that class. That's an example of something that isn't listed.

MovementHelper checks. Endlessly expanding nests of if statements, that get worse as we get into newer versions of MC. This is not good for performance. This is pretty much entirely fixed by #3479

World height in chunk caching. This is a big stinky, see #3375 and #3815. The on disk format is also nasty, for example it uses strings for blocks, and it wasn't updated between 1.12 and 1.13. Some blocks have the same name between those versions, some don't.

Basically all the movements executors. For example, MovementFall has this absolutely nasty bit of code

baritone/src/main/java/baritone/pathing/movement/movements/MovementFall.java

Lines 134 to 167 in 9c64067

	Vec3d destCenter = VecUtils.getBlockPosCenter(dest); // we are moving to the 0.5 center not the edge (like if we were falling on a ladder)
	if (Math.abs(ctx.player().posX + ctx.player().motionX - destCenter.x) > 0.1 || Math.abs(ctx.player().posZ + ctx.player().motionZ - destCenter.z) > 0.1) {
	if (!ctx.player().onGround && Math.abs(ctx.player().motionY) > 0.4) {
	state.setInput(Input.SNEAK, true);
	}
	state.setInput(Input.MOVE_FORWARD, true);
	}
	Vec3i avoid = Optional.ofNullable(avoid()).map(EnumFacing::getDirectionVec).orElse(null);
	if (avoid == null) {
	avoid = src.subtract(dest);
	} else {
	double dist = Math.abs(avoid.getX() * (destCenter.x - avoid.getX() / 2.0 - ctx.player().posX)) + Math.abs(avoid.getZ() * (destCenter.z - avoid.getZ() / 2.0 - ctx.player().posZ));
	if (dist < 0.6) {
	state.setInput(Input.MOVE_FORWARD, true);
	} else if (!ctx.player().onGround) {
	state.setInput(Input.SNEAK, false);
	}
	}
	if (targetRotation == null) {
	Vec3d destCenterOffset = new Vec3d(destCenter.x + 0.125 * avoid.getX(), destCenter.y, destCenter.z + 0.125 * avoid.getZ());
	state.setTarget(new MovementTarget(RotationUtils.calcRotationFromVec3d(ctx.playerHead(), destCenterOffset, ctx.playerRotations()), false));
	}
	return state;
	}
	private EnumFacing avoid() {
	for (int i = 0; i < 15; i++) {
	IBlockState state = ctx.world().getBlockState(ctx.playerFeet().down(i));
	if (state.getBlock() == Blocks.LADDER) {
	return state.getValue(BlockLadder.FACING);
	}
	}
	return null;
	}

that governs how it strafes in midair while falling. It combines looking around, WASD, and sneaking. I tested this by making a 1x1 column of air through lava, and seeing if it could properly dodge ladders without ever touching any of the four lava walls. I have no idea if my testing was thorough. I have no idea if this works beyond 1.12.2. I have no idea how any of it works. The weird logic around when to start or stop sneaking is crazy. You may have noticed that baritone sneaks for a little bit at the beginning of every fall, this is why. Also PathExecutor just makes the whole thing much worse with all its bajillion special cases that override the individual movements.

BuilderProcess. All of it.

InputOverrideHandler. Baritone was rewritten in #245 to have processes, in which Baritone calls each process and asks it what it wants to do. Previously, processes would call a function to set the goal, now Baritone chooses which is in control and asks it what the goal should be. This is an improvement because it structurally prevents nastiness and processes stepping on each other's paws. Sadly, the same treatment was not given to InputOverrideHandler. There is a start, which is how MovementState holds the desired keyboard/mouse inputs of each movement, which is then applied in the superclass Movement.update. However, this hybrid is a worst of both worlds, in which Movements use a different way of controlling the keyboard/mouse than the rest of Baritone, but there still is no centralized authority that reaches out rather than being called. As an example, right now BackfillProcess and BuilderProcess directly set keyboard/mouse inputs, and there's complicated code to avoid conflicting with a path that is trying to execute at the same time. Instead, the PathingControlManager should ask the process whether it wants to execute a path or control keyboard/mouse (it can't do both). If a path, it should then ask the PathingBehavior to ask the PathExecutor to ask the current Movement what the keyboard/mouse should be. This way there is no possibility of conflict and stepping on each other's paws.

PathingControlManager and also PathingCommandType. The difference between REVALIDATE_GOAL_AND_PATH and FORCE_REVALIDATE_GOAL_AND_PATH is so arcane and weird that it's almost impossible to reason about except through guess and check. Essentially, during the rewrite around #245, I wanted to preserve reasonable behavior when switching between processes. For example, when following a player, if you ran the command to follow a different player it should immediately stop walking towards the old one, then compute a path to the new one. There were a million tiny little behaviors like that. There were also issues with recomputing goals. For example, when running MineProcess, the goal is constantly changing as ores enter and exit your render distance. With FollowProcess, the goal changes all the time as the followed entity moves. It simply doesn't work to throw away and recompute the entire path every time the goal changes, if the destination is more than a few blocks away a new path can't be computed before the next tick, so the player just stands still and constantly computes paths and throws them away before even moving a single tick. Therefore, some kind of logic is needed for when a path needs to be thrown away, which I generally called "revalidating". With MineProcess, it's simple: if the path ended at the goal, and still ends at the current goal, then stick with it. This works fine, and it helps it ignore ores that are far away popping into and out of existence. But there's endlessly complicated logic. So, here goes. I am going to admit and point out the single nastiest line of code in all of Baritone, here it is: here As you can see, we're deciding whether to recompute the path based on the goal expressed as a string. This was helpful because constructing a new Goal for the same destination would stay the same (e.g. if your followed entity was not moving at all), and also because a Goal with no toString implementation would use the default, which is the hexadecimal of the hashcode, which is rerolled if you reconstruct the goal, but stays the same otherwise. It was like a halfway mix between == and an actual equals implementation. Anyway, all the interactions between processes and the overall decision making of "when do I throw away the current path, and when do I keep it" is incredibly intricate, brittle, and nasty. If you don't believe me, check this out: you wouldn't expect that turning on the setting #censorCoordinates would cause MineProcess and FollowProcess to glitch out, but it will. This is because all the Goal toString implementations call SettingsUtil.maybeCensor. When you turn on censorCoordinates, all your goals look like GoalBlock{x=<censored>, y=<censored>, z=<censored>}. Now, even when the actual goal changes, the toString representation of it doesn't, so PathingControlManager thinks that there's no need to cancel the current path, even in the strictest PathingCommandType of FORCE_REVALIDATE_GOAL_AND_PATH, so following entities and switching goals and probably a bajillion other things entirely stop working. That last bit is fixed by #3984

Honorable mention: canceling. Everything having to do with canceling is scuffed, but scuffed in a way that it kind-of has to be. The simple reason is that some movements are not cancelable. If you're in the middle of a parkour jump or water bucket fall, while trying to follow an entity, and now the entity has moved and your path has to be recomputed, you cannot simply discard the current path, you'd die. This results in some insanely complicated logic (such as mentioned before with InputOverrideHandler). For example, just search for "cancel" in PathingBehavior, and you'll see such beauties as secretInternalSegmentCancel, cancelEverything, safeToCancel, cancelRequested, softCancelIfSafe, forceCancel. This also causes friction with the InputOverrideHandler issues above, for example through how processes need to be told isSafeToCancel every tick so that they can decide whether it's safe to force the player's look direction or whether the current movement critically needs it to prevent the player from dying (this is a terrible hack solution).

Also every "pathing" should really be "pathfinding".

[wagyourtail]

add using Block instead of BlockState everywhere, baritone shouldn't trip on schematics with non-default block states, or have issues with creating a mine selector for a specific state of a block, or need to unbox the block state to block in many places, this would also result in a performance improvement probably as mc chunks store block states so turning them into blocks wouldn't be needed

[ZacSharp]

should ask the process whether it wants to execute a path or control keyboard/mouse (it can't do both).

I think I have to contradict you here. Things like #3133 are perfectly valid and desireable so processes should still be able to partially override movements, just not like it is currently done. E.g. MovementTraverse without bridging doesn't need any rotations so it could walk sideways (or even backwards) as needed while PathingBehavior takes mouse inputs from BuilderProcess.
Basically PathingBehavior would ask the process for desired inputs and a goal, transferring control between movements and process roughly like

1. If there's an uncancellable movement in control it stays in control
2. If no goal is provided the process takes control
3. If a goal is provided process inputs will only be applied if they don't conflict with the movement (conflict groups will probably be something like KEYBOARD, MOUSE or WASD, SHIFT, SPACE, LOOK, CLICK)
   If you have a clean solution to also allow conditioning non-input actions on the input or allowing partial input execution (e.g. the process supplies KEYBOARD and MOUSE inputs but only MOUSE would also be acceptable) that could be added, however I think all-or-nothing is enough.

Also processes should also be able to declare being uncancellable (anything providing inputs/controlling Baritone should be able to do that).

[leijurv]

Partially overriding movements such as "I need the freelook mouse but not the WASD" (e.g. BuilderProcess is trying to place a block while also requesting a path to walk forward as in #3133) are very complicated and while sure they're possible I wouldn't prioritize it.

[ZeroMemes]

Event system needs to be overhauled. Having to write code like this to get around not having a priority system, or at least defined phases for each event (especially for tick event since the order in which its callbacks are invoked is really fragile and confusing to work with) is BAD!!!