Fix block network hash handling - #142
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_f66c72b1-ea78-40c0-824b-84046cfa9c16) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBlock network codecs are initialized per player session and used to translate block runtime IDs, key chunk caches by representation mode, re-encode chunk packets, and validate backend transfers. Chunk acknowledgements and world updates now pass through the session codec. ChangesBlock Network Hash Handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Backend
participant Player
participant WorldCache
participant Client
Backend->>Player: Send chunk or block runtime IDs
Player->>WorldCache: Decode and cache using session codec
WorldCache-->>Player: Canonical chunk data
Player->>Client: Encode and send client representation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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_8387516c-7c56-42e8-85ae-cff3c3c450aa) |
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_22690428-4b3d-4ec4-824b-a5e2967a1e90) |
…ix/block-network-hashes
…ix/block-network-hashes
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_d73ce6f6-c002-4431-9e70-588cc735e3b2) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
player/world.go (1)
77-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAsymmetric use of "Network" naming between
BlockRuntimeIDFromNetwork/BlockRuntimeIDToNetwork.
BlockRuntimeIDFromNetwork(Line 78) translates usingp.backendBlockNetwork, butBlockRuntimeIDToNetwork(Line 84) translates usingp.clientBlockNetwork. Given the existing, clearly-namedBlockRuntimeIDFromClient/BlockRuntimeIDToBackend/BlockRuntimeIDFromBackendToClient/BlockRuntimeIDFromClientToBackendcounterparts, having a...FromNetwork/...ToNetworkpair that silently means "backend" in one direction and "client" in the other is a footgun for future maintainers touching this hash-sensitive code path — a natural assumption would be that "Network" refers to the same endpoint in both methods.Consider renaming (e.g.
BlockRuntimeIDFromBackendNetwork/BlockRuntimeIDToClientNetwork) or at minimum strengthening the doc comments to explicitly flag the asymmetry.✏️ Suggested doc/name clarification
+// BlockRuntimeIDFromNetwork converts a block ID from the current backend's network representation to Oomph's +// canonical registry runtime ID. Note this intentionally uses the backend codec, not the client codec. func (p *Player) BlockRuntimeIDFromNetwork(id uint32) uint32 { return blockRuntimeIDFromNetwork(p.backendBlockNetwork, id) } -// BlockRuntimeIDToNetwork converts an Oomph block runtime ID to the hash expected by clients when the backend enabled -// hashed block network IDs. Unknown values are preserved so custom block fallbacks remain intact. +// BlockRuntimeIDToNetwork converts an Oomph block runtime ID to the network representation expected by the CLIENT +// (not the backend) — intended for packets Oomph crafts itself and sends directly to the client (e.g. SyncBlock). +// Unknown values are preserved so custom block fallbacks remain intact. func (p *Player) BlockRuntimeIDToNetwork(id uint32) uint32 { return blockRuntimeIDToNetwork(p.clientBlockNetwork, id) }🤖 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 `@player/world.go` around lines 77 - 120, Clarify the asymmetric endpoint meaning of BlockRuntimeIDFromNetwork and BlockRuntimeIDToNetwork, preferably by renaming them to explicitly identify backend and client networks; otherwise strengthen both doc comments to state which network each method uses. Update all call sites consistently while preserving their existing translation behavior.
🤖 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 `@player/world.go`:
- Around line 77-120: Clarify the asymmetric endpoint meaning of
BlockRuntimeIDFromNetwork and BlockRuntimeIDToNetwork, preferably by renaming
them to explicitly identify backend and client networks; otherwise strengthen
both doc comments to state which network each method uses. Update all call sites
consistently while preserving their existing translation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f6b69af-97a4-4881-99b0-14945deefdd4
📒 Files selected for processing (14)
player/block_network_packet.goplayer/component/acknowledgement/chunks.goplayer/component/combat.goplayer/component/world_hash_test.goplayer/network.goplayer/packet.goplayer/player.goplayer/world.goplayer/world_hash_test.goworld/blocknetwork/codec.goworld/blocknetwork/codec_test.goworld/blocknetwork/mode.goworld/cache.goworld/world.go
🚧 Files skipped from review as they are similar to previous changes (2)
- player/component/acknowledgement/chunks.go
- player/component/world_hash_test.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_15ea57b8-c4b1-43ac-87cc-8d821be62bd9) |
|
@coderabbitai review |
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_f33af374-acbe-4f0c-8c85-f9ca62894334) |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@player/block_network_packet.go`:
- Around line 66-106: The rewriteRecipeBlockNetworkIDs switch does not translate
outputs for protocol.FurnaceRecipe and protocol.FurnaceDataRecipe. Add cases for
both recipe types, copy each recipe, apply rewriteStackBlockNetworkID to its
Output, and return the copied recipe with modified=true only when translation
occurs; otherwise preserve the existing fallback.
In `@player/items.go`:
- Around line 21-24: Update the BlockByRuntimeID handling in the item conversion
logic to preserve the existing item when the translated backend ID cannot be
resolved; only replace it when the resolved block implements world.Item,
otherwise retain the current fallback behavior for known non-item blocks. Add
coverage for both unknown hashes and signed-negative hashes, ensuring neither
causes the item to be lost.
🪄 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: f2afca7c-f432-4586-80a7-27da94075a90
📒 Files selected for processing (5)
player/block_network_packet.goplayer/component/world.goplayer/items.goplayer/packet.goplayer/world.go
🚧 Files skipped from review as they are similar to previous changes (2)
- player/component/world.go
- player/packet.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_4c508ea4-049f-4a5d-b2d1-fd4311d4333c) |
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_68db26e9-714c-4887-bdff-ee9059dac15c) |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
integration/proxy/proxy_test.go (1)
203-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the new mode-mismatch transfer error path.
This PR introduces block-network mode validation in
TransferServerConn(erroring + backend cleanup inproxy.go'stransfer()), but no test exercises a mismatchedUseBlockNetworkIDHashesbetween the initial backend and a transfer target.fakeBackendalready carriesGameData, so this is easy to add alongside the existing test.🤖 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 `@integration/proxy/proxy_test.go` around lines 203 - 218, The existing TestTransferResetSynchronizesWithPlayerTick coverage should also exercise TransferServerConn’s block-network mode mismatch path. Add a transfer target with UseBlockNetworkIDHashes differing from the initial fakeBackend, assert the transfer returns the expected error, and verify the rejected backend is cleaned up, while preserving the existing synchronization test.world/cache.go (1)
132-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: duplicate
codec.Mode()check.The same
codec.Mode() == blocknetwork.Hashescondition is evaluated twice back-to-back; merging avoids the repetition.♻️ Suggested consolidation
- if codec.Mode() == blocknetwork.Hashes { - decoded.ConvertBlockNetworkHashesToRuntimeIDs() - } var data chunk.SerialisedData if codec.Mode() == blocknetwork.Hashes { + decoded.ConvertBlockNetworkHashesToRuntimeIDs() data = chunk.EncodeWithBlockNetworkHashes(decoded) } else { data = chunk.Encode(decoded, chunk.NetworkEncoding) }🤖 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 `@world/cache.go` around lines 132 - 140, Consolidate the adjacent codec.Mode() == blocknetwork.Hashes checks in the surrounding decode-and-encode flow: evaluate the mode once and use that result for both ConvertBlockNetworkHashesToRuntimeIDs and the corresponding encoding selection, preserving the existing behavior for blocknetwork.Hashes and other modes.
🤖 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 `@integration/proxy/proxy_test.go`:
- Around line 203-218: The existing TestTransferResetSynchronizesWithPlayerTick
coverage should also exercise TransferServerConn’s block-network mode mismatch
path. Add a transfer target with UseBlockNetworkIDHashes differing from the
initial fakeBackend, assert the transfer returns the expected error, and verify
the rejected backend is cleaned up, while preserving the existing
synchronization test.
In `@world/cache.go`:
- Around line 132-140: Consolidate the adjacent codec.Mode() ==
blocknetwork.Hashes checks in the surrounding decode-and-encode flow: evaluate
the mode once and use that result for both ConvertBlockNetworkHashesToRuntimeIDs
and the corresponding encoding selection, preserving the existing behavior for
blocknetwork.Hashes and other modes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 729bf9ea-f207-45b2-ac52-e91031ce2070
📒 Files selected for processing (10)
integration/proxy/proxy.gointegration/proxy/proxy_test.goplayer/component/acknowledgement/chunks.goplayer/component/world.goplayer/items.goplayer/network.goplayer/packet.goplayer/player.goplayer/world.goworld/cache.go
🚧 Files skipped from review as they are similar to previous changes (1)
- player/component/world.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_0ea5b316-16e6-43a7-97b7-5d198d31788a) |
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_21ca5be4-a663-461c-9b9d-1f1f64a9a54d) |
|
Closes #132 |
1 participant
Summary
Depends on oomph-ac/dragonfly#2 for non-mutating chunk palette conversion helpers.
This supersedes #141 while keeping the design scoped to same-mode server fleets.
Validation
go test -race ./... -count=1go vet ./...git diff --checkcodex review --base origin/stable— no actionable regressions identifiedSummary by CodeRabbit
Note
Medium Risk
Touches core world simulation, chunk caching, and proxy backend swaps; incorrect ID mapping could desync blocks, but changes are gated by explicit mode checks and transfer rejection on mismatch.
Overview
Adds a per-session block-network codec (runtime IDs vs network hashes) fixed from the first backend’s
StartGame, withDecodeBlockRuntimeID/EncodeBlockRuntimeIDwired through chunk caching, block updates, inventory block IDs, and client-facing sync packets so Oomph’s world simulation always uses canonical registry IDs.Chunk path:
CacheChunk/CacheSubChunktake the codec, convert hashed palettes when needed, and include block-network mode in cache keys; optionalAttemptFixChunksnow usesReencodeLevelChunkinstead of inline decode/re-encode. Transfers:TransferServerConnreturns an error if a new backend’s hash setting disagrees with the session; the native proxy aborts the swap and closes the dialed backend on mismatch.Bumps the
oomph-ac/dragonflyreplace to pick up hash conversion / encode helpers.Reviewed by Cursor Bugbot for commit b0deafd. Bugbot is set up for automated code reviews on this repo. Configure here.
Closes #132