Dan/block view layer#37
Open
HashimTheArab wants to merge 66 commits into
Open
Conversation
Route break and item-use paths through the player-visible block state, and keep live layer updates from leaking stale liquid layers. This prevents private overrides from accidentally interacting with hidden public blocks.\n\nConstraint: Review feedback required view-layer overrides to control break, item-use, and liquid-layer update behavior.\nRejected: Forcing BreakBlock to always use public world blocks | Direct client break paths must still remove private overrides.\nConfidence: high\nScope-risk: moderate\nDirective: Keep player interactions aligned with viewedBlock when client actions target block positions.\nTested: go test ./server/player ./server/session; go test ./server/...\nNot-tested: Manual in-client waterlogged private override interaction.
Private view-layer blocks are allowed to satisfy client-visible validation, but public transaction mutations must not run against them. Return before activation/use/place paths when the clicked or replacement target block is private.\n\nConstraint: Review feedback flagged private viewed blocks flowing into p.tx mutation paths.\nRejected: Mutating private overrides through p.tx | p.tx represents public world state, not per-player view state.\nConfidence: high\nScope-risk: narrow\nDirective: Any future view-layer interaction that mutates world state must explicitly re-check public block state or operate on the view layer.\nTested: go test ./server/player ./server/session; go test ./server/...\nNot-tested: Manual in-client fake activatable/fake replaceable right-click scenarios.
Incremental block updates now mirror chunk/view-layer refresh behavior by forcing non-zero layers to air while a private block override exists at the same position.\n\nConstraint: Chunk sends already clear layer 1 for private overrides; live world updates needed the same masking.\nRejected: Only masking layer 0 | Liquid/second-layer updates can leak through behind private blocks.\nConfidence: high\nScope-risk: narrow\nDirective: Keep all block update paths consistent with view-layer masking across block layers.\nTested: go test ./server/session; go test ./server/...\nNot-tested: Manual in-client SetLiquid/secondLayer update while private override is active.
Session block actions now respect view-layer block overrides, while private breaking animations use an explicit bypass for the private block path. This prevents public chest/pot/etc actions from leaking through fake blocks.\n\nConstraint: Public block actions are broadcast through the generic Session.ViewBlockAction interface.\nRejected: Filtering in individual block action emitters | Central session filtering covers all current and future public action sources.\nConfidence: high\nScope-risk: narrow\nDirective: Use ViewPrivateBlockAction only for actions that intentionally target the session's private override.\nTested: go test ./server/player ./server/session; go test ./server/...\nNot-tested: Manual in-client fake block over chest/decorated pot action scenarios.
Immediate view-layer block refresh now resends the chunk height advert before block updates when a private override occupies a subchunk above the public chunk's highest filled subchunk. This lets clients request/load the subchunk before receiving the private block update.\n\nConstraint: Sub-chunk request mode only loads up to the advertised highest subchunk from the chunk send.\nRejected: Sending only UpdateBlock | Updates can be ignored for subchunks the client was never asked to load.\nConfidence: medium\nScope-risk: moderate\nDirective: Keep private override refreshes ordered as subchunk advert before per-block updates when they expand visible chunk height.\nTested: go test ./server/session; go test ./server/...\nNot-tested: Manual client subchunk request flow for high private override in otherwise empty subchunk.
UseItemOnBlock now re-reads the player-visible block after HandleItemUseOnBlock before activation or placement checks. This preserves handler-side world/view-layer changes and avoids mutating against stale state.\n\nConstraint: Handlers may modify world or view-layer block state before default item-use logic continues.\nRejected: Reusing the pre-handler viewed block | It can be stale after event handlers run.\nConfidence: high\nScope-risk: narrow\nDirective: Re-read mutable world/view state after cancellable handlers before applying default mutations.\nTested: go test ./server/player ./server/session; go test ./server/...\nNot-tested: Manual plugin handler that swaps the clicked block during item use.
Avoid carrying an unused private flag from the pre-handler viewed block lookup; the flag is only needed after handlers may have changed visible state.\n\nConstraint: make lint failed on ineffassign for the initial private assignment.\nRejected: Keeping the initial private value | It is intentionally superseded after HandleItemUseOnBlock.\nConfidence: high\nScope-risk: narrow\nDirective: Keep pre-handler lookup limited to values used before the handler.\nTested: make lint; go test ./server/player ./server/session; go test ./server/...\nNot-tested: Manual client interaction flows.
Restore the existing session constructor pattern for selecting the block registry after initialising the session. NewViewLayer no longer needs the registry, so the temporary local was unnecessary.\n\nConstraint: Review feedback identified the precomputed block registry local as unnecessary.\nRejected: Keeping br in the struct literal | It creates avoidable churn from the existing constructor flow.\nConfidence: high\nScope-risk: narrow\nDirective: Keep registry fallback assignment near other post-construction session setup unless a constructor dependency requires it earlier.\nTested: make lint; go test ./server/session; go test ./server/...\nNot-tested: Custom block registry integration smoke test.
When a private block override disappears while the player is still finishing a private break, resend the public block instead of silently returning. This corrects client prediction without breaking the public block.\n\nConstraint: Private break completion must not mutate the public block if the private override vanished mid-break.\nRejected: Falling through to BreakBlock | It can break the public block for a stale private interaction.\nConfidence: high\nScope-risk: narrow\nDirective: Stale private interactions should resynchronise the client rather than mutate public world state.\nTested: make lint; go test ./server/player ./server/session; go test ./server/...\nNot-tested: Manual client finish packet after plugin removes private override mid-break.
Owner
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Owner
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 09ad9ab. Configure here.
Owner
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit faeb7f1. Configure here.
Owner
Author
|
@cursor review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/player/player.go (1)
1875-1879:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
firePosfor fire-extinguish sound origin.The fire block is located at
firePos(computed aspos.Side(face)on Line 1864), but the sound is played atpos.Vec3()on both Lines 1875 and 1879. This plays the extinguish sound at the wrong block location.🤖 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/player/player.go` around lines 1875 - 1879, The fire-extinguish sound is being played at the wrong block location. In the fire extinguishing block (around the PlaySound calls with sound.FireExtinguish), replace the sound origin position from pos.Vec3() to firePos.Vec3() on both occurrences (lines 1875 and 1879). This ensures the sound plays at the actual fire block location (firePos) rather than at the player's current position (pos).
🤖 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 `@go.mod`:
- Around line 21-38: The github.com/go-jose/go-jose/v4 dependency in the require
block is pinned to the vulnerable version v4.1.3 which has a DoS vulnerability
(CVE-2026-34986). Update the version constraint for
github.com/go-jose/go-jose/v4 from v4.1.3 to v4.1.4 or a later version in the
go.mod require block. After updating the version string, run go mod tidy to
ensure all transitive dependencies are properly resolved and validated.
In `@server/player/view_layer_block_test.go`:
- Around line 70-78: The test case for "use item on private block does not
mutate public world" at line 70 has an incorrect expectation for
expectedPrivateBlock. Currently it asserts expectedPrivateBlock: block.Lever{}
(line 77), which expects the private override to remain after the UseItemOnBlock
interaction. However, based on the test name and intended behavior, the private
override should be cleared after a private interaction. Change
expectedPrivateBlock from block.Lever{} to nil or the appropriate cleared/zero
state to reflect that the private block override should be removed after using
an item on a private block. This fix aligns with the assertion logic at lines
93-98 that validates the final private block state.
---
Duplicate comments:
In `@server/player/player.go`:
- Around line 1875-1879: The fire-extinguish sound is being played at the wrong
block location. In the fire extinguishing block (around the PlaySound calls with
sound.FireExtinguish), replace the sound origin position from pos.Vec3() to
firePos.Vec3() on both occurrences (lines 1875 and 1879). This ensures the sound
plays at the actual fire block location (firePos) rather than at the player's
current position (pos).
🪄 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
Run ID: 02cb79a7-4140-4496-875a-c3b04eea0c29
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/pr.yml.github/workflows/push.ymlMakefilego.modserver/player/player.goserver/player/view_layer_block.goserver/player/view_layer_block_test.goserver/session/chunk.goserver/session/controllable.goserver/session/handler_inventory_transaction.goserver/session/handler_player_auth_input.goserver/session/view_layer.goserver/session/world.goserver/world/view_layer.go
✅ Files skipped from review due to trivial changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (3)
- server/session/controllable.go
- server/session/view_layer.go
- server/session/chunk.go
Owner
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d5a42f4. Configure here.
Block overrides are now keyed by world while entity overrides remain session-wide. HandleBlockBreak receives the private break mode directly. BREAKING CHANGE: HandleBlockBreak now receives a private bool argument, and ViewLayer block override methods take a *world.World.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/player/block_break.go (1)
177-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrivate breaks apply exhaustion when they likely shouldn't.
The
Exhaust(0.005)call at line 198 runs for both private and public breaks. Private breaks only remove a view-layer override without modifying the actual world, so exhausting the player's hunger bar seems inconsistent with the "no world mutation" semantics described in the handler documentation.Suggested fix
p.SwingArm() if private { audience.ClearOverride(pos) audience.AddParticle(pos.Vec3Centre(), particle.BlockBreak{Block: b}) return } p.tx.SetBlock(pos, nil, nil) audience.AddParticle(pos.Vec3Centre(), particle.BlockBreak{Block: b}) if ok { info := breakable.BreakInfo() if info.BreakHandler != nil { info.BreakHandler(pos, p.tx, p) } for _, orb := range entity.NewExperienceOrbs(pos.Vec3Centre(), xp) { p.tx.AddEntity(orb) } } for _, drop := range drops { opts := world.EntitySpawnOpts{Position: pos.Vec3Centre(), Velocity: mgl64.Vec3{rand.Float64()*0.2 - 0.1, 0.2, rand.Float64()*0.2 - 0.1}} p.tx.AddEntity(entity.NewItem(opts, drop)) } + // Only exhaust the player for public block breaks. p.Exhaust(0.005)🤖 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/player/block_break.go` around lines 177 - 198, The p.Exhaust(0.005) call at the end of the function currently executes after the private break handling block, which creates ambiguity about when exhaustion should apply. Move the p.Exhaust(0.005) call to only execute for public breaks by placing it before the return statement in the private break block (as an intentional omission) or by restructuring the code so exhaustion is explicitly part of the public break handling path. This makes the intent clear that exhaustion only applies when the world state is actually modified, not for private view-only breaks.
🤖 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.
Outside diff comments:
In `@server/player/block_break.go`:
- Around line 177-198: The p.Exhaust(0.005) call at the end of the function
currently executes after the private break handling block, which creates
ambiguity about when exhaustion should apply. Move the p.Exhaust(0.005) call to
only execute for public breaks by placing it before the return statement in the
private break block (as an intentional omission) or by restructuring the code so
exhaustion is explicitly part of the public break handling path. This makes the
intent clear that exhaustion only applies when the world state is actually
modified, not for private view-only breaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d07b34aa-8ea4-4cda-8aee-0a3546c1dae7
📒 Files selected for processing (9)
server/player/block_break.goserver/player/block_break_test.goserver/player/handler.goserver/player/player.goserver/session/chunk.goserver/session/view_layer.goserver/session/world.goserver/world/view_layer.goserver/world/view_layer_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/session/world.go
- server/session/chunk.go
- server/player/player.go
ContinueBreaking now re-reads the current public or private break target before computing effects and break duration. Public break sounds, particles, and actions skip viewers with private block overrides. go mod tidy is clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Touches core block break/place/sync and chunk encoding for every connected player, but behavior is gated on view-layer overrides and covered by new tests.
Overview
Adds per-player block overrides on
ViewLayer, so each client can see and interact with blocks that differ from the shared world state (similar to existing entity view-layer overrides).API:
Player/SessiongainViewBlockandViewPublicBlock;BreakVisibleBlockbreaks what the player sees (clears a private override without world drops/XP), whileBreakBlockstill targets the public block only.HandleBlockBreakis documented for private breaks.Gameplay path: Breaking, item-on-block, placement checks, and picking use
visibleBlock; break progress tracksblockBreakTarget(public vs private) with separate sound/particles/crack actions. Client break actions route toBreakVisibleBlock.Networking: Chunk/sub-chunk send paths clone and patch blocks from the view layer;
ViewBlockUpdateand public block actions skip positions with overrides; private crack actions useViewPrivateBlockAction.Tooling: PR/push workflows and
make testsrungo test ./...;testifyadded forview_layer_block_test.go.Reviewed by Cursor Bugbot for commit d5a42f4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores