implement liquid movement simulation - #145
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds Bedrock layer-1 block update handling and storage, extends movement state for swimming and liquid speeds, processes dolphin boost effects, synchronizes swimming flags, and introduces water/lava movement simulation with flow, drag, gravity, and collision behavior. ChangesLayered world updates and acknowledgement flow
Swimming state and movement speed integration
Liquid traversal and physics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ServerPacket
participant Player
participant MovementComponent
participant World
ServerPacket->>Player: MovementEffect
Player->>MovementComponent: ServerUpdate(packet)
MovementComponent->>MovementComponent: apply swimming and dolphin boost state
Player->>World: read touching liquid blocks
Player->>MovementComponent: simulate liquid travel
MovementComponent-->>Player: update velocity and movement state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
world/world.go (1)
94-127: 🚀 Performance & Scalability | 🔵 Trivial
BlockLayermutates the override map on every cache miss — unnecessary allocation in a read path.When a chunk hasn't been seen in
blockUpdates/extraBlockUpdatesyet,BlockLayereagerly creates an empty inner map for it (updates[chunkPos] = make(...)) even though it's just reading.SetBlockLayeralready lazily creates this map when an actual override is written (Lines 144-146), so the read-path allocation is pure overhead — and it now happens for two layers instead of one, and is exercised far more heavily sinceplayer/simulation/movement.go's new liquid-detection helpers (touchingLiquidBlocks,liquidAt) callBlockLayermany times per tick, per player.♻️ Proposed fix
chunkPos := protocol.ChunkPos{int32(blockPos[0]) >> 4, int32(blockPos[2]) >> 4} updates := w.blockUpdates if layer == 1 { updates = w.extraBlockUpdates } - blockUpdates, found := updates[chunkPos] - if found { - if b, ok := blockUpdates[df_cube.Pos(blockPos)]; ok { - return b - } - } else { - updates[chunkPos] = make(map[df_cube.Pos]world.Block) - } + if blockUpdates, found := updates[chunkPos]; found { + if b, ok := blockUpdates[df_cube.Pos(blockPos)]; ok { + return b + } + }🤖 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/world.go` around lines 94 - 127, Remove the eager inner-map allocation from the cache-miss branch in World.BlockLayer. Keep BlockLayer read-only when no override exists, and rely on SetBlockLayer’s existing lazy initialization when an override is written.
🤖 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/simulation/movement.go`:
- Around line 555-558: Update liquidFlowSideClosed to call
p.World().Block(pos).Model().FaceSolid(pos, pos.Face(side), p.World())
unconditionally, removing the block.Stairs type assertion and stairs-only gating
so all block models, including IronBars, can determine whether the side is
closed.
---
Nitpick comments:
In `@world/world.go`:
- Around line 94-127: Remove the eager inner-map allocation from the cache-miss
branch in World.BlockLayer. Keep BlockLayer read-only when no override exists,
and rely on SetBlockLayer’s existing lazy initialization when an override is
written.
🪄 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: 6a55e410-0a81-41d5-9df7-f1c95a26c14e
📒 Files selected for processing (8)
player/component/acknowledgement/block.goplayer/component/acknowledgement/movement.goplayer/component/movement.goplayer/component/world.goplayer/movement.goplayer/packet.goplayer/simulation/movement.goworld/world.go
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)
player/simulation/movement.go (1)
65-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear glide state for airborne liquid entries.
This branch returns before the existing glide handling, but only clears gliding when
movement.OnGround()is true. An airborne player entering water or lava therefore retainsGliding/GlideBoost; after leaving the liquid, the stale glide state can resume without a new glide input. (raw.githubusercontent.com)🐛 Proposed fix
- if movement.Gliding() && movement.OnGround() { + if movement.Gliding() { movement.SetGliding(false) movement.SetGlideBoost(0) }🤖 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/simulation/movement.go` around lines 65 - 92, Update the liquid-contact branch in the movement simulation so entering water or lava always clears the glide state before returning, regardless of movement.OnGround(). In the existing movement.Gliding() handling, remove the on-ground condition while preserving the resets for Gliding and GlideBoost.
🧹 Nitpick comments (1)
player/simulation/movement.go (1)
132-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign climb telemetry with the updated jump condition.
The branch now uses
EffectiveJumping(), but the debug output still labels and printsPressingJump(). Auto-jump or other effective-jump states will therefore produce misleading diagnostics. (raw.githubusercontent.com)- p.Dbg.Notify(..., "added climb velocity: %v (collided=%v pressingJump=%v)", ..., movement.PressingJump()) + p.Dbg.Notify(..., "added climb velocity: %v (collided=%v effectiveJumping=%v)", ..., movement.EffectiveJumping())🤖 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/simulation/movement.go` around lines 132 - 140, Update the climb-related debug output near the vertical velocity assignment to use EffectiveJumping() instead of PressingJump(), including the corresponding telemetry label, so auto-jump and other effective-jump states are reported consistently with the branch condition.
🤖 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 `@player/simulation/movement.go`:
- Around line 65-92: Update the liquid-contact branch in the movement simulation
so entering water or lava always clears the glide state before returning,
regardless of movement.OnGround(). In the existing movement.Gliding() handling,
remove the on-ground condition while preserving the resets for Gliding and
GlideBoost.
---
Nitpick comments:
In `@player/simulation/movement.go`:
- Around line 132-140: Update the climb-related debug output near the vertical
velocity assignment to use EffectiveJumping() instead of PressingJump(),
including the corresponding telemetry label, so auto-jump and other
effective-jump states are reported consistently with the branch condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36addafd-e451-4f9c-9678-5bff666c6f3b
📒 Files selected for processing (3)
game/movement.goplayer/component/movement.goplayer/simulation/movement.go
💤 Files with no reviewable changes (1)
- game/movement.go
🚧 Files skipped from review as they are similar to previous changes (1)
- player/component/movement.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
Can we get some approvals and testers 😅 |
Due to the current state of my region, I am unable to test/review anything, maybe @didntpot is nice enough to test this and give proper feedback! |
|
newcoolboys is the most annoying for me Cakey Bot Yesterday at 8:23 AM |
|
merged in bedsim |
4 participants
Allows movement simulation to remain fully functional in liquid environments, preserving accurate player movement behavior within Oomph
Summary by CodeRabbit
New Features
Bug Fixes