feat: expand movement simulation parity - #6
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 simulator adds new movement inputs, pose-state fields, collision-provider hooks, block movement semantics, bubble columns, Riptide handling, and block-effect processing. It also updates liquid travel, movement parity, and README coverage for the expanded movement model. ChangesMovement simulation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![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
🤖 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 `@bubble.go`:
- Around line 50-60: Update the bubble-column processing loop to apply an
impulse only once per tick: after finding the first applicable column cell,
apply its direction and stop further cell processing. Preserve the existing
liquid-above and air checks, and add an integration test covering an AABB
spanning two bubble-column blocks to verify only one impulse is applied.
In `@liquid.go`:
- Around line 85-89: Update the Depth Strider selection in the equipment branch
to fall back to the legacy DepthStriderProvider when EnchantmentDepthStrider
reports level zero, while preserving equipment as authoritative for positive
levels and the existing clamping to 0–3. Add coverage for equipment with level
zero combined with an inventory DepthStriderProvider.
🪄 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: e24a8f37-fccd-46ee-a2da-e078c2e0e9a6
📒 Files selected for processing (17)
README.mdblock_effects.goblock_effects_test.gobubble.gobubble_test.goconstants.godynamic_collision_test.goinput.gointerfaces.goliquid.gomovement.gomovement_environment_test.goparity_test.goplayer_features_test.gosimulation.gosimulator.gosimulator_test.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)
liquid.go (1)
53-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
PressingDescendat the liquid surface.The new descent impulse can be immediately erased by
updateSwimTravelwhen the player looks upward: that clamp only checks!state.WantDownSlow, whilePressingDescendis also set for sneaking andDescendBlock. Skip the surface clamp whenstate.PressingDescendis true, and add an upward-pitch regression 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 `@liquid.go` around lines 53 - 57, Update the liquid surface clamp in updateSwimTravel to also skip clamping when state.PressingDescend is true, preserving the descent impulse for sneaking and DescendBlock while looking upward. Add an upward-pitch regression test covering PressingDescend at the liquid surface.
🤖 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 `@liquid.go`:
- Around line 53-57: Update the liquid surface clamp in updateSwimTravel to also
skip clamping when state.PressingDescend is true, preserving the descent impulse
for sneaking and DescendBlock while looking upward. Add an upward-pitch
regression test covering PressingDescend at the liquid surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c5c2fe3-c5f4-4ca3-9f34-f9f39027144b
📒 Files selected for processing (18)
bedrock_semantics_test.goblock_effects.goblock_effects_test.gobubble.gobubble_test.goconstants.godynamic_collision_test.goinput.gointerfaces.goliquid.goliquid_test.gomovement.gomovement_environment_test.goparity_test.goplayer_features_test.gosimulation.gosimulator.gosimulator_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- movement_environment_test.go
- bubble.go
- bedrock_semantics_test.go
- bubble_test.go
- player_features_test.go
- dynamic_collision_test.go
- block_effects_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
player_features_test.go (2)
23-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the Soul Speed test oracle.
The assertion only checks that
with.Vel.Z()is greater thanwithout.Vel.Z(). It does not prove that the unenchanted run receives soul-sand slowdown or that the enchanted run matches normal-ground movement.Add a normal-ground control run. Assert both expected relationships with a tolerance.
🤖 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_features_test.go` around lines 23 - 30, Add a normal-ground control simulation alongside the existing `without` and `with` runs in the Soul Speed test. Update the assertions to verify, using an appropriate tolerance, that the unenchanted soul-sand velocity is below normal-ground movement and the Soul Speed velocity matches normal-ground movement, while preserving the existing test setup.
76-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise crawling through
Simulator.Simulate.Line 76 calls private
applyInput, so this test bypasses the documented public path that applies input and advances simulation counters. Add a public-path assertion for crawling. Keep direct helper coverage only as a separate unit-level test.As per coding guidelines,
Simulator.Simulate(state, input)applies input and ticks counters.🤖 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_features_test.go` at line 76, Update the crawling test to exercise the public Simulator.Simulate(state, input) path instead of directly calling sim.applyInput, and assert the resulting crawling behavior and simulation counter updates. Keep direct applyInput coverage separate as a focused unit test if needed.Source: Coding guidelines
🤖 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 `@block_effects.go`:
- Around line 32-43: Update applyStuckSpeedMultiplier so the NoClip branch
clears state.StuckSpeedMultiplier before returning false. Preserve the existing
behavior for zero multipliers and normal multiplier application.
In `@bubble.go`:
- Around line 53-61: Remove the early return after applyBubbleColumn in the
bubble-column processing loop so every occupied bubble-column cell is processed
during the tick. Update TestBubbleColumnAppliesOnceAcrossMultipleCells to assert
the accumulated impulse from each occupied cell while preserving the existing
per-cell direction and above-liquid checks.
---
Nitpick comments:
In `@player_features_test.go`:
- Around line 23-30: Add a normal-ground control simulation alongside the
existing `without` and `with` runs in the Soul Speed test. Update the assertions
to verify, using an appropriate tolerance, that the unenchanted soul-sand
velocity is below normal-ground movement and the Soul Speed velocity matches
normal-ground movement, while preserving the existing test setup.
- Line 76: Update the crawling test to exercise the public
Simulator.Simulate(state, input) path instead of directly calling
sim.applyInput, and assert the resulting crawling behavior and simulation
counter updates. Keep direct applyInput coverage separate as a focused unit test
if needed.
🪄 Autofix
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: 105faed2-4956-475a-9203-9da49da7d0c4
📒 Files selected for processing (23)
README.mdbedrock_semantics_test.goblock/contact.goblock/environment.goblock/ground.goblock/semantics.goblock_effects.goblock_effects_test.goblock_semantics_test.gobubble.gobubble_test.goconstants.godynamic_collision_test.gointerfaces.goliquid.goliquid_test.gomovement.gomovement_environment_test.goparity_test.goplayer_features_test.gosimulation.gosimulator.gosimulator_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- bedrock_semantics_test.go
- parity_test.go
- simulator.go
- movement_environment_test.go
- interfaces.go
- movement.go
- constants.go
|
@coderabbitai review |
|
HashimTheArab
left a comment
HashimTheArab
left a comment
There was a problem hiding this comment.
Audit findings for the current head: the Depth Strider formula/fallback, Riptide impulse direction/force, bed bounce coefficient, and bubble impulse constants line up. I found four correctness issues: bubble-column entries are truncated after the first, Riptide lacks the mounted-player eligibility gate, and web/queued inside slowdowns do not scale the pending input request. Inline comments below describe the fixes and regression coverage. Normal tests and vet pass; race testing could not run here because the environment has no C compiler.
| above := pos.Side(cube.FaceUp) | ||
| _, liquidAbove := s.liquidAt(above) | ||
| applyBubbleColumn(state, direction, !liquidAbove && s.blockAir(s.blockAtPos(above))) | ||
| return |
There was a problem hiding this comment.
The client keeps one bubble-column entry for each intersecting column and processes the complete entry list on a tick. Returning here makes the result depend on scan order and drops every later column. Please continue the scan after applying this impulse, and add a regression test with two overlapping same-direction columns that expects both contributions.
| } | ||
|
|
||
| func (s *Simulator) attemptRiptide(state *MovementState, touchingWater bool) bool { | ||
| if s.Equipment == nil || state.RiptideTicks > 0 || !state.RiptideReady || (!touchingWater && !state.RiptideInRain) { |
There was a problem hiding this comment.
Riptide eligibility is missing the mounted-player gate. A validated release while the player is riding a vehicle must not create the spin-attack impulse, but this path has no vehicle state to reject that case. Add an authoritative vehicle/mounted input to MovementState or the provider contract and reject it here; keep it separate from RiptideReady.
| if state.NoClip || multiplier.LenSqr() <= 1e-7 { | ||
| return false | ||
| } | ||
| state.SetVel(mgl32.Vec3{ |
There was a problem hiding this comment.
This only scales the existing velocity, but the slowdown also applies to the pending movement request. With a queued berry/powder-snow multiplier, a zero-velocity player with new input reaches moveRelative unchanged, and the later stuckMovement branch only clears velocity after recording the full displacement. Scale the pending horizontal impulse/request too and add a two-tick input-driven regression test.
| xz, y = 0.5, 0.25 | ||
| } | ||
| } | ||
| newVel[0] *= xz |
There was a problem hiding this comment.
The web factor is applied only to the pre-existing velocity; moveRelative adds state.Impulse afterward. A zero-velocity player moving into a web therefore records an unscaled input displacement before the later zeroing step. Apply the same factor to the pending horizontal request (or equivalent pre-collision move) and add an input-driven web test.
1 participant
Summary
mainWhy
bedsim covered core ground, collision, gliding, and teleport physics but was missing several client movement paths and had a handful of numeric differences. These gaps could produce authoritative position or velocity divergence for players interacting with liquids, special blocks, poses, and movement-affecting equipment.
Impact
World adapters may optionally implement
BubbleColumnProviderandMovementCollisionProvider. Player integrations may provideMovementEquipmentProviderfor Depth Strider, Soul Speed, Swift Sneak, Riptide, and leather-boots behavior. Existing providers remain supported, including the legacy inventory-based Depth Strider extension.The README documents the new optional capabilities and pose dimensions.
Validation
go test ./...go test -race ./...go vet ./...git diff --checkSummary by CodeRabbit