feat: add long-term chunk geometry caching #477
Conversation
Implements chunk geometry caching to improve performance when returning to previously visited areas: - IndexedDB-based caching for servers that support chunk-cache channel - In-memory caching with LRU eviction for other servers - Automatic cache invalidation when blocks change - Cache hit/miss tracking for performance monitoring Files added: - src/chunkGeometryCache.ts: IndexedDB cache manager with LRU eviction - renderer/viewer/lib/chunkCacheIntegration.ts: Block hash computation utilities Files modified: - src/customChannels.ts: Register chunk-cache channel for server support detection - renderer/viewer/lib/worldrendererCommon.ts: Integrate cache into geometry handling This addresses issue zardoy#476 - longterm chunk geometry caching. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds client-side chunk geometry and packet caches, renderer integration to reuse/invalidate cached geometries, an in-memory section block-state store with hashing utilities, and a custom client-server channel to coordinate cache hits/misses. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as World Renderer
participant Cache as ChunkGeometryCache
participant Disk as Disk/Storage
participant Worker as Geometry Worker
rect rgba(100,200,150,0.5)
Note over Renderer,Cache: attempt to render section with caching
Renderer->>Cache: tryUseCachedGeometry(sectionKey)
alt Cache Hit
Cache->>Renderer: emit synthetic geometry message
Renderer->>Renderer: mark section finished
else Cache Miss
Renderer->>Worker: dispatch geometry generation
Worker-->>Renderer: geometry result
Renderer->>Cache: set(sectionKey, blockHash, geometry)
Cache->>Disk: persist serialized geometry (async)
end
end
sequenceDiagram
participant Client as Game Client
participant Server as Remote Server
participant PacketCache as ChunkPacketCache
rect rgba(150,150,200,0.5)
Note over Client,Server: channel negotiation & chunk-hash exchange
Client->>Server: register "minecraft-web-client:chunk-cache"
alt Server Supports Channel
Server-->>Client: register ack
Client->>PacketCache: setServerSupportsChannel(true, serverAddress)
Client->>Server: sendCachedChunksList(chunksJson)
Server-->>Client: respond with cacheHit/hash per chunk
alt server indicates cacheHit
Client->>PacketCache: load cached map_chunk -> inject to pipeline
end
else Server Rejects
Client->>PacketCache: keep memory/disk local only
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Use numeric separators for FNV hash constants - Use for-of loop instead of indexed for loop - Use optional chaining for crypto.subtle check - Use spread operator instead of Array.from - Simplify boolean return in isGeometryCacheable Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove multi-space inline comments - Add readonly modifier to memoryCache - Use spread operator instead of Array.from() - Add async keyword to promise-returning functions Co-Authored-By: Claude Opus 4.5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@renderer/viewer/lib/chunkCacheIntegration.ts`:
- Around line 69-82: The computeBlockHashAsync function currently passes
blockStateIds.buffer to crypto.subtle.digest which hashes the entire underlying
ArrayBuffer (including bytes outside the Uint16Array view), causing incorrect
cache behavior; update computeBlockHashAsync to pass the typed array view
(blockStateIds) directly to crypto.subtle.digest('SHA-256', blockStateIds) and
keep the existing try/catch fallback to computeBlockHash(blockStateIds)
unchanged so behavior and error handling remain intact.
In `@renderer/viewer/lib/worldrendererCommon.ts`:
- Around line 999-1004: The cache-hit short-circuit is skipping necessary
recomputation during block-update/AO paths; modify the guard in the method
containing tryUseCachedGeometry(key) so it only returns on cache hit when the
renderer is not in change-worker mode (i.e., require !this.useChangeWorker).
Concretely, update the conditional that currently checks value &&
this.tryUseCachedGeometry(key) to also verify the change-worker flag before
logging and returning (so keep logWorkerWork(() => `<- cache hit for section
${key}`) and the early return only when !this.useChangeWorker), ensuring
block-update/AO/lighting recalculation paths always bypass the cache.
- Around line 964-988: tryUseCachedGeometry is using cached.geometry without
validating against current sectionHashes and it only sets finishedSections,
skipping the sectionFinished bookkeeping (finishedChunks/stat) which can leave
chunks stuck; update the method to compare cached.hash (or cached.sectionHash)
to this.sectionHashes[sectionKey] before accepting the cache, and if it matches
call the same path used for real worker messages to record completion: invoke
handleWorkerMessage and then emit/dispatch the synthetic sectionFinished
workflow (or call the existing sectionFinished handler) so finishedChunks and
related stats are updated, and only set finishedSections after all bookkeeping
completes; reference geometryCache, sectionHashes, handleWorkerMessage,
finishedSections, and the sectionFinished/finishedChunks/stat update logic to
locate where to wire this in.
In `@src/chunkGeometryCache.ts`:
- Around line 110-118: In generateBlockHash: guard against missing WebCrypto
(check globalThis.crypto?.subtle and fall back to
require('crypto').webcrypto?.subtle or throw a clear error) and ensure you hash
only the Uint16Array view bytes (use a Uint8Array view over data.buffer with
data.byteOffset and data.byteLength so subarray views don't include unrelated
bytes) before calling subtle.digest('SHA-256'); then convert the resulting
buffer to hex as before.
In `@src/customChannels.ts`:
- Around line 655-711: The memory cache is being set without a server-scoped
address when the channel is unsupported; in registerChunkCacheChannel initialize
the serverAddress from lastConnectOptions (or 'unknown') and call
chunkGeometryCache.setServerSupportsChannel(false, serverAddress) before
probing, then when the CHANNEL_NAME handler sees data.cacheEnabled set it should
call chunkGeometryCache.setServerSupportsChannel(true, serverAddress) to flip
support to true; update the try/catch flow around bot._client.registerChannel,
bot._client.on(CHANNEL_NAME,...), and the initial writeChannel so the
serverAddress is established up front and reused in both the false and true
setServerSupportsChannel calls.
| /** | ||
| * Try to use cached geometry for a section instead of regenerating | ||
| * Returns true if cache was used, false otherwise | ||
| */ | ||
| tryUseCachedGeometry (sectionKey: string): boolean { | ||
| const cached = this.geometryCache.get(sectionKey) | ||
| if (!cached) return false | ||
|
|
||
| // Use the cached geometry by simulating a worker message | ||
| this.geometryCacheHits++ | ||
| const fakeWorkerMessage = { | ||
| type: 'geometry' as const, | ||
| key: sectionKey, | ||
| geometry: cached.geometry, | ||
| workerIndex: -1 // Mark as cached | ||
| } | ||
|
|
||
| // Process the cached geometry | ||
| this.handleWorkerMessage(fakeWorkerMessage as any) | ||
|
|
||
| // Emit sectionFinished for tracking | ||
| this.finishedSections[sectionKey] = true | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
Validate cached geometry and complete section-finished bookkeeping.
Cached geometry is used without validating the current section hash, and the sectionFinished workflow (finishedChunks/stat updates) is skipped; this can leave chunks stuck as “loading” and allow stale geometry after reloads. Consider validating against sectionHashes and emitting a synthetic sectionFinished with proper bookkeeping.
🛠️ Suggested fix
tryUseCachedGeometry (sectionKey: string): boolean {
const cached = this.geometryCache.get(sectionKey)
if (!cached) return false
+ const currentHash = this.sectionHashes.get(sectionKey)
+ if (!currentHash || currentHash !== cached.hash) return false
// Use the cached geometry by simulating a worker message
this.geometryCacheHits++
const fakeWorkerMessage = {
type: 'geometry' as const,
key: sectionKey,
geometry: cached.geometry,
workerIndex: -1 // Mark as cached
}
// Process the cached geometry
this.handleWorkerMessage(fakeWorkerMessage as any)
- // Emit sectionFinished for tracking
- this.finishedSections[sectionKey] = true
+ // Emit sectionFinished for tracking (mirror worker flow)
+ this.sectionsWaiting.set(sectionKey, (this.sectionsWaiting.get(sectionKey) ?? 0) + 1)
+ this.handleMessage({ type: 'sectionFinished', key: sectionKey, workerIndex: -1, processTime: 0 } as any)
return true
}🤖 Prompt for AI Agents
In `@renderer/viewer/lib/worldrendererCommon.ts` around lines 964 - 988,
tryUseCachedGeometry is using cached.geometry without validating against current
sectionHashes and it only sets finishedSections, skipping the sectionFinished
bookkeeping (finishedChunks/stat) which can leave chunks stuck; update the
method to compare cached.hash (or cached.sectionHash) to
this.sectionHashes[sectionKey] before accepting the cache, and if it matches
call the same path used for real worker messages to record completion: invoke
handleWorkerMessage and then emit/dispatch the synthetic sectionFinished
workflow (or call the existing sectionFinished handler) so finishedChunks and
related stats are updated, and only set finishedSections after all bookkeeping
completes; reference geometryCache, sectionHashes, handleWorkerMessage,
finishedSections, and the sectionFinished/finishedChunks/stat update logic to
locate where to wire this in.
…uard - Fix computeBlockHashAsync to pass typed array view instead of .buffer - Add !useChangeWorker check to cache-hit shortcut in setSectionDirty - Add sectionHashes validation in tryUseCachedGeometry - Add WebCrypto guard with FNV-1a fallback in generateBlockHash - Set serverAddress even when channel is unsupported for cache scoping Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Addressed all CodeRabbit feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@renderer/viewer/lib/worldrendererCommon.ts`:
- Around line 419-428: The geometry cache is broken because sectionHashes is
never populated; call computeBlockHash and storeSectionBlockStates at the points
where chunk data / section block states are loaded or when blocks update (e.g.,
in the chunk load handler and block update handler) before geometry generation
so you can set this.sectionHashes.set(sectionKey, computedHash) and only then
call geometry caching (the code around isGeometryCacheable -> geometryCache.set
and incrementing geometryCacheMisses); ensure the same sectionKey/hash scheme is
used by tryUseCachedGeometry so currentHash is defined and cache validation can
succeed.
In `@src/chunkGeometryCache.ts`:
- Around line 297-312: The clear() method currently purges all IndexedDB entries
because clearIndexedDB() calls store.clear(), but the intent (and comment) is to
remove only the current server's cached geometry; update clearIndexedDB() so it
deletes only entries matching the current server identifier (e.g., serverAddress
stored on this instance) instead of calling store.clear(). Locate clear() and
clearIndexedDB() and change the IndexedDB logic to iterate a cursor or use an
index to find entries with the matching server key and delete them (or run a
keyRange-based delete), leaving other servers' records intact; ensure
memoryCache.clear() behavior remains unchanged and update the comment to reflect
the actual scoped behavior if needed.
♻️ Duplicate comments (1)
renderer/viewer/lib/worldrendererCommon.ts (1)
964-992: Hash validation added, but sectionFinished bookkeeping remains incomplete.The hash validation (lines 973-974) addresses part of the past review. However, the
sectionFinishedbookkeeping is still incomplete. SettingfinishedSections[sectionKey] = truedirectly (line 989) bypasses the normal flow that:
- Updates
sectionsWaitingtracking- Triggers
finishedChunksdetection- Calls
checkAllFinished()- Emits the
chunkFinishedeventThis can leave chunks perpetually marked as "loading" even though all their sections are cached and rendered.
🛠️ Suggested fix to properly complete bookkeeping
tryUseCachedGeometry (sectionKey: string): boolean { const cached = this.geometryCache.get(sectionKey) if (!cached) return false // Validate that the cached hash matches the current section hash const currentHash = this.sectionHashes.get(sectionKey) if (!currentHash || cached.hash !== currentHash) return false // Use the cached geometry by simulating a worker message this.geometryCacheHits++ const fakeWorkerMessage = { type: 'geometry' as const, key: sectionKey, geometry: cached.geometry, workerIndex: -1 // Mark as cached } // Process the cached geometry this.handleWorkerMessage(fakeWorkerMessage as any) - // Emit sectionFinished for tracking - this.finishedSections[sectionKey] = true + // Complete the sectionFinished workflow for proper tracking + this.sectionsWaiting.set(sectionKey, (this.sectionsWaiting.get(sectionKey) ?? 0) + 1) + this.handleMessage({ type: 'sectionFinished', key: sectionKey, workerIndex: -1, processTime: 0 } as any) return true }
🧹 Nitpick comments (2)
renderer/viewer/lib/worldrendererCommon.ts (1)
202-208: Cache properties look good; consider if counters should reset on world cleanup.The
geometryCacheandsectionHashesmaps are properly decorated with@worldCleanup()for cleanup on world reset.Note that
geometryCacheHitsandgeometryCacheMissescounters are not decorated, so they persist across world switches. This may be intentional for lifetime statistics, but if per-session stats are preferred, consider adding the decorator or manually resetting them inresetWorld().src/chunkGeometryCache.ts (1)
204-238: Good two-tier cache strategy; consider batching lastAccessed updates.The memory-first lookup with IndexedDB fallback is well-designed. Hash validation ensures cache coherency.
Note:
saveToIndexedDB(cached)on line 225 performs a write operation on every IndexedDB cache hit to updatelastAccessed. This could impact performance under heavy cache access. Consider batching these updates or deferring them to idle time if performance issues arise.
- Add computeChunkDataHash to populate sectionHashes in addColumn - Fix tryUseCachedGeometry to properly complete sectionFinished workflow - Update clear() to only remove current server's entries, not all data - Delete sectionHashes entries when invalidating section cache Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@renderer/viewer/lib/chunkCacheIntegration.ts`:
- Around line 119-136: The computeChunkDataHash function fails because
ArrayLike<number> isn't Iterable and cannot be spread; update the conversion to
Uint8Array to use Array.from for the non-ArrayBuffer branch so both ArrayBuffer
and ArrayLike<number> are handled safely (i.e., replace the spread [...chunkData
as Iterable<number>] with Array.from(chunkData as ArrayLike<number>) when
constructing the Uint8Array), keeping the rest of the FNV-1a hashing logic in
computeChunkDataHash unchanged.
🧹 Nitpick comments (1)
src/chunkGeometryCache.ts (1)
415-452: Consider race condition in concurrent eviction.The
evictOldEntriesmethod doesn't guard against concurrent invocations. If multiplesetoperations complete around the same time, multiple eviction runs could occur simultaneously. While IndexedDB transactions provide some isolation, this could lead to over-eviction.Consider adding a simple flag to prevent concurrent eviction:
💡 Optional improvement
+ private isEvicting = false + private async evictOldEntries (): Promise<void> { - if (!this.db) return + if (!this.db || this.isEvicting) return + this.isEvicting = true + try { // ... existing eviction logic ... + } finally { + this.isEvicting = false + } }
Move eslint-disable comment to the correct line to properly suppress unicorn/prefer-spread rule. ArrayLike<number> is not Iterable so we must use Array.from() instead of spread operator. Co-Authored-By: Claude Opus 4.5 <[email protected]>
src/customChannels.ts
Outdated
| type: 'i32' | ||
| }, | ||
| { | ||
| name: 'cacheEnabled', |
There was a problem hiding this comment.
The communication between client and server should be done in a bit other way (it was not clearly described in the issue).
I think of the following:
- (Client acknowledge) Right after login we send to the server using another channel all cached chunks that we know from
the persistent storage (array of {pos and hash}). Some hashing algorithm that can be clearly implemented in Java/js by parsing the map_chunk packet, we should clearly replicate it. - (Server understands). If server sees client already know of some chunks. map_chunk packet will not be send to save network bandwidth (it was the whole point of this implementation). Instead it sends a packet with on this channel with x,y,data="{cacheHit:true}", we get cached in persistent storage map packet packet data and simulate it with bot._client.emit(map_chunk, …)
- In case if hash doesn't match or cache was not provided before by the client, the server still sends the same packet but with data={hash:"…"} and we use that knowledge, after that server sends map_chunk packet for that x,y (we fallback to generating cache on our side only if we don't receive that custom channel packet first and do caching inside world renderer only in that case)
That said we should cache in persistent storage not only geometry but also map chunk packet.
|
Thanks for brining Claude to the party! The implementation looks promising! |
|
Thanks for the detailed feedback! I've completely redesigned the implementation to match your proposed protocol: New Implementation1. Client Acknowledges Cached Chunks on Login
[{"x": 0, "z": 0, "hash": "abc123"}, ...]2. Server Response Handling
3. Hash AlgorithmUsing FNV-1a (32-bit) which is simple to implement in Java: int hash = 2166136261; // FNV offset basis
for (byte b : data) {
hash ^= (b & 0xFF);
hash *= 16777619; // FNV prime
}
return String.format("%08x", hash & 0xFFFFFFFFL);Packet Structures
All tests pass and TypeScript compiles cleanly. Let me know if you'd like any adjustments to the protocol or hash algorithm! |
- Add chunkPacketCache.ts for storing raw map_chunk packet data - Redesign protocol per maintainer feedback: 1. Client sends cached chunk hashes on login 2. Server responds with cacheHit or hash for new chunks 3. Client emits cached packets locally on cache hit - Use FNV-1a hash algorithm (reproducible in Java) - Cache both packet data and geometry for optimal performance Co-Authored-By: Claude <[email protected]>
|
Thanks! Claude has been super helpful for understanding the architecture and implementing these changes quickly. Really appreciate the detailed protocol description - it made the implementation much clearer! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/chunkPacketCache.ts`:
- Around line 236-252: The invalidate method is deleting IndexedDB entries using
an unscoped chunkKey (`"${x},${z}"`) which can remove another server's data;
update invalidate (and any callers) to use the fully scoped cache key (the same
key produced by getCacheKey(x, z)) when calling deleteFromIndexedDB so the
deletion includes server/instance scoping, and keep memoryCache.delete(cacheKey)
as-is; ensure deleteFromIndexedDB accepts/uses that full cache key format so
entries are only removed for the correct server.
- Around line 66-77: The object store keyPath currently uses chunkKey that is
just "x,z", causing cross-server collisions; change chunkKey to include the
serverAddress prefix (e.g. `${serverAddress}|${x},${z}`) and keep the object
store keyPath as 'chunkKey' (update creation in request.onupgradeneeded if
necessary). Update the CachedChunkPacket.chunkKey generation and all places that
read/write the cache to use this server-scoped key (notably the set(), get(),
and invalidate() methods and any in-memory map keys) so writes and deletes are
scoped per server; also ensure any index usage (like 'serverAddress' or
'lastAccessed') remains valid and consider a DB migration/upgrade path to
rewrite existing entries to the new server-prefixed chunkKey if needed.
🧹 Nitpick comments (3)
src/customChannels.ts (3)
743-772: Consider logging or metrics for the silent failure path.At lines 768-770, chunk caching failures are silently swallowed. While this is intentional (caching is optional), it may make debugging harder if caching consistently fails. Consider at least a
console.debugfor visibility during development.♻️ Optional: Add debug logging for silent failures
} catch (error) { - // Silently fail - caching is optional + // Silently fail - caching is optional + console.debug(`Optional chunk caching failed for ${chunkKey}:`, error) }
824-840: JSON serialization of binary chunk data is inefficient for production.The current approach converts
chunkData(potentially 16KB+) into an array of numbers in JSON. Each byte expands from 1 byte to 1-4 characters plus comma overhead, significantly increasing storage size and parse time.As noted in the comment, this is a temporary approach. For production, consider:
- Base64 encoding for
chunkDataandbiomes- Using the actual prismarine-chunk packet serialization
♻️ Example: Base64 encoding for efficiency
function serializeMapChunkPacket (packet: any): ArrayBuffer { - // For now, use JSON serialization as a simple approach - // In production, you'd want to use the actual packet serialization const json = JSON.stringify({ x: packet.x, z: packet.z, groundUp: packet.groundUp, bitMap: packet.bitMap, - biomes: packet.biomes ? [...packet.biomes] : undefined, + biomes: packet.biomes ? btoa(String.fromCharCode(...new Uint8Array(packet.biomes.buffer))) : undefined, heightmaps: packet.heightmaps, - chunkData: packet.chunkData ? [...new Uint8Array(packet.chunkData)] : undefined, + chunkData: packet.chunkData ? btoa(String.fromCharCode(...new Uint8Array(packet.chunkData))) : undefined, blockEntities: packet.blockEntities })Note: For large chunks, consider using a streaming approach or
arrayBufferToBase64utility to avoid call stack limits with the spread operator.
701-702:pendingChunkHashesmap may grow unboundedly if server misbehaves.If the server sends a cache-miss message with a hash but never sends the corresponding
map_chunkpacket, entries will accumulate inpendingChunkHashes. This is unlikely with well-behaved servers but could be a concern for robustness.♻️ Optional: Add cleanup for stale pending hashes
// Track pending chunk hashes from server (for chunks we'll receive via map_chunk) const pendingChunkHashes = new Map<string, string>() + + // Periodically clean up stale pending hashes (older than 30 seconds) + const PENDING_HASH_TIMEOUT = 30_000 + setInterval(() => { + // Simple cleanup - in practice you'd track timestamps per entry + if (pendingChunkHashes.size > 1000) { + pendingChunkHashes.clear() + console.debug('Cleared stale pending chunk hashes') + } + }, PENDING_HASH_TIMEOUT)
zardoy
left a comment
zardoy
left a comment
There was a problem hiding this comment.
Some early feedback, reviewed 80% at first glance everything looks good, but I hope to test it next week in game
| import type { MesherGeometryOutput } from './mesher/shared' | ||
|
|
||
| // Store for block state IDs by section for hash computation | ||
| const sectionBlockStates = new Map<string, Uint16Array>() |
There was a problem hiding this comment.
I don't quite understand how this cache is used, don't we already have cache in the world renderer? We better not introduce unscoped variables (it's fine to split some methods into its file like you done).
| /** | ||
| * Serialize geometry for storage | ||
| */ | ||
| private serializeGeometry (geometry: MesherGeometryOutput): SerializedGeometry { |
There was a problem hiding this comment.
Cant we use browserfs.ts implementation that already does fa mounting into IndexedDB and here write binary files for each chunk-server key using nodejs fs api? And can't we have here more serialize/deserialise functions so they take input to serialize into binary file which should be more efficient then eg json storage. We better save the space since there are many chunks can be stored.
Also get the device storage quota to not enable this feature if device is running low on space and handle case when fs is not available due to mounting failure (notice how singleplayer is also disabled if there is no space left on device - we just skip writing)
src/chunkPacketCache.ts
Outdated
| return | ||
| } | ||
|
|
||
| const transaction = this.db.transaction(STORE_NAME, 'readwrite') |
There was a problem hiding this comment.
Same for this let's save using fs api in the same place where we save geometry cache - these data should be saved in a similar way like using same file base (even if into different files for easier debugging / different methods since packet is saved earlier)
src/customChannels.ts
Outdated
| }) | ||
|
|
||
| // Set up cache with server info | ||
| chunkPacketCache.setServerInfo(serverAddress, true) |
There was a problem hiding this comment.
This is not correct, the channel registration function will never throw an error, registration / custom payload message will be sent even if server doesn't understand it - server will just silently ignore it.
Instead we should listen here for server registrations eg using custom_payload packet lister
example from logs of what server can send:
S play:custom_payload 1768301420893 {"channel":"minecraft:register","data":{"type":"Buffer","data":[109,105,110,101,99,114,97,102,116,45,119,101,98,45,99,108,105,101,110,116,58,100,105,118,107,105,116,45,114,101,109,111,118,101,0,109,105,110,101,99,114,97,102,116,45,119,101,98,45,99,108,105,101,110,116,58,118,111,105,99,101,45,99,104,97,116,45,115,116,97,116,117,115,0,109,105,110,101,99,114,97,102,116,45,119,101,98,45,99,108,105,101,110,116,58,115,10…
Sadly I don't remember exactly how to decode the buffer but it should be straightforward- Minecraft-protocol source implementation can help with it looking at how they handle it
- Replace direct IndexedDB API with browserfs fs.promises
- Store chunks as files: /data/chunk-cache/{server}/{x},{z}.bin
- Store metadata in metadata.json per server directory
- Server isolation via directory structure (fixes cross-server collision)
- Debounced metadata saves for better performance
- Maintain same public API for compatibility
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/customChannels.ts`:
- Around line 824-861: The current serializeMapChunkPacket and
deserializeMapChunkPacket drop version-specific fields (e.g., addBitMap,
compressedChunkData) and force biomes to Int32Array, causing data loss and
incorrect types; update serialization to be version-aware by using bot.version
(or the packet's version) and either (1) store the raw packet buffer/packet
binary instead of JSON, or (2) use prismarine-chunk to serialize/deserialize
chunk data per-version, ensuring you include addBitMap/compressedChunkData when
present and reconstruct biomes with the correct typed array (Uint8Array vs
Int32Array) and chunkData as a Buffer/Uint8Array so no binary data is converted
to JSON; modify serializeMapChunkPacket and deserializeMapChunkPacket
accordingly to branch on version and preserve all original packet fields and
types.
- Around line 704-787: The try-catch around registerChannel can't reliably
detect server support and leaves listeners attached; instead listen for the
'custom_payload' (meta.name === 'custom_payload') / "minecraft:register"
payload, parse the NUL-separated channel names and check for CHANNEL_NAME, and
only then call bot._client.registerChannel(CHANNEL_NAME,...), attach the
CHANNEL_NAME listener (bot._client.on(CHANNEL_NAME,...)) and the 'packet'
map_chunk interceptor (bot._client.on('packet',...)), call
chunkPacketCache.setServerInfo(serverAddress, true) and
chunkGeometryCache.setServerSupportsChannel(true, serverAddress) and invoke
sendCachedChunksList(); if the register payload does not include CHANNEL_NAME
call chunkPacketCache.setServerInfo(serverAddress, false) and
chunkGeometryCache.setServerSupportsChannel(false, serverAddress) and do not
attach those listeners (or remove them if previously attached). Ensure you
reference CHANNEL_NAME, bot._client.registerChannel,
bot._client.on(CHANNEL_NAME,...), bot._client.on('packet',...),
chunkPacketCache.setServerInfo, chunkGeometryCache.setServerSupportsChannel and
sendCachedChunksList when making the change.
🧹 Nitpick comments (3)
src/chunkPacketCache.ts (2)
74-78: UsereplaceAll()for global string replacement.Per ESLint hint, prefer
String#replaceAll()overString#replace()with a global regex for clarity.Suggested fix
private getServerDir (): string { - const sanitized = sanitize(this.serverAddress.replace(/[/:]/g, '_')) + const sanitized = sanitize(this.serverAddress.replaceAll(/[/:]/g, '_')) return join(CACHE_BASE, sanitized || 'unknown') }
225-228: Potential issue with Buffer to ArrayBuffer conversion.
new Uint8Array(data).buffermay not create a properly-aligned ArrayBuffer whendatais a Node.js Buffer. Buffer instances can share an underlying ArrayBuffer with an offset, causing.bufferto return more data than expected.Suggested fix using slice to ensure proper bounds
if (await existsViaStats(chunkPath)) { const data = await fs.promises.readFile(chunkPath) - const packetData = new Uint8Array(data).buffer + // Ensure we get a properly bounded ArrayBuffer from the Buffer + const uint8 = new Uint8Array(data) + const packetData = uint8.buffer.slice(uint8.byteOffset, uint8.byteOffset + uint8.byteLength)src/customChannels.ts (1)
836-840: Potential ArrayBuffer alignment issue with TextEncoder.Similar to the earlier Buffer issue,
encoder.encode(json).buffermay return an ArrayBuffer with incorrect bounds if the underlying buffer is shared.Suggested fix
const encoder = new TextEncoder() - return encoder.encode(json).buffer + const encoded = encoder.encode(json) + return encoded.buffer.slice(encoded.byteOffset, encoded.byteOffset + encoded.byteLength) }
- Migrate chunkGeometryCache.ts from IndexedDB to browserfs fs storage - Fix channel registration to use custom_payload listener instead of try/catch - Add clearAllBlockStates() calls in resetWorld/destroy to prevent memory leaks Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix import order (type imports before regular imports) - Use replaceAll instead of replace with /g flag - Improve map_chunk serialization to handle all version-specific fields - Properly reconstruct typed arrays with correct constructor type Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
I've addressed all the feedback: Changes in the latest commits:
The CI should pass now. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/customChannels.ts`:
- Around line 752-773: The cache-hit path can leave a chunk permanently missing
when cached data is absent or deserialization fails; update the block that
handles data.cacheHit (around chunkPacketCache, deserializeMapChunkPacket, and
bot._client.emit) to trigger a recovery request after invalidation or when
cached is null: after calling chunkPacketCache.invalidate(data.x, data.z) (or in
the else branch where cached is falsy), send a follow-up to the server to
request retransmission of that chunk (e.g., call an existing
requestChunkResend/notifyCachedListUpdate helper or write a small request packet
via bot._client) so the server will resend the map_chunk; ensure both the
deserialization catch and the cached==null branch perform the invalidate +
resend action and log the recovery attempt.
♻️ Duplicate comments (1)
src/customChannels.ts (1)
842-878: Map_chunk serialization is still version-incomplete.JSON serialization drops version-specific fields (e.g., legacy
addBitMap/compressedChunkData) and forcesbiomesintoInt32Array, which breaks older protocol variants. Please keep this version-aware or store raw packet bytes (or use prismarine-chunk).Minecraft map_chunk packet fields addBitMap compressedChunkData biomes type version differences
🧹 Nitpick comments (2)
src/chunkGeometryCache.ts (1)
212-266: Consider binary serialization to avoid JSON bloat for geometry.Spreading large typed arrays into JSON significantly increases CPU and storage. Since this is file-backed via browserfs, consider serializing binary buffers (or a compact binary schema) instead of JSON arrays to reduce size and parse overhead.
renderer/viewer/lib/worldrendererCommon.ts (1)
883-897: Consider recomputing section hashes after block updates to maintain cache effectiveness.After
invalidateSectionCachedeletessectionHashes, subsequent geometry regeneration caches geometry with hash'unknown'(sincesectionHashes.get()returns undefined, falling back to'unknown'). WhentryUseCachedGeometryvalidates the cache, it fails becausecurrentHashis undefined, rendering the cached geometry unusable. To preserve cache reuse for modified sections, regenerate and store the section hash after block state updates—either by computing it from the updated block states or by recomputing it when geometry is regenerated by workers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/customChannels.ts`:
- Around line 781-811: pendingChunkHashes can grow forever if a 'map_chunk'
never arrives; update the packet handler module to clear stale entries by adding
a cleanup strategy: when you set pendingChunkHashes (in the packet listener
attached to bot._client.on('packet')) include a timestamp or start a timeout per
entry and expire it after a TTL, and also attach a disconnect/close handler on
bot._client (or bot's 'end'/'kicked' events) to clear pendingChunkHashes
entirely on disconnect; reference the pendingChunkHashes Map and the packet
listener registration (bot._client.on('packet', ...)) to implement either
per-entry timeouts (store {hash, ts} or start setTimeout to delete) or a
periodic sweep function, and ensure you remove any timers and clear the Map in
the client disconnect handler to avoid memory leaks.
♻️ Duplicate comments (1)
src/customChannels.ts (1)
752-779: Cache-hit failure does not trigger server resend - chunk may remain missing.When cached data is missing (line 771-773) or deserialization fails (line 767-769), the code only invalidates the local cache. Since the server already sent
cacheHit: true, it won't resend the chunk data, potentially leaving the chunk permanently absent until the next session.🛠️ Suggested recovery mechanism
Consider implementing a recovery path when cache-hit fails:
} catch (error) { console.warn(`Failed to emit cached chunk ${chunkKey}:`, error) - // Request fresh chunk from server by invalidating cache await chunkPacketCache.invalidate(data.x, data.z) + // Request server to resend - send updated cache list without this chunk + // or implement a dedicated "request-chunk" message in the protocol + console.warn(`Chunk ${chunkKey} needs manual reload (rejoin or move away and back)`) } } else { - console.warn(`Cache hit but no cached data for ${chunkKey} - server/client out of sync`) + console.warn(`Cache hit but no cached data for ${chunkKey} - invalidating`) + await chunkPacketCache.invalidate(data.x, data.z) + // Consider: bot._client.writeChannel(CLIENT_CHANNEL, { requestChunk: { x: data.x, z: data.z } }) }A proper fix would require protocol support for requesting chunk resends from the server.
🧹 Nitpick comments (5)
src/chunkGeometryCache.ts (2)
212-236: Large geometry arrays spread into plain arrays may cause performance issues.Spreading Float32Array/Uint32Array into plain number arrays (lines 217-225) creates significant memory overhead and CPU cost for large chunks. Consider using a binary format instead of JSON for geometry storage.
💡 Suggested binary serialization approach
// Example binary serialization for better performance private serializeGeometryBinary(geometry: MesherGeometryOutput): ArrayBuffer { // Calculate total size needed const positionsBytes = geometry.positions.byteLength; const normalsBytes = geometry.normals.byteLength; // ... etc // Write header with offsets, then raw typed array bytes // This avoids JSON overhead and array spreading }This could reduce storage size by ~4x and improve serialization speed significantly. However, this is a larger refactor that could be deferred if the current approach works acceptably.
449-482: Eviction runs on everyset()call, even when under capacity.The
evictOldEntries()method is called after every disk write (line 368), but it returns early if under capacity. Consider moving the capacity check before sorting to avoid unnecessary work, or batch eviction to run less frequently.♻️ Minor optimization
The current implementation is fine since it returns early, but you could consider only calling eviction after every N writes or when a threshold is crossed, to reduce the overhead of the
Object.keys()call on every write.src/chunkPacketCache.ts (2)
52-107: Significant code duplication withChunkGeometryCache.The structure of
ChunkPacketCacheclosely mirrorsChunkGeometryCache: same initialization pattern, samegetServerDir(),getMetadataPath(), same debounced metadata save, same eviction logic. Consider extracting a shared base class or utility module to reduce duplication.💡 Suggested abstraction
// Shared base class or utility abstract class ServerScopedCache<TEntry, TMeta> { protected serverAddress = 'unknown' protected metadata: { entries: Record<string, TMeta> } = { entries: {} } protected metadataDirty = false // ... shared methods: getServerDir, loadMetadata, saveMetadata, scheduleSaveMetadata, evictOldEntries }This would reduce ~150 lines of duplicated code between the two cache implementations.
357-370: Memory cache eviction threshold differs from disk cache.Memory cache evicts at
MAX_CACHE_SIZE / 2(500) while disk evicts atMAX_CACHE_SIZE(1000). This is intentional but the magic number/ 2on line 361 could benefit from a named constant for clarity.♻️ Minor clarity improvement
+const MAX_MEMORY_CACHE_SIZE = MAX_CACHE_SIZE / 2 // 500 entries + private addToMemoryCache (key: string, entry: CachedChunkPacket): void { this.memoryCache.set(key, entry) - if (this.memoryCache.size > MAX_CACHE_SIZE / 2) { + if (this.memoryCache.size > MAX_MEMORY_CACHE_SIZE) {src/customChannels.ts (1)
842-870: JSON serialization of binary packet data is inefficient.Spreading buffer bytes into arrays and encoding as JSON significantly inflates storage size (each byte becomes a 1-4 character number plus comma). For a typical chunk, this could be 3-4x larger than necessary.
💡 Consider binary serialization
A more efficient approach would be to use a binary format:
function serializeMapChunkPacket(packet: any): ArrayBuffer { // Option 1: Use prismarine-chunk's serialization // Option 2: Store raw packet buffer if available from minecraft-protocol // Option 3: Custom binary format with header + raw buffers }However, the current JSON approach is functionally correct and simpler to debug. This optimization could be deferred if storage space isn't critical.
- Add requestChunkResend function to recover when cache-hit fails - Request chunk resend when cached data is missing or deserialization fails - Fix ArrayBuffer alignment issues with Buffer.slice for proper bounds - Ensure TextEncoder output has proper ArrayBuffer bounds Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add TTL-based cleanup for stale pending hash entries (30s timeout) - Clear pendingChunkHashes on bot disconnect - Add periodic cleanup interval (every 10s) - Stop cleanup interval when bot ends Co-Authored-By: Claude Opus 4.5 <[email protected]>
2 participants
Summary
Implements long-term chunk geometry caching to improve performance when returning to previously visited areas, addressing issue #476.
Features:
chunk-cachechannelImplementation Details:
src/chunkGeometryCache.ts: IndexedDB cache managerrenderer/viewer/lib/chunkCacheIntegration.ts: Cache utilitiessrc/customChannels.ts: Channel registrationminecraft-web-client:chunk-cachechannelrenderer/viewer/lib/worldrendererCommon.ts: Cache integrationHow It Works:
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.