fix: close BedSim movement parity edge cases - #14
Conversation
📝 WalkthroughWalkthroughThe PR adds finite-input validation, mounted and invalid-input outcomes, queued motion, expanded collision providers, updated liquid and block physics, special movement handling, and regression coverage. ChangesMovement simulation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant Simulator
participant MovementState
participant World
participant CollisionProviders
Caller->>Simulator: Submit input and movement state
Simulator->>MovementState: Validate queued state and transient flags
Simulator->>World: Check movement-area coverage
World-->>Simulator: Loaded-area result
Simulator->>CollisionProviders: Resolve contacts and support
CollisionProviders-->>Simulator: Collision boxes and supporting block
Simulator->>MovementState: Apply liquid, block, teleport, or Riptide updates
Simulator-->>Caller: Return SimulationResult
🚥 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 |
02caad8 to
9338a65
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
interfaces.go (1)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the coordinate space of
aabbin the new provider interfaces.
WorldProvider.BlockCollisionsreturns block-local boxes, and that distinction is already called out at Line 12. The three new methods receive world-space boxes fromSimulator. Adapter authors can mix the two conventions and produce wrong contact or support results. State the coordinate space in each doc comment.📝 Proposed documentation change
// MovementAreaProvider can provide a precise loaded/known check for a swept -// movement volume. Worlds that only expose chunk loading use BedSim's +// movement volume in world space. Worlds that only expose chunk loading use BedSim's // conservative chunk-range fallback. type MovementAreaProvider interface { IsMovementAreaLoaded(aabb cube.BBox32) bool }-// ClimbableContactProvider resolves orientation-aware ladder and vine contact. +// ClimbableContactProvider resolves orientation-aware ladder and vine contact. +// aabb is in world space. // The built-in fallback scans intersecting block volumes when this is absent. type ClimbableContactProvider interface { HasClimbableContact(aabb cube.BBox32) bool } // MovementSupportProvider resolves the exact support block for dynamic shapes. +// aabb is in world space. // It is optional because a generic collision provider may not retain source // block identities. type MovementSupportProvider interface { SupportingBlock(aabb cube.BBox32, context MovementCollisionContext) (cube.Pos, bool) }Also applies to: 46-58
🤖 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 `@interfaces.go` around lines 18 - 24, Update the doc comments for MovementAreaProvider.IsMovementAreaLoaded and the other two new provider methods in this interface section to explicitly state that their aabb parameters are world-space coordinates supplied by Simulator, distinguishing them from WorldProvider.BlockCollisions block-local boxes.Source: Learnings
simulation.go (2)
404-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
SimulateStatecallers must clear the pending flags.
tickStateclearsKnockbackPendingandStoppedSwimmingThisTick.SimulateStatedoes not calltickState. A caller that usesSimulateStateandQueueKnockbackkeepsKnockbackPendingset, soHasKnockbackstays true on every later tick and the storedKnockbackvelocity is reapplied.Add this lifecycle note to README next to the
QueueKnockbackdocumentation, in the same way theDolphinBoostTicksnote at README Line 153 does.Also applies to: 418-418
🤖 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 `@simulation.go` at line 404, Document next to the README QueueKnockback documentation that callers using SimulateState must clear KnockbackPending and StoppedSwimmingThisTick themselves, since SimulateState does not invoke tickState. Match the existing DolphinBoostTicks lifecycle-note style and preserve the current simulation behavior.
1420-1448: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse
slices.SortStableFuncforsortedCollisionBoxes.
sort.SliceStablerequires reflection for element access in this sorting path. Useslices.SortStableFunconfilteredand update the import to usecmpandslicesinstead ofsort.🤖 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 `@simulation.go` around lines 1420 - 1448, The sortedCollisionBoxes function should use slices.SortStableFunc instead of sort.SliceStable to avoid reflection. Replace the sort import with cmp and slices, and implement the comparator using cmp.Compare for corresponding Min and Max coordinates while preserving the existing lexicographic ordering.parity_regressions_test.go (1)
154-169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the negative case for climbable contact.
TestAdjacentClimbableContactIsDetectedcovers only the positive case.hasClimbableContactgrows the bounding box by 0.05 on the Y axis, so a player who stands on top of a ladder also reports contact. See the comment onsimulation.goLine 1290. A negative test pins the intended boundary.Place the ladder at
{0, -1, 0}with the player at{0.5, 0, 0.5}, and assert thatstate.Vel.Y()does not becomeClimbSpeedwhileEffectiveJumpingis true.🤖 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 `@parity_regressions_test.go` around lines 154 - 169, Add a negative test alongside TestAdjacentClimbableContactIsDetected using a ladder at {0, -1, 0} and player position {0.5, 0, 0.5}; with EffectiveJumping enabled, simulate the state and assert state.Vel.Y() does not equal ClimbSpeed. This should verify that standing on top of a ladder is not treated as climbable contact.
🤖 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 `@simulation.go`:
- Around line 1348-1363: Apply the same BBHasZeroVolume filter in the fallback
collision scan of Simulator.findSupportingBlock before considering boxes
returned by w.BlockCollisions(pos). Skip degenerate or inverted boxes so
SupportingBlockPos matches the boxes accepted by sortedCollisionBoxes and does
not select an invalid support block.
- Around line 332-335: Document MovementState.JumpHeight as an output-only field
in movement.go, clarifying that Simulate derives it from JumpStrength and direct
customization is unsupported. Add a corresponding note near the JumpStrength
documentation in README, without changing the simulation behavior.
- Around line 533-538: In the supporting-block fallback within the surrounding
simulation logic, update only insideSemantics.Traversal from
supportingSemantics.Traversal when the supporting block provides traversal
behavior. Do not replace the entire insideSemantics bundle, preserving its
existing Climbable, Cobweb, bounce, and inside-block behavior fields.
- Around line 80-82: Change invalidSimulationResult to a SimulationState method
that derives NeedsCorrection from s.Options.Mode, returning false for
SimulationModePassive while preserving true for default and permissive modes.
Update both existing call sites to invoke s.invalidSimulationResult(), keeping
the invalid-input outcome unchanged.
- Around line 1290-1307: Update Simulator.hasClimbableContact to grow the
bounding box only along the horizontal X and Z axes, leaving the Y growth at
zero so blocks beneath the player are not treated as climbable contact. Preserve
the provider and nearby-block detection logic.
- Around line 443-450: Update the riptide branch in the simulation flow around
simulateRiptide to decrement SwimWaterGraceTicks before returning when riptide
travel occurs without water contact. Preserve the existing normal-tick defer
bookkeeping and ensure the riptide path consumes one grace tick on every such
tick.
- Line 494: Update the movement-speed calculation around moveRelativeSpeed and
state.OnGround so s.movementEffectMultiplier() is applied only to grounded
state.MovementSpeed, while airborne state.AirSpeed remains unscaled. Preserve
the existing acceleration behavior for both branches.
- Around line 1309-1333: Update Simulator.movementAreaLoaded’s fallback path to
validate the computed chunk coordinates and bound the chunk span before
converting values to int32 or entering the nested loop. Return false for any
coordinate or span outside the supported safe range, including ranges that could
overflow or require excessive iteration; retain normal IsChunkLoaded checks for
bounded ranges.
---
Nitpick comments:
In `@interfaces.go`:
- Around line 18-24: Update the doc comments for
MovementAreaProvider.IsMovementAreaLoaded and the other two new provider methods
in this interface section to explicitly state that their aabb parameters are
world-space coordinates supplied by Simulator, distinguishing them from
WorldProvider.BlockCollisions block-local boxes.
In `@parity_regressions_test.go`:
- Around line 154-169: Add a negative test alongside
TestAdjacentClimbableContactIsDetected using a ladder at {0, -1, 0} and player
position {0.5, 0, 0.5}; with EffectiveJumping enabled, simulate the state and
assert state.Vel.Y() does not equal ClimbSpeed. This should verify that standing
on top of a ladder is not treated as climbable contact.
In `@simulation.go`:
- Line 404: Document next to the README QueueKnockback documentation that
callers using SimulateState must clear KnockbackPending and
StoppedSwimmingThisTick themselves, since SimulateState does not invoke
tickState. Match the existing DolphinBoostTicks lifecycle-note style and
preserve the current simulation behavior.
- Around line 1420-1448: The sortedCollisionBoxes function should use
slices.SortStableFunc instead of sort.SliceStable to avoid reflection. Replace
the sort import with cmp and slices, and implement the comparator using
cmp.Compare for corresponding Min and Max coordinates while preserving the
existing lexicographic ordering.
🪄 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: aab9a9c3-24f4-47de-9b4a-6a36d3843d20
📒 Files selected for processing (18)
README.mdblock/environment.goblock/semantics.goblock_effects.goblock_effects_test.goblock_semantics_test.goblock_test.gobubble.gobubble_test.gocollision.gointerfaces.goliquid.goliquid_test.gomovement.goparity_regressions_test.goresult.gosimulation.govalidation.go
| accelerationFriction := blockFriction * accelerationMultiplier | ||
| moveRelativeSpeed = mSpeed * (0.16277136 / (accelerationFriction * accelerationFriction * accelerationFriction)) | ||
| } | ||
| moveRelativeSpeed *= s.movementEffectMultiplier() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether MovementSpeed is already effect-adjusted, and where AirSpeed is set.
set -euo pipefail
rg -nP --type=go -C4 '\b(MovementSpeed|DefaultMovementSpeed|ServerUpdatedSpeed)\b'
rg -nP --type=go -C4 '\bAirSpeed\b'
rg -nP --type=go -C4 'EffectSpeed|EffectSlowness'Repository: oomph-ac/bedsim
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Go files =="
git ls-files '*.go' | sed -n '1,120p'
echo "== simulation.go location candidates =="
fd -a 'simulation\.go$' . || true
echo "== symbol searches =="
rg -n --type=go -C3 'movementEffectMultiplier|MoveRelative|ServerUpdatedSpeed|DefaultMovementSpeed|MovementSpeed|AirSpeed|OnGround|EffectSpeed|EffectSlowness' . || true
echo "== simulation.go excerpt if present =="
for f in $(fd 'simulation\.go$' .); do
echo "--- $f lines 460-510 ---"
sed -n '460,510p' "$f" | nl -ba -v460
echo "--- $f lines 850-905 ---"
sed -n '850,905p' "$f" | nl -ba -v850
doneRepository: oomph-ac/bedsim
Length of output: 26004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate interfaces/packet types =="
fd . . -e go | while read -r f; do
rg -n -q 'type .*Effects|EffectSpeed|EffectSlowness|GetEffect|EffectLevitation|DepthStriderLevel' "$f" && echo "$f"
done
echo "== exact simulation.go excerpts =="
python3 - <<'PY'
from pathlib import Path
p = Path('simulation.go')
for start,end in [(460,510),(850,876)]:
print(f'--- {p}:{start}-{end} ---')
for i,line in enumerate(p.read_text().splitlines(),1):
if start <= i <= end:
print(f'{i:5}: {line}')
PY
echo "== movement speed initialization/usages in tests =="
python3 - <<'PY'
from pathlib import Path
for path in ['simulator_test.go','movement_environment_test.go','parity_test.go','block_effects_test.go']:
p = Path(path)
if not p.exists(): continue
print(f'--- {path} ---')
for i,line in enumerate(p.read_text().splitlines(),1):
if 'MovementSpeed' in line or 'DefaultMovementSpeed' in line or 'AirSpeed' in line or 'Effects' in line:
print(f'{i:5}: {line}')
PY
echo "== packet/types candidates =="
git ls-files | rg -E '(^|/)packet|effect|effects|type.*effect|EffectSpeed|EffectSlowness' || true
rg -n -q 'EffectSpeed|EffectSlowness' . || trueRepository: oomph-ac/bedsim
Length of output: 314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== interfaces.go =="
cat -n interfaces.go
echo "== test effect references =="
for f in simulator_test.go movement_environment_test.go parity_test.go; do
echo "--- $f ---"
rg -n -C3 'Effects|EffectSpeed|EffectSlowness|MovementSpeed|DefaultMovementSpeed|AirSpeed|MovementEffect|movementEffectMultiplier' "$f" || true
done
echo "== all EffectSpeed/EffectSlowness references =="
rg -n -C3 'EffectSpeed|EffectSlowness|movementEffectMultiplier' . || true
echo "== simulation.go relevant lines with numbers =="
python3 - <<'PY'
from pathlib import Path
p=Path('simulation.go')
for start,end in [(180,230),(270,400),(480,505),(868,890)]:
print(f'--- {p}:{start}-{end} ---')
for i,line in enumerate(p.read_text().splitlines(),1):
if start <= i <= end:
print(f'{i}: {line}')
PYRepository: oomph-ac/bedsim
Length of output: 18846
🌐 Web query:
Minecraft Bedrock Speed effect movement speed attribute Slowness effect movement speed no air acceleration scaling
💡 Result:
In Minecraft, the movement speed attribute and status effects like Speed and Slowness primarily influence ground movement, while their impact on airborne, aquatic, and certain other states is limited or non-existent [1][2][3][4]. Movement Speed Attribute and Effects The minecraft:generic.movement_speed attribute is a base value that determines how fast an entity moves on the ground [5]. Status effects such as Speed and Slowness modify this speed by applying multipliers to the walking speed [6][7][8]. Specifically, Speed increases movement speed (typically by 20% per level), while Slowness decreases it (typically by 15% per level) [6][7][9]. Air Acceleration and Scaling A well-documented behavior in Minecraft (often tracked in bug reports as working as intended or long-standing behavior) is that the movement speed attribute and potion effects do not scale proportionally when an entity is airborne [1][3][4]. When a player is in the air, the game uses different movement factors than those applied on the ground [10][11]. Consequently, players can often maintain relatively normal movement speeds while sprint-jumping or falling, even if their generic.movement_speed is significantly modified or they are affected by Slowness [1][8][4]. Because the air movement acceleration logic is largely independent of the ground-based attribute, the attribute does not scale the entity's speed in the air as it would on the ground [1][3][11]. Water and Other States Similarly, movement speed attribute modifiers and status effects like Speed and Slowness often do not correctly apply to swimming speed in water [1][2]. The movement mechanics in water and air are distinct from ground movement, leading to a discrepancy where ground-based speed modifications fail to scale or influence velocity in these states [1][2][4]. Summary of Behavior 1. Ground Speed: Directly influenced by generic.movement_speed and status effects [5][7]. 2. Air/Water Speed: Generally does not scale with generic.movement_speed or status effects, often resulting in "normal" movement speeds even when heavily slowed or sped up on the ground [1][2][3][4]. 3. Sprint-Jumping: Effectively mitigates the impact of Slowness because the airborne portion of the jump ignores the ground-based movement reduction [8][4].
Citations:
- 1: https://mojira.dev/MC-269477
- 2: https://mojira.dev/MC-43269
- 3: https://mojira.dev/MC-268323
- 4: https://mojira.dev/MC-2112
- 5: https://calculatrex.com/minecraft-horse-speed-calculator
- 6: https://wiki.sasgaming.net/wiki/Minecraft:Speed
- 7: https://minecraft.wiki/w/Slowness
- 8: https://www.mcpk.wiki/wiki/Status_Effects
- 9: https://minecraft.fandom.com/wiki/Slowness
- 10: https://www.mcpk.wiki/wiki/Sprinting
- 11: https://www.mcpk.wiki/wiki/Horizontal_Movement_Formulas
Avoid applying movement effects to airborne acceleration.
moveRelativeSpeed *= s.movementEffectMultiplier() also scales state.AirSpeed when state.OnGround is false. Ground effects should use state.MovementSpeed, but air acceleration does not inherit the movement-speed/potion multiplier the same way in Bedrock. Apply this factor only in the grounded branch, or move the factor into moveRelativeSpeed assignment so the airborne path remains unscaled.
🤖 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 `@simulation.go` at line 494, Update the movement-speed calculation around
moveRelativeSpeed and state.OnGround so s.movementEffectMultiplier() is applied
only to grounded state.MovementSpeed, while airborne state.AirSpeed remains
unscaled. Preserve the existing acceleration behavior for both branches.
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)
simulation.go (1)
748-752: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a minimum horizontal-look threshold before division.
A pitch near
-90degrees makeslookHzpositive but very small. Lines 751-752 then divide by that value and can create an extreme horizontal velocity. Apply the same epsilon check to every branch that divides bylookHz.Proposed fix
- if vel[1] < 0 && lookHz > 0 { + if vel[1] < 0 && lookHz > 1e-4 { yAccel := vel[1] * -0.1 * sqrPitchCos vel[1] += yAccel vel[0] += lookX * yAccel / lookHz vel[2] += lookZ * yAccel / lookHz } - if pitch < 0 && lookHz > 0 { + if pitch < 0 && lookHz > 1e-4 { yAccel := velHz * -pitchSin * 0.04 vel[1] += yAccel * 3.2 vel[0] -= lookX * yAccel / lookHz vel[2] -= lookZ * yAccel / lookHz } - if lookHz > 0 { + if lookHz > 1e-4 { vel[0] += (lookX/lookHz*velHz - vel[0]) * 0.1 vel[2] += (lookZ/lookHz*velHz - vel[2]) * 0.1 }🤖 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 `@simulation.go` around lines 748 - 752, Update the velocity adjustment branch around the lookHz divisions so horizontal-look calculations only divide when lookHz exceeds the established minimum epsilon, applying the same threshold consistently to every branch that divides by lookHz. Preserve the existing acceleration updates when the threshold is satisfied and avoid producing extreme velocity values for near-vertical pitch.
🤖 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 `@simulation.go`:
- Around line 748-752: Update the velocity adjustment branch around the lookHz
divisions so horizontal-look calculations only divide when lookHz exceeds the
established minimum epsilon, applying the same threshold consistently to every
branch that divides by lookHz. Preserve the existing acceleration updates when
the threshold is satisfied and avoid producing extreme velocity values for
near-vertical pitch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7723546a-87d0-4bf7-a897-b6c33e460b5a
📒 Files selected for processing (8)
README.mdblock_effects_test.gobubble.gointerfaces.goliquid.gomovement.goparity_regressions_test.gosimulation.go
💤 Files with no reviewable changes (1)
- liquid.go
🚧 Files skipped from review as they are similar to previous changes (4)
- block_effects_test.go
- interfaces.go
- movement.go
- parity_regressions_test.go
1 participant
What changed
SimulateState.SimulateStatetransient lifecycles, with regression coverage for the reviewed edge cases.Why
The audit found cases where state-only simulation could accept impossible zero-value events, continue physics beyond loaded data, lose grounding after a step, misclassify shallow fluids, or depend on provider iteration order. The movement pass also corrected sprint air acceleration, active Riptide state, authoritative knockback ordering, scaffolding support handling, climb/support contact behavior, and extreme-coordinate safety.
Scope
Vehicle movement remains an adapter-owned concern; BedSim reports mounted state after aligning to the client state. Provider interfaces let integrations supply vehicle-adjacent collision, exact bubble-surface, climb-contact, and world-streaming details without coupling BedSim to a session or pathfinding implementation.
Checks
go test -count=1 ./...go test -count=20 ./...go vet ./...git diff --checkThe race suite could not run in this Windows environment because cgo is disabled and no GCC compiler is installed.