cabaletta / cabaletta/baritone
Nasty parts of Baritone
- Dominant language
- Java
- Stars
- 9.2k
- Forks
- 2.1k
- Avg merge
- 4d 3h
- Merged PRs (30d)
- 3
Description
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 https://github.com/cabaletta/baritone/blob/9c64067d492e3fdd7d255bdf39092524046f7121/src/main/java/baritone/pathing/movement/movements/MovementFall.java#L134-L167 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](https://github.com/cabaletta/baritone/blob/0a38e6f6909c5baf007b31eae3a66c7eabbed327/src/main/java/baritone/utils/PathingControlManager.java#L163) 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](https://github.com/cabaletta/baritone/blob/14178fcd14f245a21da0240c3fa4b507a8a00446/src/api/java/baritone/api/pathing/goals/GoalBlock.java#L69-L77) call [SettingsUtil.maybeCensor](https://github.com/cabaletta/baritone/blob/14178fcd14f245a21da0240c3fa4b507a8a00446/src/api/java/baritone/api/utils/SettingsUtil.java#L160-L166). When you turn on censorCoordinates, all your goals look like `GoalBlock{x=, y=, z=}`. 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".
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.