Conversation
The whole of babbaj's nether-pathfinder in plain Java, under baritone.process.elytra.pathfinder, as it stands on the java-port branch of 0Mattias/nether-pathfinder at 6d807d0, written for babbaj/nether-pathfinder#31. Each class is the C++ file of the same name: the chunk octree, the raytracer, the A* search over octree cubes, the Nether terrain generator and the region-file reader. The page allocator, the thread pool, the JNI layer and the Unsafe accessors have no counterpart: a chunk is an object that lives as long as it is referenced, the table is a ConcurrentHashMap, and lookups, inserts, the search and rays can run at once from any thread. The tests hold the port to the native library's answers. An FNV-1a hash of each of 625 generated chunks and the hit and hit position of 4000 rays over generated terrain, recorded by the oracle in that repository, are reproduced to the bit. The search, the table, region files, cancel, the segment time-out and the rays the native library could not take -- a point, a coordinate that is not finite, an end on a voxel corner -- have tests of their own. Nothing uses the package yet; the next commit does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
NetherPathfinderContext asks baritone.process.elytra.pathfinder for paths and lines of sight, with a Chunk object where the native library handed out a pointer: the chunk packer fills a Chunk, a block update sets a bit on one, the solver's block lookups cache one, and there is nothing to free. The Unsafe fill of an all-solid section is Chunk.fillSection, and destroy() closes the table. The dev.babbaj:nether-pathfinder dependency, its maven repository, the nested jar on Fabric, the shadowed jar on Forge and NeoForge, the ProGuard keep rule and the natives it carried for each platform go, and with them the system check: every system is supported, so NullElytraProcess goes too, and elytraCustomAllocator is kept only so that a settings file naming it still loads. The read-write lock stays, on the same sides as before, so that the threads run in the order they always have. The port needs none of it -- a chunk is an object that stays valid for whoever holds it, and the table takes lookups, inserts and culls from any thread at once -- and dropping it is a follow-up. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Baritone's CachedRegion writes a chunk's two bits per block with BitSet.toByteArray(): bit n sits in byte n / 8 at position n % 8, the lowest bit first. The native reader took a byte's bits from the top down (baritone.cpp, get2Bits: `>> (6 - (i % 8))`) and the port copied it, so block x of every aligned run of four along x came back as block x ^ 3: each such run mirrored, in every chunk a search took from the region cache rather than from the game. Hard to see in flight, since the cache only fills in what the game has not loaded and terrain in runs of four is close to its own mirror image, but wrong. The test packed its file the same wrong way and passed; it now builds the chunk with a BitSet as CachedChunk does, sets one block solid and one water, and checks that the mirror positions are air. The region directory is Baritone's `cache`, not `regions`, in the docs and the test, and the oracle's doc names its real path. The same fix, on the port's own branch, is 0Mattias/nether-pathfinder 4784d69. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
wait realized something, fixing now |
| * settings file naming it still loads. | ||
| */ | ||
| @Deprecated | ||
| public final Setting<Boolean> elytraCustomAllocator = new Setting<>(true); |
There was a problem hiding this comment.
I think this one can be safely removed
| /** Reads the chunks of one of Baritone's cached region files ({@code r.X.Z.bcr}). */ | ||
| final class BaritoneRegion { | ||
|
|
||
| private static final int MAGIC = 456022911; |
There was a problem hiding this comment.
make baritone.cache.CachedRegion.CACHED_REGION_MAGIC public and reference that instead of having this duplicate field
| public static final int DIMENSION_NETHER = 1; | ||
| public static final int DIMENSION_END = 2; | ||
|
|
||
| static final int STATE_FROM_JAVA = 0; |
There was a problem hiding this comment.
"from java" is now a silly name now that both sides are written in java. maybe "from caller" would be better? im not really sure what to call this.
There was a problem hiding this comment.
I'd suggest "real", in line with the existing "fake chunk" terminology, but "from caller" works as well.
| public final class NetherPathfinder implements AutoCloseable { | ||
|
|
||
| // How the raytracer will treat chunks that aren't actually observed. | ||
| public static final int CACHE_MISS_GENERATE = 0; |
There was a problem hiding this comment.
these can just be an enum now
| public static final int CACHE_MISS_AIR = 1; | ||
| public static final int CACHE_MISS_SOLID = 2; | ||
|
|
||
| public static final int DIMENSION_OVERWORLD = 0; |
| * hit a solid block, and if hitPosOutCanBeNull is given, where. Points and the segments between | ||
| * them must have 0 <= y < 384. | ||
| */ | ||
| public void raytrace(int fakeChunkMode, int inputs, double[] start, double[] end, boolean[] hitsOut, double[] hitPosOutCanBeNull) { |
There was a problem hiding this comment.
this function can be removed now that we can just call Raytracer.raytrace directly.
the array arguments were also an ffi optimization so wherever arrays were constructed to call this can be simplified
| * first that is blocked; -1 if there is none. So -1 means "none clear" in the first mode and | ||
| * "all clear" in the second. | ||
| */ | ||
| public int isVisibleMulti(int fakeChunkMode, int inputs, double[] start, double[] end, boolean anyIfTrueElseAll) { |
There was a problem hiding this comment.
for the same reasons as above this function can be removed/move to the file it was called
| } | ||
|
|
||
| /** Whether there is line of sight between the two points. */ | ||
| public boolean isVisible(int fakeChunkMode, double x1, double y1, double z1, double x2, double y2, double z2) { |
There was a problem hiding this comment.
another redundant function
| * of no length is a point, which is a hit if it is inside a block; a coordinate that is not a | ||
| * finite number is refused. | ||
| */ | ||
| static boolean raytrace(NetherPathfinder ctx, double fx, double fy, double fz, double tx, double ty, double tz, int fakeChunkMode, double[] hitOut, int hitIndex) { |
There was a problem hiding this comment.
instead of a boolean this should return the hit position if there's a hit, otherwise null. the last 2 arguments can be removed
| s[off >>> 3] |= mask; | ||
| } else { | ||
| s[off >>> 3] &= ~mask; | ||
| if (allZero(s, x8 * (X8_BYTES / 8), X8_BYTES / 8)) { |
Every inline comment from the first review, in order: the native allocator setting is gone; BaritoneRegion checks CachedRegion.CACHED_REGION_MAGIC instead of a copy; "from java" is "from caller"; the cache-miss modes and the dimensions are enums; PathSegment carries a List<BlockPos> instead of packed longs; the search and region-read timers use currentTimeMillis; the port uses Minecraft's BlockPos and its own class is deleted; slabs are sections; the Raytracer's switch computes the child origin and offset once; the batched raytrace, isVisible and isVisibleMulti wrappers are removed and callers use Raytracer.raytrace directly, which returns the hit position or null; and the x8 summary bit is no longer maintained on clears, since a stale set bit only costs a reader a scan. Also carried: pathFind clears the cancel flag on the way out rather than in, so a cancel that lands while a search is still queued behind the lock is honoured by that search instead of wiped; CancelTest covers it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the review. All of it (including your new comments) is now applied and pushed here. Quick overview of how the port fits together first, since a few of your points touch the seams, then each comment in order, then what I checked. How the port is put togetherThe Java port lives in The integration is thin on purpose. Correctness is checked against recorded output from the native library: chunk hashes for a generated world, 4,000 rays with their hit positions, and searches over the same terrain. Those tests did not change in this round and still match bit for bit. Your comments, in order
12., 13. and 14.
Also in this pushTwo things that are not from the review. Verified
Let me know what you think, I can change any of this as needed. |
| * Magic value to detect invalid cache files, or incompatible cache files saved in an old version of Baritone | ||
| */ | ||
| private static final int CACHED_REGION_MAGIC = 456022911; | ||
| // Public: the elytra pathfinder's region reader checks the same header. |
| // outlives its blocks costs a reader a scan of the cube and never a wrong answer; | ||
| // fillSection resets it. | ||
| s[off >>> 3] &= ~mask; | ||
| if (allZero(s, x8 * (X8_BYTES / 8), X8_BYTES / 8)) { |
There was a problem hiding this comment.
This is still needed because queueBlockUpdate may set random blocks to air but for performance this check should be done when updating individual blocks from queueBlockUpdate. In all other places blocks are set the entire chunk is being set at once and in those cases this check is not needed. Because this function is a bit complicated instead of creating a separate function just add an argument to this one that conditionally runs this code that is only set to try by queueBlockUpdate. then for convenience add another setBlock function with the same signature as we currently have but passes false for that new argument.
There was a problem hiding this comment.
Is there actually any place which calls this with solid=false and exact=false outside tests? From a quick search it looks like all callers either call setBlock(x, y, z, solid, true) or know that they are writing to an empty chunk and only call setBlock(x, y, z, true), suppressing calls with solid=false.
| public final boolean finished; | ||
| public final long[] packed; | ||
|
|
||
| /** The blocks of the path, in order. */ |
babbaj's second review, in order. The x8 summary bit has to be cleared with its cube's last block after all: queueBlockUpdate sets single blocks to air in chunks the search is using, and a stale bit would leave a cube reading as possibly solid for as long as the chunk lives. But the scan that keeping the bit exact costs is wasted on the callers that fill a whole chunk at once, which only ever set blocks. So setBlock takes an `exact` flag that runs the scan on a clear, the four-argument setBlock passes false, and queueBlockUpdate alone passes true; ChunkTest checks both kinds of clear. The comment above CachedRegion.CACHED_REGION_MAGIC and the javadoc on PathSegment.blocks go. Also dropped, since they were unused: EOFException in BaritoneRegion, OutputStream in BaritoneRegionTest, List in NetherPathfinder and assertNull in RaytraceTest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks again for the review. Pushed 8827e15. |
| event.getBlocks().forEach(pair -> { | ||
| BlockPos pos = pair.first().below(minY); | ||
| if (pos.getY() < 0 || pos.getY() >= 384) return; | ||
| boolean isSolid = pair.second() != AIR_BLOCK_STATE; |
There was a problem hiding this comment.
while we're here we should also replace this with .isAir()
|
|
||
| import net.minecraft.core.BlockPos; | ||
|
|
||
| enum Face { |
There was a problem hiding this comment.
this can be deleted and replaced with minecraft's Direction
babbaj's third review. The block update test compared the state against Blocks.AIR's default state, so a block that became cave air counted as solid. isAir covers cave air and void air too, and writeChunkData already treats cave air as air when it packs a whole chunk, so a single block update and a chunk fill now agree on what air is. AIR_BLOCK_STATE had no other use and goes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
babbaj's fourth review. Face only named the six directions and moved a BlockPos along one, which Direction and BlockPos.relative already do, so the enum goes and PathFinder takes a Direction. The neighbour array keeps the native library's order, UP, DOWN, NORTH, SOUTH, EAST, WEST, rather than Direction.values(), so the search expands neighbours as it always has. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Both in, pushed eb62fc0. |
ZacSharp
left a comment
There was a problem hiding this comment.
I'm not at all familiar with the C++ codebase so I cannot say much about the port. I've tried to not leave comments which likely apply to the C++ code as well.
I've mostly skipped over the tests.
One thing to perhaps care about later is that this spams allocations. Some years ago benchmarks showed the main pathfinder was faster after removing every single BlockPos allocation. The JVM might since have gotten better at handling this kind of short-lived-allocation spam, but it might be worth a try once things have settled.
ZacSharp's review, on Chunk.setBlock and isEmptyX8. The exact flag existed so that the callers that fill a whole chunk would not pay the scan that keeping the summary exact costs on a clear, but none of them ever clears a block: writeChunkData, insertChunkData, BaritoneRegion and the generator only set, and the block update, the one caller that clears, already asked for exact. So the flag decided nothing, and with it gone the summary is exact by construction, which answers the question of whether isEmptyX8 and isEmptyX16 may say no for an empty cube: they may not, and a summary that disagrees with the blocks is a bug in whatever wrote them, as babbaj put it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ZacSharp asked whether the JIT or the CPU may reorder the two plain writes in setBlock, and babbaj asked why a race was expected there at all. The comment claimed an order the language does not promise: without a fence, a reader on another thread may see the block before the summary bit. It was written for a reader that does not exist. Chunk was ported to be safe on its own, like the native code, but every reader in Baritone takes NetherPathfinderContext's read lock, the solver thread, the game thread's tick and a search that does not generate alike, while a block update, a chunk pack, a cull and a generating search hold its write lock, so no reader can watch a block being set. The class comment now says so instead of describing a race, and the comment on the write goes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ZacSharp's review. A section has eight x8 cubes, so its summary needs eight bits, and the class comment already called it a byte; the int array was an oversight. filled(y) still returns an int, the byte masked to its eight bits, so the raytracer's tests of it are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ZacSharp found it unused and babbaj said to remove it. It was the native library's entry point for a chunk as an array of booleans; the game's chunks come in through allocateAndInsertChunk and are packed straight into the Chunk, and only a test called this. dimensionHeight served only its length check and goes with it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ZacSharp asked whether the chunk state should be an enum or a boolean, and babbaj noticed that setChunkState, its only writer, is never called, so the field should be final and not volatile. It is now a final boolean, fromCaller, in place of the two int constants, and setChunkState goes; the test helper that marked generated chunks as the game's now generates them itself and copies the blocks into chunks it inserts as the game's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ZacSharp's review. The BlockPos was private and only ever read back through its three getters, so the node holds the coordinates itself and allocates one object fewer per node the search creates. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ZacSharp's review, babbaj agreed. The set of chunks whose neighbours the search has generated was a HashSet of boxed Longs; fastutil's long set holds the keys unboxed. The keys are what NetherPathfinder.key gives, as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ZacSharp's review, babbaj agreed the comment was outdated: the package took Minecraft's BlockPos, Direction, Vec3 and Mth in the first review round and the comment still said it used nothing of Minecraft's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks both. All eight are in, pushed 1a3c211..63a5813, one commit per comment:
On the allocations: the |
|
There's still a lot of BlockPos objects being created, especially where they are made just to construct NodePos objects. |
| // The native library needed this lock held while there were pointers to its chunks in Java. | ||
| // The port needs none of that -- a chunk is an object that stays valid for whoever holds it, | ||
| // and the table takes lookups, inserts and culls from any thread at once -- and the lock is | ||
| // kept so that the threads still run in the order they always have. |
There was a problem hiding this comment.
Isn't this the lock that prevents writing to a chunk while search is reading it?
Fifteen commits: the three below, then the changes from the reviews, one commit per comment.
baritone.process.elytra.pathfinder: the whole library in plain Java, one class per C++ file, with its tests and oracle files (what the native library answered for 625 generated chunks and 4000 rays; the port reproduces every one, hit positions to the bit). 34 tests, junit 4 like the rest.NetherPathfinderContextgets aChunkwhere it had a pointer,BlockStateOctreeInterfacecaches one, and the dependency, the babbaj maven repo, the fabricinclude, the forge/neoforgeshadowCommon, the proguard keep rule and the natives for each platform go. Every system is supported now, soNullElytraProcessand the check inElytraProcess.creatego too.elytraCustomAllocatorstays as a deprecated no-op so settings files still load.BitSet.toByteArray(), lowest bit first; the native reader took each byte's bits from the top down (baritone.cpp,get2Bits), so every aligned run of four blocks along x came back mirrored in any chunk a search took from the region cache. The port had copied it; now it reads what baritone writes, with a test built the wayCachedChunkbuilds a chunk.Since the first push, from babbaj's and ZacSharp's reviews: the FFI leftovers are gone (arguments packed into longs, the port's own
BlockPosandFace, the raytrace wrappers, the duplicate region magic),Raytracerreturns the hit position instead of a boolean, the block update asksisAir(), the x8 summary is abyte[]per section and stays exact on every clear (theexactflag is gone, no whole-chunk writer clears a block),Chunksays what keeps its readers and writers apart instead of describing a race, a chunk's origin is afinal boolean fromCaller,insertChunkDataandsetChunkStateare deleted,NodePosholds three ints, the set of generated chunks is a fastutilLongOpenHashSet, and the neighbour order is kept as the native library had it so the A* search expands the same way.Left alone on purpose: the read-write lock and the two executors. The port doesn't need them (chunks are objects, the table is a
ConcurrentHashMap, lookups, inserts, culls, searches and rays can all run at once), but dropping them is its own change.Three inputs the native library couldn't take are handled now, because the elytra solver does send them: a zero-length ray (native:
raytrace whiffed,exit(696969)), a NaN or infinite coordinate (native hangs on an infinity in z; the port throwsIllegalArgumentException), and a ray ending exactly on a voxel corner, which every ray at a path node is (native could step into a node the ray never enters and exit; the port terminates, tested).Same machine and inputs as the tables on #31:
pathFindover 800 blocks, 20 runsTried for real in my fabric fork (0Mattias/cheesecake#19, same seam): unit tests plus four in-world elytra flights, overworld above the build limit, auto-jump, nether below and above the roof, all pass with the port. Here:
./gradlew buildon JDK 21, all four loaders, tests included.