Add block network hash conversion helpers - #2
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f9c9df6d-22b7-4e6c-b84e-70ad3e5fa575) |
📝 WalkthroughWalkthroughChangesChunk storage and encoding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Chunk
participant BlockRegistry
participant NetworkEncoding
Caller->>Chunk: EncodeWithBlockNetworkHashes
Chunk->>BlockRegistry: RuntimeIDToHash for palette entries
BlockRegistry-->>Chunk: Network hashes or unmapped values
Chunk->>NetworkEncoding: Encode cloned chunk
NetworkEncoding-->>Caller: SerialisedData
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9c0e31b1-a941-4a1f-b31e-2951c67e6c7e) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/world/chunk/block_network_hash_test.go (1)
10-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover unmapped-value preservation.
These tests only exercise successful mappings. Add unknown hash and runtime-ID cases to verify the fallback paths preserve custom palette values unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/world/chunk/block_network_hash_test.go` around lines 10 - 60, Extend the tests around ConvertBlockNetworkHashesToRuntimeIDs, EncodeWithBlockNetworkHashes, and EncodeSubChunkWithBlockNetworkHashes to cover unmapped hashes and runtime IDs. Verify unknown network hashes remain unchanged during conversion, and unknown runtime IDs remain unchanged in encoded output while the source chunk is not mutated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/world/chunk/block_network_hash_test.go`:
- Around line 10-60: Extend the tests around
ConvertBlockNetworkHashesToRuntimeIDs, EncodeWithBlockNetworkHashes, and
EncodeSubChunkWithBlockNetworkHashes to cover unmapped hashes and runtime IDs.
Verify unknown network hashes remain unchanged during conversion, and unknown
runtime IDs remain unchanged in encoded output while the source chunk is not
mutated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2c93858-c585-4951-806e-4225b43cda96
📒 Files selected for processing (3)
server/world/chunk/block_network_hash.goserver/world/chunk/block_network_hash_test.goserver/world/chunk/decode.go
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f238b780-1d53-4b30-9c46-8069608972c9) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_17d9ccce-dd7c-451e-b907-833873c59ee1) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_23232594-d94d-4fe4-bfa5-a406e06c3b62) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/world/chunk/compact_runtime_test.go (1)
124-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for growth after a runtime-cache shrink.
This test confirms the shrink itself, but doesn't verify that adding a third distinct palette value afterward still stores/retrieves correctly. Given the potential
storage.palette.size/bitsPerIndexdesync flagged inpaletted_storage.go(Lines 169-188), a follow-up assertion here (e.g.storage.Seta new distinct runtime ID beyond the current 2-value palette and confirmAt()returns it correctly) would catch that class of regression directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/world/chunk/compact_runtime_test.go` around lines 124 - 142, The test TestPalettedStorageCompactForRuntimeCacheShrinksOversizedMultiValueStorage should also exercise growth after compactForRuntimeCache: set a coordinate to a third distinct runtime ID after the shrink, then assert At returns that ID and the storage expands consistently with the new palette value.server/world/chunk/paletted_storage.go (1)
190-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate bit-packing math instead of reusing
filledBitsPerIndex.
indicesPerWord := uint32BitSize / int(storage.bitsPerIndex)recomputes the same relationship already cached instorage.filledBitsPerIndex(used bypaletteIndex()for offset math). Deriving it independently here risks silent divergence if the packing formula changes in one place but not the other.♻️ Suggested refactor
- indicesPerWord := uint32BitSize / int(storage.bitsPerIndex) + indicesPerWord := int(storage.filledBitsPerIndex / storage.bitsPerIndex)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/world/chunk/paletted_storage.go` around lines 190 - 212, Update PalettedStorage.uniformPaletteIndex to derive indicesPerWord from the existing storage.filledBitsPerIndex value instead of recomputing it from uint32BitSize and bitsPerIndex. Keep the fullWords and remainder calculations and validation behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/world/chunk/paletted_storage.go`:
- Around line 169-188: Update compactForRuntimeCache and the resize path so
shrinking packed storage also synchronizes Palette.size with the new width.
Ensure subsequent Palette.Add operations detect when growth is needed and never
write indices that exceed indexMask.
---
Nitpick comments:
In `@server/world/chunk/compact_runtime_test.go`:
- Around line 124-142: The test
TestPalettedStorageCompactForRuntimeCacheShrinksOversizedMultiValueStorage
should also exercise growth after compactForRuntimeCache: set a coordinate to a
third distinct runtime ID after the shrink, then assert At returns that ID and
the storage expands consistently with the new palette value.
In `@server/world/chunk/paletted_storage.go`:
- Around line 190-212: Update PalettedStorage.uniformPaletteIndex to derive
indicesPerWord from the existing storage.filledBitsPerIndex value instead of
recomputing it from uint32BitSize and bitsPerIndex. Keep the fullWords and
remainder calculations and validation behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63bfd3e4-387e-41fc-9bfb-e28e572926aa
📒 Files selected for processing (4)
server/world/chunk/chunk.goserver/world/chunk/compact_runtime_test.goserver/world/chunk/paletted_storage.goserver/world/chunk/sub_chunk.go
| // compactForRuntimeCache performs the cheap subset of compact that is useful for chunks kept in memory. | ||
| // It collapses single-value storages and shrinks oversized storage widths, but avoids scanning multi-value | ||
| // storages for unused palette entries. | ||
| func (storage *PalettedStorage) compactForRuntimeCache() { | ||
| if storage.palette.Len() == 0 { | ||
| return | ||
| } | ||
| if storage.palette.Len() == 1 { | ||
| storage.collapseToPaletteIndex(0) | ||
| return | ||
| } | ||
| if index, ok := storage.uniformPaletteIndex(); ok { | ||
| storage.collapseToPaletteIndex(index) | ||
| return | ||
| } | ||
|
|
||
| if size := paletteSizeFor(storage.palette.Len()); size < storage.palette.size { | ||
| storage.resize(size) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd palette.go --exact-path server/world/chunk
rg -n 'size' server/world/chunk/palette.go -B2 -A2
rg -n 'func.*Add' server/world/chunk/palette.go -A 20
rg -n '\.resize\(' server/world/chunk -B3 -A3
rg -n 'func newPalettedStorage' server/world/chunk/paletted_storage.go -A 15Repository: oomph-ac/dragonfly
Length of output: 5286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== paletted_storage.go relevant ranges =="
sed -n '1,220p' server/world/chunk/paletted_storage.go | cat -n | sed -n '1,220p'
echo
echo "== palette.go relevant ranges =="
sed -n '1,180p' server/world/chunk/palette.go | cat -n | sed -n '1,180p'Repository: oomph-ac/dragonfly
Length of output: 16135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resize/addNew/compactForRuntimeCache references =="
rg -n 'compactForRuntimeCache|func \(storage \*PalettedStorage\) resize|func \(storage \*PalettedStorage\) addNew|storage\.palette\.size|needsResize\(\)' server/world/chunk -A 4 -B 4Repository: oomph-ac/dragonfly
Length of output: 7739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'compactForRuntimeCache\(' server/world/chunk -A 3 -B 3
rg -n '\.resize\(size\)|\.resize\(storage\.palette\.size\)|increaseSize\(\)' server/world/chunk -A 2 -B 2
rg -n 'type PalettedStorage|func \(storage \*PalettedStorage\) resize' server/world/chunk/paletted_storage.go -A 80 -B 10Repository: oomph-ac/dragonfly
Length of output: 11297
Sync Palette.size when shrinking storage. compactForRuntimeCache() calls resize(size) without updating storage.palette.size, so the palette can keep a wider capacity than the packed indices. A later Palette.Add may then skip the grow resize and write an index that no longer fits indexMask, corrupting live chunk data. Update the palette size here or inside resize().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/world/chunk/paletted_storage.go` around lines 169 - 188, Update
compactForRuntimeCache and the resize path so shrinking packed storage also
synchronizes Palette.size with the new width. Ensure subsequent Palette.Add
operations detect when growth is needed and never write indices that exceed
indexMask.
1 participant
Summary
These are helper APIs for Oomph proxy translation. This PR intentionally does not change Dragonfly session serialization or enable hash mode for native Dragonfly servers.
Validation
go test ./server/world/... -count=1go vet ./server/world/...git diff --checkNote
Medium Risk
New conversion paths touch core chunk encoding and palette identity; incorrect hash mapping could corrupt block data on the wire, though cloning limits mutation of cached state.
Overview
Adds chunk palette translation between Bedrock network block hashes and registry runtime IDs for proxy-style workflows, without changing how native Dragonfly serializes chunks.
In-place normalization via
ConvertBlockNetworkHashesToRuntimeIDsrewrites decoded palettes throughBlockRegistry.HashToRuntimeID, leaving unknown values unchanged. Network encoding usesEncodeWithBlockNetworkHashesandEncodeSubChunkWithBlockNetworkHashes, which clone the chunk (or a single sub-chunk) beforeRuntimeIDToHashconversion so cached chunks stay in runtime-ID form.Introduces
CompactForRuntimeCacheon chunks, sub-chunks, and paletted storages: collapses single-value and uniform storages (detected from packed index words), shrinks oversized bit widths, and drops air-only layers—without the full palette scan used before disk save. Includes a trivial comment whitespace fix in network decode.Reviewed by Cursor Bugbot for commit 536885c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests